git: Move cleanup into execution environments

We return a cleanup function whenever we construct Git execution
environments. Move this functionality into the environments themselves
to have less state to keep track of. Furthermore, this prepares for a
refactoring where we rework the way Git execution environments are
constructed in the first place.

3 files changed, 56 insertions, 47 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 01c2a36e3..3c3935d06 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -181,22 +181,22 @@ func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactory
 	var execEnvs executionEnvironments
 	var cleanups []func()
 
-	if execEnv, cleanup, err := (DistributedGitEnvironmentConstructor{}).Construct(cfg); err != nil {
+	if execEnv, err := (DistributedGitEnvironmentConstructor{}).Construct(cfg); err != nil {
 		if !errors.Is(err, ErrNotConfigured) {
 			return executionEnvironments{}, nil, fmt.Errorf("constructing distributed Git environment: %w", err)
 		}
 	} else {
 		execEnvs.externalGit = execEnv
-		cleanups = append(cleanups, cleanup)
+		cleanups = append(cleanups, execEnv.Cleanup)
 	}
 
-	if execEnv, cleanup, err := (BundledGitEnvironmentConstructor{}).Construct(cfg); err != nil {
+	if execEnv, err := (BundledGitEnvironmentConstructor{}).Construct(cfg); err != nil {
 		if !errors.Is(err, ErrNotConfigured) {
 			return executionEnvironments{}, nil, fmt.Errorf("constructing bundled Git environment: %w", err)
 		}
 	} else {
 		execEnvs.bundledGit = execEnv
-		cleanups = append(cleanups, cleanup)
+		cleanups = append(cleanups, execEnv.Cleanup)
 	}
 
 	// If neither an external Git distribution nor a bundled Git was configured we need to
@@ -204,13 +204,13 @@ func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactory
 	// in case either one of those has been configured though: we have no clue where system Git
 	// comes from and what its version is, so it's the worst of all choices.
 	if execEnvs.externalGit.BinaryPath == "" && execEnvs.bundledGit.BinaryPath == "" {
-		execEnv, cleanup, err := (FallbackGitEnvironmentConstructor{}).Construct(cfg)
+		execEnv, err := (FallbackGitEnvironmentConstructor{}).Construct(cfg)
 		if err != nil {
 			return executionEnvironments{}, nil, fmt.Errorf("constructing fallback Git environment: %w", err)
 		}
 
 		execEnvs.externalGit = execEnv
-		cleanups = append(cleanups, cleanup)
+		cleanups = append(cleanups, execEnv.Cleanup)
 	}
 
 	return execEnvs, func() {
diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go
index 88982990e..a0759c213 100644
--- a/internal/git/execution_environment.go
+++ b/internal/git/execution_environment.go
@@ -21,6 +21,15 @@ type ExecutionEnvironment struct {
 	BinaryPath string
 	// EnvironmentVariables are variables which must be set when running the Git binary.
 	EnvironmentVariables []string
+
+	cleanup func()
+}
+
+// Cleanup cleans up any state set up by this ExecutionEnvironment.
+func (e ExecutionEnvironment) Cleanup() {
+	if e.cleanup != nil {
+		e.cleanup()
+	}
 }
 
 // DistributedGitEnvironmentConstructor creates ExecutionEnvironments via the Git binary path
@@ -31,24 +40,23 @@ type ExecutionEnvironment struct {
 type DistributedGitEnvironmentConstructor struct{}
 
 // Construct sets up an ExecutionEnvironment for a complete Git distribution. No setup needs to be
-// performed given that the Git environment is expected to be self-contained. The returned function
-// is a cleanup function that shall be executed when the ExecutionEnvironment is no longer used.
+// performed given that the Git environment is expected to be self-contained.
 //
 // For testing purposes, this function overrides the configured Git binary path if the
 // `GITALY_TESTING_GIT_BINARY` environment variable is set.
-func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (ExecutionEnvironment, func(), error) {
+func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (ExecutionEnvironment, error) {
 	binaryPath := cfg.Git.BinPath
 	if override := os.Getenv("GITALY_TESTING_GIT_BINARY"); binaryPath == "" && override != "" {
 		binaryPath = override
 	}
 
 	if binaryPath == "" {
-		return ExecutionEnvironment{}, nil, ErrNotConfigured
+		return ExecutionEnvironment{}, ErrNotConfigured
 	}
 
 	return ExecutionEnvironment{
 		BinaryPath: binaryPath,
-	}, func() {}, nil
+	}, nil
 }
 
 // BundledGitEnvironmentConstructor sets up an ExecutionEnvironment for a bundled Git installation.
@@ -67,12 +75,12 @@ type BundledGitEnvironmentConstructor struct{}
 //
 // For testing purposes, this function will automatically enable use of bundled Git in case the
 // `GITALY_TESTING_BUNDLED_GIT_PATH` environment variable is set.
-func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ ExecutionEnvironment, _ func(), returnedErr error) {
+func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ ExecutionEnvironment, returnedErr error) {
 	useBundledBinaries := cfg.Git.UseBundledBinaries
 
 	if bundledGitPath := os.Getenv("GITALY_TESTING_BUNDLED_GIT_PATH"); bundledGitPath != "" {
 		if cfg.BinDir == "" {
-			return ExecutionEnvironment{}, nil, errors.New("cannot use bundled binaries without bin path being set")
+			return ExecutionEnvironment{}, errors.New("cannot use bundled binaries without bin path being set")
 		}
 
 		// We need to symlink pre-built Git binaries into Gitaly's binary directory.
@@ -81,7 +89,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 		for _, binary := range []string{"gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend"} {
 			bundledGitBinary := filepath.Join(bundledGitPath, binary)
 			if _, err := os.Stat(bundledGitBinary); err != nil {
-				return ExecutionEnvironment{}, nil, fmt.Errorf("statting %q: %w", binary, err)
+				return ExecutionEnvironment{}, fmt.Errorf("statting %q: %w", binary, err)
 			}
 
 			if err := os.Symlink(bundledGitBinary, filepath.Join(cfg.BinDir, binary)); err != nil {
@@ -92,7 +100,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 				if errors.Is(err, os.ErrExist) {
 					continue
 				}
-				return ExecutionEnvironment{}, nil, fmt.Errorf("symlinking bundled %q: %w", binary, err)
+				return ExecutionEnvironment{}, fmt.Errorf("symlinking bundled %q: %w", binary, err)
 			}
 		}
 
@@ -100,7 +108,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 	}
 
 	if !useBundledBinaries {
-		return ExecutionEnvironment{}, nil, ErrNotConfigured
+		return ExecutionEnvironment{}, ErrNotConfigured
 	}
 
 	// In order to support having a single Git binary only as compared to a complete Git
@@ -108,7 +116,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 	// binary for executables which Git expects to be present.
 	gitExecPath, err := os.MkdirTemp("", "gitaly-git-exec-path-*")
 	if err != nil {
-		return ExecutionEnvironment{}, nil, fmt.Errorf("creating Git exec path: %w", err)
+		return ExecutionEnvironment{}, fmt.Errorf("creating Git exec path: %w", err)
 	}
 
 	cleanup := func() {
@@ -137,7 +145,7 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 			filepath.Join(cfg.BinDir, target),
 			filepath.Join(gitExecPath, executable),
 		); err != nil {
-			return ExecutionEnvironment{}, nil, fmt.Errorf("linking Git executable %q: %w", executable, err)
+			return ExecutionEnvironment{}, fmt.Errorf("linking Git executable %q: %w", executable, err)
 		}
 	}
 
@@ -146,7 +154,8 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 		EnvironmentVariables: []string{
 			"GIT_EXEC_PATH=" + gitExecPath,
 		},
-	}, cleanup, nil
+		cleanup: cleanup,
+	}, nil
 }
 
 // FallbackGitEnvironmentConstructor sets up a fallback execution environment where Git is resolved
@@ -155,14 +164,14 @@ func (c BundledGitEnvironmentConstructor) Construct(cfg config.Cfg) (_ Execution
 type FallbackGitEnvironmentConstructor struct{}
 
 // Construct sets up an execution environment by searching `PATH` for a `git` executable.
-func (c FallbackGitEnvironmentConstructor) Construct(config.Cfg) (ExecutionEnvironment, func(), error) {
+func (c FallbackGitEnvironmentConstructor) Construct(config.Cfg) (ExecutionEnvironment, error) {
 	resolvedPath, err := exec.LookPath("git")
 	if err != nil {
 		if errors.Is(err, exec.ErrNotFound) {
-			return ExecutionEnvironment{}, nil, fmt.Errorf("%w: no git executable found in PATH", ErrNotConfigured)
+			return ExecutionEnvironment{}, fmt.Errorf("%w: no git executable found in PATH", ErrNotConfigured)
 		}
 
-		return ExecutionEnvironment{}, nil, fmt.Errorf("resolving git executable: %w", err)
+		return ExecutionEnvironment{}, fmt.Errorf("resolving git executable: %w", err)
 	}
 
 	logrus.WithFields(logrus.Fields{
@@ -171,5 +180,5 @@ func (c FallbackGitEnvironmentConstructor) Construct(config.Cfg) (ExecutionEnvir
 
 	return ExecutionEnvironment{
 		BinaryPath: resolvedPath,
-	}, func() {}, nil
+	}, nil
 }
diff --git a/internal/git/execution_environment_test.go b/internal/git/execution_environment_test.go
index 3c56e585b..8816ee405 100644
--- a/internal/git/execution_environment_test.go
+++ b/internal/git/execution_environment_test.go
@@ -19,18 +19,18 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "")
 
 	t.Run("empty configuration fails", func(t *testing.T) {
-		_, _, err := constructor.Construct(config.Cfg{})
+		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, git.ErrNotConfigured, err)
 	})
 
 	t.Run("configuration with Git binary path succeeds", func(t *testing.T) {
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			Git: config.Git{
 				BinPath: "/foo/bar",
 			},
 		})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		require.Equal(t, "/foo/bar", execEnv.BinaryPath)
 		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)
@@ -39,9 +39,9 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	t.Run("empty configuration with environment override", func(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "/foo/bar")
 
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{})
+		execEnv, err := constructor.Construct(config.Cfg{})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		require.Equal(t, "/foo/bar", execEnv.BinaryPath)
 		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)
@@ -50,13 +50,13 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 	t.Run("configuration overrides environment variable", func(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_GIT_BINARY", "envvar")
 
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			Git: config.Git{
 				BinPath: "config",
 			},
 		})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		require.Equal(t, "config", execEnv.BinaryPath)
 		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)
@@ -77,12 +77,12 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 	}
 
 	t.Run("disabled bundled Git fails", func(t *testing.T) {
-		_, _, err := constructor.Construct(config.Cfg{})
+		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, git.ErrNotConfigured, err)
 	})
 
 	t.Run("bundled Git without binary directory fails", func(t *testing.T) {
-		_, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			Git: config.Git{
 				UseBundledBinaries: true,
 			},
@@ -91,11 +91,11 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 		// It is a bug that this succeeds: if the binary directory is not set we cannot
 		// derive the location of the bundled Git executables either.
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 	})
 
 	t.Run("incomplete binary directory succeeds", func(t *testing.T) {
-		_, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			BinDir: seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http"),
 			Git: config.Git{
 				UseBundledBinaries: true,
@@ -105,20 +105,20 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 		// It is a bug that this succeeds, we really should check that all expected binaries
 		// exist. We thus don't bother to check the generated execution environment.
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 	})
 
 	t.Run("complete binary directory succeeds", func(t *testing.T) {
 		binDir := seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend")
 
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			BinDir: binDir,
 			Git: config.Git{
 				UseBundledBinaries: true,
 			},
 		})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		// We create a temporary directory where the symlinks are created, and we cannot
 		// predict its exact path.
@@ -137,7 +137,7 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 	})
 
 	t.Run("cleanup removes temporary directory", func(t *testing.T) {
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			BinDir: seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend"),
 			Git: config.Git{
 				UseBundledBinaries: true,
@@ -148,20 +148,20 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 		execPrefix := filepath.Dir(execEnv.BinaryPath)
 		require.DirExists(t, execPrefix)
 
-		cleanup()
+		execEnv.Cleanup()
 
 		require.NoDirExists(t, execPrefix)
 	})
 
 	t.Run("bundled Git path without binary directory fails", func(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
-		_, _, err := constructor.Construct(config.Cfg{})
+		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, errors.New("cannot use bundled binaries without bin path being set"), err)
 	})
 
 	t.Run("nonexistent bundled Git path via environment fails", func(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "/does/not/exist")
-		_, _, err := constructor.Construct(config.Cfg{
+		_, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
 		})
 		require.Error(t, err)
@@ -172,7 +172,7 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 		bundledGitPath := seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http")
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
 
-		_, _, err := constructor.Construct(config.Cfg{
+		_, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
 		})
 		require.Error(t, err)
@@ -183,11 +183,11 @@ func TestBundledGitEnvironmentConstructor(t *testing.T) {
 		bundledGitPath := seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend")
 		testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", bundledGitPath)
 
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+		execEnv, err := constructor.Construct(config.Cfg{
 			BinDir: testhelper.TempDir(t),
 		})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		require.Equal(t, "git", filepath.Base(execEnv.BinaryPath))
 		execPrefix := filepath.Dir(execEnv.BinaryPath)
@@ -210,7 +210,7 @@ func TestFallbackGitEnvironmentConstructor(t *testing.T) {
 	t.Run("failing lookup of executable causes failure", func(t *testing.T) {
 		testhelper.ModifyEnvironment(t, "PATH", "/does/not/exist")
 
-		_, _, err := constructor.Construct(config.Cfg{})
+		_, err := constructor.Construct(config.Cfg{})
 		require.Equal(t, fmt.Errorf("%w: no git executable found in PATH", git.ErrNotConfigured), err)
 	})
 
@@ -221,9 +221,9 @@ func TestFallbackGitEnvironmentConstructor(t *testing.T) {
 
 		testhelper.ModifyEnvironment(t, "PATH", "/does/not/exist:"+tempDir)
 
-		execEnv, cleanup, err := constructor.Construct(config.Cfg{})
+		execEnv, err := constructor.Construct(config.Cfg{})
 		require.NoError(t, err)
-		defer cleanup()
+		defer execEnv.Cleanup()
 
 		require.Equal(t, gitPath, execEnv.BinaryPath)
 		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)