operations: Return detailed errors for hook failures in UserMergeBranchpks-user-merge-branch-custom-hook-errors

Custom server-side hooks provide a feature where if they return an
error, anything that was printed to stdout or stderr and is prefixed
with "GL-HOOK-ERR":" will be displayed in the web interface. The
interface to make this work between Rails and Gitaly is really fragile
though: Rails relies on Gitaly passing up the hook error without any
further decorations so that it can directly check whether the error has
the specified prefix.

This calling convention is not documented in any public interface, but
it has been like this since Gitaly has been born out of Rails. Of
course, people repeatedly didn't know about this small detail and would
improve error messages in a quest to make logging more useful in this
context. And they cannot be blamed: the calling convention is simply
bad.

Now that we have bought into gRPC's detailed error model though we can
handle this problem in a much better way. Instead of relying on the
exact error string returned by Gitaly, we use the new `CustomHookError`
Protobuf message and directly tell Rails what the standard output and
standard error streams contained. This will eventually free up the use
of our errors so that we can make them meaningful, all while finally
having an explicit and documented calling convention to bubble up hook
errors to the caller.

Note that this change is not done behind a feature flag: the actual
error message returned to Rails remains the same for now. We only use a
proper error code now and embed the detailed error. And the way Rails
inspects errors right now, nothing changes. Furthermore, it is currently
broken anyway, so there doesn't seem to be much of a point to be careful
and try not to break something that is already broken.

Changelog: changed

3 files changed, 94 insertions, 9 deletions

diff --git a/internal/git/updateref/update_with_hooks.go b/internal/git/updateref/update_with_hooks.go
index ffc585327..27d51f5e7 100644
--- a/internal/git/updateref/update_with_hooks.go
+++ b/internal/git/updateref/update_with_hooks.go
@@ -61,6 +61,25 @@ func (e CustomHookError) Error() string {
 	return e.err.Error()
 }
 
+// Proto returns the Protobuf representation of this error.
+func (e CustomHookError) Proto() *gitalypb.CustomHookError {
+	hookType := gitalypb.CustomHookError_HOOK_TYPE_UNSPECIFIED
+	switch e.hookType {
+	case git.PreReceiveHook:
+		hookType = gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE
+	case git.UpdateHook:
+		hookType = gitalypb.CustomHookError_HOOK_TYPE_UPDATE
+	case git.PostReceiveHook:
+		hookType = gitalypb.CustomHookError_HOOK_TYPE_POSTRECEIVE
+	}
+
+	return &gitalypb.CustomHookError{
+		HookType: hookType,
+		Stdout:   []byte(e.stdout),
+		Stderr:   []byte(e.stderr),
+	}
+}
+
 // Unwrap will return the embedded error.
 func (e CustomHookError) Unwrap() error {
 	return e.err
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index d89b8c092..e8f3e498e 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -113,6 +113,7 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 
 	if err := s.updateReferenceWithHooks(ctx, firstRequest.GetRepository(), firstRequest.User, quarantineDir, referenceName, mergeOID, revision); err != nil {
 		var notAllowedError hook.NotAllowedError
+		var customHookErr updateref.CustomHookError
 		var updateRefError updateref.Error
 
 		if errors.As(err, &notAllowedError) {
@@ -134,6 +135,23 @@ func (s *Server) UserMergeBranch(stream gitalypb.OperationService_UserMergeBranc
 			}
 
 			return detailedErr
+		} else if errors.As(err, &customHookErr) {
+			// When an error happens updating the reference, e.g. because of a
+			// race with another update, then we should tell the user that a
+			// precondition failed. A retry may fix this.
+			detailedErr, err := helper.ErrWithDetails(
+				helper.ErrPermissionDenied(customHookErr),
+				&gitalypb.UserMergeBranchError{
+					Error: &gitalypb.UserMergeBranchError_CustomHook{
+						CustomHook: customHookErr.Proto(),
+					},
+				},
+			)
+			if err != nil {
+				return helper.ErrInternalf("error details: %w", err)
+			}
+
+			return detailedErr
 		} else if errors.As(err, &updateRefError) {
 			// When an error happens updating the reference, e.g. because of a
 			// race with another update, then we should tell the user that a
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index db133eeb3..cdfeaeca0 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -131,7 +131,8 @@ func TestUserMergeBranch_quarantine(t *testing.T) {
 	gittest.WriteCustomHook(t, repoPath, "pre-receive", []byte(
 		`#!/bin/sh
 		read oldval newval ref &&
-		git rev-parse $newval^{commit} &&
+		git rev-parse $newval^{commit} >&2 &&
+		git rev-parse $oldval^{commit} &&
 		exit 1
 	`))
 
@@ -153,7 +154,18 @@ func TestUserMergeBranch_quarantine(t *testing.T) {
 
 	require.NoError(t, stream.Send(&gitalypb.UserMergeBranchRequest{Apply: true}), "apply merge")
 	secondResponse, err := stream.Recv()
-	testhelper.RequireGrpcError(t, helper.ErrInternalf("%s\n", firstResponse.CommitId), err)
+	testhelper.RequireGrpcError(t, errWithDetails(t,
+		helper.ErrPermissionDeniedf("%s\n", firstResponse.CommitId),
+		&gitalypb.UserMergeBranchError{
+			Error: &gitalypb.UserMergeBranchError_CustomHook{
+				CustomHook: &gitalypb.CustomHookError{
+					HookType: gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
+					Stdout:   []byte(fmt.Sprintf("%s\n", mergeBranchHeadBefore)),
+					Stderr:   []byte(fmt.Sprintf("%s\n", firstResponse.CommitId)),
+				},
+			},
+		},
+	), err)
 	require.Nil(t, secondResponse)
 
 	oid, err := git.NewObjectIDFromHex(strings.TrimSpace(firstResponse.CommitId))
@@ -423,11 +435,32 @@ func TestUserMergeBranch_failingHooks(t *testing.T) {
 
 	gittest.Exec(t, cfg, "-C", repoPath, "branch", mergeBranchName, mergeBranchHeadBefore)
 
-	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
+	hookContent := []byte("#!/bin/sh\necho 'stdout' && echo 'stderr' >&2\nexit 1")
 
-	for _, hookName := range gitlabPreHooks {
-		t.Run(hookName, func(t *testing.T) {
-			gittest.WriteCustomHook(t, repoPath, hookName, hookContent)
+	for _, tc := range []struct {
+		hookName     string
+		hookType     gitalypb.CustomHookError_HookType
+		shouldUpdate bool
+	}{
+		{
+			hookName:     "pre-receive",
+			hookType:     gitalypb.CustomHookError_HOOK_TYPE_PRERECEIVE,
+			shouldUpdate: false,
+		},
+		{
+			hookName:     "update",
+			hookType:     gitalypb.CustomHookError_HOOK_TYPE_UPDATE,
+			shouldUpdate: false,
+		},
+		{
+			hookName: "post-receive",
+			hookType: gitalypb.CustomHookError_HOOK_TYPE_POSTRECEIVE,
+			// The post-receive hook runs after references have been updated.
+			shouldUpdate: true,
+		},
+	} {
+		t.Run(tc.hookName, func(t *testing.T) {
+			gittest.WriteCustomHook(t, repoPath, tc.hookName, hookContent)
 
 			mergeBidi, err := client.UserMergeBranch(ctx)
 			require.NoError(t, err)
@@ -443,18 +476,33 @@ func TestUserMergeBranch_failingHooks(t *testing.T) {
 
 			require.NoError(t, mergeBidi.Send(firstRequest), "send first request")
 
-			_, err = mergeBidi.Recv()
+			firstResponse, err := mergeBidi.Recv()
 			require.NoError(t, err, "receive first response")
 
 			require.NoError(t, mergeBidi.Send(&gitalypb.UserMergeBranchRequest{Apply: true}), "apply merge")
 			require.NoError(t, mergeBidi.CloseSend(), "close send")
 
 			secondResponse, err := mergeBidi.Recv()
-			testhelper.RequireGrpcError(t, helper.ErrInternalf("failure\n"), err)
+			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)
 
 			currentBranchHead := gittest.Exec(t, cfg, "-C", repoPath, "rev-parse", mergeBranchName)
-			require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated")
+			if tc.shouldUpdate {
+				require.Equal(t, firstResponse.CommitId, text.ChompBytes(currentBranchHead), "branch head updated")
+			} else {
+				require.Equal(t, mergeBranchHeadBefore, text.ChompBytes(currentBranchHead), "branch head updated")
+			}
 		})
 	}
 }