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.

1 files changed, 13 insertions, 3 deletions

diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index 73ce55c95..e42fce7ec 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}
 		}
 	}