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 0000000000000000000000000000000000000000..204f8cc0533284dd543818b68b3737be6cf1b629 --- /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/branches_test.go b/internal/gitaly/service/operations/branches_test.go index 4cd90be95bc130dd6ed141008b07fe790e02796f..4caea6deeb6ad86ed4783b9385d9c5c7f229c9ab 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/merge.go b/internal/gitaly/service/operations/merge.go index 73ce55c95f0d3d0eea0bb2bf676ee7c86a5fc522..e42fce7ec7cfcaab3661ac518616f2506fa5d90a 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} } } diff --git a/internal/gitaly/service/operations/tags_test.go b/internal/gitaly/service/operations/tags_test.go index 65ffefa7ffd17565fe6b9160cb1904a184884f93..3fbb419446c88174f3d02ea68df29f6508ceaf39 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) { @@ -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) + }) + } + } +}