From b400c8c1ade2c9d44cffdaf979697aca86ff1f56 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 18 Nov 2020 20:12:53 +0100 Subject: [PATCH 1/3] tags tests: don't inconsistently pipe GL_ID=* to stderr Remove piping to stderr added in a7ac5fde ("Enable feature flag for go update hooks through operations service", 2020-04-28). This made it inconsistent with the corresponding branch test. This removes test coverage for the stdout v.s. stderr case, that'll be rectified in a follow-up commit which fixes a bug in that logic and tests for it explicitly. --- internal/gitaly/service/operations/tags_test.go | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 65ffefa7ffd..23412933068 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -361,7 +361,7 @@ func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) { User: testhelper.TestUser, } - hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID >&2\nexit 1") + hookContent := []byte("#!/bin/sh\necho GL_ID=$GL_ID\nexit 1") for _, hookName := range gitlabPreHooks { t.Run(hookName, func(t *testing.T) { -- GitLab From 9931076ba0a3ac84abcc6b719fd2ceaa47eb7ea5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Wed, 18 Nov 2020 20:15:02 +0100 Subject: [PATCH 2/3] updateReferenceWithHooks: add tests for hook stdout/stderr output Add a test copy/pasted between the branch & tags tests to test what happens when hooks emit stdout and/or stderr. These test currently fail on the branch GoUserCreateBranch feature. I'm not bothering to make them pass because the next commit will fix the bug. --- .../service/operations/branches_test.go | 73 +++++++++++++++++++ .../gitaly/service/operations/tags_test.go | 73 +++++++++++++++++++ 2 files changed, 146 insertions(+) diff --git a/internal/gitaly/service/operations/branches_test.go b/internal/gitaly/service/operations/branches_test.go index 4cd90be95bc..4caea6deeb6 100644 --- a/internal/gitaly/service/operations/branches_test.go +++ b/internal/gitaly/service/operations/branches_test.go @@ -518,3 +518,76 @@ func testFailedUserDeleteBranchDueToHooks(t *testing.T, ctx context.Context) { }) } } + +func TestBranchHookOutput(t *testing.T) { + testWithFeature(t, featureflag.GoUserCreateBranch, testBranchHookOutput) +} + +func testBranchHookOutput(t *testing.T, ctx context.Context) { + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + testCases := []struct { + desc string + hookContent string + output string + }{ + { + desc: "empty stdout and empty stderr", + hookContent: "#!/bin/sh\nexit 1", + output: "", + }, + { + desc: "empty stdout and some stderr", + hookContent: "#!/bin/sh\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "some stdout and empty stderr", + hookContent: "#!/bin/sh\necho stdout\nexit 1", + output: "stdout\n", + }, + { + desc: "some stdout and some stderr", + hookContent: "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "whitespace stdout and some stderr", + hookContent: "#!/bin/sh\necho ' '\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "some stdout and whitespace stderr", + hookContent: "#!/bin/sh\necho stdout\necho ' ' >&2\nexit 1", + output: "stdout\n", + }, + } + + for _, hookName := range gitlabPreHooks { + for _, testCase := range testCases { + t.Run(hookName+"/"+testCase.desc, func(t *testing.T) { + request := &gitalypb.UserCreateBranchRequest{ + Repository: testRepo, + BranchName: []byte("some-branch"), + StartPoint: []byte("master"), + User: testhelper.TestUser, + } + + remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, []byte(testCase.hookContent)) + require.NoError(t, err) + defer remove() + + response, err := client.UserCreateBranch(ctx, request) + require.NoError(t, err) + require.Equal(t, testCase.output, response.PreReceiveError) + }) + } + } +} diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 23412933068..3fbb419446c 100644 --- a/internal/gitaly/service/operations/tags_test.go +++ b/internal/gitaly/service/operations/tags_test.go @@ -504,3 +504,76 @@ func TestFailedUserCreateTagRequestDueToValidation(t *testing.T) { }) } } + +func TestTagHookOutput(t *testing.T) { + serverSocketPath, stop := runOperationServiceServer(t) + defer stop() + + client, conn := newOperationClient(t, serverSocketPath) + defer conn.Close() + + testRepo, testRepoPath, cleanupFn := testhelper.NewTestRepo(t) + defer cleanupFn() + + testCases := []struct { + desc string + hookContent string + output string + }{ + { + desc: "empty stdout and empty stderr", + hookContent: "#!/bin/sh\nexit 1", + output: "", + }, + { + desc: "empty stdout and some stderr", + hookContent: "#!/bin/sh\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "some stdout and empty stderr", + hookContent: "#!/bin/sh\necho stdout\nexit 1", + output: "stdout\n", + }, + { + desc: "some stdout and some stderr", + hookContent: "#!/bin/sh\necho stdout\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "whitespace stdout and some stderr", + hookContent: "#!/bin/sh\necho ' '\necho stderr >&2\nexit 1", + output: "stderr\n", + }, + { + desc: "some stdout and whitespace stderr", + hookContent: "#!/bin/sh\necho stdout\necho ' ' >&2\nexit 1", + output: "stdout\n", + }, + } + + 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, + } + + ctx, cancel := testhelper.Context() + defer cancel() + + remove, err := testhelper.WriteCustomHook(testRepoPath, hookName, []byte(testCase.hookContent)) + require.NoError(t, err) + defer remove() + + response, err := client.UserCreateTag(ctx, request) + require.NoError(t, err) + require.Equal(t, false, response.Exists) + require.Equal(t, testCase.output, response.PreReceiveError) + }) + } + } +} -- GitLab From 1f801575d3c314b746c12edae9272ff5d7376f29 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?=C3=86var=20Arnfj=C3=B6r=C3=B0=20Bjarmason?= Date: Tue, 17 Nov 2020 17:01:35 +0100 Subject: [PATCH 3/3] updateReferenceWithHooks: fix hook std(err|out) handling to be like Ruby In the Ruby code in GitLab::Git::Hook we return either stderr if there's anything there, or fall back to stdout. This behavior dates back to gitlab-org/gitlab@926a8ab4765 ("Handle custom Git hook result in GitLab UI", 2016-07-04). The current Ruby code Gitaly runs was copy/pasted into the tree in 5d0dc144 ("Implement CommitService.FindCommits", 2017-08-14). When this was ported to Go in f7bb6743 ("operations: Port UserMergeBranch from Ruby to Go", 2020-09-09) this logic error of always using stdout was introduced. Fix it. This makes the test added in the previous commit work. I'm fixing this to be bug-compatible with the Ruby implementation which uses/used ".blank?". I asked @vsizov whether he could recall whether the ".blank?" test was needed or just some mistaken idiom for "empty string". He thought it was safe to change it in Go to just use the len() test, but wasn't sure. I'm keeping the ".blank?" emulation here not so much out of the paranoia of being bug-compatible, but because it's easier to run the tests without things being conditionally different under the in-flight features to migrate functionality to Go. --- .../avar-hook-stdout-and-stderr-like-ruby.yml | 5 +++++ internal/gitaly/service/operations/merge.go | 16 +++++++++++++--- 2 files changed, 18 insertions(+), 3 deletions(-) create mode 100644 changelogs/unreleased/avar-hook-stdout-and-stderr-like-ruby.yml diff --git a/changelogs/unreleased/avar-hook-stdout-and-stderr-like-ruby.yml b/changelogs/unreleased/avar-hook-stdout-and-stderr-like-ruby.yml new file mode 100644 index 00000000000..204f8cc0533 --- /dev/null +++ b/changelogs/unreleased/avar-hook-stdout-and-stderr-like-ruby.yml @@ -0,0 +1,5 @@ +--- +title: Fix `updateReferenceWithHooks()` not forwarding stderr +merge_request: 2804 +author: +type: fixed diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go index 73ce55c95f0..e42fce7ec7c 100644 --- a/internal/gitaly/service/operations/merge.go +++ b/internal/gitaly/service/operations/merge.go @@ -109,6 +109,13 @@ func validateMergeBranchRequest(request *gitalypb.UserMergeBranchRequest) error return nil } +func hookErrorFromStdoutAndStderr(sout string, serr string) string { + if len(strings.TrimSpace(serr)) > 0 { + return serr + } + return sout +} + func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Repository, user *gitalypb.User, reference, newrev, oldrev string) error { gitlabshellEnv, err := gitlabshell.Env() if err != nil { @@ -156,17 +163,20 @@ func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re var stdout, stderr bytes.Buffer if err := s.hookManager.PreReceiveHook(ctx, repo, env, strings.NewReader(changes), &stdout, &stderr); err != nil { - return preReceiveError{message: stdout.String()} + msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String()) + return preReceiveError{message: msg} } if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil { - return preReceiveError{message: stdout.String()} + msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String()) + return preReceiveError{message: msg} } // For backwards compatibility with Ruby, we need to only call the reference-transaction // hook if the corresponding Ruby feature flag is set. if featureflag.IsEnabled(ctx, featureflag.RubyReferenceTransactionHook) { if err := s.hookManager.ReferenceTransactionHook(ctx, hook.ReferenceTransactionPrepared, env, strings.NewReader(changes)); err != nil { - return preReceiveError{message: stdout.String()} + msg := hookErrorFromStdoutAndStderr(stdout.String(), stderr.String()) + return preReceiveError{message: msg} } } -- GitLab