From 09737ad4cd3774508d682ac23cfee8cd8d4a1156 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Tue, 2 Feb 2021 12:19:07 +0100 Subject: [PATCH 1/3] check if error is not found in UserDeleteBranch and UserCreateBranch UserDeleteBranch and UserCreateBranch do not check currently if the error is a not found type error before responding so. This causes them to respond with a not found even if the error is something unrelated. This commit adds checks for the error types. --- internal/gitaly/service/operations/branches.go | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index c6b39eb7331..2bee1d8059b 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -7,6 +7,7 @@ import ( "strings" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/git/localrepo" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" @@ -38,7 +39,11 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB startPointCommit, err := log.GetCommit(ctx, s.locator, req.Repository, git.Revision(req.StartPoint)) // END TODO if err != nil { - return nil, status.Errorf(codes.FailedPrecondition, "revspec '%s' not found", req.StartPoint) + if catfile.IsNotFound(err) { + return nil, status.Errorf(codes.FailedPrecondition, "revspec '%s' not found", req.StartPoint) + } + + return nil, err } referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) @@ -163,7 +168,11 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB referenceName := fmt.Sprintf(referenceFmt, req.BranchName) referenceValue, err := localrepo.New(req.Repository, s.cfg).GetReference(ctx, git.ReferenceName(referenceName)) if err != nil { - return nil, status.Errorf(codes.FailedPrecondition, "branch not found: %s", req.BranchName) + if errors.Is(err, git.ErrReferenceNotFound) { + return nil, status.Errorf(codes.FailedPrecondition, "branch not found: %s", req.BranchName) + } + + return nil, err } if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, git.ZeroOID.String(), referenceValue.Target); err != nil { -- GitLab From f747aef48c410f5177e9e3c3446132870dc74cd7 Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Tue, 2 Feb 2021 12:44:26 +0100 Subject: [PATCH 2/3] evaluate repo path's symlinks in UserUpdateBranch tests macOS' default temporary directory is under '/var/...'. '/var' is a symlink to '/private/var'. UserUpdateBranch tests compare the full path with symlinks evaluated to a path that does not have the symlinks expanded, causing the test to fail on macOS. This commit evaluates the symlinks prior to comparing the two paths to fix this. --- .../gitaly/service/operations/update_branches_test.go | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index f31641202b3..859889748bb 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -4,6 +4,7 @@ import ( "context" "crypto/sha1" "fmt" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -233,6 +234,13 @@ func testFailedUserUpdateBranchDueToHooks(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() + // macOS' default tmp directory is under /var/... but /var itself is a symlink + // to /private/var. The repository path in the environment variables has its symlinked + // evaluated, causing the comparison to fail, thus we evaluate them here already so the + // assertion later works. + testRepoPath, err := filepath.EvalSymlinks(testRepoPath) + require.NoError(t, err) + serverSocketPath, stop := runOperationServiceServer(t) defer stop() -- GitLab From 626e9745fe18b571ad1f59c5a4f86e54df7f116e Mon Sep 17 00:00:00 2001 From: Sami Hiltunen Date: Tue, 2 Feb 2021 12:47:37 +0100 Subject: [PATCH 3/3] log errors in User{Create,Update,Delete}Branch RPCs UserCreateBranch, UserUpdateBranch and UserDeleteBranch do not log errors they are dropping in favor of alternative error messages. Some errors are also packed in to RPC responses which. This causes the errors to look like successful responses which do not get logged by Gitaly's gRPC logging interceptor. These behaviors can make debugging difficult as it's not clear what was the original cause of the error. This commit addresses the situation by logging the swallowed errors. --- internal/gitaly/service/operations/branches.go | 5 +++++ 1 file changed, 5 insertions(+) diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index 2bee1d8059b..9b3a19dc41b 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -6,6 +6,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/internal/git/localrepo" @@ -49,6 +50,7 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) _, err = localrepo.New(req.Repository, s.cfg).GetReference(ctx, git.ReferenceName(referenceName)) if err == nil { + ctxlogrus.Extract(ctx).Error("branch already exists") return nil, status.Errorf(codes.FailedPrecondition, "Could not update %s. Please refresh and try again.", req.BranchName) } else if !errors.Is(err, git.ErrReferenceNotFound) { return nil, status.Error(codes.Internal, err.Error()) @@ -57,6 +59,7 @@ func (s *Server) UserCreateBranch(ctx context.Context, req *gitalypb.UserCreateB if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, startPointCommit.Id, git.ZeroOID.String()); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to create branch") return &gitalypb.UserCreateBranchResponse{ PreReceiveError: preReceiveError.message, }, nil @@ -113,6 +116,7 @@ func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdat referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, string(req.Newrev), string(req.Oldrev)); err != nil { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to update branch") var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserUpdateBranchResponse{ @@ -178,6 +182,7 @@ func (s *Server) UserDeleteBranch(ctx context.Context, req *gitalypb.UserDeleteB if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, referenceName, git.ZeroOID.String(), referenceValue.Target); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { + ctxlogrus.Extract(ctx).WithError(err).Error("failed to delete branch") return &gitalypb.UserDeleteBranchResponse{ PreReceiveError: preReceiveError.message, }, nil -- GitLab