gittest: Extend the protocol-detecting Git command factory

WWe have an intercepting Git command factory whose only purpose it is to
listen in on the `GIT_PROTOCOL` environment variable, but otherwise
execute the Git commands as we'd normally do. It's quite inflexible
though and only allows us to read the resulting protocol, without any
ability to also reset the current state again. This makes it hard to use
it in table-driven tests, where we may reuse the factory across multiple
test cases.

Refactor it into a proper type to be more extensible and add a new
`Reset()` function to allow resetting the currently recorded protocol
versions. This new functionality will be used in a subsequent commit.

7 files changed, 45 insertions, 29 deletions

diff --git a/internal/git/gittest/protocol.go b/internal/git/gittest/protocol.go
index 447197c7c..ed44ef0b5 100644
--- a/internal/git/gittest/protocol.go
+++ b/internal/git/gittest/protocol.go
@@ -13,10 +13,15 @@ import (
 	"gitlab.com/gitlab-org/gitaly/v15/internal/testhelper"
 )
 
-// NewProtocolDetectingCommandFactory creates a new intercepting Git command factory that allows the
-// protocol to be tested. It returns this factory and a function to read the GIT_PROTOCOL
-// environment variable created by the wrapper script.
-func NewProtocolDetectingCommandFactory(ctx context.Context, t testing.TB, cfg config.Cfg) (git.CommandFactory, func() string) {
+// ProtocolDetectingCommandFactory is an intercepting Git command factory that allows the protocol
+// to be tested.
+type ProtocolDetectingCommandFactory struct {
+	git.CommandFactory
+	envPath string
+}
+
+// NewProtocolDetectingCommandFactory returns a new ProtocolDetectingCommandFactory.
+func NewProtocolDetectingCommandFactory(ctx context.Context, t testing.TB, cfg config.Cfg) ProtocolDetectingCommandFactory {
 	envPath := filepath.Join(testhelper.TempDir(t), "git-env")
 
 	gitCmdFactory := NewInterceptingCommandFactory(ctx, t, cfg, func(execEnv git.ExecutionEnvironment) string {
@@ -27,9 +32,20 @@ func NewProtocolDetectingCommandFactory(ctx context.Context, t testing.TB, cfg c
 		`, envPath, execEnv.BinaryPath)
 	})
 
-	return gitCmdFactory, func() string {
-		data, err := os.ReadFile(envPath)
-		require.NoError(t, err)
-		return string(data)
+	return ProtocolDetectingCommandFactory{
+		CommandFactory: gitCmdFactory,
+		envPath:        envPath,
 	}
 }
+
+// ReadProtocol reads the protocol used by previous Git executions.
+func (p *ProtocolDetectingCommandFactory) ReadProtocol(t *testing.T) string {
+	data, err := os.ReadFile(p.envPath)
+	require.NoError(t, err)
+	return string(data)
+}
+
+// Reset resets previously recorded protocols.
+func (p *ProtocolDetectingCommandFactory) Reset(t *testing.T) {
+	require.NoError(t, os.RemoveAll(p.envPath))
+}
diff --git a/internal/git/localrepo/remote_extra_test.go b/internal/git/localrepo/remote_extra_test.go
index d8446f7a5..012843230 100644
--- a/internal/git/localrepo/remote_extra_test.go
+++ b/internal/git/localrepo/remote_extra_test.go
@@ -25,7 +25,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
-	gitCmdFactory, readGitProtocol := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
 	cfg.SocketPath = testserver.RunGitalyServer(t, cfg, nil, func(srv *grpc.Server, deps *service.Dependencies) {
 		gitalypb.RegisterSSHServiceServer(srv, ssh.NewServer(
@@ -49,7 +49,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 			deps.GetGit2goExecutor(),
 			deps.GetHousekeepingManager(),
 		))
-	}, testserver.WithGitCommandFactory(gitCmdFactory))
+	}, testserver.WithGitCommandFactory(protocolDetectingFactory))
 
 	remoteRepoProto, _ := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -95,7 +95,7 @@ func TestRepo_FetchInternal(t *testing.T) {
 		require.NoDirExists(t, filepath.Join(repoPath, "objects/info/commit-graphs"))
 
 		// Assert that we're using the expected Git protocol version, which is protocol v2.
-		require.Equal(t, "GIT_PROTOCOL=version=2\n", readGitProtocol())
+		require.Equal(t, "GIT_PROTOCOL=version=2\n", protocolDetectingFactory.ReadProtocol(t))
 
 		require.NoFileExists(t, filepath.Join(repoPath, "FETCH_HEAD"))
 	})
diff --git a/internal/gitaly/service/smarthttp/inforefs_test.go b/internal/gitaly/service/smarthttp/inforefs_test.go
index 7fee61bda..3aa10036f 100644
--- a/internal/gitaly/service/smarthttp/inforefs_test.go
+++ b/internal/gitaly/service/smarthttp/inforefs_test.go
@@ -132,10 +132,10 @@ func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
 	cfg := testcfg.Build(t)
 	ctx := testhelper.Context(t)
 
-	gitCmdFactory, readProtocol := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
 	server := startSmartHTTPServerWithOptions(t, cfg, nil, []testserver.GitalyServerOpt{
-		testserver.WithGitCommandFactory(gitCmdFactory),
+		testserver.WithGitCommandFactory(protocolDetectingFactory),
 	})
 	cfg.SocketPath = server.Address()
 
@@ -161,7 +161,7 @@ func TestSuccessfulInfoRefsUploadPackWithGitProtocol(t *testing.T) {
 		}
 	}
 
-	envData := readProtocol()
+	envData := protocolDetectingFactory.ReadProtocol(t)
 	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
diff --git a/internal/gitaly/service/smarthttp/receive_pack_test.go b/internal/gitaly/service/smarthttp/receive_pack_test.go
index 22ba8f62c..42ab98ca0 100644
--- a/internal/gitaly/service/smarthttp/receive_pack_test.go
+++ b/internal/gitaly/service/smarthttp/receive_pack_test.go
@@ -178,10 +178,10 @@ func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
 	testcfg.BuildGitalyHooks(t, cfg)
 	ctx := testhelper.Context(t)
 
-	gitCmdFactory, readProto := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
 	server := startSmartHTTPServerWithOptions(t, cfg, nil, []testserver.GitalyServerOpt{
-		testserver.WithGitCommandFactory(gitCmdFactory),
+		testserver.WithGitCommandFactory(protocolDetectingFactory),
 	})
 
 	cfg.SocketPath = server.Address()
@@ -200,7 +200,7 @@ func TestSuccessfulReceivePackRequestWithGitProtocol(t *testing.T) {
 	firstRequest := &gitalypb.PostReceivePackRequest{Repository: repo, GlId: "user-123", GlRepository: "project-123", GitProtocol: git.ProtocolV2}
 	doPush(t, stream, firstRequest, push.body)
 
-	envData := readProto()
+	envData := protocolDetectingFactory.ReadProtocol(t)
 	require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
 
 	// The fact that this command succeeds means that we got the commit correctly, no further checks should be needed.
diff --git a/internal/gitaly/service/smarthttp/upload_pack_test.go b/internal/gitaly/service/smarthttp/upload_pack_test.go
index 9fdca62c5..5678a1866 100644
--- a/internal/gitaly/service/smarthttp/upload_pack_test.go
+++ b/internal/gitaly/service/smarthttp/upload_pack_test.go
@@ -193,9 +193,9 @@ func TestServer_PostUploadPackWithSidechannel_gitProtocol(t *testing.T) {
 
 func testServerPostUploadPackGitProtocol(t *testing.T, ctx context.Context, makeRequest requestMaker, opts ...testcfg.Option) {
 	cfg := testcfg.Build(t, opts...)
-	gitCmdFactory, readProto := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 	server := startSmartHTTPServerWithOptions(t, cfg, nil, []testserver.GitalyServerOpt{
-		testserver.WithGitCommandFactory(gitCmdFactory),
+		testserver.WithGitCommandFactory(protocolDetectingFactory),
 	})
 	cfg.SocketPath = server.Address()
 
@@ -219,7 +219,7 @@ func testServerPostUploadPackGitProtocol(t *testing.T, ctx context.Context, make
 	_, err := makeRequest(ctx, t, server.Address(), cfg.Auth.Token, rpcRequest, requestBody)
 	require.NoError(t, err)
 
-	envData := readProto()
+	envData := protocolDetectingFactory.ReadProtocol(t)
 	require.Equal(t, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2), envData)
 }
 
diff --git a/internal/gitaly/service/ssh/receive_pack_test.go b/internal/gitaly/service/ssh/receive_pack_test.go
index 2158add1b..42bb9f812 100644
--- a/internal/gitaly/service/ssh/receive_pack_test.go
+++ b/internal/gitaly/service/ssh/receive_pack_test.go
@@ -180,9 +180,9 @@ func TestReceivePackPushSuccessWithGitProtocol(t *testing.T) {
 	testcfg.BuildGitalyHooks(t, cfg)
 	ctx := testhelper.Context(t)
 
-	gitCmdFactory, readProto := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
-	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(gitCmdFactory))
+	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(protocolDetectingFactory))
 
 	repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -198,7 +198,7 @@ func TestReceivePackPushSuccessWithGitProtocol(t *testing.T) {
 
 	require.Equal(t, lHead, rHead, "local and remote head not equal. push failed")
 
-	envData := readProto()
+	envData := protocolDetectingFactory.ReadProtocol(t)
 	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
@@ -570,8 +570,8 @@ func TestSSHReceivePackToHooks(t *testing.T) {
 	)
 	ctx := testhelper.Context(t)
 
-	gitCmdFactory, readProto := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
-	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(gitCmdFactory))
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(protocolDetectingFactory))
 
 	repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -612,7 +612,7 @@ func TestSSHReceivePackToHooks(t *testing.T) {
 	require.NoError(t, err)
 	require.Equal(t, lHead, rHead, "local and remote head not equal. push failed")
 
-	envData := readProto()
+	envData := protocolDetectingFactory.ReadProtocol(t)
 	require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 }
 
diff --git a/internal/gitaly/service/ssh/upload_pack_test.go b/internal/gitaly/service/ssh/upload_pack_test.go
index a13fb60b0..c6d04234f 100644
--- a/internal/gitaly/service/ssh/upload_pack_test.go
+++ b/internal/gitaly/service/ssh/upload_pack_test.go
@@ -492,9 +492,9 @@ func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Opt
 	cfg := testcfg.Build(t, opts...)
 	ctx := testhelper.Context(t)
 
-	gitCmdFactory, readProto := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
+	protocolDetectingFactory := gittest.NewProtocolDetectingCommandFactory(ctx, t, cfg)
 
-	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(gitCmdFactory))
+	cfg.SocketPath = runSSHServer(t, cfg, testserver.WithGitCommandFactory(protocolDetectingFactory))
 
 	repo, repoPath := gittest.CreateRepository(ctx, t, cfg, gittest.CreateRepositoryConfig{
 		Seed: gittest.SeedGitLabTest,
@@ -541,7 +541,7 @@ func testUploadPackCloneSuccessWithGitProtocol(t *testing.T, opts ...testcfg.Opt
 			lHead, rHead, _, _ := cmd.test(t, cfg, repoPath, localRepoPath)
 			require.Equal(t, lHead, rHead, "local and remote head not equal")
 
-			envData := readProto()
+			envData := protocolDetectingFactory.ReadProtocol(t)
 			require.Contains(t, envData, fmt.Sprintf("GIT_PROTOCOL=%s\n", git.ProtocolV2))
 		})
 	}