From abb1fcd6457b02dd4bffd697ebf92c76fac9bbdf Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Tue, 18 Aug 2020 15:58:52 +0200
Subject: [PATCH 1/9] tests: Fix calls to `exec.Command` using wrong Git
 executable

While production code uses the Git DSL to execute Git commands in most
places which always respects the configured Git binary, many of our
tests don't. This commit fixes them by modifying `MustRunCommand()` to
automatically adjust invocations of `"git"` and fixing up any other
locations which used `exec.Cmd` directly.
---
 cmd/gitaly-debug/simulatehttp.go                  |  5 +++--
 cmd/gitaly-ssh/auth_test.go                       |  3 ++-
 internal/git/gittest/http_server.go               |  4 ++--
 internal/service/blob/lfs_pointers_test.go        |  3 ++-
 internal/service/commit/find_commits_test.go      |  3 ++-
 internal/service/commit/isancestor_test.go        |  3 ++-
 internal/service/operations/branches_test.go      | 13 +++++++------
 internal/service/operations/merge_test.go         | 13 +++++++------
 internal/service/operations/tags_test.go          | 11 ++++++-----
 internal/service/ref/refs_test.go                 |  5 +++--
 .../service/repository/calculate_checksum_test.go |  3 ++-
 internal/service/repository/cleanup_test.go       |  3 ++-
 .../repository/redirecting_test_server_test.go    |  2 +-
 internal/service/repository/repack_test.go        |  3 ++-
 internal/service/repository/search_files_test.go  |  3 ++-
 internal/service/repository/snapshot_test.go      |  7 ++++---
 internal/service/ssh/receive_pack_test.go         |  3 ++-
 internal/service/ssh/upload_archive_test.go       |  3 ++-
 internal/service/ssh/upload_pack_test.go          | 15 ++++++++-------
 internal/testhelper/commit.go                     |  3 ++-
 internal/testhelper/testhelper.go                 |  8 +++++---
 21 files changed, 68 insertions(+), 48 deletions(-)

diff --git a/cmd/gitaly-debug/simulatehttp.go b/cmd/gitaly-debug/simulatehttp.go
index c87b4f5ee8..ac9a63fe31 100644
--- a/cmd/gitaly-debug/simulatehttp.go
+++ b/cmd/gitaly-debug/simulatehttp.go
@@ -9,12 +9,13 @@ import (
 	"regexp"
 	"time"
 
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git/pktline"
 )
 
 func simulateHTTPClone(gitDir string) {
 	msg("Generating server response for HTTP clone. Data goes to /dev/null.")
-	infoRefs := exec.Command("git", "upload-pack", "--stateless-rpc", "--advertise-refs", gitDir)
+	infoRefs := exec.Command(command.GitPath(), "upload-pack", "--stateless-rpc", "--advertise-refs", gitDir)
 	infoRefs.Stderr = os.Stderr
 	out, err := infoRefs.StdoutPipe()
 	noError(err)
@@ -57,7 +58,7 @@ func simulateHTTPClone(gitDir string) {
 	}
 	fmt.Fprint(request, "00000009done\n")
 
-	uploadPack := exec.Command("git", "upload-pack", "--stateless-rpc", gitDir)
+	uploadPack := exec.Command(command.GitPath(), "upload-pack", "--stateless-rpc", gitDir)
 	uploadPack.Stdin = request
 	uploadPack.Stderr = os.Stderr
 	out, err = uploadPack.StdoutPipe()
diff --git a/cmd/gitaly-ssh/auth_test.go b/cmd/gitaly-ssh/auth_test.go
index 6206aede4b..bc6d554384 100644
--- a/cmd/gitaly-ssh/auth_test.go
+++ b/cmd/gitaly-ssh/auth_test.go
@@ -12,6 +12,7 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/rubyserver"
 	"gitlab.com/gitlab-org/gitaly/internal/server"
@@ -91,7 +92,7 @@ func TestConnectivity(t *testing.T) {
 	require.NoError(t, err)
 	for _, testcase := range testCases {
 		t.Run(testcase.name, func(t *testing.T) {
-			cmd := exec.Command("git", "ls-remote", "git@localhost:test/test.git", "refs/heads/master")
+			cmd := exec.Command(command.GitPath(), "ls-remote", "git@localhost:test/test.git", "refs/heads/master")
 			cmd.Stderr = os.Stderr
 			cmd.Env = []string{
 				fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
diff --git a/internal/git/gittest/http_server.go b/internal/git/gittest/http_server.go
index 4e5d1916a9..25ff78f089 100644
--- a/internal/git/gittest/http_server.go
+++ b/internal/git/gittest/http_server.go
@@ -30,7 +30,7 @@ func RemoteUploadPackServer(ctx context.Context, t *testing.T, repoName, httpTok
 				}
 				defer r.Body.Close()
 
-				cmd, err := command.New(ctx, exec.Command("git", "-C", repoPath, "upload-pack", "--stateless-rpc", "."), reader, w, nil)
+				cmd, err := command.New(ctx, exec.Command(command.GitPath(), "-C", repoPath, "upload-pack", "--stateless-rpc", "."), reader, w, nil)
 				require.NoError(t, err)
 				require.NoError(t, cmd.Wait())
 			case fmt.Sprintf("/%s.git/info/refs?service=git-upload-pack", repoName):
@@ -44,7 +44,7 @@ func RemoteUploadPackServer(ctx context.Context, t *testing.T, repoName, httpTok
 				w.Write([]byte("001e# service=git-upload-pack\n"))
 				w.Write([]byte("0000"))
 
-				cmd, err := command.New(ctx, exec.Command("git", "-C", repoPath, "upload-pack", "--advertise-refs", "."), nil, w, nil)
+				cmd, err := command.New(ctx, exec.Command(command.GitPath(), "-C", repoPath, "upload-pack", "--advertise-refs", "."), nil, w, nil)
 				require.NoError(t, err)
 				require.NoError(t, cmd.Wait())
 			default:
diff --git a/internal/service/blob/lfs_pointers_test.go b/internal/service/blob/lfs_pointers_test.go
index 7126b35f86..9f2c8de4f1 100644
--- a/internal/service/blob/lfs_pointers_test.go
+++ b/internal/service/blob/lfs_pointers_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -137,7 +138,7 @@ func TestSuccessfulGetNewLFSPointersRequest(t *testing.T) {
 	revision := []byte("46abbb087fcc0fd02c340f0f2f052bd2c7708da3")
 	commiterArgs := []string{"-c", "user.name=Scrooge McDuck", "-c", "user.email=scrooge@mcduck.com"}
 	cmdArgs := append(commiterArgs, "-C", testRepoPath, "cherry-pick", string(revision))
-	cmd := exec.Command("git", cmdArgs...)
+	cmd := exec.Command(command.GitPath(), cmdArgs...)
 	// Skip smudge since it doesn't work with file:// remotes and we don't need it
 	cmd.Env = append(cmd.Env, "GIT_LFS_SKIP_SMUDGE=1")
 	altDirs := "./alt-objects"
diff --git a/internal/service/commit/find_commits_test.go b/internal/service/commit/find_commits_test.go
index 8e906a3c96..c94b801735 100644
--- a/internal/service/commit/find_commits_test.go
+++ b/internal/service/commit/find_commits_test.go
@@ -11,6 +11,7 @@ import (
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -455,7 +456,7 @@ func TestSuccessfulFindCommitsRequestWithAltGitObjectDirs(t *testing.T) {
 	testRepoCopy, testRepoCopyPath, cleanupFn := testhelper.NewTestRepoWithWorktree(t)
 	defer cleanupFn()
 
-	cmd := exec.Command("git", "-C", testRepoCopyPath,
+	cmd := exec.Command(command.GitPath(), "-C", testRepoCopyPath,
 		"-c", fmt.Sprintf("user.name=%s", committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"commit", "--allow-empty", "-m", "An empty commit")
diff --git a/internal/service/commit/isancestor_test.go b/internal/service/commit/isancestor_test.go
index 48998d09a8..f63b422a84 100644
--- a/internal/service/commit/isancestor_test.go
+++ b/internal/service/commit/isancestor_test.go
@@ -6,6 +6,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -190,7 +191,7 @@ func TestSuccessfulIsAncestorRequestWithAltGitObjectDirs(t *testing.T) {
 
 	previousHead := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoCopyPath, "show", "--format=format:%H", "--no-patch", "HEAD")
 
-	cmd := exec.Command("git", "-C", testRepoCopyPath,
+	cmd := exec.Command(command.GitPath(), "-C", testRepoCopyPath,
 		"-c", fmt.Sprintf("user.name=%s", committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"commit", "--allow-empty", "-m", "An empty commit")
diff --git a/internal/service/operations/branches_test.go b/internal/service/operations/branches_test.go
index bd235e572a..e6879d9f19 100644
--- a/internal/service/operations/branches_test.go
+++ b/internal/service/operations/branches_test.go
@@ -8,6 +8,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -79,7 +80,7 @@ func TestSuccessfulCreateBranchRequest(t *testing.T) {
 
 			response, err := client.UserCreateBranch(ctx, request)
 			if testCase.expectedBranch != nil {
-				defer exec.Command("git", "-C", testRepoPath, "branch", "-D", branchName).Run()
+				defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-D", branchName).Run()
 			}
 
 			require.NoError(t, err)
@@ -162,7 +163,7 @@ func TestUserCreateBranchWithTransaction(t *testing.T) {
 
 	for _, tc := range testcases {
 		t.Run(tc.desc, func(t *testing.T) {
-			defer exec.Command("git", "-C", testRepoPath, "branch", "-D", "new-branch").Run()
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-D", "new-branch").Run()
 
 			client, conn := newOperationClient(t, tc.address)
 			defer conn.Close()
@@ -211,7 +212,7 @@ func testSuccessfulGitHooksForUserCreateBranchRequest(t *testing.T, ctx context.
 
 	for _, hookName := range GitlabHooks {
 		t.Run(hookName, func(t *testing.T) {
-			defer exec.Command("git", "-C", testRepoPath, "branch", "-D", branchName).Run()
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-D", branchName).Run()
 
 			hookOutputTempPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName)
 			defer cleanup()
@@ -348,7 +349,7 @@ func TestSuccessfulUserDeleteBranchRequest(t *testing.T) {
 
 			branchNameInput := "to-be-deleted-soon-branch"
 
-			defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
 
@@ -378,7 +379,7 @@ func TestSuccessfulGitHooksForUserDeleteBranchRequest(t *testing.T) {
 	defer conn.Close()
 
 	branchNameInput := "to-be-deleted-soon-branch"
-	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 	request := &gitalypb.UserDeleteBranchRequest{
 		Repository: testRepo,
@@ -470,7 +471,7 @@ func TestFailedUserDeleteBranchDueToHooks(t *testing.T) {
 
 	branchNameInput := "to-be-deleted-soon-branch"
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", branchNameInput)
-	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchNameInput).Run()
 
 	request := &gitalypb.UserDeleteBranchRequest{
 		Repository: testRepo,
diff --git a/internal/service/operations/merge_test.go b/internal/service/operations/merge_test.go
index 2c60c37180..5a9ad356e7 100644
--- a/internal/service/operations/merge_test.go
+++ b/internal/service/operations/merge_test.go
@@ -10,6 +10,7 @@ import (
 	"time"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	gitlog "gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -307,7 +308,7 @@ func TestSuccessfulUserFFBranchRequest(t *testing.T) {
 	}
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchName).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 	resp, err := client.UserFFBranch(ctx, request)
 	require.NoError(t, err)
@@ -330,7 +331,7 @@ func TestFailedUserFFBranchRequest(t *testing.T) {
 	branchName := "test-ff-target-branch"
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchName).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 	testCases := []struct {
 		desc     string
@@ -431,7 +432,7 @@ func TestFailedUserFFBranchDueToHooks(t *testing.T) {
 	}
 
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "branch", "-f", branchName, "6d394385cf567f80a8fd85055db1ab4c5295806f")
-	defer exec.Command("git", "-C", testRepoPath, "branch", "-d", branchName).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-d", branchName).Run()
 
 	hookContent := []byte("#!/bin/sh\necho 'failure'\nexit 1")
 
@@ -469,7 +470,7 @@ func TestSuccessfulUserMergeToRefRequest(t *testing.T) {
 
 	// Writes in existingTargetRef
 	beforeRefreshCommitSha := "a5391128b0ef5d21df5dd23d98557f4ef12fae20"
-	out, err := exec.Command("git", "-C", testRepoPath, "update-ref", string(existingTargetRef), beforeRefreshCommitSha).CombinedOutput()
+	out, err := exec.Command(command.GitPath(), "-C", testRepoPath, "update-ref", string(existingTargetRef), beforeRefreshCommitSha).CombinedOutput()
 	require.NoError(t, err, "give an existing state to the target ref: %s", out)
 
 	testCases := []struct {
@@ -706,12 +707,12 @@ func TestUserMergeToRefIgnoreHooksRequest(t *testing.T) {
 
 func prepareMergeBranch(t *testing.T, testRepoPath string) {
 	deleteBranch(testRepoPath, mergeBranchName)
-	out, err := exec.Command("git", "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore).CombinedOutput()
+	out, err := exec.Command(command.GitPath(), "-C", testRepoPath, "branch", mergeBranchName, mergeBranchHeadBefore).CombinedOutput()
 	require.NoError(t, err, "set up branch to merge into: %s", out)
 }
 
 func deleteBranch(testRepoPath, branchName string) {
-	exec.Command("git", "-C", testRepoPath, "branch", "-D", branchName).Run()
+	exec.Command(command.GitPath(), "-C", testRepoPath, "branch", "-D", branchName).Run()
 }
 
 // This error is used as a sentinel value
diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go
index 549efdad3a..42479cd2c4 100644
--- a/internal/service/operations/tags_test.go
+++ b/internal/service/operations/tags_test.go
@@ -9,6 +9,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/metadata/featureflag"
@@ -39,7 +40,7 @@ func TestSuccessfulUserDeleteTagRequest(t *testing.T) {
 
 			tagNameInput := "to-be-deleted-soon-tag"
 
-			defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
 
 			testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput)
 
@@ -84,7 +85,7 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con
 	defer cleanupFn()
 
 	tagNameInput := "to-be-déleted-soon-tag"
-	defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
 
 	request := &gitalypb.UserDeleteTagRequest{
 		Repository: testRepo,
@@ -195,7 +196,7 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 					require.NoError(t, err, "error from calling RPC")
 					require.Empty(t, response.PreReceiveError, "PreReceiveError must be empty, signalling the push was accepted")
 
-					defer exec.Command("git", "-C", testRepoPath, "tag", "-d", inputTagName).Run()
+					defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", inputTagName).Run()
 
 					id := testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "rev-parse", inputTagName)
 					testCase.expectedTag.Id = text.ChompBytes(id)
@@ -241,7 +242,7 @@ func testSuccessfulGitHooksForUserCreateTagRequest(t *testing.T, ctx context.Con
 
 	for _, hookName := range GitlabHooks {
 		t.Run(hookName, func(t *testing.T) {
-			defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagName).Run()
+			defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagName).Run()
 
 			hookOutputTempPath, cleanup := testhelper.WriteEnvToCustomHook(t, testRepoPath, hookName)
 			defer cleanup()
@@ -329,7 +330,7 @@ func testFailedUserDeleteTagDueToHooks(t *testing.T, ctx context.Context) {
 
 	tagNameInput := "to-be-deleted-soon-tag"
 	testhelper.MustRunCommand(t, nil, "git", "-C", testRepoPath, "tag", tagNameInput)
-	defer exec.Command("git", "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
+	defer exec.Command(command.GitPath(), "-C", testRepoPath, "tag", "-d", tagNameInput).Run()
 
 	request := &gitalypb.UserDeleteTagRequest{
 		Repository: testRepo,
diff --git a/internal/service/ref/refs_test.go b/internal/service/ref/refs_test.go
index cb98c62058..72c5070b29 100644
--- a/internal/service/ref/refs_test.go
+++ b/internal/service/ref/refs_test.go
@@ -13,6 +13,7 @@ import (
 	"github.com/golang/protobuf/ptypes/timestamp"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/git/log"
 	"gitlab.com/gitlab-org/gitaly/internal/git/updateref"
@@ -770,7 +771,7 @@ func TestFindAllTagNestedTags(t *testing.T) {
 	for _, tc := range testCases {
 		t.Run(tc.description, func(t *testing.T) {
 			tags := bytes.NewReader(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoCopyPath, "tag"))
-			testhelper.MustRunCommand(t, tags, "xargs", "git", "-C", testRepoCopyPath, "tag", "-d")
+			testhelper.MustRunCommand(t, tags, "xargs", command.GitPath(), "-C", testRepoCopyPath, "tag", "-d")
 
 			batch, err := catfile.New(ctx, testRepoCopy)
 			require.NoError(t, err)
@@ -1756,7 +1757,7 @@ func TestFindTagNestedTag(t *testing.T) {
 
 		t.Run(tc.description, func(t *testing.T) {
 			tags := bytes.NewReader(testhelper.MustRunCommand(t, nil, "git", "-C", testRepoCopyPath, "tag"))
-			testhelper.MustRunCommand(t, tags, "xargs", "git", "-C", testRepoCopyPath, "tag", "-d")
+			testhelper.MustRunCommand(t, tags, "xargs", command.GitPath(), "-C", testRepoCopyPath, "tag", "-d")
 
 			batch, err := catfile.New(ctx, testRepoCopy)
 			require.NoError(t, err)
diff --git a/internal/service/repository/calculate_checksum_test.go b/internal/service/repository/calculate_checksum_test.go
index 450127165a..949c403b30 100644
--- a/internal/service/repository/calculate_checksum_test.go
+++ b/internal/service/repository/calculate_checksum_test.go
@@ -7,6 +7,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -28,7 +29,7 @@ func TestSuccessfulCalculateChecksum(t *testing.T) {
 		require.NoError(t, os.MkdirAll(path.Join(testRepoPath, d), 0755))
 	}
 	require.NoError(t, exec.Command("cp", "testdata/checksum-test-packed-refs", path.Join(testRepoPath, "packed-refs")).Run())
-	require.NoError(t, exec.Command("git", "-C", testRepoPath, "symbolic-ref", "HEAD", "refs/heads/feature").Run())
+	require.NoError(t, exec.Command(command.GitPath(), "-C", testRepoPath, "symbolic-ref", "HEAD", "refs/heads/feature").Run())
 
 	request := &gitalypb.CalculateChecksumRequest{Repository: testRepo}
 	testCtx, cancelCtx := testhelper.Context()
diff --git a/internal/service/repository/cleanup_test.go b/internal/service/repository/cleanup_test.go
index 7b8256c170..d29aeb73af 100644
--- a/internal/service/repository/cleanup_test.go
+++ b/internal/service/repository/cleanup_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -239,7 +240,7 @@ func TestCleanupDisconnectedWorktrees(t *testing.T) {
 
 	if !pre2_20_0 {
 		err := exec.Command(
-			"git",
+			command.GitPath(),
 			testhelper.AddWorktreeArgs(testRepoPath, worktreePath)...,
 		).Run()
 		require.Error(t, err,
diff --git a/internal/service/repository/redirecting_test_server_test.go b/internal/service/repository/redirecting_test_server_test.go
index f5a2004884..3ae1e37d4b 100644
--- a/internal/service/repository/redirecting_test_server_test.go
+++ b/internal/service/repository/redirecting_test_server_test.go
@@ -44,7 +44,7 @@ func TestRedirectingServerRedirects(t *testing.T) {
 	httpServerState, redirectingServer := StartRedirectingTestServer()
 
 	// we only test for redirection, this command can fail after that
-	cmd := exec.Command("git", "-c", "http.followRedirects=true", "clone", "--bare", redirectingServer.URL, dir)
+	cmd := exec.Command(command.GitPath(), "-c", "http.followRedirects=true", "clone", "--bare", redirectingServer.URL, dir)
 	cmd.Env = append(command.GitEnv, cmd.Env...)
 	cmd.Run()
 
diff --git a/internal/service/repository/repack_test.go b/internal/service/repository/repack_test.go
index bd2ec0635f..39d89d3840 100644
--- a/internal/service/repository/repack_test.go
+++ b/internal/service/repository/repack_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/sirupsen/logrus"
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git/gittest"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -89,7 +90,7 @@ func TestRepackLocal(t *testing.T) {
 
 	commiterArgs := []string{"-c", "user.name=Scrooge McDuck", "-c", "user.email=scrooge@mcduck.com"}
 	cmdArgs := append(commiterArgs, "-C", repoPath, "commit", "--allow-empty", "-m", "An empty commit")
-	cmd := exec.Command("git", cmdArgs...)
+	cmd := exec.Command(command.GitPath(), cmdArgs...)
 	altObjectsDir := "./alt-objects"
 	altDirsCommit := testhelper.CreateCommitInAlternateObjectDirectory(t, repoPath, altObjectsDir, cmd)
 
diff --git a/internal/service/repository/search_files_test.go b/internal/service/repository/search_files_test.go
index ffb596196a..e3738d0d4e 100644
--- a/internal/service/repository/search_files_test.go
+++ b/internal/service/repository/search_files_test.go
@@ -11,6 +11,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -128,7 +129,7 @@ func TestSearchFilesByContentSuccessful(t *testing.T) {
 			ref:    "many_files",
 			output: contentOutputLines,
 			skip: func(t *testing.T) {
-				cmd := exec.Command("git", "-C", testRepoPath, "grep", "--perl-regexp", "(*LIMIT_MATCH=1)foobar", "many_files")
+				cmd := exec.Command(command.GitPath(), "-C", testRepoPath, "grep", "--perl-regexp", "(*LIMIT_MATCH=1)foobar", "many_files")
 				err := cmd.Run()
 				if exitError, ok := err.(*exec.ExitError); ok && exitError.ExitCode() == 128 {
 					t.Skip("Git does not seem to be compiled with support for PCRE2")
diff --git a/internal/service/repository/snapshot_test.go b/internal/service/repository/snapshot_test.go
index 8659b00b08..4802100fcb 100644
--- a/internal/service/repository/snapshot_test.go
+++ b/internal/service/repository/snapshot_test.go
@@ -16,6 +16,7 @@ import (
 	"github.com/stretchr/testify/assert"
 	"github.com/stretchr/testify/require"
 	"gitlab.com/gitlab-org/gitaly/internal/archive"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/catfile"
 	"gitlab.com/gitlab-org/gitaly/internal/helper"
@@ -127,7 +128,7 @@ func TestGetSnapshotWithDedupe(t *testing.T) {
 			const committerName = "Scrooge McDuck"
 			const committerEmail = "scrooge@mcduck.com"
 
-			cmd := exec.Command("git", "-C", repoPath,
+			cmd := exec.Command(command.GitPath(), "-C", repoPath,
 				"-c", fmt.Sprintf("user.name=%s", committerName),
 				"-c", fmt.Sprintf("user.email=%s", committerEmail),
 				"commit", "--allow-empty", "-m", "An empty commit")
@@ -147,7 +148,7 @@ func TestGetSnapshotWithDedupe(t *testing.T) {
 			require.NoError(t, ioutil.WriteFile(alternatesPath, []byte(path.Join(repoPath, ".git", fmt.Sprintf("%s\n", alternateObjDir))), 0644))
 
 			// write another commit and ensure we can find it
-			cmd = exec.Command("git", "-C", repoPath,
+			cmd = exec.Command(command.GitPath(), "-C", repoPath,
 				"-c", fmt.Sprintf("user.name=%s", committerName),
 				"-c", fmt.Sprintf("user.email=%s", committerEmail),
 				"commit", "--allow-empty", "-m", "Another empty commit")
@@ -203,7 +204,7 @@ func TestGetSnapshotWithDedupeSoftFailures(t *testing.T) {
 	committerName := "Scrooge McDuck"
 	committerEmail := "scrooge@mcduck.com"
 
-	cmd := exec.Command("git", "-C", repoPath,
+	cmd := exec.Command(command.GitPath(), "-C", repoPath,
 		"-c", fmt.Sprintf("user.name=%s", committerName),
 		"-c", fmt.Sprintf("user.email=%s", committerEmail),
 		"commit", "--allow-empty", "-m", "An empty commit")
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index 059ed950bf..ff48d20e35 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -15,6 +15,7 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/config"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/hooks"
@@ -317,7 +318,7 @@ func sshPush(t *testing.T, cloneDetails SSHCloneDetails, serverSocketPath string
 	})
 	require.NoError(t, err)
 
-	cmd := exec.Command("git", "-C", cloneDetails.LocalRepoPath, "push", "-v", "git@localhost:test/test.git", "master")
+	cmd := exec.Command(command.GitPath(), "-C", cloneDetails.LocalRepoPath, "push", "-v", "git@localhost:test/test.git", "master")
 	cmd.Env = []string{
 		fmt.Sprintf("GITALY_PAYLOAD=%s", payload),
 		fmt.Sprintf("GITALY_ADDRESS=%s", serverSocketPath),
diff --git a/internal/service/ssh/upload_archive_test.go b/internal/service/ssh/upload_archive_test.go
index 334e118648..d39f636f2f 100644
--- a/internal/service/ssh/upload_archive_test.go
+++ b/internal/service/ssh/upload_archive_test.go
@@ -9,6 +9,7 @@ import (
 
 	"github.com/golang/protobuf/jsonpb"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 	"google.golang.org/grpc/codes"
@@ -101,7 +102,7 @@ func TestUploadArchiveSuccess(t *testing.T) {
 	serverSocketPath, stop := runSSHServer(t)
 	defer stop()
 
-	cmd := exec.Command("git", "archive", "master", "--remote=git@localhost:test/test.git")
+	cmd := exec.Command(command.GitPath(), "archive", "master", "--remote=git@localhost:test/test.git")
 
 	err := testArchive(t, serverSocketPath, testRepo, cmd)
 	require.NoError(t, err)
diff --git a/internal/service/ssh/upload_pack_test.go b/internal/service/ssh/upload_pack_test.go
index 351364418a..5b147eedf7 100644
--- a/internal/service/ssh/upload_pack_test.go
+++ b/internal/service/ssh/upload_pack_test.go
@@ -14,6 +14,7 @@ import (
 	"github.com/prometheus/client_golang/prometheus"
 	promtest "github.com/prometheus/client_golang/prometheus/testutil"
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 	"gitlab.com/gitlab-org/gitaly/internal/testhelper"
@@ -205,12 +206,12 @@ func TestUploadPackCloneSuccess(t *testing.T) {
 		deepen float64
 	}{
 		{
-			cmd:    exec.Command("git", "clone", "git@localhost:test/test.git", localRepoPath),
+			cmd:    exec.Command(command.GitPath(), "clone", "git@localhost:test/test.git", localRepoPath),
 			desc:   "full clone",
 			deepen: 0,
 		},
 		{
-			cmd:    exec.Command("git", "clone", "--depth", "1", "git@localhost:test/test.git", localRepoPath),
+			cmd:    exec.Command(command.GitPath(), "clone", "--depth", "1", "git@localhost:test/test.git", localRepoPath),
 			desc:   "shallow clone",
 			deepen: 1,
 		},
@@ -271,7 +272,7 @@ func TestUploadPackCloneWithPartialCloneFilter(t *testing.T) {
 			localPath := path.Join(testRepoRoot, fmt.Sprintf("gitlab-test-upload-pack-local-%s", tc.desc))
 			cmd := cloneCommand{
 				repository: testRepo,
-				command:    exec.Command("git", append(tc.cloneArgs, localPath)...),
+				command:    exec.Command(command.GitPath(), append(tc.cloneArgs, localPath)...),
 				server:     serverSocketPath,
 			}
 			err := cmd.execute(t)
@@ -298,11 +299,11 @@ func TestUploadPackCloneSuccessWithGitProtocol(t *testing.T) {
 		desc string
 	}{
 		{
-			cmd:  exec.Command("git", "clone", "git@localhost:test/test.git", localRepoPath),
+			cmd:  exec.Command(command.GitPath(), "clone", "git@localhost:test/test.git", localRepoPath),
 			desc: "full clone",
 		},
 		{
-			cmd:  exec.Command("git", "clone", "--depth", "1", "git@localhost:test/test.git", localRepoPath),
+			cmd:  exec.Command(command.GitPath(), "clone", "--depth", "1", "git@localhost:test/test.git", localRepoPath),
 			desc: "shallow clone",
 		},
 	}
@@ -335,7 +336,7 @@ func TestUploadPackCloneHideTags(t *testing.T) {
 
 	cmd := cloneCommand{
 		repository: testRepo,
-		command:    exec.Command("git", "clone", "--mirror", "git@localhost:test/test.git", localRepoPath),
+		command:    exec.Command(command.GitPath(), "clone", "--mirror", "git@localhost:test/test.git", localRepoPath),
 		server:     serverSocketPath,
 		gitConfig:  "transfer.hideRefs=refs/tags",
 	}
@@ -360,7 +361,7 @@ func TestUploadPackCloneFailure(t *testing.T) {
 			StorageName:  "foobar",
 			RelativePath: testRepo.GetRelativePath(),
 		},
-		command: exec.Command("git", "clone", "git@localhost:test/test.git", localRepoPath),
+		command: exec.Command(command.GitPath(), "clone", "git@localhost:test/test.git", localRepoPath),
 		server:  serverSocketPath,
 	}
 	err := cmd.execute(t)
diff --git a/internal/testhelper/commit.go b/internal/testhelper/commit.go
index d8199882ba..3769f0b665 100644
--- a/internal/testhelper/commit.go
+++ b/internal/testhelper/commit.go
@@ -10,6 +10,7 @@ import (
 	"testing"
 
 	"github.com/stretchr/testify/require"
+	"gitlab.com/gitlab-org/gitaly/internal/command"
 	"gitlab.com/gitlab-org/gitaly/internal/helper/text"
 )
 
@@ -80,7 +81,7 @@ func CreateCommitInAlternateObjectDirectory(t testing.TB, repoPath, altObjectsDi
 		t.Fatalf("stdout: %s, stderr: %s", output, stderr)
 	}
 
-	cmd = exec.Command("git", "-C", repoPath, "rev-parse", "HEAD")
+	cmd = exec.Command(command.GitPath(), "-C", repoPath, "rev-parse", "HEAD")
 	cmd.Env = gitObjectEnv
 	currentHead, err := cmd.Output()
 	require.NoError(t, err)
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 16084860db..80db0ddf1c 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -216,15 +216,17 @@ func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string)
 		t.Helper()
 	}
 
-	cmd := exec.Command(name, args...)
-
+	var cmd *exec.Cmd
 	if name == "git" {
+		cmd = exec.Command(command.GitPath(), args...)
 		cmd.Env = os.Environ()
 		cmd.Env = append(command.GitEnv, cmd.Env...)
 		cmd.Env = append(cmd.Env,
 			"GIT_AUTHOR_DATE=1572776879 +0100",
 			"GIT_COMMITTER_DATE=1572776879 +0100",
 		)
+	} else {
+		cmd = exec.Command(name, args...)
 	}
 
 	if stdin != nil {
@@ -678,7 +680,7 @@ func GitObjectMustNotExist(t testing.TB, repoPath, sha string) {
 }
 
 func gitObjectExists(t testing.TB, repoPath, sha string, exists bool) {
-	cmd := exec.Command("git", "-C", repoPath, "cat-file", "-e", sha)
+	cmd := exec.Command(command.GitPath(), "-C", repoPath, "cat-file", "-e", sha)
 	cmd.Env = []string{
 		"GIT_ALLOW_PROTOCOL=", // To prevent partial clone reaching remote repo over SSH
 	}
-- 
GitLab


From fc5219d4ffc79a64164f83c7fd488483ad0bb45e Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 12:29:56 +0200
Subject: [PATCH 2/9] 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.
---
 internal/service/smarthttp/inforefs_test.go   |  4 +-
 .../service/smarthttp/receive_pack_test.go    |  2 +-
 internal/service/smarthttp/testdata/env_git   |  1 -
 .../service/smarthttp/upload_pack_test.go     |  2 +-
 internal/service/ssh/receive_pack_test.go     | 10 ++---
 internal/service/ssh/testdata/env_git         |  1 -
 internal/service/ssh/upload_pack_test.go      | 14 +++----
 internal/testhelper/env_git                   |  4 --
 internal/testhelper/git_protocol.go           | 39 ++++++++++++++-----
 9 files changed, 46 insertions(+), 31 deletions(-)
 delete mode 120000 internal/service/smarthttp/testdata/env_git
 delete mode 120000 internal/service/ssh/testdata/env_git
 delete mode 100755 internal/testhelper/env_git

diff --git a/internal/service/smarthttp/inforefs_test.go b/internal/service/smarthttp/inforefs_test.go
index e87b007e6e..18efdca100 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 eeec6e91a5..c4bf736fc6 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 a01a70012e..0000000000
--- 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 7b6b6e6317..2077904a60 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 ff48d20e35..f61d7211f2 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 a01a70012e..0000000000
--- 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 5b147eedf7..b59ce5b2dd 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 7739eaeef9..0000000000
--- 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 ab03775518..a57b85906b 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()
 	}
 }
-- 
GitLab


From 8941c9126692fef30fd136a21e5a7158634edbcc Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 13:54:55 +0200
Subject: [PATCH 3/9] tests: Fix hook scripts using Git from PATH

In various tests we're setting up hooks in order to verify certain
operations behave as we intend them to. Some of those hooks we have
invoke Git again, but they'll use the PATH variable to do so. As a
result, they may use the wrong Git executable instead of the configured
one.

Fix this by generating hook scripts with the configured and resolved Git
path.
---
 internal/service/operations/tags_test.go      | 71 +++++++++++++++++--
 .../testdata/assert_git_object_type.rb        |  8 ---
 .../testdata/pre-receive-expect-object-type   | 12 ----
 .../testdata/update-expect-object-type        |  9 ---
 .../service/smarthttp/receive_pack_test.go    |  3 +-
 internal/service/ssh/receive_pack_test.go     |  3 +-
 internal/testhelper/testhelper.go             | 23 ++++--
 7 files changed, 86 insertions(+), 43 deletions(-)
 delete mode 100644 internal/service/operations/testdata/assert_git_object_type.rb
 delete mode 100755 internal/service/operations/testdata/pre-receive-expect-object-type
 delete mode 100755 internal/service/operations/testdata/update-expect-object-type

diff --git a/internal/service/operations/tags_test.go b/internal/service/operations/tags_test.go
index 42479cd2c4..8f774f454a 100644
--- a/internal/service/operations/tags_test.go
+++ b/internal/service/operations/tags_test.go
@@ -3,9 +3,10 @@ package operations
 import (
 	"context"
 	"fmt"
+	"io/ioutil"
 	"os"
 	"os/exec"
-	"path/filepath"
+	"path"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -109,6 +110,65 @@ func testSuccessfulGitHooksForUserDeleteTagRequest(t *testing.T, ctx context.Con
 	}
 }
 
+func writeAssertObjectTypePreReceiveHook(t *testing.T) (string, func()) {
+	t.Helper()
+
+	hook := fmt.Sprintf(`#!/usr/bin/env ruby
+
+commands = STDIN.each_line.map(&:chomp)
+unless commands.size == 1
+  abort "expected 1 ref update command, got #{commands.size}"
+end
+
+new_value = commands[0].split(' ', 3)[1]
+abort 'missing new_value' unless new_value
+
+out = IO.popen(%%W[%s cat-file -t #{new_value}], &:read)
+abort 'cat-file failed' unless $?.success?
+
+unless out.chomp == ARGV[0]
+  abort "error: expected #{ARGV[0]} object, got #{out}"
+end`, command.GitPath())
+
+	dir, err := ioutil.TempDir("", "gitaly-temp-dir-*")
+	require.NoError(t, err)
+	hookPath := path.Join(dir, "pre-receive")
+
+	require.NoError(t, ioutil.WriteFile(hookPath, []byte(hook), 0755))
+
+	return hookPath, func() {
+		os.RemoveAll(dir)
+	}
+}
+
+func writeAssertObjectTypeUpdateHook(t *testing.T) (string, func()) {
+	t.Helper()
+
+	hook := fmt.Sprintf(`#!/usr/bin/env ruby
+
+expected_object_type = ARGV.shift
+new_value = ARGV[2]
+
+abort "missing new_value" unless new_value
+
+out = IO.popen(%%W[%s cat-file -t #{new_value}], &:read)
+abort 'cat-file failed' unless $?.success?
+
+unless out.chomp == expected_object_type
+  abort "error: expected #{expected_object_type} object, got #{out}"
+end`, command.GitPath())
+
+	dir, err := ioutil.TempDir("", "gitaly-temp-dir-*")
+	require.NoError(t, err)
+	hookPath := path.Join(dir, "pre-receive")
+
+	require.NoError(t, ioutil.WriteFile(hookPath, []byte(hook), 0755))
+
+	return hookPath, func() {
+		os.RemoveAll(dir)
+	}
+}
+
 func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 	featureSets, err := testhelper.NewFeatureSets([]featureflag.FeatureFlag{featureflag.ReferenceTransactions})
 	require.NoError(t, err)
@@ -135,10 +195,11 @@ func TestSuccessfulUserCreateTagRequest(t *testing.T) {
 
 			inputTagName := "to-be-créated-soon"
 
-			cwd, err := os.Getwd()
-			require.NoError(t, err)
-			preReceiveHook := filepath.Join(cwd, "testdata/pre-receive-expect-object-type")
-			updateHook := filepath.Join(cwd, "testdata/update-expect-object-type")
+			preReceiveHook, cleanup := writeAssertObjectTypePreReceiveHook(t)
+			defer cleanup()
+
+			updateHook, cleanup := writeAssertObjectTypeUpdateHook(t)
+			defer cleanup()
 
 			testCases := []struct {
 				desc               string
diff --git a/internal/service/operations/testdata/assert_git_object_type.rb b/internal/service/operations/testdata/assert_git_object_type.rb
deleted file mode 100644
index 2e1be4b09d..0000000000
--- a/internal/service/operations/testdata/assert_git_object_type.rb
+++ /dev/null
@@ -1,8 +0,0 @@
-def assert_git_object_type!(oid, expected_type)
-  out = IO.popen(%W[git cat-file -t #{oid}], &:read)
-  abort 'cat-file failed' unless $?.success?
-
-  unless out.chomp == expected_type
-    abort "error: expected #{expected_type} object, got #{out}"
-  end
-end
diff --git a/internal/service/operations/testdata/pre-receive-expect-object-type b/internal/service/operations/testdata/pre-receive-expect-object-type
deleted file mode 100755
index 10c55de3ab..0000000000
--- a/internal/service/operations/testdata/pre-receive-expect-object-type
+++ /dev/null
@@ -1,12 +0,0 @@
-#!/usr/bin/env ruby
-require_relative 'assert_git_object_type.rb'
-
-commands = STDIN.each_line.map(&:chomp)
-unless commands.size == 1
-  abort "expected 1 ref update command, got #{commands.size}"
-end
-
-new_value = commands[0].split(' ', 3)[1]
-abort 'missing new_value' unless new_value
-
-assert_git_object_type!(new_value, ARGV[0])
diff --git a/internal/service/operations/testdata/update-expect-object-type b/internal/service/operations/testdata/update-expect-object-type
deleted file mode 100755
index 8889e3e3c9..0000000000
--- a/internal/service/operations/testdata/update-expect-object-type
+++ /dev/null
@@ -1,9 +0,0 @@
-#!/usr/bin/env ruby
-require_relative 'assert_git_object_type.rb'
-
-expected_object_type = ARGV.shift
-new_value = ARGV[2]
-
-abort "missing new_value" unless new_value
-
-assert_git_object_type!(new_value, expected_object_type)
diff --git a/internal/service/smarthttp/receive_pack_test.go b/internal/service/smarthttp/receive_pack_test.go
index c4bf736fc6..64e472aa08 100644
--- a/internal/service/smarthttp/receive_pack_test.go
+++ b/internal/service/smarthttp/receive_pack_test.go
@@ -416,7 +416,8 @@ func testPostReceivePackToHooks(t *testing.T, ctx context.Context) {
 	testhelper.WriteTemporaryGitlabShellConfigFile(t, tempGitlabShellDir, testhelper.GitlabShellConfig{GitlabURL: ts.URL})
 	testhelper.WriteShellSecretFile(t, tempGitlabShellDir, secretToken)
 
-	testhelper.WriteCustomHook(testRepoPath, "pre-receive", []byte(testhelper.CheckNewObjectExists))
+	cleanup = testhelper.WriteCheckNewObjectExistsHook(t, testRepoPath)
+	defer cleanup()
 
 	config.Config.Gitlab.URL = ts.URL
 	config.Config.Gitlab.SecretFile = filepath.Join(tempGitlabShellDir, ".gitlab_shell_secret")
diff --git a/internal/service/ssh/receive_pack_test.go b/internal/service/ssh/receive_pack_test.go
index f61d7211f2..613adf085b 100644
--- a/internal/service/ssh/receive_pack_test.go
+++ b/internal/service/ssh/receive_pack_test.go
@@ -248,7 +248,8 @@ func TestSSHReceivePackToHooks(t *testing.T) {
 	config.Config.Gitlab.URL = ts.URL
 	config.Config.Gitlab.SecretFile = filepath.Join(tempGitlabShellDir, ".gitlab_shell_secret")
 
-	testhelper.WriteCustomHook(cloneDetails.RemoteRepoPath, "pre-receive", []byte(testhelper.CheckNewObjectExists))
+	cleanup = testhelper.WriteCheckNewObjectExistsHook(t, cloneDetails.RemoteRepoPath)
+	defer cleanup()
 
 	lHead, rHead, err := sshPush(t, cloneDetails, serverSocketPath, pushParams{
 		storageName:  testRepo.GetStorageName(),
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index 80db0ddf1c..df95450131 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -764,17 +764,26 @@ func WriteExecutable(path string, content []byte) (func(), error) {
 	}, ioutil.WriteFile(path, content, 0755)
 }
 
-// CheckNewObjectExists is a script meant to be used as a pre-receive custom hook.
-// It only succeeds if it can find the object in the quarantine directory.
-// if GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES were not passed through correctly to the hooks,
-// it will fail
-const CheckNewObjectExists = `#!/usr/bin/env ruby
+// WriteCheckNewObjectExistsHook writes a pre-receive hook which only succeeds
+// if it can find the object in the quarantine directory. if
+// GIT_OBJECT_DIRECTORY and GIT_ALTERNATE_OBJECT_DIRECTORIES were not passed
+// through correctly to the hooks, it will fail
+func WriteCheckNewObjectExistsHook(t *testing.T, repoPath string) func() {
+	hook := fmt.Sprintf(`#!/usr/bin/env ruby
 STDIN.each_line do |line|
   new_object = line.split(' ')[1]
   exit 1 unless new_object
-  exit 1 unless	system(*%W[git cat-file -e #{new_object}])
+  exit 1 unless	system(*%%W[%s cat-file -e #{new_object}])
 end
-`
+`, command.GitPath())
+
+	ioutil.WriteFile("/tmp/file", []byte(hook), 0644)
+
+	cleanup, err := WriteCustomHook(repoPath, "pre-receive", []byte(hook))
+	require.NoError(t, err)
+
+	return cleanup
+}
 
 func WriteBlobs(t testing.TB, testRepoPath string, n int) []string {
 	var blobIDs []string
-- 
GitLab


From 7471d0b42e4c5250ed37e08c5de224eb87a900be Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 15:19:43 +0200
Subject: [PATCH 4/9] rspec: Fix tests using system-provided Git

Two of our helpers in the rspec tests use the system Git instead of the
configured one. While this isn't much of a problem by itself as it's
only tests, we're going to change test execution so that we can verify
only the configured Git executable is ever executed. Executing Git via
the PATH variable is always going to fail with this setup.

Prepare for this by honoring the GITALY_TESTING_GIT_BINARY environment
variable like our Go tests do. Convert callers to use the configured
value. As there's now a dependency cycle between the spec helper and the
test repo helper because the test repo helper depends on the overrides
defined in the spec helper, this commit also moves those overrides into
the test repo helper.
---
 ruby/spec/spec_helper.rb      | 4 ----
 ruby/spec/test_repo_helper.rb | 8 ++++++--
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/ruby/spec/spec_helper.rb b/ruby/spec/spec_helper.rb
index 49e956f921..93a600b7fb 100644
--- a/ruby/spec/spec_helper.rb
+++ b/ruby/spec/spec_helper.rb
@@ -12,10 +12,6 @@ require File.join(__dir__, 'support/helpers/gitlab_shell_helper.rb')
 
 Dir[File.join(__dir__, 'support/helpers/*.rb')].each { |f| require f }
 
-Gitlab.config.git.test_global_ivar_override(:bin_path, 'git')
-Gitlab.config.git.test_global_ivar_override(:hooks_directory, File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks"))
-Gitlab.config.gitaly.test_global_ivar_override(:bin_dir, __dir__)
-
 RSpec.configure do |config|
   config.include FactoryBot::Syntax::Methods
 
diff --git a/ruby/spec/test_repo_helper.rb b/ruby/spec/test_repo_helper.rb
index 5250bfaa77..4ae4c6fe97 100644
--- a/ruby/spec/test_repo_helper.rb
+++ b/ruby/spec/test_repo_helper.rb
@@ -5,6 +5,10 @@ require 'rugged'
 $:.unshift(File.expand_path('../proto', __dir__))
 require 'gitaly'
 
+Gitlab.config.git.test_global_ivar_override(:bin_path, ENV.fetch('GITALY_TESTING_GIT_BINARY', 'git'))
+Gitlab.config.git.test_global_ivar_override(:hooks_directory, File.join(Gitlab.config.gitlab_shell.path.to_s, "hooks"))
+Gitlab.config.gitaly.test_global_ivar_override(:bin_dir, __dir__)
+
 DEFAULT_STORAGE_DIR = File.expand_path('../tmp/repositories', __dir__)
 DEFAULT_STORAGE_NAME = 'default'.freeze
 TEST_REPO_PATH = File.join(DEFAULT_STORAGE_DIR, 'gitlab-test.git')
@@ -104,13 +108,13 @@ module TestRepo
   end
 
   def self.clone_new_repo!(origin, destination)
-    return if system("git", "clone", "--quiet", "--bare", origin.to_s, destination.to_s)
+    return if system(Gitlab.config.git.bin_path, "clone", "--quiet", "--bare", origin.to_s, destination.to_s)
 
     abort "Failed to clone test repo. Try running 'make prepare-tests' and try again."
   end
 
   def self.init_new_repo!(destination)
-    return if system("git", "init", "--quiet", "--bare", destination.to_s)
+    return if system(Gitlab.config.git.bin_path, "init", "--quiet", "--bare", destination.to_s)
 
     abort "Failed to init test repo."
   end
-- 
GitLab


From 8cf48086d19d1d53f535864c528a6c9888c99077 Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 13:28:23 +0200
Subject: [PATCH 5/9] packfile: Fix `git show-index` using wrong Git executable

When reading the index in `internal/git/packfile`, we're directly
executing the "git" executable which is thus going to be resolved via
the PATH environment variable.

Fix this to instead use the configured Git command.
---
 internal/git/packfile/index.go | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/internal/git/packfile/index.go b/internal/git/packfile/index.go
index 8f61544c36..29640bcb12 100644
--- a/internal/git/packfile/index.go
+++ b/internal/git/packfile/index.go
@@ -93,7 +93,7 @@ func ReadIndex(idxPath string) (*Index, error) {
 		return nil, err
 	}
 
-	showIndex, err := command.New(ctx, exec.Command("git", "show-index"), f, nil, nil)
+	showIndex, err := command.New(ctx, exec.Command(command.GitPath(), "show-index"), f, nil, nil)
 	if err != nil {
 		return nil, err
 	}
-- 
GitLab


From 489e4eace7fb09c42771e94e7fb2eee52f5de6c2 Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 15:05:58 +0200
Subject: [PATCH 6/9] linguist: Use configured Git executable

When executing git-linguist, one of the first things it'll do is verify
whether it's running in a Git directory by executing `git rev-parse
--git-dir`. git-linguist doesn't allow specifying which Git executable
is executed here, so it'll always use the first one it finds in PATH,
which isn't necessarily the one configured in Gitaly's configuration.

Fix the issue by always prepending the configured Git executable's
directory to PATH previous to executing git-linguist. As our own command
interface will overwrite and PATH environment variables passed to it, we
need to use a hack here and specify the PATH variable by executing
git-linguist with `env PATH=$GITDIR:$PATH`.
---
 internal/linguist/linguist.go | 27 ++++++++++++++++++++++++++-
 1 file changed, 26 insertions(+), 1 deletion(-)

diff --git a/internal/linguist/linguist.go b/internal/linguist/linguist.go
index 252e360473..523fe9e6a1 100644
--- a/internal/linguist/linguist.go
+++ b/internal/linguist/linguist.go
@@ -74,7 +74,32 @@ func LoadColors(cfg config.Cfg) error {
 }
 
 func startGitLinguist(ctx context.Context, repoPath string, commitID string, linguistCommand string) (*command.Command, error) {
-	cmd := exec.Command("bundle", "exec", "bin/ruby-cd", repoPath, "git-linguist", "--commit="+commitID, linguistCommand)
+	args := []string{
+		"bundle",
+		"exec",
+		"bin/ruby-cd",
+		repoPath,
+		"git-linguist",
+		"--commit=" + commitID,
+		linguistCommand,
+	}
+
+	// This is a horrible hack. git-linguist will execute `git rev-parse
+	// --git-dir` to check whether it is in a Git directory or not. We don't
+	// want to use the one provided by PATH, but instead the one specified
+	// via the configuration. git-linguist doesn't specify any way to choose
+	// a different Git implementation, so we need to prepend the configured
+	// Git's directory to PATH. But as our internal command interface will
+	// overwrite PATH even if we pass it in here, we need to work around it
+	// and instead execute the command with `env PATH=$GITDIR:$PATH`.
+	gitDir := path.Dir(command.GitPath())
+	if path, ok := os.LookupEnv("PATH"); ok && gitDir != "." {
+		args = append([]string{
+			"env", fmt.Sprintf("PATH=%s:%s", gitDir, path),
+		}, args...)
+	}
+
+	cmd := exec.Command(args[0], args[1:]...)
 	cmd.Dir = config.Config.Ruby.Dir
 
 	internalCmd, err := command.New(ctx, cmd, nil, nil, nil, exportEnvironment()...)
-- 
GitLab


From 1184a055dea81ff9f2f72216823a87b8a003ae94 Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Thu, 20 Aug 2020 07:38:58 +0200
Subject: [PATCH 7/9] Makefile: Inline check-mod-tidy

With PATH now containing a broken version of Git by design, we need to
adjust all callers of Git to use the real executable instead of the
broken one. The script `check-mod-tidy` is one of those which needs
adjusting.

Given it's such a tiny script, let's just inline it into our Makefile
and have it use the `${GIT}` variable.
---
 Makefile                |  4 +++-
 _support/check-mod-tidy | 19 -------------------
 2 files changed, 3 insertions(+), 20 deletions(-)
 delete mode 100755 _support/check-mod-tidy

diff --git a/Makefile b/Makefile
index ca998c0426..16d4d44157 100644
--- a/Makefile
+++ b/Makefile
@@ -205,7 +205,9 @@ verify: check-mod-tidy check-formatting notice-up-to-date check-proto rubocop
 
 .PHONY: check-mod-tidy
 check-mod-tidy:
-	${Q}${SOURCE_DIR}/_support/check-mod-tidy
+	${Q}${GIT} diff --quiet --exit-code go.mod go.sum || (echo "error: uncommitted changes in go.mod or go.sum" && exit 1)
+	${Q}go mod tidy
+	${Q}${GIT} diff --quiet --exit-code go.mod go.sum || (echo "error: uncommitted changes in go.mod or go.sum" && exit 1)
 
 .PHONY: lint
 lint: ${GOLANGCI_LINT}
diff --git a/_support/check-mod-tidy b/_support/check-mod-tidy
deleted file mode 100755
index 2bce952211..0000000000
--- a/_support/check-mod-tidy
+++ /dev/null
@@ -1,19 +0,0 @@
-#!/usr/bin/env ruby
-
-require_relative 'run.rb'
-
-def main
-  mod_not_changed!
-  run!(%w[go mod tidy])
-  mod_not_changed!
-end
-
-def mod_not_changed!
-  %w[go.mod go.sum].each do |f|
-    unless system(*%W[git diff --quiet --exit-code #{f}])
-      abort "error: uncommitted changes in #{f}"
-    end
-  end
-end
-
-main
-- 
GitLab


From 658cd92c2f23e27ebcc066108f645e609ee1dfd4 Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Tue, 18 Aug 2020 10:23:50 +0200
Subject: [PATCH 8/9] config: Allow overriding Git binary via environment

In order to allow easy testing of Gitaly against custom Git binaries and
different versions thereof, this commit introduces a new environment
variable `GITALY_TESTING_GIT_BINARY`. If this variable is set, then
we'll use its value as the Git binary for testing.
---
 internal/config/config.go      |  5 +++++
 internal/config/config_test.go | 25 ++++++++++++++++---------
 2 files changed, 21 insertions(+), 9 deletions(-)

diff --git a/internal/config/config.go b/internal/config/config.go
index c7aef2f084..c43d4b6a81 100644
--- a/internal/config/config.go
+++ b/internal/config/config.go
@@ -323,6 +323,11 @@ func SetGitPath() error {
 		return nil
 	}
 
+	if path, ok := os.LookupEnv("GITALY_TESTING_GIT_BINARY"); ok {
+		Config.Git.BinPath = path
+		return nil
+	}
+
 	resolvedPath, err := exec.LookPath("git")
 	if err != nil {
 		return err
diff --git a/internal/config/config_test.go b/internal/config/config_test.go
index b747e20a45..383ffb61db 100644
--- a/internal/config/config_test.go
+++ b/internal/config/config_test.go
@@ -438,6 +438,10 @@ func TestValidateHooks(t *testing.T) {
 }
 
 func TestLoadGit(t *testing.T) {
+	defer func(oldGitSettings Git) {
+		Config.Git = oldGitSettings
+	}(Config.Git)
+
 	tmpFile := configFileReader(`[git]
 bin_path = "/my/git/path"
 catfile_cache_size = 50
@@ -455,10 +459,13 @@ func TestSetGitPath(t *testing.T) {
 		Config.Git = oldGitSettings
 	}(Config.Git)
 
-	resolvedGitPath, err := exec.LookPath("git")
-
-	if err != nil {
-		t.Fatal(err)
+	var resolvedGitPath string
+	if path, ok := os.LookupEnv("GITALY_TESTING_GIT_BINARY"); ok {
+		resolvedGitPath = path
+	} else {
+		path, err := exec.LookPath("git")
+		require.NoError(t, err)
+		resolvedGitPath = path
 	}
 
 	testCases := []struct {
@@ -479,11 +486,11 @@ func TestSetGitPath(t *testing.T) {
 	}
 
 	for _, tc := range testCases {
-		Config.Git.BinPath = tc.gitBinPath
-
-		SetGitPath()
-
-		assert.Equal(t, tc.expected, Config.Git.BinPath, tc.desc)
+		t.Run(tc.desc, func(t *testing.T) {
+			Config.Git.BinPath = tc.gitBinPath
+			SetGitPath()
+			assert.Equal(t, tc.expected, Config.Git.BinPath, tc.desc)
+		})
 	}
 }
 
-- 
GitLab


From 6f2467328e066fbaaeb65ec33b06c967b354f8f8 Mon Sep 17 00:00:00 2001
From: Patrick Steinhardt <psteinhardt@gitlab.com>
Date: Wed, 19 Aug 2020 13:04:28 +0200
Subject: [PATCH 9/9] tests: Verify we never call Git resolved via PATH

When configuring Gitaly setups, admins can choose which Git executable
they want Gitaly to use for all Git invocations by specifing the
`config.git_path` key. Because of this, Gitaly is never allowed to
lookup Git via the PATH environment, but always needs to consult the
configured value. While most of our code (except for tests which have
been fixed with preceding commits) always honored this, there were two
locations which didn't, which wen't silently under the radar waiting for
trouble.

This commit introduces an improved test setup which will always catch
such bugs: we simply set up a Git executable in a separate directory
which writes an error message and immediately aborts with a strange
error value and then adjust our PATH variable to have the executable's
directory as first item. As a result, whenever Git is being executed
with an unqualified path, we'll pick this binary instead of the real Git
and produce an error. Tests would quickly catch this, e.g. like the
following:

```
--- FAIL: TestSuccessfulIsAncestorRequestWithAltGitObjectDirs (0.04s)
    commit.go:80: stdout: , stderr: Git executable from $PATH was picked up. Please fix code to use `command.GitPath()` instead.
```

The real Git binary is injected via the `GITALY_TESTING_GIT_BINARY`
environment variable, which is being picked up by our configuration
code.
---
 Makefile                                  | 24 ++++++++++++-----------
 internal/testhelper/testdata/home/bin/git | 12 ++++++++++++
 2 files changed, 25 insertions(+), 11 deletions(-)
 create mode 100755 internal/testhelper/testdata/home/bin/git

diff --git a/Makefile b/Makefile
index 16d4d44157..236f3161cb 100644
--- a/Makefile
+++ b/Makefile
@@ -34,6 +34,7 @@ ASSEMBLY_ROOT    ?= ${BUILD_DIR}/assembly
 GIT_PREFIX       ?= ${GIT_INSTALL_DIR}
 
 # Tools
+GIT               := $(shell which git)
 GOIMPORTS         := ${BUILD_DIR}/bin/goimports
 GITALYFMT         := ${BUILD_DIR}/bin/gitalyfmt
 GOLANGCI_LINT     := ${BUILD_DIR}/bin/golangci-lint
@@ -111,9 +112,10 @@ find_go_sources  = $(shell find ${SOURCE_DIR} -type d \( -name ruby -o -name ven
 find_go_packages = $(dir $(call find_go_sources, 's|[^/]*\.go||'))
 
 unexport GOROOT
-export GOBIN        = ${BUILD_DIR}/bin
-export GOPROXY     ?= https://proxy.golang.org
-export PATH        := ${BUILD_DIR}/bin:${PATH}
+export GOBIN                      = ${BUILD_DIR}/bin
+export GOPROXY                   ?= https://proxy.golang.org
+export PATH                      := ${SOURCE_DIR}/internal/testhelper/testdata/home/bin:${BUILD_DIR}/bin:${PATH}
+export GITALY_TESTING_GIT_BINARY ?= ${GIT}
 
 .NOTPARALLEL:
 
@@ -292,7 +294,7 @@ proto-lint: ${PROTOC} ${PROTOC_GEN_GO}
 
 .PHONY: no-changes
 no-changes:
-	${Q}git status --porcelain | awk '{ print } END { if (NR > 0) { exit 1 } }'
+	${Q}${GIT} status --porcelain | awk '{ print } END { if (NR > 0) { exit 1 } }'
 
 .PHONY: smoke-test
 smoke-test: all rspec
@@ -308,8 +310,8 @@ download-git: ${BUILD_DIR}/git_full_bins.tgz
 build-git:
 	${Q}echo "Getting Git from ${GIT_REPO_URL}"
 	${Q}rm -rf ${GIT_SOURCE_DIR} ${GIT_INSTALL_DIR}
-	git clone ${GIT_REPO_URL} ${GIT_SOURCE_DIR}
-	git -C ${GIT_SOURCE_DIR} checkout ${GIT_VERSION}
+	${GIT} clone ${GIT_REPO_URL} ${GIT_SOURCE_DIR}
+	${GIT} -C ${GIT_SOURCE_DIR} checkout ${GIT_VERSION}
 	${Q}rm -rf ${GIT_INSTALL_DIR}
 	${Q}mkdir -p ${GIT_INSTALL_DIR}
 	${MAKE} -C ${GIT_SOURCE_DIR} -j$(shell nproc) prefix=${GIT_PREFIX} ${GIT_BUILD_OPTIONS} install
@@ -377,20 +379,20 @@ ${GOLANGCI_LINT}: Makefile ${BUILD_DIR}/go.mod | ${BUILD_DIR}/bin
 	${Q}cd ${BUILD_DIR} && go get github.com/golangci/golangci-lint/cmd/golangci-lint@v${GOLANGCI_LINT_VERSION}
 
 ${TEST_REPO}:
-	git clone --bare --quiet https://gitlab.com/gitlab-org/gitlab-test.git $@
+	${GIT} clone --bare --quiet https://gitlab.com/gitlab-org/gitlab-test.git $@
 	# Git notes aren't fetched by default with git clone
-	git -C $@ fetch origin refs/notes/*:refs/notes/*
+	${GIT} -C $@ fetch origin refs/notes/*:refs/notes/*
 	rm -rf $@/refs
 	mkdir -p $@/refs/heads $@/refs/tags
 	cp ${SOURCE_DIR}/_support/gitlab-test.git-packed-refs $@/packed-refs
-	git -C $@ fsck --no-progress
+	${GIT} -C $@ fsck --no-progress
 
 ${TEST_REPO_GIT}:
-	git clone --bare --quiet https://gitlab.com/gitlab-org/gitlab-git-test.git $@
+	${GIT} clone --bare --quiet https://gitlab.com/gitlab-org/gitlab-git-test.git $@
 	rm -rf $@/refs
 	mkdir -p $@/refs/heads $@/refs/tags
 	cp ${SOURCE_DIR}/_support/gitlab-git-test.git-packed-refs $@/packed-refs
-	git -C $@ fsck --no-progress
+	${GIT} -C $@ fsck --no-progress
 
 ${GITLAB_SHELL_DIR}/config.yml: ${GITLAB_SHELL_DIR}/config.yml.example
 	cp $< $@
diff --git a/internal/testhelper/testdata/home/bin/git b/internal/testhelper/testdata/home/bin/git
new file mode 100755
index 0000000000..78f4de70b8
--- /dev/null
+++ b/internal/testhelper/testdata/home/bin/git
@@ -0,0 +1,12 @@
+#!/bin/sh
+
+# This wrapper has the single intention of catching any Git invocations via
+# $PATH instead of via either our Git DSL or via `config.GitPath()`. Our tests
+# are thus set up with PATH including this binary. As the binary always prints
+# an error message and exits with a weird status code, tests should fail
+# quickly and with a hopefully helpful message making the actual error quick to
+# spot.
+
+echo 'Git executable from $PATH was picked up. Please fix code to use `command.GitPath()` instead.' >&2
+
+exit 63
-- 
GitLab