tests: Fix Git environment wrapper using wrong executable

In order to test our support for Git protocol v2, we have set up a
wrapper script around Git which captures its environment and writes it
to a file. This allows us to check whether required options are set up
correctly for protocol v2 to be enabled. The wrapper script we currently
use doesn't invoke the correct Git executable, though, as it will simply
use the one present in `$PATH`.

Fix the issue by writing a temporary script containing the correct path
configured via `config.Config.Git.Binary`. All the other alternatives
like injecting another environment variable would've been non-trivial to
implement, as we'd need to manually inject them at the right spots.

Note that there's one peculiarity here: as we're about to migrate to use
`command.GitPath()` everywhere, it will cause us to invoke the wrapper
multiple times. To fix this, this commit starts appending to the
environment file instead of overwriting it on each execution, deleting
the file after the test is done.

9 files changed, 46 insertions, 31 deletions

diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index e87b007e6..18efdca10 100644
--- a/internal/service/smarthttp/inforefs_test.go
+++ b/internal/service/smarthttp/inforefs_test.go
@@ -92,7 +92,7 @@ func TestSuccessfulInfoRefsUploadPackWithGitConfigOptions(t *testing.T) {
 }
 
 func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
-	restore := testhelper.EnableGitProtocolV2Support()
+	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSmartHTTPServer(t)
@@ -125,7 +125,7 @@ func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
 	envData, err := testhelper.GetGitEnvData()
 
 	require.NoError(t, err)
-	require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
+	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
 func makeInfoRefsUploadPackRequest(ctx context.Context, t *testing.T, serverSocketPath string, rpcRequest *gitalypb.InfoRefsRequest) ([]byte, error) {
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index eeec6e91a..c4bf736fc 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -83,7 +83,7 @@ func TestSuccessfulReceivePackRequest(t *testing.T) {
 }
 
 func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
-	restore := testhelper.EnableGitProtocolV2Support()
+	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSmartHTTPServer(t)
diff --git a/internal/service/smarthttp/testdata/env_git b/internal/service/smarthttp/testdata/env_git
deleted file mode 120000
index a01a70012..000000000
--- a/internal/service/smarthttp/testdata/env_git
+++ /dev/null
@@ -1 +0,0 @@
-../../../testhelper/env_git
\ No newline at end of file
diff --git a/internal/service/smarthttp/upload_pack_test.go b/internal/service/smarthttp/upload_pack_test.go
index 7b6b6e631..2077904a6 100644
--- a/internal/service/smarthttp/upload_pack_test.go
+++ b/internal/service/smarthttp/upload_pack_test.go
@@ -168,7 +168,7 @@ func TestUploadPackRequestWithGitConfigOptions(t *testing.T) {
 }
 
 func TestUploadPackRequestWithGitProtocol(t *testing.T) {
-	restore := testhelper.EnableGitProtocolV2Support()
+	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSmartHTTPServer(t)
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index ff48d20e3..f61d7211f 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -111,7 +111,7 @@ func TestReceivePackPushSuccess(t *testing.T) {
 }
 
 func TestReceivePackPushSuccessWithGitProtocol(t *testing.T) {
-	restore := testhelper.EnableGitProtocolV2Support()
+	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSSHServer(t)
@@ -127,7 +127,7 @@ func TestReceivePackPushSuccessWithGitProtocol(t *testing.T) {
 	envData, err := testhelper.GetGitEnvData()
 
 	require.NoError(t, err)
-	require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
+	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
 func TestReceivePackPushFailure(t *testing.T) {
@@ -212,7 +212,7 @@ func TestSSHReceivePackToHooks(t *testing.T) {
 	glRepository := "some_repo"
 	glID := "key-123"
 
-	restore := testhelper.EnableGitProtocolV2Support()
+	restore := testhelper.EnableGitProtocolV2Support(t)
 	defer restore()
 
 	serverSocketPath, stop := runSSHServer(t)
@@ -262,7 +262,7 @@ func TestSSHReceivePackToHooks(t *testing.T) {
 	envData, err := testhelper.GetGitEnvData()
 
 	require.NoError(t, err)
-	require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
+	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
 // SSHCloneDetails encapsulates values relevant for a test clone
@@ -322,9 +322,9 @@ func sshPush(t *testing.T, cloneDetails SSHCloneDetails, serverSocketPath string
 	cmd.Env = []string{
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_ADDRESS=%s", serverSocketPath),
-		fmt.Sprintf("PATH=%s", ".:"+os.Getenv("PATH")),
 		fmt.Sprintf(`GIT_SSH_COMMAND=%s receive-pack`, gitalySSHPath),
 	}
+
 	out, err := cmd.CombinedOutput()
 	if err != nil {
 		return "", "", fmt.Errorf("error pushing: %v: %q", err, out)
diff --git a/internal/service/ssh/testdata/env_git b/internal/service/ssh/testdata/env_git
deleted file mode 120000
index a01a70012..000000000
--- a/internal/service/ssh/testdata/env_git
+++ /dev/null
@@ -1 +0,0 @@
-../../../testhelper/env_git
\ No newline at end of file
diff --git a/internal/service/ssh/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index 5b147eedf..b59ce5b2d 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -286,12 +286,6 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 }
 
 func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
-	restore := testhelper.EnableGitProtocolV2Support()
-	defer restore()
-
-	serverSocketPath, stop := runSSHServer(t)
-	defer stop()
-
 	localRepoPath := path.Join(testRepoRoot, "gitlab-test-upload-pack-local")
 
 	tests := []struct {
@@ -310,6 +304,12 @@ func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
 
 	for _, tc := range tests {
 		t.Run(tc.desc, func(t *testing.T) {
+			restore := testhelper.EnableGitProtocolV2Support(t)
+			defer restore()
+
+			serverSocketPath, stop := runSSHServer(t)
+			defer stop()
+
 			cmd := cloneCommand{
 				repository:  testRepo,
 				command:     tc.cmd,
@@ -323,7 +323,7 @@ func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
 			envData, err := testhelper.GetGitEnvData()
 
 			require.NoError(t, err)
-			require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
+			require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 		})
 	}
 }
diff --git a/internal/testhelper/env_git b/internal/testhelper/env_git
deleted file mode 100755
index 7739eaeef..000000000
--- a/internal/testhelper/env_git
+++ /dev/null
@@ -1,4 +0,0 @@
-#!/bin/sh
-mkdir -p testdata
-env | grep ^GIT_PROTOCOL= > testdata/git-env
-exec git "$@"
diff --git a/internal/testhelper/git_protocol.go b/internal/testhelper/git_protocol.go
index ab0377551..a57b85906 100644
--- a/internal/testhelper/git_protocol.go
+++ b/internal/testhelper/git_protocol.go
@@ -1,20 +1,41 @@
 package testhelper
 
 import (
+	"fmt"
+	"io/ioutil"
+	"os"
+	"path"
+	"testing"
+
+	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 )
 
-// EnableGitProtocolV2Support replaces the git binary in config with an
-// `env_git` wrapper that allows the protocol to be tested. It returns a
-// function that restores the given settings.
-//
-// Because we don't know how to get to that wrapper script from the current
-// working directory, callers must create a symbolic link to the `env_git` file
-// in their own `testdata` directories.
-func EnableGitProtocolV2Support() func() {
+// EnableGitProtocolV2Support replaces the git binary in config with a
+// wrapper that allows the protocol to be tested. It returns a function that
+// restores the given settings as well as an array of environment variables
+// which need to be set when invoking Git with this setup.
+func EnableGitProtocolV2Support(t *testing.T) func() {
+	script := fmt.Sprintf(`#!/bin/sh
+mkdir -p testdata
+env | grep ^GIT_PROTOCOL= >>testdata/git-env
+exec "%s" "$@"
+`, command.GitPath())
+
+	dir, err := ioutil.TempDir("", "gitaly-test-*")
+	require.NoError(t, err)
+
+	path := path.Join(dir, "git")
+
+	cleanup, err := WriteExecutable(path, []byte(script))
+	require.NoError(t, err)
+
 	oldGitBinPath := config.Config.Git.BinPath
-	config.Config.Git.BinPath = "testdata/env_git"
+	config.Config.Git.BinPath = path
 	return func() {
+		os.Remove("testdata/git-env")
 		config.Config.Git.BinPath = oldGitBinPath
+		cleanup()
 	}
 }