diff options
| -rw-r--r-- | changelogs/unreleased/jv-bug-2541.yml | 5 | ||||
| -rw-r--r-- | internal/praefect/nodes/local_elector.go | 2 | ||||
| -rw-r--r-- | internal/service/operations/tags_test.go | 35 | ||||
| -rw-r--r-- | internal/service/operations/testdata/assert_git_object_type.rb | 8 | ||||
| -rwxr-xr-x | internal/service/operations/testdata/pre-receive-expect-object-type | 12 | ||||
| -rwxr-xr-x | internal/service/operations/testdata/update-expect-object-type | 9 | ||||
| -rw-r--r-- | ruby/lib/gitlab/git/operation_service.rb | 20 | ||||
| -rw-r--r-- | ruby/lib/gitlab/git/repository.rb | 22 |
8 files changed, 89 insertions, 24 deletions
diff --git a/changelogs/unreleased/jv-bug-2541.yml b/changelogs/unreleased/jv-bug-2541.yml new file mode 100644 index 000000000..a48033f0d --- /dev/null +++ b/changelogs/unreleased/jv-bug-2541.yml @@ -0,0 +1,5 @@ +--- +title: 'UserCreateTag: pass tag object to hooks when creating annotated tag' +merge_request: 1956 +author: +type: fixed diff --git a/internal/praefect/nodes/local_elector.go b/internal/praefect/nodes/local_elector.go index 12308ab26..98a0d2a26 100644 --- a/internal/praefect/nodes/local_elector.go +++ b/internal/praefect/nodes/local_elector.go @@ -105,6 +105,8 @@ func (s *localElector) monitor(d time.Duration) { defer ticker.Stop() for { + <-ticker.C + ctx := context.Background() s.checkNodes(ctx) } diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go index fdc469233..d4ed59b3b 100644 --- a/internal/service/operations/tags_test.go +++ b/internal/service/operations/tags_test.go @@ -1,7 +1,10 @@ package operations import ( + "fmt" + "os" "os/exec" + "path/filepath" "testing" "github.com/stretchr/testify/require" @@ -125,12 +128,18 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) { } inputTagName := "to-be-créated-soon" + cwd, err := os.Getwd() + require.NoError(t, err) + preReceiveHook := filepath.Join(cwd, "testdata/pre-receive-expect-object-type") + updateHook := filepath.Join(cwd, "testdata/update-expect-object-type") + testCases := []struct { - desc string - tagName string - message string - targetRevision string - expectedTag *gitalypb.Tag + desc string + tagName string + message string + targetRevision string + expectedTag *gitalypb.Tag + expectedObjectType string }{ { desc: "lightweight tag", @@ -140,6 +149,7 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) { Name: []byte(inputTagName), TargetCommit: targetRevisionCommit, }, + expectedObjectType: "commit", }, { desc: "annotated tag", @@ -152,11 +162,21 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) { Message: []byte("This is an annotated tag"), MessageSize: 24, }, + expectedObjectType: "tag", }, } for _, testCase := range testCases { t.Run(testCase.desc, func(t *testing.T) { + for hook, content := range map[string]string{ + "pre-receive": fmt.Sprintf("#!/bin/sh\n%s %s \"$@\"", preReceiveHook, testCase.expectedObjectType), + "update": fmt.Sprintf("#!/bin/sh\n%s %s \"$@\"", updateHook, testCase.expectedObjectType), + } { + hookCleanup, err := testhelper.WriteCustomHook(testRepoPath, hook, []byte(content)) + require.NoError(t, err) + defer hookCleanup() + } + request := &gitalypb.UserCreateTagRequest{ Repository: testRepo, TagName: []byte(testCase.tagName), @@ -172,14 +192,15 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) { defer cancel() response, err := client.UserCreateTag(ctx, request) + require.NoError(t, err, "error from calling RPC") + require.Empty(t, response.PreReceiveError, "PreReceiveError must be empty, signalling the push was accepted") + defer exec.Command("git", "-C", testRepoPath, "tag", "-d", inputTagName).Run() id := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", inputTagName) testCase.expectedTag.Id = text.ChompBytes(id) - require.NoError(t, err) require.Equal(t, testCase.expectedTag, response.Tag) - require.Empty(t, response.PreReceiveError) tag := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag") require.Contains(t, string(tag), inputTagName) diff --git a/internal/service/operations/testdata/assert_git_object_type.rb b/internal/service/operations/testdata/assert_git_object_type.rb new file mode 100644 index 000000000..2e1be4b09 --- /dev/null +++ b/internal/service/operations/testdata/assert_git_object_type.rb @@ -0,0 +1,8 @@ +def assert_git_object_type!(oid, expected_type) + out = IO.popen(%W[git cat-file -t #{oid}], &:read) + abort 'cat-file failed' unless $?.success? + + unless out.chomp == expected_type + abort "error: expected #{expected_type} object, got #{out}" + end +end diff --git a/internal/service/operations/testdata/pre-receive-expect-object-type b/internal/service/operations/testdata/pre-receive-expect-object-type new file mode 100755 index 000000000..10c55de3a --- /dev/null +++ b/internal/service/operations/testdata/pre-receive-expect-object-type @@ -0,0 +1,12 @@ +#!/usr/bin/env ruby +require_relative 'assert_git_object_type.rb' + +commands = STDIN.each_line.map(&:chomp) +unless commands.size == 1 + abort "expected 1 ref update command, got #{commands.size}" +end + +new_value = commands[0].split(' ', 3)[1] +abort 'missing new_value' unless new_value + +assert_git_object_type!(new_value, ARGV[0]) diff --git a/internal/service/operations/testdata/update-expect-object-type b/internal/service/operations/testdata/update-expect-object-type new file mode 100755 index 000000000..8889e3e3c --- /dev/null +++ b/internal/service/operations/testdata/update-expect-object-type @@ -0,0 +1,9 @@ +#!/usr/bin/env ruby +require_relative 'assert_git_object_type.rb' + +expected_object_type = ARGV.shift +new_value = ARGV[2] + +abort "missing new_value" unless new_value + +assert_git_object_type!(new_value, expected_object_type) diff --git a/ruby/lib/gitlab/git/operation_service.rb b/ruby/lib/gitlab/git/operation_service.rb index 8c62a13b7..5be4d27e0 100644 --- a/ruby/lib/gitlab/git/operation_service.rb +++ b/ruby/lib/gitlab/git/operation_service.rb @@ -40,19 +40,19 @@ module Gitlab update_ref_in_hooks(ref, newrev, oldrev) end - def add_tag(tag_name, newrev, options = {}) + def add_lightweight_tag(tag_name, tag_target) ref = Gitlab::Git::TAG_REF_PREFIX + tag_name oldrev = Gitlab::Git::BLANK_SHA - with_hooks(ref, newrev, oldrev) do |service| - # We want to pass the OID of the tag object to the hooks. For an - # annotated tag we don't know that OID until after the tag object - # (raw_tag) is created in the repository. That is why we have to - # update the value after creating the tag object. Only the - # "post-receive" hook will receive the correct value in this case. - raw_tag = repository.rugged.tags.create(tag_name, newrev, options) - service.newrev = raw_tag.target_id - end + update_ref_in_hooks(ref, tag_target, oldrev) + end + + def add_annotated_tag(tag_name, tag_target, options) + ref = Gitlab::Git::TAG_REF_PREFIX + tag_name + oldrev = Gitlab::Git::BLANK_SHA + annotation = repository.rugged.tags.create_annotation(tag_name, tag_target, options) + + update_ref_in_hooks(ref, annotation.oid, oldrev) end def rm_tag(tag) diff --git a/ruby/lib/gitlab/git/repository.rb b/ruby/lib/gitlab/git/repository.rb index 0ca8daca2..dcc75e7b1 100644 --- a/ruby/lib/gitlab/git/repository.rb +++ b/ruby/lib/gitlab/git/repository.rb @@ -219,21 +219,29 @@ module Gitlab target_object = Ref.dereference_object(lookup(target)) raise InvalidRef, "target not found: #{target}" unless target_object - options = nil # Use nil, not the empty hash. Rugged cares about this. + target_oid = target_object.oid + operation_service = Gitlab::Git::OperationService.new(user, self) + if message - options = { + operation_service.add_annotated_tag( + tag_name, + target_oid, message: message, tagger: Gitlab::Git.committer_hash(email: user.email, name: user.name) - } + ) + else + operation_service.add_lightweight_tag(tag_name, target_oid) end - Gitlab::Git::OperationService.new(user, self).add_tag(tag_name, target_object.oid, options) - find_tag(tag_name) rescue Rugged::ReferenceError => ex raise InvalidRef, ex - rescue Rugged::TagError - raise TagExistsError + rescue Gitlab::Git::CommitError => ex + if find_tag(tag_name) + raise TagExistsError + else + raise ex + end end def update_branch(branch_name, user:, newrev:, oldrev:, push_options: nil) |