From 8091f482d8bca76426c8a498dd111223a0f82ca3 Mon Sep 17 00:00:00 2001 From: Patrick Steinhardt Date: Fri, 15 Jul 2022 13:21:11 +0200 Subject: updateref: Ignore failures of custom post-receive hooks The post-receive hook run by git-receive-pack(1) is special compared to all the other hooks because any errors returned by it have no impact on the end result of git-receive-pack(1). This is because the hook runs after references have been updated already, so it cannot have any impact on the end result anymore. Our hooks updater, which emulates roughly the same semantics for hooks execution as git-receive-pack(1), behaves differently though and will in fact report any custom-hook errors to the caller. This has the result that depending on how the post-receive hook is executed, an error code has different impacts. Align the behaviour of the hooks updater to match what Git is doing and ignore errors returned by custom hooks. Any custom hooks which relied on this exact behaviour were broken already because the error code wouldn't have changed the outcome, and neither would the hook work correctly when executed by git-receive-pack(1). This thus shouldn't be a breaking change, but at most fix behaviour in context of misbehaving post-receive hook scripts. Changelog: fixed --- internal/git/updateref/update_with_hooks.go | 19 +++++++- internal/git/updateref/update_with_hooks_test.go | 3 +- internal/gitaly/service/operations/merge_test.go | 56 ++++++++++++++---------- 3 files changed, 52 insertions(+), 26 deletions(-) diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go index acb91562e..90ac23099 100644 --- a/internal/git/updateref/update_with_hooks.go +++ b/internal/git/updateref/update_with_hooks.go @@ -7,6 +7,7 @@ import ( "fmt" "strings" + "github.com/grpc-ecosystem/go-grpc-middleware/logging/logrus/ctxlogrus" "gitlab.com/gitlab-org/gitaly/v15/internal/git" "gitlab.com/gitlab-org/gitaly/v15/internal/git/catfile" "gitlab.com/gitlab-org/gitaly/v15/internal/git/localrepo" @@ -269,7 +270,23 @@ func (u *UpdaterWithHooks) UpdateReference( } if err := u.hookManager.PostReceiveHook(ctx, repo, pushOptions, []string{hooksPayload}, strings.NewReader(changes), &stdout, &stderr); err != nil { - return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String())) + // CustomHook errors are returned in case a custom hook has returned an error code. + // The post-receive hook has special semantics though. Quoting githooks(5): + // + // This hook does not affect the outcome of git receive-pack, as it is called + // after the real work is done. + // + // This means that even if the hook returns an error, then that error should not + // impact whatever git-receive-pack(1) has been doing. And given that we emulate + // behaviour of this command here, we need to behave the same. + var customHookErr hook.CustomHookError + if errors.As(err, &customHookErr) { + // Only log the error when we've got a custom-hook error, but otherwise + // ignore it and continue with whatever we have been doing. + ctxlogrus.Extract(ctx).WithError(err).Error("custom post-receive hook returned an error") + } else { + return fmt.Errorf("running post-receive hooks: %w", wrapHookError(err, git.PostReceiveHook, stdout.String(), stderr.String())) + } } return nil diff --git a/internal/git/updateref/update_with_hooks_test.go b/internal/git/updateref/update_with_hooks_test.go index f952f3505..0afe76bb6 100644 --- a/internal/git/updateref/update_with_hooks_test.go +++ b/internal/git/updateref/update_with_hooks_test.go @@ -228,7 +228,7 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { expectedErr: "reference-transaction failure", }, { - desc: "post-receive error", + desc: "post-receive error is ignored", preReceive: func(t *testing.T, ctx context.Context, repo *gitalypb.Repository, pushOptions, env []string, stdin io.Reader, stdout, stderr io.Writer) error { return nil }, @@ -243,7 +243,6 @@ func TestUpdaterWithHooks_UpdateReference(t *testing.T) { require.NoError(t, err) return errors.New("ignored") }, - expectedErr: "post-receive failure", expectedRefDeletion: true, }, } diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go index fabf76446..387518399 100644 --- a/internal/gitaly/service/operations/merge_test.go +++ b/internal/gitaly/service/operations/merge_test.go @@ -438,25 +438,26 @@ func TestUserMergeBranch_failingHooks(t *testing.T) { hookContent := []byte("#!/bin/sh\necho 'stdout' && echo 'stderr' >&2\nexit 1") for _, tc := range []struct { - hookName string - hookType gitalypb.CustomHookError_HookType - shouldUpdate bool + hookName string + hookType gitalypb.CustomHookError_HookType + shouldFail bool }{ { - hookName: "pre-receive", - hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, - shouldUpdate: false, + hookName: "pre-receive", + hookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE, + shouldFail: true, }, { - hookName: "update", - hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, - shouldUpdate: false, + hookName: "update", + hookType: gitalypb.CustomHookError_HOOK_TYPE_UPDATE, + shouldFail: true, }, { hookName: "post-receive", hookType: gitalypb.CustomHookError_HOOK_TYPE_POSTRECEIVE, - // The post-receive hook runs after references have been updated. - shouldUpdate: true, + // The post-receive hook runs after references have been updated and any + // failures of it are ignored. + shouldFail: false, }, } { t.Run(tc.hookName, func(t *testing.T) { @@ -483,22 +484,31 @@ func TestUserMergeBranch_failingHooks(t *testing.T) { require.NoError(t, mergeBidi.CloseSend(), "close send") secondResponse, err := mergeBidi.Recv() - testhelper.RequireGrpcError(t, errWithDetails(t, - helper.ErrPermissionDeniedf("stderr\n"), - &gitalypb.UserMergeBranchError{ - Error: &gitalypb.UserMergeBranchError_CustomHook{ - CustomHook: &gitalypb.CustomHookError{ - HookType: tc.hookType, - Stdout: []byte("stdout\n"), - Stderr: []byte("stderr\n"), + if tc.shouldFail { + testhelper.RequireGrpcError(t, errWithDetails(t, + helper.ErrPermissionDeniedf("stderr\n"), + &gitalypb.UserMergeBranchError{ + Error: &gitalypb.UserMergeBranchError_CustomHook{ + CustomHook: &gitalypb.CustomHookError{ + HookType: tc.hookType, + Stdout: []byte("stdout\n"), + Stderr: []byte("stderr\n"), + }, }, }, - }, - ), err) - require.Nil(t, secondResponse) + ), err) + require.Nil(t, secondResponse) + } else { + testhelper.ProtoEqual(t, &gitalypb.UserMergeBranchResponse{ + BranchUpdate: &gitalypb.OperationBranchUpdate{ + CommitId: firstResponse.CommitId, + }, + }, secondResponse) + require.NoError(t, err) + } currentBranchHead := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", mergeBranchName) - if tc.shouldUpdate { + if !tc.shouldFail { require.Equal(t, firstResponse.CommitId, text.ChompBytes(currentBranchHead), "branch head updated") } else { require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated") -- cgit v1.2.3