From dd7d04f60b378de9c441183d184d116eeb46b482 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 10:18:38 +0100 Subject: [PATCH 01/10] UserUpdateBranch tests: test full responses & errors Change the UserUpdateBranch tests to use the same pattern as we use for the rest of the Branch/Tag tests, see e.g. b6987c25a (UserCreateTag tests: test full response & error in validation, 2020-12-04) and other examples in those tests of comparing "responseOk" & "err". --- .../operations/update_branches_test.go | 32 ++++++++++++------- 1 file changed, 21 insertions(+), 11 deletions(-) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 7871899b9bc..d907632ce16 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 ( @@ -40,6 +41,7 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() + responseOk := &gitalypb.UserUpdateBranchResponse{} request := &gitalypb.UserUpdateBranchRequest{ Repository: testRepo, BranchName: []byte(updateBranchName), @@ -51,7 +53,7 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { response, err := client.UserUpdateBranch(ctx, request) require.NoError(t, err) - require.Empty(t, response.PreReceiveError) + require.Equal(t, responseOk, response) branchCommit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(updateBranchName)) @@ -89,10 +91,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) }) @@ -131,6 +135,11 @@ 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) } } @@ -152,7 +161,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev []byte oldrev []byte user *gitalypb.User - code codes.Code + err error }{ { desc: "empty branch name", @@ -160,7 +169,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: newrev, oldrev: oldrev, user: testhelper.TestUser, - code: codes.InvalidArgument, + err: status.Error(codes.InvalidArgument, "empty branch name"), }, { desc: "empty newrev", @@ -168,7 +177,7 @@ 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", @@ -176,7 +185,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: newrev, oldrev: nil, user: testhelper.TestUser, - code: codes.InvalidArgument, + err: status.Error(codes.InvalidArgument, "empty oldrev"), }, { desc: "empty user", @@ -184,7 +193,7 @@ 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", @@ -192,7 +201,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: newrev, oldrev: oldrev, user: testhelper.TestUser, - code: codes.FailedPrecondition, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "i-dont-exist"), }, { desc: "non-existing newrev", @@ -200,7 +209,7 @@ 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", @@ -208,7 +217,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { newrev: newrev, oldrev: []byte(revDoesntExist), user: testhelper.TestUser, - code: codes.FailedPrecondition, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", updateBranchName), }, } @@ -225,8 +234,9 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { ctx, cancel := testhelper.Context() defer cancel() - _, err := client.UserUpdateBranch(ctx, request) - testhelper.RequireGrpcError(t, err, testCase.code) + response, err := client.UserUpdateBranch(ctx, request) + require.Nil(t, response) + require.Equal(t, testCase.err, err) }) } } -- GitLab From a440973fb5221ed24d89c8236d44e00967b88365 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 10:29:33 +0100 Subject: [PATCH 02/10] UserUpdateBranch tests: make "create" requests a for-loop No functional changes here, just the preparatory work of turning the "create" tests into a for-loop for a later addition of more tests. --- .../operations/update_branches_test.go | 44 +++++++++++++------ 1 file changed, 30 insertions(+), 14 deletions(-) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index d907632ce16..1bc036c201e 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -41,24 +41,40 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() - responseOk := &gitalypb.UserUpdateBranchResponse{} - 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"), + }, } - response, err := client.UserUpdateBranch(ctx, request) - - require.NoError(t, err) - require.Equal(t, responseOk, response) + 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(updateBranchName)) + branchCommit, err := log.GetCommit(ctx, locator, testRepo, git.Revision(testCase.updateBranchName)) - require.NoError(t, err) - require.Equal(t, string(newrev), branchCommit.Id) + require.NoError(t, err) + require.Equal(t, string(testCase.newRev), branchCommit.Id) + }) + } } func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { -- GitLab From 5bec74e08590421f678301aa9a3ab993de1b74f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 10:35:29 +0100 Subject: [PATCH 03/10] UserUpdateBranch tests: add non-ff "create" tests --- .../service/operations/update_branches_test.go | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 1bc036c201e..4dcbcdecde4 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -53,6 +53,18 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { 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"), + }, } for _, testCase := range testCases { -- GitLab From 3c6697d8d459c0d19593fbe0a2a72eabffb8091a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 13:59:37 +0100 Subject: [PATCH 04/10] UserUpdateBranch tests: expand "failure" tests As shown here sometimes we have a "failure", but actually do end up updating the branch. Let's test for that. --- .../operations/update_branches_test.go | 86 +++++++++++++++---- 1 file changed, 67 insertions(+), 19 deletions(-) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 4dcbcdecde4..9529d2a0e32 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -175,6 +175,8 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() + locator := config.NewLocator(config.Config) + client, conn := newOperationClient(t, serverSocketPath) defer conn.Close() @@ -184,20 +186,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 - err error + 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, - err: status.Error(codes.InvalidArgument, "empty branch name"), + 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", @@ -212,6 +218,7 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { branchName: updateBranchName, newrev: newrev, oldrev: nil, + gotrev: oldrev, user: testhelper.TestUser, err: status.Error(codes.InvalidArgument, "empty oldrev"), }, @@ -224,12 +231,13 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { err: status.Error(codes.InvalidArgument, "empty user"), }, { - desc: "non-existing branch", - branchName: "i-dont-exist", - newrev: newrev, - oldrev: oldrev, - user: testhelper.TestUser, - err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "i-dont-exist"), + 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: "non-existing newrev", @@ -244,9 +252,36 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { branchName: updateBranchName, newrev: newrev, oldrev: []byte(revDoesntExist), + gotrev: oldrev, user: testhelper.TestUser, 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{}, + }, } for _, testCase := range testCases { @@ -263,8 +298,21 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { defer cancel() response, err := client.UserUpdateBranch(ctx, request) - require.Nil(t, response) + require.Equal(t, testCase.response, response) require.Equal(t, testCase.err, err) + + 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) }) } } -- GitLab From f99b196d5b70f292026f2565a3b5c5507b834ed2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 13:57:56 +0100 Subject: [PATCH 05/10] UserUpdateBranch tests: add tests for "deletion" Yes, you can use "UserUpdateBranch" instead of "UserDeleteBranch", let's test for that, and for a failed deletion. --- .../operations/update_branches_test.go | 85 +++++++++++++++++++ 1 file changed, 85 insertions(+) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index 9529d2a0e32..a6aeb2415da 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -89,6 +89,82 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { } } +func TestSuccessfulUserUpdateBranchRequestToDelete(t *testing.T) { + ctx, cancel := testhelper.Context() + defer cancel() + + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + 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) { ctx, cancel := testhelper.Context() defer cancel() @@ -239,6 +315,15 @@ func TestFailedUserUpdateBranchRequest(t *testing.T) { 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, + err: status.Errorf(codes.FailedPrecondition, "Could not update %v. Please refresh and try again.", "csv"), + }, { desc: "non-existing newrev", branchName: updateBranchName, -- GitLab From 018ddeea1ca9cd0648cde2b68866470a99eb7488 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 20 Jan 2021 15:18:16 +0100 Subject: [PATCH 06/10] UserUpdateBranch tests: add heads/* and refs/heads/* creation tests See my 856e2493d (UserDeleteBranch: unify API responses between Go & Ruby response paths, 2020-12-01) for prior art. --- .../operations/update_branches_test.go | 21 ++++++++++++++++++- 1 file changed, 20 insertions(+), 1 deletion(-) diff --git a/internal/gitaly/service/operations/update_branches_test.go b/internal/gitaly/service/operations/update_branches_test.go index a6aeb2415da..f5ae0dcf1c2 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -30,7 +30,7 @@ func TestSuccessfulUserUpdateBranchRequest(t *testing.T) { } 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) @@ -65,6 +65,22 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { 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"), + }, } for _, testCase := range testCases { @@ -85,6 +101,9 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { require.NoError(t, err) require.Equal(t, string(testCase.newRev), branchCommit.Id) + + branches := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "for-each-ref", "--", "refs/heads/"+branchName) + require.Contains(t, string(branches), "refs/heads/"+branchName) }) } } -- GitLab From f50dae9c062915702657f5508f1b2944604e32d0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 09:54:50 +0100 Subject: [PATCH 07/10] UserUpdateBranch in Go: add basic skeleton As in 6f1046db5 (GoUserCreateTag: add basic skeleton, 2020-12-04) add the boilerplate needed for UserCreateBranch in Go first. --- .../gitaly/service/operations/branches.go | 7 ++++++ .../operations/update_branches_test.go | 23 ++++++++++--------- .../metadata/featureflag/feature_flags.go | 3 +++ 3 files changed, 22 insertions(+), 11 deletions(-) diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index 460a71df3fa..d64d191230c 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -93,6 +93,13 @@ 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.userUpdateBranchRuby(ctx, req) +} + +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 f5ae0dcf1c2..f31641202b3 100644 --- a/internal/gitaly/service/operations/update_branches_test.go +++ b/internal/gitaly/service/operations/update_branches_test.go @@ -26,6 +26,7 @@ var ( func TestSuccessfulUserUpdateBranchRequest(t *testing.T) { testhelper.NewFeatureSets([]featureflag.FeatureFlag{ featureflag.ReferenceTransactions, + featureflag.GoUserUpdateBranch, }).Run(t, testSuccessfulUserUpdateBranchRequest) } @@ -109,9 +110,10 @@ func testSuccessfulUserUpdateBranchRequest(t *testing.T, ctx context.Context) { } func TestSuccessfulUserUpdateBranchRequestToDelete(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testWithFeature(t, featureflag.GoUserUpdateBranch, testSuccessfulUserUpdateBranchRequestToDelete) +} +func testSuccessfulUserUpdateBranchRequestToDelete(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -185,10 +187,7 @@ func TestSuccessfulUserUpdateBranchRequestToDelete(t *testing.T) { } func TestSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - testSuccessfulGitHooksForUserUpdateBranchRequest(t, ctx) + testWithFeature(t, featureflag.GoUserUpdateBranch, testSuccessfulGitHooksForUserUpdateBranchRequest) } func testSuccessfulGitHooksForUserUpdateBranchRequest(t *testing.T, ctx context.Context) { @@ -227,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() @@ -267,6 +267,10 @@ func TestFailedUserUpdateBranchDueToHooks(t *testing.T) { } func TestFailedUserUpdateBranchRequest(t *testing.T) { + testWithFeature(t, featureflag.GoUserUpdateBranch, testFailedUserUpdateBranchRequest) +} + +func testFailedUserUpdateBranchRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -398,9 +402,6 @@ 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) diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 9071c36b3cc..6b895c2346c 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, -- GitLab From 1ad5983dce1b04b2ee9e5fff3278f5fa225978d9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 20 Jan 2021 15:01:01 +0100 Subject: [PATCH 08/10] Git library: add a IsZeroOID() helper This makes "something == git.ZeroOID.String()" slightly easier to read as "git.ObjectID(something).IsZeroOID". --- internal/git/id.go | 5 ++++ internal/git/id_test.go | 23 +++++++++++++++++++ .../gitaly/service/repository/raw_changes.go | 8 +++---- 3 files changed, 32 insertions(+), 4 deletions(-) diff --git a/internal/git/id.go b/internal/git/id.go index cc058ef4a14..ff6bc716443 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 0ab6f0dd9fe..f20a3ac63b6 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/repository/raw_changes.go b/internal/gitaly/service/repository/raw_changes.go index 2eab8825dcc..b9bbd8fe9d5 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() } -- GitLab From 021ffce4ca1f30a76feb0d1b0ec0a67042bb853a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 18 Jan 2021 13:06:47 +0100 Subject: [PATCH 09/10] Port UserUpdateBranch to Go Port UserUpdateBranch to Go, resolving [1]. See the UserCreateTag [2] for for similar recent code. 1. https://gitlab.com/gitlab-org/gitaly/-/issues/3066 2. 649c7a793 (Port UserCreateTag to Go, 2020-12-04) --- .../avar-user-update-branch-in-go.yml | 5 ++ .../gitaly/service/operations/branches.go | 70 ++++++++++++++++++- 2 files changed, 74 insertions(+), 1 deletion(-) create mode 100644 changelogs/unreleased/avar-user-update-branch-in-go.yml 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 00000000000..c8314970ee1 --- /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/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index d64d191230c..ca22f53349f 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -96,7 +96,75 @@ func (s *Server) UserUpdateBranch(ctx context.Context, req *gitalypb.UserUpdateB if featureflag.IsDisabled(ctx, featureflag.GoUserUpdateBranch) { return s.userUpdateBranchRuby(ctx, req) } - 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) + referenceValue, err := git.NewRepository(req.Repository, s.cfg).GetReference(ctx, git.ReferenceName(referenceName)) + if err != nil && !errors.Is(err, git.ErrReferenceNotFound) { + return nil, status.Error(codes.Internal, err.Error()) + } + + 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 + } + + couldNotUpdateError := status.Errorf(codes.FailedPrecondition, "Could not update %s. Please refresh and try again.", req.BranchName) + var updateRefError updateRefError + if errors.As(err, &updateRefError) { + if !git.ObjectID(req.Oldrev).IsZeroOID() && + !git.ObjectID(referenceValue.Target).IsZeroOID() { + // 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, couldNotUpdateError + } + + // Ditto Gitlab::Git::CommitError case in Ruby + return nil, couldNotUpdateError + } + + // Ditto Gitlab::Git::CommitError case in Ruby + return nil, couldNotUpdateError + } + + return &gitalypb.UserUpdateBranchResponse{}, nil } func (s *Server) userUpdateBranchRuby(ctx context.Context, req *gitalypb.UserUpdateBranchRequest) (*gitalypb.UserUpdateBranchResponse, error) { -- GitLab From f108ee331b30a2eea2419515336b3917ad43370e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Fri, 22 Jan 2021 14:57:48 +0100 Subject: [PATCH 10/10] Simplify GoUserUpdateBranch in Go error handling Instead of checking early if the reference is found we can just proceed and let update-ref fail. And as noted in [1] and [2] much of the rest of this can be simplified. I think I might need to bring some of this back for [3], so I'm doing this as a commit on top instead of squashing it in. 1. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3013#note_491544309 2. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/3013#note_491544302 3. https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2954 --- .../gitaly/service/operations/branches.go | 35 ++++--------------- 1 file changed, 7 insertions(+), 28 deletions(-) diff --git a/internal/gitaly/service/operations/branches.go b/internal/gitaly/service/operations/branches.go index ca22f53349f..a7cac180d3b 100644 --- a/internal/gitaly/service/operations/branches.go +++ b/internal/gitaly/service/operations/branches.go @@ -126,11 +126,6 @@ func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdat } referenceName := fmt.Sprintf("refs/heads/%s", req.BranchName) - referenceValue, err := git.NewRepository(req.Repository, s.cfg).GetReference(ctx, git.ReferenceName(referenceName)) - if err != nil && !errors.Is(err, git.ErrReferenceNotFound) { - return nil, status.Error(codes.Internal, err.Error()) - } - 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) { @@ -139,29 +134,13 @@ func (s *Server) userUpdateBranchGo(ctx context.Context, req *gitalypb.UserUpdat }, nil } - couldNotUpdateError := status.Errorf(codes.FailedPrecondition, "Could not update %s. Please refresh and try again.", req.BranchName) - var updateRefError updateRefError - if errors.As(err, &updateRefError) { - if !git.ObjectID(req.Oldrev).IsZeroOID() && - !git.ObjectID(referenceValue.Target).IsZeroOID() { - // 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, couldNotUpdateError - } - - // Ditto Gitlab::Git::CommitError case in Ruby - return nil, couldNotUpdateError - } - - // Ditto Gitlab::Git::CommitError case in Ruby - return nil, couldNotUpdateError + // 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 -- GitLab