operations: Fix PostReceive hook receiving no input

The `updateReferenceWithHooks` function will update a reference and
manually invoke the pre-receive, update and post-receive Git hooks for
the updated reference. To do so, we construct various input parameters
and pass them over to the hook manager, which then executes the custom
hook logic for us. One of these parameters is the standard input
expected by the spawned hooks, which has the same format "<oldrev>
<newrev> <reference>" for both pre-receive and post-receive hooks. As
such, we currently create this buffer once and then pass it to both
functions as a `bytes.Buffer`.

There's a small gotcha here: as the buffer returned by `NewStringBuffer`
is a pointer, it'll get consumed upon reading it when passed to another
function. As a result, as soon as `PostReceiveHook` gets hold of the
buffer it'll be empty already and not contain any references to be
updated. This will lead us to not execute any post-receive hook logic at
all as the assumption now is that no references were updated.

Fix the issue by creating two buffers instead of one only and modify an
existing test case to also check standard input of custom hooks.

3 files changed, 27 insertions, 7 deletions

diff --git a/changelogs/unreleased/pks-user-merge-branch-post-receive-hook.yml b/changelogs/unreleased/pks-user-merge-branch-post-receive-hook.yml
new file mode 100644
index 000000000..7dd7030f4
--- /dev/null
+++ b/changelogs/unreleased/pks-user-merge-branch-post-receive-hook.yml
@@ -0,0 +1,5 @@
+---
+title: 'operations: Fix PostReceive hook receiving no input'
+merge_request: 2653
+author:
+type: fixed
diff --git a/internal/gitaly/service/operations/merge.go b/internal/gitaly/service/operations/merge.go
index f5ab8a346..5568872b8 100644
--- a/internal/gitaly/service/operations/merge.go
+++ b/internal/gitaly/service/operations/merge.go
@@ -125,10 +125,10 @@ func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re
 		fmt.Sprintf("GITALY_TOKEN=%s", s.cfg.Auth.Token),
 	}, gitlabshellEnv...)
 
-	stdin := bytes.NewBufferString(fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference))
+	changes := fmt.Sprintf("%s %s %s\n", oldrev, newrev, reference)
 	var stdout, stderr bytes.Buffer
 
-	if err := s.hookManager.PreReceiveHook(ctx, repo, env, stdin, &stdout, &stderr); err != nil {
+	if err := s.hookManager.PreReceiveHook(ctx, repo, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
 		return preReceiveError{message: stdout.String()}
 	}
 	if err := s.hookManager.UpdateHook(ctx, repo, reference, oldrev, newrev, env, &stdout, &stderr); err != nil {
@@ -148,7 +148,7 @@ func (s *server) updateReferenceWithHooks(ctx context.Context, repo *gitalypb.Re
 		return updateRefError{reference: reference}
 	}
 
-	if err := s.hookManager.PostReceiveHook(ctx, repo, nil, env, stdin, &stdout, &stderr); err != nil {
+	if err := s.hookManager.PostReceiveHook(ctx, repo, nil, env, strings.NewReader(changes), &stdout, &stderr); err != nil {
 		return err
 	}
 
diff --git a/internal/gitaly/service/operations/merge_test.go b/internal/gitaly/service/operations/merge_test.go
index 8e0cb3faa..eb9e01d32 100644
--- a/internal/gitaly/service/operations/merge_test.go
+++ b/internal/gitaly/service/operations/merge_test.go
@@ -5,6 +5,7 @@ import (
 	"context"
 	"fmt"
 	"io/ioutil"
+	"os"
 	"os/exec"
 	"strings"
 	"testing"
@@ -65,10 +66,18 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) {
 
 	hooks := GitlabHooks
 	hookTempfiles := make([]string, len(hooks))
-	for i, h := range hooks {
-		var cleanup func()
-		hookTempfiles[i], cleanup = testhelper.WriteEnvToCustomHook(t, testRepoPath, h)
+	for i, hook := range hooks {
+		outputFile, err := ioutil.TempFile("", "")
+		require.NoError(t, err)
+		require.NoError(t, outputFile.Close())
+		defer os.Remove(outputFile.Name())
+
+		script := fmt.Sprintf("#!/bin/sh\n(cat && env) >%s \n", outputFile.Name())
+		cleanup, err := testhelper.WriteCustomHook(testRepoPath, hook, []byte(script))
+		require.NoError(t, err)
 		defer cleanup()
+
+		hookTempfiles[i] = outputFile.Name()
 	}
 
 	mergeCommitMessage := "Merged by Gitaly"
@@ -117,7 +126,13 @@ func testSuccessfulMerge(t *testing.T, ctx context.Context) {
 	for i, h := range hooks {
 		hookEnv, err := ioutil.ReadFile(hookTempfiles[i])
 		require.NoError(t, err)
-		require.Contains(t, strings.Split(string(hookEnv), "\n"), expectedGlID, "expected env of hook %q to contain %q", h, expectedGlID)
+
+		lines := strings.Split(string(hookEnv), "\n")
+		require.Contains(t, lines, expectedGlID, "expected env of hook %q to contain %q", h, expectedGlID)
+
+		if h == "pre-receive" || h == "post-receive" {
+			require.Regexp(t, mergeBranchHeadBefore+" .* refs/heads/"+mergeBranchName, lines[0], "expected env of hook %q to contain reference change", h)
+		}
 	}
 }