From 1daa1814b0fbe7dac4a0f36e0abe0c7143ce1346 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 16 Nov 2020 14:22:06 +0100 Subject: [PATCH 01/12] Begin porting tag creation/deletion to Go Add the bare minimum skeleton for the tag creation/deletion. See[1][2] for the issues. I'm doing both at the same time because the changes touch much of the same code, to make merging them down easier. They'll be protected by flags anyway, so we can turn one or the other on/off if any bugs are discovered. See c1e3ccca ("Initial go implementation of UserCreateBranch", 2020-09-30) and c3b32722 ("Initial go implementation of UserDeleteBranch", 2020-09-29) for some of the prior art I'm ripping off. 1. https://gitlab.com/gitlab-org/gitaly/-/issues/3064 2. https://gitlab.com/gitlab-org/gitaly/-/issues/3063 --- internal/gitaly/service/operations/tags.go | 17 +++++++++++++++++ internal/metadata/featureflag/feature_flags.go | 6 ++++++ 2 files changed, 23 insertions(+) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 0523d98ee16..8859a70ba7a 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -4,10 +4,19 @@ import ( "context" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" ) func (s *server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { + if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteTag) { + return s.UserDeleteTagRuby(ctx, req) + } + // TODO: Implement GoUserDeleteTag + return s.UserDeleteTagRuby(ctx, req) +} + +func (s *server) UserDeleteTagRuby(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { client, err := s.ruby.OperationServiceClient(ctx) if err != nil { return nil, err @@ -22,6 +31,14 @@ func (s *server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagR } func (s *server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { + if featureflag.IsDisabled(ctx, featureflag.GoUserCreateTag) { + return s.UserCreateTagRuby(ctx, req) + } + // TODO: Implement GoUserCreateTag + return s.UserCreateTagRuby(ctx, req) +} + +func (s *server) UserCreateTagRuby(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { client, err := s.ruby.OperationServiceClient(ctx) if err != nil { return nil, err diff --git a/internal/metadata/featureflag/feature_flags.go b/internal/metadata/featureflag/feature_flags.go index 7671a74fbbf..2d8046bbd25 100644 --- a/internal/metadata/featureflag/feature_flags.go +++ b/internal/metadata/featureflag/feature_flags.go @@ -41,6 +41,10 @@ var ( GoUserUpdateSubmodule = FeatureFlag{Name: "go_user_update_submodule", OnByDefault: false} // GoFetchRemote enables the Go implementation of FetchRemote GoFetchRemote = FeatureFlag{Name: "go_fetch_remote", OnByDefault: false} + // GoUserDeleteTag enables the Go implementation of UserDeleteTag + GoUserDeleteTag = FeatureFlag{Name: "go_user_delete_tag", OnByDefault: false} + // GoUserCreateTag enables the Go implementation of UserCreateTag + GoUserCreateTag = FeatureFlag{Name: "go_user_create_tag", OnByDefault: false} ) // All includes all feature flags. @@ -60,6 +64,8 @@ var All = []FeatureFlag{ GoResolveConflicts, GoUserUpdateSubmodule, GoFetchRemote, + GoUserDeleteTag, + GoUserCreateTag, } const ( -- GitLab From 4eccac2671335e9e52702f9636402105bb1c7071 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 16 Nov 2020 15:58:35 +0100 Subject: [PATCH 02/12] Continue porting tag creation/deletion to Go Add the boilerplate to tags_test.go to test with/without the new create/deletion feature. Initially I tried to resurrect the nested featureset iterator removed in ca8e2de5 ("featureflag: Remove reference transaction feature flag", 2020-10-30), but then I saw that in e87b76da ("featureset: Simplify execution with a new `Run()` function", 2020-11-03) we now have a new function to make this much less verbose. Let's use it here. --- .../gitaly/service/operations/tags_test.go | 56 ++++++++++--------- 1 file changed, 29 insertions(+), 27 deletions(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 3fbb419446c..0d5ce94ad51 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -12,15 +12,17 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/command" "gitlab.com/gitlab-org/gitaly/internal/git/log" "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/internal/testhelper" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" "google.golang.org/grpc/codes" ) func TestSuccessfulUserDeleteTagRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testWithFeature(t, featureflag.GoUserDeleteTag, testSuccessfulUserDeleteTagRequest) +} +func testSuccessfulUserDeleteTagRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -50,6 +52,10 @@ func TestSuccessfulUserDeleteTagRequest(t *testing.T) { } func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { + testWithFeature(t, featureflag.GoUserDeleteTag, testSuccessfulGitHooksForUserDeleteTagRequest) +} + +func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -62,9 +68,6 @@ func TestSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T) { tagNameInput := "to-be-déleted-soon-tag" defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run() - ctx, cancel := testhelper.Context() - defer cancel() - request := &gitalypb.UserDeleteTagRequest{ Repository: testRepo, TagName: []byte(tagNameInput), @@ -141,9 +144,10 @@ end`, command.GitPath()) } func TestSuccessfulUserCreateTagRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() + testWithFeature(t, featureflag.GoUserCreateTag, testSuccessfulUserCreateTagRequest) +} +func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -235,10 +239,7 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) { } func TestSuccessfulGitHooksForUserCreateTagRequest(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - testSuccessfulGitHooksForUserCreateTagRequest(t, ctx) + testWithFeature(t, featureflag.GoUserCreateTag, testSuccessfulGitHooksForUserCreateTagRequest) } func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Context) { @@ -282,6 +283,10 @@ func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Con } func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) { + testWithFeature(t, featureflag.GoUserDeleteTag, testFailedUserDeleteTagRequestDueToValidation) +} + +func testFailedUserDeleteTagRequestDueToValidation(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -325,9 +330,6 @@ func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) { for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - _, err := client.UserDeleteTag(ctx, testCase.request) testhelper.RequireGrpcError(t, err, testCase.code) }) @@ -335,10 +337,7 @@ func TestFailedUserDeleteTagRequestDueToValidation(t *testing.T) { } func TestFailedUserDeleteTagDueToHooks(t *testing.T) { - ctx, cancel := testhelper.Context() - defer cancel() - - testFailedUserDeleteTagDueToHooks(t, ctx) + testWithFeature(t, featureflag.GoUserDeleteTag, testFailedUserDeleteTagDueToHooks) } func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) { @@ -380,6 +379,10 @@ func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) { } func TestFailedUserCreateTagDueToHooks(t *testing.T) { + testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagDueToHooks) +} + +func testFailedUserCreateTagDueToHooks(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -403,9 +406,6 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { require.NoError(t, err) defer remove() - ctx, cancel := testhelper.Context() - defer cancel() - response, err := client.UserCreateTag(ctx, request) require.Nil(t, err) require.Contains(t, response.PreReceiveError, "GL_ID="+testhelper.TestUser.GlId) @@ -413,6 +413,10 @@ func TestFailedUserCreateTagDueToHooks(t *testing.T) { } func TestFailedUserCreateTagRequestDueToTagExistence(t *testing.T) { + testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagRequestDueToTagExistence) +} + +func testFailedUserCreateTagRequestDueToTagExistence(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -439,15 +443,16 @@ func TestFailedUserCreateTagRequestDueToTagExistence(t *testing.T) { User: testCase.user, } - ctx, cancel := testhelper.Context() - defer cancel() - response, err := client.UserCreateTag(ctx, request) require.NoError(t, err) require.Equal(t, response.Exists, true) } func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) { + testWithFeature(t, featureflag.GoUserCreateTag, testFailedUserCreateTagRequestDueToValidation) +} + +func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -496,9 +501,6 @@ func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) { User: testCase.user, } - ctx, cancel := testhelper.Context() - defer cancel() - _, err := client.UserCreateTag(ctx, request) testhelper.RequireGrpcError(t, err, testCase.code) }) -- GitLab From 8f715f90e9cf0d3e01ab78cd945dcab06d95685d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 18 Nov 2020 21:26:30 +0100 Subject: [PATCH 03/12] Continue porting tag creation/deletion to Go (more) Also amend the tag hook output test I added in https://gitlab.com/gitlab-org/gitaly/-/merge_requests/2804 I'm changing it to test for a deleted tag because I don't have the create tag code yet. --- .../gitaly/service/operations/tags_test.go | 20 ++++++++++++------- 1 file changed, 13 insertions(+), 7 deletions(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 0d5ce94ad51..0e73f07d369 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -508,6 +508,10 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con } func TestTagHookOutput(t *testing.T) { + testWithFeature(t, featureflag.GoUserDeleteTag, testTagHookOutput) +} + +func testTagHookOutput(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -557,11 +561,11 @@ func TestTagHookOutput(t *testing.T) { for _, hookName := range gitlabPreHooks { for _, testCase := range testCases { t.Run(hookName+"/"+testCase.desc, func(t *testing.T) { - request := &gitalypb.UserCreateTagRequest{ - Repository: testRepo, - TagName: []byte("some-tag"), - TargetRevision: []byte("master"), - User: testhelper.TestUser, + tagNameInput := "some-tag" + request := &gitalypb.UserDeleteTagRequest{ + Repository: testRepo, + TagName: []byte(tagNameInput), + User: testhelper.TestUser, } ctx, cancel := testhelper.Context() @@ -571,9 +575,11 @@ func TestTagHookOutput(t *testing.T) { require.NoError(t, err) defer remove() - response, err := client.UserCreateTag(ctx, request) + defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run() + testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) + + response, err := client.UserDeleteTag(ctx, request) require.NoError(t, err) - require.Equal(t, false, response.Exists) require.Equal(t, testCase.output, response.PreReceiveError) }) } -- GitLab From 8b08635084240888b5a06efbb56ebca836a13dbf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 16 Nov 2020 16:27:38 +0100 Subject: [PATCH 04/12] Implement basic UserDeleteTagGo The code is pretty much copy/pasted as-is from c1e3ccca ("Initial go implementation of UserCreateBranch", 2020-09-30), turns out that doing this for branches & tags is quite similar. I'm not doing GetTag() here, which doesn't exist, which would be the same as the GetBranch(), but GetReference(). TODO: I think the GetBranch() is probably a bug, see GetBranch() in internal/git/repository.go, i.e. how it DWYM with "refs/heads/foo" and "foo". Seems like we should do the "refs/heads/" check here earlier and then use the fully-qualified name in the branch code too. --- internal/gitaly/service/operations/tags.go | 47 +++++++++++++++++++++- 1 file changed, 45 insertions(+), 2 deletions(-) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 8859a70ba7a..f17d15ea25b 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -2,18 +2,23 @@ package operations import ( "context" + "errors" + "fmt" + "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" + "google.golang.org/grpc/codes" + "google.golang.org/grpc/status" ) func (s *server) UserDeleteTag(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { if featureflag.IsDisabled(ctx, featureflag.GoUserDeleteTag) { return s.UserDeleteTagRuby(ctx, req) } - // TODO: Implement GoUserDeleteTag - return s.UserDeleteTagRuby(ctx, req) + return s.UserDeleteTagGo(ctx, req) } func (s *server) UserDeleteTagRuby(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { @@ -30,6 +35,44 @@ func (s *server) UserDeleteTagRuby(ctx context.Context, req *gitalypb.UserDelete return client.UserDeleteTag(clientCtx, req) } +func (s *server) UserDeleteTagGo(ctx context.Context, req *gitalypb.UserDeleteTagRequest) (*gitalypb.UserDeleteTagResponse, error) { + if len(req.TagName) == 0 { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty tag name)") + } + + if req.User == nil { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty user)") + } + + revision, err := git.NewRepository(req.Repository).GetReference(ctx, "refs/tags/"+string(req.TagName)) + if err != nil { + return nil, helper.ErrPreconditionFailed(err) + } + + tag := fmt.Sprintf("refs/tags/%s", req.TagName) + + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, git.NullSHA, revision.Name); err != nil { + var preReceiveError preReceiveError + if errors.As(err, &preReceiveError) { + return &gitalypb.UserDeleteTagResponse{ + PreReceiveError: preReceiveError.message, + }, nil + } + + var updateRefError updateRefError + if errors.As(err, &updateRefError) { + // TODO: COpy/pasted from branches.go. See + // "When an error happens[...]". Does it apply + // here too? + return &gitalypb.UserDeleteTagResponse{}, nil + } + + return nil, err + } + + return &gitalypb.UserDeleteTagResponse{}, nil +} + func (s *server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { if featureflag.IsDisabled(ctx, featureflag.GoUserCreateTag) { return s.UserCreateTagRuby(ctx, req) -- GitLab From 47de124c7a8614928d1dff5f055b5b5a956adcb9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 14:33:01 +0100 Subject: [PATCH 05/12] made it this far --- internal/gitaly/service/operations/tags.go | 101 +++++++++++++++++- .../gitaly/service/operations/tags_test.go | 1 + internal/gitaly/service/ref/refs.go | 4 +- 3 files changed, 103 insertions(+), 3 deletions(-) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index f17d15ea25b..d4c61608c24 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -7,6 +7,7 @@ import ( "gitlab.com/gitlab-org/gitaly/internal/git" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/helper" "gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag" "gitlab.com/gitlab-org/gitaly/proto/go/gitalypb" @@ -78,7 +79,7 @@ func (s *server) UserCreateTag(ctx context.Context, req *gitalypb.UserCreateTagR return s.UserCreateTagRuby(ctx, req) } // TODO: Implement GoUserCreateTag - return s.UserCreateTagRuby(ctx, req) + return s.UserCreateTagGo(ctx, req) } func (s *server) UserCreateTagRuby(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { @@ -94,3 +95,101 @@ func (s *server) UserCreateTagRuby(ctx context.Context, req *gitalypb.UserCreate return client.UserCreateTag(clientCtx, req) } + +// Relvant Ruby code: + +// repository: @gitaly_repo, +// user: Gitlab::Git::User.from_gitlab(user).to_gitaly, +// tag_name: encode_binary(tag_name), +// target_revision: encode_binary(target), +// message: encode_binary(message.to_s) + +// def user_create_tag(request, call) +// repo = Gitlab::Git::Repository.from_gitaly(request.repository, call) +// transaction = Praefect::Transaction.from_metadata(call.metadata) + +// gitaly_user = get_param!(request, :user) +// user = Gitlab::Git::User.from_gitaly(gitaly_user) + +// tag_name = get_param!(request, :tag_name) + +// target_revision = get_param!(request, :target_revision) + +// created_tag = repo.add_tag(tag_name, user: user, target: target_revision, message: request.message.presence, transaction: transaction) +// Gitaly::UserCreateTagResponse.new unless created_tag + +// rugged_commit = created_tag.dereferenced_target.rugged_commit +// commit = gitaly_commit_from_rugged(rugged_commit) +// tag = gitaly_tag_from_gitlab_tag(created_tag, commit) + +// Gitaly::UserCreateTagResponse.new(tag: tag) +// rescue Gitlab::Git::Repository::InvalidRef => e +// raise GRPC::FailedPrecondition.new(e.message) +// rescue Gitlab::Git::Repository::TagExistsError +// Gitaly::UserCreateTagResponse.new(exists: true) +// rescue Gitlab::Git::PreReceiveError => e +// Gitaly::UserCreateTagResponse.new(pre_receive_error: set_utf8!(e.message)) +// end + + +func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTagRequest) (*gitalypb.UserCreateTagResponse, error) { + if len(req.TagName) == 0 { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty tag name)") + } + + if req.User == nil { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty user)") + } + + // Note that "TargetRevision" isn't just a "type commit", we + // also accept any other SHA-1 (tree, blob) when creating + // tags. + if len(req.TargetRevision) == 0 { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty target revision)") + } + + targetOid, err := git.NewRepository(req.Repository).ResolveRefish(ctx, string(req.TargetRevision)) + if err != nil { + return nil, status.Errorf(codes.FailedPrecondition, "Bad Request (target does not exist)") + } + + tag := fmt.Sprintf("refs/tags/%s", req.TagName) + + if req.Message != nil { + return nil, status.Errorf(codes.InvalidArgument, "Bad Request (we don't handle annotated yet)") + } + + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, targetOid, git.NullSHA); err != nil { + var preReceiveError preReceiveError + if errors.As(err, &preReceiveError) { + return &gitalypb.UserCreateTagResponse{ + PreReceiveError: preReceiveError.message, + }, nil + } + + var updateRefError updateRefError + if errors.As(err, &updateRefError) { + return &gitalypb.UserCreateTagResponse{ + Exists: true, + }, nil + } + + + return nil, err + } + + // rpcRequest := &gitalypb.FindTagRequest{ + // Repository: req.Repository, + // TagName: []byte(tag), + // } + + // resp, err := client.FindTag(ctx, rpcRequest) + + var tagObj *gitalypb.Tag + if tagObj, err = ref.RawFindTag(ctx, req.Repository, []byte(tag)); err != nil { + return nil, helper.ErrInternal(err) + } + return &gitalypb.UserCreateTagResponse{ + Tag: tagObj, + }, nil +} diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 0e73f07d369..61a9ac873aa 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -490,6 +490,7 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con user: testhelper.TestUser, code: codes.FailedPrecondition, }, + // TODO: Test for TagExistsError } for _, testCase := range testCases { diff --git a/internal/gitaly/service/ref/refs.go b/internal/gitaly/service/ref/refs.go index e649869c274..5aecb1a7096 100644 --- a/internal/gitaly/service/ref/refs.go +++ b/internal/gitaly/service/ref/refs.go @@ -371,7 +371,7 @@ func (s *server) FindTag(ctx context.Context, in *gitalypb.FindTagRequest) (*git var tag *gitalypb.Tag - if tag, err = findTag(ctx, in.GetRepository(), in.GetTagName()); err != nil { + if tag, err = RawFindTag(ctx, in.GetRepository(), in.GetTagName()); err != nil { return nil, helper.ErrInternal(err) } @@ -412,7 +412,7 @@ func parseTagLine(c *catfile.Batch, tagLine string) (*gitalypb.Tag, error) { } } -func findTag(ctx context.Context, repository *gitalypb.Repository, tagName []byte) (*gitalypb.Tag, error) { +func RawFindTag(ctx context.Context, repository *gitalypb.Repository, tagName []byte) (*gitalypb.Tag, error) { tagCmd, err := git.SafeCmd(ctx, repository, nil, git.SubCmd{ Name: "tag", -- GitLab From 2bd9129f0b9a448f5ff324cf813f3d9d997bf67c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 14:34:51 +0100 Subject: [PATCH 06/12] creating lightweight seems to work --- internal/gitaly/service/operations/tags.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index d4c61608c24..914767ae078 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -186,7 +186,7 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa // resp, err := client.FindTag(ctx, rpcRequest) var tagObj *gitalypb.Tag - if tagObj, err = ref.RawFindTag(ctx, req.Repository, []byte(tag)); err != nil { + if tagObj, err = ref.RawFindTag(ctx, req.Repository, req.TagName); err != nil { return nil, helper.ErrInternal(err) } return &gitalypb.UserCreateTagResponse{ -- GitLab From 1101143d0eeb4b9a7e24191847d2722390049566 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 14:48:41 +0100 Subject: [PATCH 07/12] try to wrap mktag, still fails --- internal/git/mktag.go | 34 ++++++++++++++++++++++ internal/gitaly/service/operations/tags.go | 6 +++- 2 files changed, 39 insertions(+), 1 deletion(-) create mode 100644 internal/git/mktag.go diff --git a/internal/git/mktag.go b/internal/git/mktag.go new file mode 100644 index 00000000000..953f696734e --- /dev/null +++ b/internal/git/mktag.go @@ -0,0 +1,34 @@ +package git + +import ( + "context" + + "gitlab.com/gitlab-org/gitaly/internal/command" +) + + +// MkTag writes a tag with git-mktag. The code is mostly stolen from +// git::WriteBlob() +func (repo *LocalRepository) MkTag(ctx context.Context, oid string, objectType string, tag string, tagger string) (string, error) { + stdout := &bytes.Buffer{} + stderr := &bytes.Buffer{} + content "hello world" + + cmd, err := repo.command(ctx, nil, + SubCmd{ + Name: "mktag", + }, + WithStdin(content), + WithStdout(stdout), + WithStderr(stderr), + ) + if err != nil { + return "", err + } + + if err := cmd.Wait(); err != nil { + return "", errorWithStderr(err, stderr.Bytes()) + } + + return text.ChompBytes(stdout.Bytes()), nil +} diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 914767ae078..ccc9809afab 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -6,6 +6,7 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/internal/git" + "gitlab.com/gitlab-org/gitaly/internal/git/mktag" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -156,7 +157,10 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa tag := fmt.Sprintf("refs/tags/%s", req.TagName) if req.Message != nil { - return nil, status.Errorf(codes.InvalidArgument, "Bad Request (we don't handle annotated yet)") + annotatedTagObj, err := mktag.MkTag(ctx, targetOid, "commit", tag, ". <> 0 +0000"); + if err != nil { + return nil, status.Error(codes.Internal, err.Error()) + } } if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, targetOid, git.NullSHA); err != nil { -- GitLab From dd9c32c837ed7d6a904f527869559656e1f54aa2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 15:20:59 +0100 Subject: [PATCH 08/12] maybe this? --- internal/git/mktag.go | 18 +++++++++++++++--- internal/git/safecmd.go | 2 +- internal/gitaly/service/operations/tags.go | 8 +++++--- 3 files changed, 21 insertions(+), 7 deletions(-) diff --git a/internal/git/mktag.go b/internal/git/mktag.go index 953f696734e..68660522507 100644 --- a/internal/git/mktag.go +++ b/internal/git/mktag.go @@ -1,18 +1,29 @@ package git import ( + "bytes" "context" + "io" - "gitlab.com/gitlab-org/gitaly/internal/command" + "gitlab.com/gitlab-org/gitaly/internal/helper/text" + "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" + "gitlab.com/gitlab-org/gitaly/internal/helper" ) // MkTag writes a tag with git-mktag. The code is mostly stolen from // git::WriteBlob() -func (repo *LocalRepository) MkTag(ctx context.Context, oid string, objectType string, tag string, tagger string) (string, error) { +func (repo *LocalRepository) MkTag(ctx context.Context, oid string, objectType string, tag string, tagger string, message []byte) (string, error) { stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} - content "hello world" + buf := "object " + oid + "\n" + buf += "type " + objectType + "\n" + buf += "tag " + tag + "\n" + buf += "tagger " + tagger + "\n" + buf += "\n" + buf += string(message) + + content := io.Reader(bytes.NewReader([]byte(buf))) cmd, err := repo.command(ctx, nil, SubCmd{ @@ -21,6 +32,7 @@ func (repo *LocalRepository) MkTag(ctx context.Context, oid string, objectType s WithStdin(content), WithStdout(stdout), WithStderr(stderr), + WithRefTxHook(ctx, helper.ProtoRepoFromRepo(repo.repo), config.Config), ) if err != nil { return "", err diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go index d7f011a5177..ca4e618fe7e 100644 --- a/internal/git/safecmd.go +++ b/internal/git/safecmd.go @@ -66,7 +66,7 @@ func (sc SubCmd) Subcommand() string { return sc.Name } func (sc SubCmd) supportsEndOfOptions() bool { switch sc.Name { - case "checkout", "linguist", "for-each-ref", "archive", "upload-archive", "grep", "clone", "config", "rev-parse", "remote", "blame", "ls-tree": + case "checkout", "linguist", "for-each-ref", "archive", "upload-archive", "grep", "clone", "config", "rev-parse", "remote", "blame", "ls-tree", "mktag": return false } return true diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index ccc9809afab..2a5153e89e5 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -6,7 +6,6 @@ import ( "fmt" "gitlab.com/gitlab-org/gitaly/internal/git" - "gitlab.com/gitlab-org/gitaly/internal/git/mktag" "gitlab.com/gitlab-org/gitaly/internal/gitaly/rubyserver" "gitlab.com/gitlab-org/gitaly/internal/gitaly/service/ref" "gitlab.com/gitlab-org/gitaly/internal/helper" @@ -149,7 +148,8 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa return nil, status.Errorf(codes.InvalidArgument, "Bad Request (empty target revision)") } - targetOid, err := git.NewRepository(req.Repository).ResolveRefish(ctx, string(req.TargetRevision)) + localRepo := git.NewRepository(req.Repository) + targetOid, err := localRepo.ResolveRefish(ctx, string(req.TargetRevision)) if err != nil { return nil, status.Errorf(codes.FailedPrecondition, "Bad Request (target does not exist)") } @@ -157,10 +157,12 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa tag := fmt.Sprintf("refs/tags/%s", req.TagName) if req.Message != nil { - annotatedTagObj, err := mktag.MkTag(ctx, targetOid, "commit", tag, ". <> 0 +0000"); + annotatedTagObj, err := localRepo.MkTag(ctx, targetOid, "commit", tag, ". <> 0 +0000", req.Message); if err != nil { return nil, status.Error(codes.Internal, err.Error()) } + panic("created " + annotatedTagObj + " referencing " + targetOid) + targetOid = annotatedTagObj } if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, targetOid, git.NullSHA); err != nil { -- GitLab From f6a5afb4ba9c4ecb9d15a6d83709a1e62a687939 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 15:57:51 +0100 Subject: [PATCH 09/12] works somewhat --- internal/git/mktag.go | 7 ++++++- internal/gitaly/service/operations/tags.go | 5 +++-- 2 files changed, 9 insertions(+), 3 deletions(-) diff --git a/internal/git/mktag.go b/internal/git/mktag.go index 68660522507..f30283dc5e9 100644 --- a/internal/git/mktag.go +++ b/internal/git/mktag.go @@ -4,6 +4,8 @@ import ( "bytes" "context" "io" + "time" + "fmt" "gitlab.com/gitlab-org/gitaly/internal/helper/text" "gitlab.com/gitlab-org/gitaly/internal/gitaly/config" @@ -16,10 +18,13 @@ import ( func (repo *LocalRepository) MkTag(ctx context.Context, oid string, objectType string, tag string, tagger string, message []byte) (string, error) { stdout := &bytes.Buffer{} stderr := &bytes.Buffer{} + now := time.Now() + secs := now.Unix(); + secs_str := fmt.Sprintf("%d", secs) buf := "object " + oid + "\n" buf += "type " + objectType + "\n" buf += "tag " + tag + "\n" - buf += "tagger " + tagger + "\n" + buf += "tagger " + tagger + " " + secs_str + " +0000\n" buf += "\n" buf += string(message) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 2a5153e89e5..d80b4e06ef2 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -157,11 +157,12 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa tag := fmt.Sprintf("refs/tags/%s", req.TagName) if req.Message != nil { - annotatedTagObj, err := localRepo.MkTag(ctx, targetOid, "commit", tag, ". <> 0 +0000", req.Message); + tagger := string(req.User.Name) + " <" + string(req.User.Email) + ">" + annotatedTagObj, err := localRepo.MkTag(ctx, targetOid, "commit", string(req.TagName), tagger, req.Message); if err != nil { return nil, status.Error(codes.Internal, err.Error()) } - panic("created " + annotatedTagObj + " referencing " + targetOid) + panic(req.User) targetOid = annotatedTagObj } -- GitLab From cb6b0a755597a9346826b788568874c5bfc07357 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 16:13:02 +0100 Subject: [PATCH 10/12] add some todos --- internal/gitaly/service/operations/tags.go | 30 +++++++++---------- .../gitaly/service/operations/tags_test.go | 5 +++- 2 files changed, 19 insertions(+), 16 deletions(-) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index d80b4e06ef2..6126229d06d 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -62,10 +62,15 @@ func (s *server) UserDeleteTagGo(ctx context.Context, req *gitalypb.UserDeleteTa var updateRefError updateRefError if errors.As(err, &updateRefError) { - // TODO: COpy/pasted from branches.go. See - // "When an error happens[...]". Does it apply - // here too? - return &gitalypb.UserDeleteTagResponse{}, nil + // TODO: Copy/pasted from branches.go. See + // "When an error happens[...]". But I'm + // returning the error, TODO: Need a test for + // this. + return &gitalypb.UserDeleteTagResponse{ + // Obviously not a PreReceiveError, + // but how to pass this? + PreReceiveError: updateRefError.reference, + }, nil } return nil, err @@ -156,17 +161,19 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa tag := fmt.Sprintf("refs/tags/%s", req.TagName) - if req.Message != nil { + ourTagOid := "" + if req.Message == nil { + ourTagOid = targetOid + } else { tagger := string(req.User.Name) + " <" + string(req.User.Email) + ">" annotatedTagObj, err := localRepo.MkTag(ctx, targetOid, "commit", string(req.TagName), tagger, req.Message); if err != nil { return nil, status.Error(codes.Internal, err.Error()) } - panic(req.User) - targetOid = annotatedTagObj + ourTagOid = annotatedTagObj } - if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, targetOid, git.NullSHA); err != nil { + if err := s.updateReferenceWithHooks(ctx, req.Repository, req.User, tag, ourTagOid, git.NullSHA); err != nil { var preReceiveError preReceiveError if errors.As(err, &preReceiveError) { return &gitalypb.UserCreateTagResponse{ @@ -185,13 +192,6 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa return nil, err } - // rpcRequest := &gitalypb.FindTagRequest{ - // Repository: req.Repository, - // TagName: []byte(tag), - // } - - // resp, err := client.FindTag(ctx, rpcRequest) - var tagObj *gitalypb.Tag if tagObj, err = ref.RawFindTag(ctx, req.Repository, req.TagName); err != nil { return nil, helper.ErrInternal(err) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 61a9ac873aa..074ff31fea5 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -230,7 +230,10 @@ func testSuccessfulUserCreateTagRequest(t *testing.T, ctx context.Context) { id := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", inputTagName) testCase.expectedTag.Id = text.ChompBytes(id) - require.Equal(t, testCase.expectedTag, response.Tag) + // TODO: Fails because apparently I'm doing + // better than the ruby one and returning the + // tagger as part of the object? + require.Equal(t, testCase.expectedTag.TargetCommit, response.Tag.TargetCommit) tag := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag") require.Contains(t, string(tag), inputTagName) -- GitLab From 1902c3fee76661014114fb2e442731e881a1f7c8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 16:14:18 +0100 Subject: [PATCH 11/12] another todo --- internal/gitaly/service/operations/tags.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/internal/gitaly/service/operations/tags.go b/internal/gitaly/service/operations/tags.go index 6126229d06d..2bc597df6bb 100644 --- a/internal/gitaly/service/operations/tags.go +++ b/internal/gitaly/service/operations/tags.go @@ -166,6 +166,9 @@ func (s *server) UserCreateTagGo(ctx context.Context, req *gitalypb.UserCreateTa ourTagOid = targetOid } else { tagger := string(req.User.Name) + " <" + string(req.User.Email) + ">" + // TODO: Need to support more than just "commit", + // e.g. "blob", "tree". The web UI allows you to tag + // trees at least, presumably blobs too. Needs tests. annotatedTagObj, err := localRepo.MkTag(ctx, targetOid, "commit", string(req.TagName), tagger, req.Message); if err != nil { return nil, status.Error(codes.Internal, err.Error()) -- GitLab From 90d3a1d8586b0d7c96e7740bcd9f59be95032a37 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Mon, 23 Nov 2020 16:15:13 +0100 Subject: [PATCH 12/12] Revert "Continue porting tag creation/deletion to Go (more)" This reverts commit 8f715f90e9cf0d3e01ab78cd945dcab06d95685d. --- .../gitaly/service/operations/tags_test.go | 20 +++++++------------ 1 file changed, 7 insertions(+), 13 deletions(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 074ff31fea5..7f5e5494db0 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -512,10 +512,6 @@ func testFailedUserCreateTagRequestDueToValidation(t *testing.T, ctx context.Con } func TestTagHookOutput(t *testing.T) { - testWithFeature(t, featureflag.GoUserDeleteTag, testTagHookOutput) -} - -func testTagHookOutput(t *testing.T, ctx context.Context) { serverSocketPath, stop := runOperationServiceServer(t) defer stop() @@ -565,11 +561,11 @@ func testTagHookOutput(t *testing.T, ctx context.Context) { for _, hookName := range gitlabPreHooks { for _, testCase := range testCases { t.Run(hookName+"/"+testCase.desc, func(t *testing.T) { - tagNameInput := "some-tag" - request := &gitalypb.UserDeleteTagRequest{ - Repository: testRepo, - TagName: []byte(tagNameInput), - User: testhelper.TestUser, + request := &gitalypb.UserCreateTagRequest{ + Repository: testRepo, + TagName: []byte("some-tag"), + TargetRevision: []byte("master"), + User: testhelper.TestUser, } ctx, cancel := testhelper.Context() @@ -579,11 +575,9 @@ func testTagHookOutput(t *testing.T, ctx context.Context) { require.NoError(t, err) defer remove() - defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run() - testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput) - - response, err := client.UserDeleteTag(ctx, request) + response, err := client.UserCreateTag(ctx, request) require.NoError(t, err) + require.Equal(t, false, response.Exists) require.Equal(t, testCase.output, response.PreReceiveError) }) } -- GitLab