updateref: Ignore failures of custom post-receive hookspks-updateref-ignore-post-receive-custom-hook-error

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

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")