diff --git a/changelogs/unreleased/avar-user-update-branch-in-go.yml b/changelogs/unreleased/avar-user-update-branch-in-go.yml new file mode 100644 index 0000000000000000000000000000000000000000..c8314970ee1476d6b8a7108bc7be33416fd68f57 --- /dev/null +++ b/changelogs/unreleased/avar-user-update-branch-in-go.yml @@ -0,0 +1,5 @@ +--- +title: 'Port UserUpdateBranch to Go' +merge_request: 3013 +author: +type: changed diff --git a/internal/git/id.go b/internal/git/id.go index cc058ef4a14288914bfdb7bc5f6c1438568e9461..ff6bc7164436b8d61a505d3185fcc7372e25cca7 100644 --- a/internal/git/id.go +++ b/internal/git/id.go @@ -56,3 +56,8 @@ func ValidateObjectID(id string) error { return fmt.Errorf("%w: %q", ErrInvalidObjectID, id) } + +// IsZeroOID is a shortcut for `something == git.ZeroOID.String()` +func (oid ObjectID) IsZeroOID() bool { + return string(oid) == string(ZeroOID) +} diff --git a/internal/git/id_test.go b/internal/git/id_test.go index 0ab6f0dd9fea1774a7378933431f156a53195ba9..f20a3ac63b6899391396552caa482d15b99ebfae 100644 --- a/internal/git/id_test.go +++ b/internal/git/id_test.go @@ -104,3 +104,26 @@ func TestNewObjectIDFromHex(t *testing.T) { }) } } + +func TestIsZeroOID(t *testing.T) { + for _, tc := range []struct { + desc string + oid ObjectID + isZero bool + }{ + { + desc: "zero object ID", + oid: ZeroOID, + isZero: true, + }, + { + desc: "zero object ID", + oid: EmptyTreeOID, + isZero: false, + }, + } { + t.Run(tc.desc, func(t *testing.T) { + require.Equal(t, tc.isZero, tc.oid.IsZeroOID()) + }) + } +} diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index 460a71df3fa0348e3710e28cdfaffa6c7ea7a3e4..a7cac180d3b17355ba66135badaf6614e50a85e2 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -93,6 +93,60 @@ func (s *Server) userCreateBranchRuby(ctx context.Context, req *gitalypb.UserCre } func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { + if featureflag.IsDisabled(ctx, featureflag.GoUserUpdateBranch) { + return s.userUpdateBranchRuby(ctx, req) + } + return s.userUpdateBranchGo(ctx, req) +} + +func validateUserUpdateBranchGo(req *gitalypb.UserUpdateBranchRequest) error { + if req.User == nil { + return status.Errorf(codes.InvalidArgument, "empty user") + } + + if len(req.BranchName) == 0 { + return status.Errorf(codes.InvalidArgument, "empty branch name") + } + + if len(req.Oldrev) == 0 { + return status.Errorf(codes.InvalidArgument, "empty oldrev") + } + + if len(req.Newrev) == 0 { + return status.Errorf(codes.InvalidArgument, "empty newrev") + } + + return nil +} + +func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { + // Validate the request + if err := validateUserUpdateBranchGo(req); err != nil { + return nil, err + } + + 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 { + var preReceiveError preReceiveError + if errors.As(err, &preReceiveError) { + return &gitalypb.UserUpdateBranchResponse{ + PreReceiveError: preReceiveError.message, + }, nil + } + + // An oddball response for compatibility with the old + // Ruby code. The "Could not update..." message is + // exactly like the default updateRefError, except we + // say "branch-name", not + // "refs/heads/branch-name". See the + // "Gitlab::Git::CommitError" case in the Ruby code. + return nil, status.Errorf(codes.FailedPrecondition, "Could not update %s. Please refresh and try again.", req.BranchName) + } + + return &gitalypb.UserUpdateBranchResponse{}, nil +} + +func (s *Server) userUpdateBranchRuby(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { client, err := s.ruby.OperationServiceClient(ctx) if err != nil { return nil, err diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 7871899b9bc1239b5ef84db5017b8dab198c3275..f31641202b30114912a6a4f85b73f77c86199c26 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -14,6 +14,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) var ( @@ -25,11 +26,12 @@ var ( func TestSuccessfulUserUpdateBranchRequest(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, + featureflag.GoUserUpdateBranch, }).Run(t, testSuccessfulUserUpdateBranchRequest) } func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { - testRepo, _, cleanupFn := testhelper.NewTestRepo(t) + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() locator := config.NewLocator(config.Config) @@ -40,30 +42,152 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() - request := &gitalypb.UserUpdateBranchRequest{ - Repository: testRepo, - BranchName: []byte(updateBranchName), - Newrev: newrev, - Oldrev: oldrev, - User: testhelper.TestUser, + testCases := []struct { + desc string + updateBranchName string + oldRev []byte + newRev []byte + }{ + { + desc: "short name fast-forward update", + updateBranchName: updateBranchName, + oldRev: []byte("0b4bc9a49b562e85de7cc9e834518ea6828729b9"), + newRev: []byte("1a35b5a77cf6af7edf6703f88e82f6aff613666f"), + }, + { + desc: "short name non-fast-forward update", + updateBranchName: "fix", + oldRev: []byte("48f0be4bd10c1decee6fae52f9ae6d10f77b60f4"), + newRev: []byte("12d65c8dd2b2676fa3ac47d955accc085a37a9c1"), + }, + { + desc: "short name branch creation", + updateBranchName: "a-new-branch", + oldRev: []byte(git.ZeroOID.String()), + newRev: []byte("845009f4d7bdc9e0d8f26b1c6fb6e108aaff9314"), + }, + // We create refs/heads/heads/BRANCH and + // refs/heads/refs/heads/BRANCH here. See a similar + // test for UserCreateBranch in + // TestSuccessfulCreateBranchRequest() + { + desc: "heads/* branch creation", + updateBranchName: "heads/a-new-branch", + oldRev: []byte(git.ZeroOID.String()), + newRev: []byte("845009f4d7bdc9e0d8f26b1c6fb6e108aaff9314"), + }, + { + desc: "refs/heads/* branch creation", + updateBranchName: "refs/heads/a-new-branch", + oldRev: []byte(git.ZeroOID.String()), + newRev: []byte("845009f4d7bdc9e0d8f26b1c6fb6e108aaff9314"), + }, } - response, err := client.UserUpdateBranch(ctx, request) + for _, testCase := range testCases { + t.Run(testCase.desc, func(t *testing.T) { + responseOk := &gitalypb.UserUpdateBranchResponse{} + request := &gitalypb.UserUpdateBranchRequest{ + Repository: testRepo, + BranchName: []byte(testCase.updateBranchName), + Newrev: testCase.newRev, + Oldrev: testCase.oldRev, + User: testhelper.TestUser, + } + response, err := client.UserUpdateBranch(ctx, request) + require.NoError(t, err) + require.Equal(t, responseOk, response) + + branchCommit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(testCase.updateBranchName)) - require.NoError(t, err) - require.Empty(t, response.PreReceiveError) + require.NoError(t, err) + require.Equal(t, string(testCase.newRev), branchCommit.Id) - branchCommit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(updateBranchName)) + branches := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+branchName) + require.Contains(t, string(branches), "refs/heads/"+branchName) + }) + } +} - require.NoError(t, err) - require.Equal(t, string(newrev), branchCommit.Id) +func TestSuccessfulUserUpdateBranchRequestToDelete(t *testing.T) { + testWithFeature(t, featureflag.GoUserUpdateBranch, testSuccessfulUserUpdateBranchRequestToDelete) } -func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() +func testSuccessfulUserUpdateBranchRequestToDelete(t *testing.T, ctx context.Context) { + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() - testSuccessfulGitHooksForUserUpdateBranchRequest(t, ctx) + locator := config.NewLocator(config.Config) + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + testCases := []struct { + desc string + updateBranchName string + oldRev []byte + newRev []byte + err error + createBranch bool + }{ + { + desc: "short name branch deletion", + updateBranchName: "csv", + oldRev: []byte("3dd08961455abf80ef9115f4afdc1c6f968b503c"), + newRev: []byte(git.ZeroOID.String()), + err: status.Error(codes.InvalidArgument, "object not found"), + }, + // We test for the failed heads/* and refs/heads/* cases below in TestFailedUserUpdateBranchRequest + { + desc: "heads/* name branch deletion", + updateBranchName: "heads/my-test-branch", + createBranch: true, + oldRev: []byte("689600b91aabec706e657e38ea706ece1ee8268f"), + newRev: []byte(git.ZeroOID.String()), + err: status.Error(codes.InvalidArgument, "object not found"), + }, + { + desc: "refs/heads/* name branch deletion", + updateBranchName: "refs/heads/my-other-test-branch", + createBranch: true, + oldRev: []byte("db46a1c5a5e474aa169b6cdb7a522d891bc4c5f9"), + newRev: []byte(git.ZeroOID.String()), + err: status.Error(codes.InvalidArgument, "object not found"), + }, + } + + for _, testCase := range testCases { + t.Run(testCase.desc, func(t *testing.T) { + if testCase.createBranch { + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "--", testCase.updateBranchName, string(testCase.oldRev)) + } + + responseOk := &gitalypb.UserUpdateBranchResponse{} + request := &gitalypb.UserUpdateBranchRequest{ + Repository: testRepo, + BranchName: []byte(testCase.updateBranchName), + Newrev: testCase.newRev, + Oldrev: testCase.oldRev, + User: testhelper.TestUser, + } + response, err := client.UserUpdateBranch(ctx, request) + require.Nil(t, err) + require.Equal(t, responseOk, response) + + _, err = log.GetCommit(ctx, locator, testRepo, git.Revision(testCase.updateBranchName)) + require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err) + + refs := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+testCase.updateBranchName) + require.NotContains(t, string(refs), testCase.oldRev, "branch deleted from refs") + }) + } +} + +func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { + testWithFeature(t, featureflag.GoUserUpdateBranch, testSuccessfulGitHooksForUserUpdateBranchRequest) } func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, ctx context.Context) { @@ -89,10 +213,12 @@ func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, ctx context. User: testhelper.TestUser, } + responseOk := &gitalypb.UserUpdateBranchResponse{} response, err := client.UserUpdateBranch(ctx, request) require.NoError(t, err) require.Empty(t, response.PreReceiveError) + require.Equal(t, responseOk, response) output := string(testhelper.MustReadFile(t, hookOutputTempPath)) require.Contains(t, output, "GL_USERNAME="+testhelper.TestUser.GlUsername) }) @@ -100,9 +226,10 @@ func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, ctx context. } func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testWithFeature(t, featureflag.GoUserUpdateBranch, testFailedUserUpdateBranchDueToHooks) +} +func testFailedUserUpdateBranchDueToHooks(t *testing.T, ctx context.Context) { testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) defer cleanupFn() @@ -131,13 +258,24 @@ func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { require.Nil(t, err) require.Contains(t, response.PreReceiveError, "GL_USERNAME="+testhelper.TestUser.GlUsername) require.Contains(t, response.PreReceiveError, "PWD="+testRepoPath) + + responseOk := &gitalypb.UserUpdateBranchResponse{ + PreReceiveError: response.PreReceiveError, + } + require.Equal(t, responseOk, response) } } func TestFailedUserUpdateBranchRequest(t *testing.T) { + testWithFeature(t, featureflag.GoUserUpdateBranch, testFailedUserUpdateBranchRequest) +} + +func testFailedUserUpdateBranchRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() + locator := config.NewLocator(config.Config) + client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() @@ -147,20 +285,24 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { revDoesntExist := fmt.Sprintf("%x", sha1.Sum([]byte("we need a non existent sha"))) testCases := []struct { - desc string - branchName string - newrev []byte - oldrev []byte - user *gitalypb.User - code codes.Code + desc string + branchName string + newrev []byte + oldrev []byte + gotrev []byte + expectNotFoundError bool + user *gitalypb.User + response *gitalypb.UserUpdateBranchResponse + err error }{ { - desc: "empty branch name", - branchName: "", - newrev: newrev, - oldrev: oldrev, - user: testhelper.TestUser, - code: codes.InvalidArgument, + desc: "empty branch name", + branchName: "", + newrev: newrev, + oldrev: oldrev, + expectNotFoundError: true, + user: testhelper.TestUser, + err: status.Error(codes.InvalidArgument, "empty branch name"), }, { desc: "empty newrev", @@ -168,15 +310,16 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: nil, oldrev: oldrev, user: testhelper.TestUser, - code: codes.InvalidArgument, + err: status.Error(codes.InvalidArgument, "empty newrev"), }, { desc: "empty oldrev", branchName: updateBranchName, newrev: newrev, oldrev: nil, + gotrev: oldrev, user: testhelper.TestUser, - code: codes.InvalidArgument, + err: status.Error(codes.InvalidArgument, "empty oldrev"), }, { desc: "empty user", @@ -184,15 +327,25 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: newrev, oldrev: oldrev, user: nil, - code: codes.InvalidArgument, + err: status.Error(codes.InvalidArgument, "empty user"), }, { - desc: "non-existing branch", - branchName: "i-dont-exist", - newrev: newrev, + desc: "non-existing branch", + branchName: "i-dont-exist", + newrev: newrev, + oldrev: oldrev, + expectNotFoundError: true, + user: testhelper.TestUser, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "i-dont-exist"), + }, + { + desc: "existing branch failed deletion attempt", + branchName: "csv", + newrev: []byte(git.ZeroOID.String()), oldrev: oldrev, + gotrev: []byte("3dd08961455abf80ef9115f4afdc1c6f968b503c"), user: testhelper.TestUser, - code: codes.FailedPrecondition, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "csv"), }, { desc: "non-existing newrev", @@ -200,15 +353,42 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: []byte(revDoesntExist), oldrev: oldrev, user: testhelper.TestUser, - code: codes.FailedPrecondition, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", updateBranchName), }, { desc: "non-existing oldrev", branchName: updateBranchName, newrev: newrev, oldrev: []byte(revDoesntExist), + gotrev: oldrev, user: testhelper.TestUser, - code: codes.FailedPrecondition, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", updateBranchName), + }, + { + desc: "existing branch, but unsupported heads/* name", + branchName: "heads/feature", + newrev: []byte("1a35b5a77cf6af7edf6703f88e82f6aff613666f"), + oldrev: []byte("0b4bc9a49b562e85de7cc9e834518ea6828729b9"), + user: testhelper.TestUser, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "heads/feature"), + }, + { + desc: "delete existing branch, but unsupported refs/heads/* name", + branchName: "refs/heads/crlf-diff", + newrev: []byte(git.ZeroOID.String()), + oldrev: []byte("593890758a6f845c600f38ffa05be2749211caee"), + user: testhelper.TestUser, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "refs/heads/crlf-diff"), + }, + { + desc: "short name branch deletion", + branchName: "csv", + oldrev: []byte("3dd08961455abf80ef9115f4afdc1c6f968b503c"), + newrev: []byte(git.ZeroOID.String()), + expectNotFoundError: true, + user: testhelper.TestUser, + err: nil, + response: &gitalypb.UserUpdateBranchResponse{}, }, } @@ -222,11 +402,22 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { User: testCase.user, } - ctx, cancel := testhelper.Context() - defer cancel() + response, err := client.UserUpdateBranch(ctx, request) + require.Equal(t, testCase.response, response) + require.Equal(t, testCase.err, err) - _, err := client.UserUpdateBranch(ctx, request) - testhelper.RequireGrpcError(t, err, testCase.code) + branchCommit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(testCase.branchName)) + if testCase.expectNotFoundError { + require.True(t, log.IsNotFound(err), "expected 'not found' error got %v", err) + return + } + require.NoError(t, err) + + if len(testCase.gotrev) == 0 { + // The common case is the update didn't succeed + testCase.gotrev = testCase.oldrev + } + require.Equal(t, string(testCase.gotrev), branchCommit.Id) }) } } diff --git a/internal/gitaly/service/repository/raw_changes.go b/internal/gitaly/service/repository/raw_changes.go index 2eab8825dcc6b01b14a390bf6ce346ef5e50c953..b9bbd8fe9d5d986a44418496c72b9adde4bfb58c 100644 --- a/internal/gitaly/service/repository/raw_changes.go +++ b/internal/gitaly/service/repository/raw_changes.go @@ -38,13 +38,13 @@ func (s *server) GetRawChanges(req *gitalypb.GetRawChangesRequest, stream gitaly } func validateRawChangesRequest(ctx context.Context, req *gitalypb.GetRawChangesRequest, batch catfile.Batch) error { - if from := req.FromRevision; from != git.ZeroOID.String() { + if from := req.FromRevision; !git.ObjectID(from).IsZeroOID() { if _, err := batch.Info(ctx, git.Revision(from)); err != nil { return fmt.Errorf("invalid 'from' revision: %q", from) } } - if to := req.ToRevision; to != git.ZeroOID.String() { + if to := req.ToRevision; !git.ObjectID(to).IsZeroOID() { if _, err := batch.Info(ctx, git.Revision(to)); err != nil { return fmt.Errorf("invalid 'to' revision: %q", to) } @@ -54,11 +54,11 @@ func validateRawChangesRequest(ctx context.Context, req *gitalypb.GetRawChangesR } func getRawChanges(stream gitalypb.RepositoryService_GetRawChangesServer, repo *gitalypb.Repository, batch catfile.Batch, from, to string) error { - if to == git.ZeroOID.String() { + if git.ObjectID(to).IsZeroOID() { return nil } - if from == git.ZeroOID.String() { + if git.ObjectID(from).IsZeroOID() { from = git.EmptyTreeOID.String() } diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 9071c36b3cc417ad409444880fca76610f27ea51..6b895c2346c68721b0d7bba8b75c839310806a2f 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -23,6 +23,8 @@ var ( GoUserFFBranch = FeatureFlag{Name: "go_user_ff_branch", OnByDefault: false} // GoUserCreateBranch enables the Go implementation of UserCreateBranch GoUserCreateBranch = FeatureFlag{Name: "go_user_create_branch", OnByDefault: true} + // GoUserUpdateBranch enables the Go implementation of UserUpdateBranch + GoUserUpdateBranch = FeatureFlag{Name: "go_user_update_branch", OnByDefault: false} // GoUserCommitFiles enables the Go implementation of UserCommitFiles GoUserCommitFiles = FeatureFlag{Name: "go_user_commit_files", OnByDefault: false} // GoResolveConflicts enables the Go implementation of ResolveConflicts @@ -108,6 +110,7 @@ var All = []FeatureFlag{ GoUserMergeBranch, GoUserFFBranch, GoUserCreateBranch, + GoUserUpdateBranch, GoUserCommitFiles, GoResolveConflicts, GoUserUpdateSubmodule,