git: Create separate constructor for bundled Git environments

Move the logic which creates execution environments for bundled Git into
its own structure. This will eventually allow us to have a simple list
of all supported Git execution environments in a central location.

4 files changed, 252 insertions, 70 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 62ec937bb..d4246128c 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -179,36 +179,6 @@ func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactory
 		}, func() {}, nil
 	}
 
-	useBundledBinaries := cfg.Git.UseBundledBinaries
-	if bundledGitPath := os.Getenv("GITALY_TESTING_BUNDLED_GIT_PATH"); bundledGitPath != "" {
-		if cfg.BinDir == "" {
-			return executionEnvironments{}, nil, errors.New("cannot use bundled binaries without bin path being set")
-		}
-
-		// We need to symlink pre-built Git binaries into Gitaly's binary directory.
-		// Normally they would of course already exist there, but in tests we create a new
-		// binary directory for each server and thus need to populate it first.
-		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 executionEnvironments{}, nil, fmt.Errorf("statting %q: %w", binary, err)
-			}
-
-			if err := os.Symlink(bundledGitBinary, filepath.Join(cfg.BinDir, binary)); err != nil {
-				// While Gitaly's Go tests use a temporary binary directory, Ruby
-				// rspecs set up the binary directory to point to our build
-				// directory. They thus already contain the Git binaries and don't
-				// need symlinking.
-				if errors.Is(err, os.ErrExist) {
-					continue
-				}
-				return executionEnvironments{}, nil, fmt.Errorf("symlinking bundled %q: %w", binary, err)
-			}
-		}
-
-		useBundledBinaries = true
-	}
-
 	var execEnvs executionEnvironments
 	var cleanups []func()
 
@@ -221,46 +191,13 @@ func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactory
 		cleanups = append(cleanups, cleanup)
 	}
 
-	if useBundledBinaries {
-		// In order to support having a single Git binary only as compared to a complete Git
-		// installation, we create our own GIT_EXEC_PATH which contains symlinks to the Git
-		// binary for executables which Git expects to be present.
-		gitExecPath, err := os.MkdirTemp("", "gitaly-git-exec-path-*")
-		if err != nil {
-			return executionEnvironments{}, nil, fmt.Errorf("creating Git exec path: %w", err)
-		}
-
-		for executable, target := range map[string]string{
-			"git":                "gitaly-git",
-			"git-receive-pack":   "gitaly-git",
-			"git-upload-pack":    "gitaly-git",
-			"git-upload-archive": "gitaly-git",
-			"git-http-backend":   "gitaly-git-http-backend",
-			"git-remote-http":    "gitaly-git-remote-http",
-			"git-remote-https":   "gitaly-git-remote-http",
-			"git-remote-ftp":     "gitaly-git-remote-http",
-			"git-remote-ftps":    "gitaly-git-remote-http",
-		} {
-			if err := os.Symlink(
-				filepath.Join(cfg.BinDir, target),
-				filepath.Join(gitExecPath, executable),
-			); err != nil {
-				return executionEnvironments{}, nil, fmt.Errorf("linking Git executable %q: %w", executable, err)
-			}
-		}
-
-		cleanups = append(cleanups, func() {
-			if err := os.RemoveAll(gitExecPath); err != nil {
-				logrus.WithError(err).Error("cleanup of Git execution environment failed")
-			}
-		})
-
-		execEnvs.bundledGit = ExecutionEnvironment{
-			BinaryPath: filepath.Join(gitExecPath, "git"),
-			EnvironmentVariables: []string{
-				"GIT_EXEC_PATH=" + gitExecPath,
-			},
+	if execEnv, cleanup, 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)
 	}
 
 	// If neither an external Git distribution nor a bundled Git was configured we need to
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 385d21459..cfd78d49d 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -184,7 +184,7 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 
 		t.Run("missing bin_dir", func(t *testing.T) {
 			_, _, err := git.NewExecCommandFactory(config.Cfg{Git: config.Git{}}, git.WithSkipHooks())
-			require.EqualError(t, err, "setting up Git execution environment: cannot use bundled binaries without bin path being set")
+			require.EqualError(t, err, "setting up Git execution environment: constructing bundled Git environment: cannot use bundled binaries without bin path being set")
 		})
 
 		t.Run("missing gitaly-git executable", func(t *testing.T) {
diff --git a/internal/git/execution_environment.go b/internal/git/execution_environment.go
index 619622abb..daf476fa3 100644
--- a/internal/git/execution_environment.go
+++ b/internal/git/execution_environment.go
@@ -2,8 +2,11 @@ package git
 
 import (
 	"errors"
+	"fmt"
 	"os"
+	"path/filepath"
 
+	"github.com/sirupsen/logrus"
 	"gitlab.com/gitlab-org/gitaly/v14/internal/gitaly/config"
 )
 
@@ -46,3 +49,101 @@ func (c DistributedGitEnvironmentConstructor) Construct(cfg config.Cfg) (Executi
 		BinaryPath: binaryPath,
 	}, func() {}, nil
 }
+
+// BundledGitEnvironmentConstructor sets up an ExecutionEnvironment for a bundled Git installation.
+// Bundled Git is a partial Git installation, where only a subset of Git binaries are installed
+// into Gitaly's binary directory. The binaries must have a `gitaly-` prefix like e.g. `gitaly-git`.
+// Bundled Git installations can be installed with Gitaly's Makefile via `make install
+// WITH_BUNDLED_GIT=YesPlease`.
+type BundledGitEnvironmentConstructor struct{}
+
+// Construct sets up an ExecutionEnvironment for a bundled Git installation. Because bundled Git
+// installations are not complete Git installations we need to set up a usable environment at
+// runtime. This is done by creating a temporary directory into which we symlink the bundled
+// binaries with their usual names as expected by Git. Furthermore, we configure the GIT_EXEC_PATH
+// environment variable to point to that directory such that Git is able to locate its auxiliary
+// binaries.
+//
+// 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) {
+	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")
+		}
+
+		// We need to symlink pre-built Git binaries into Gitaly's binary directory.
+		// Normally they would of course already exist there, but in tests we create a new
+		// binary directory for each server and thus need to populate it first.
+		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)
+			}
+
+			if err := os.Symlink(bundledGitBinary, filepath.Join(cfg.BinDir, binary)); err != nil {
+				// While Gitaly's Go tests use a temporary binary directory, Ruby
+				// rspecs set up the binary directory to point to our build
+				// directory. They thus already contain the Git binaries and don't
+				// need symlinking.
+				if errors.Is(err, os.ErrExist) {
+					continue
+				}
+				return ExecutionEnvironment{}, nil, fmt.Errorf("symlinking bundled %q: %w", binary, err)
+			}
+		}
+
+		useBundledBinaries = true
+	}
+
+	if !useBundledBinaries {
+		return ExecutionEnvironment{}, nil, ErrNotConfigured
+	}
+
+	// In order to support having a single Git binary only as compared to a complete Git
+	// installation, we create our own GIT_EXEC_PATH which contains symlinks to the Git
+	// 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)
+	}
+
+	cleanup := func() {
+		if err := os.RemoveAll(gitExecPath); err != nil {
+			logrus.WithError(err).Error("cleanup of Git execution environment failed")
+		}
+	}
+	defer func() {
+		if returnedErr != nil {
+			cleanup()
+		}
+	}()
+
+	for executable, target := range map[string]string{
+		"git":                "gitaly-git",
+		"git-receive-pack":   "gitaly-git",
+		"git-upload-pack":    "gitaly-git",
+		"git-upload-archive": "gitaly-git",
+		"git-http-backend":   "gitaly-git-http-backend",
+		"git-remote-http":    "gitaly-git-remote-http",
+		"git-remote-https":   "gitaly-git-remote-http",
+		"git-remote-ftp":     "gitaly-git-remote-http",
+		"git-remote-ftps":    "gitaly-git-remote-http",
+	} {
+		if err := os.Symlink(
+			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{
+		BinaryPath: filepath.Join(gitExecPath, "git"),
+		EnvironmentVariables: []string{
+			"GIT_EXEC_PATH=" + gitExecPath,
+		},
+	}, cleanup, nil
+}
diff --git a/internal/git/execution_environment_test.go b/internal/git/execution_environment_test.go
index 467eacab7..237d19fdc 100644
--- a/internal/git/execution_environment_test.go
+++ b/internal/git/execution_environment_test.go
@@ -1,6 +1,9 @@
 package git_test
 
 import (
+	"errors"
+	"os"
+	"path/filepath"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -58,3 +61,144 @@ func TestDistributedGitEnvironmentConstructor(t *testing.T) {
 		require.Equal(t, []string(nil), execEnv.EnvironmentVariables)
 	})
 }
+
+func TestBundledGitEnvironmentConstructor(t *testing.T) {
+	testhelper.ModifyEnvironment(t, "GITALY_TESTING_BUNDLED_GIT_PATH", "")
+
+	constructor := git.BundledGitEnvironmentConstructor{}
+
+	seedDirWithExecutables := func(t *testing.T, executableNames ...string) string {
+		dir := testhelper.TempDir(t)
+		for _, executableName := range executableNames {
+			require.NoError(t, os.WriteFile(filepath.Join(dir, executableName), nil, 0o777))
+		}
+		return dir
+	}
+
+	t.Run("disabled bundled Git fails", func(t *testing.T) {
+		_, _, 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{
+			Git: config.Git{
+				UseBundledBinaries: true,
+			},
+		})
+
+		// 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()
+	})
+
+	t.Run("incomplete binary directory succeeds", func(t *testing.T) {
+		_, cleanup, err := constructor.Construct(config.Cfg{
+			BinDir: seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http"),
+			Git: config.Git{
+				UseBundledBinaries: true,
+			},
+		})
+
+		// 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()
+	})
+
+	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{
+			BinDir: binDir,
+			Git: config.Git{
+				UseBundledBinaries: true,
+			},
+		})
+		require.NoError(t, err)
+		defer cleanup()
+
+		// We create a temporary directory where the symlinks are created, and we cannot
+		// predict its exact path.
+		require.Equal(t, "git", filepath.Base(execEnv.BinaryPath))
+
+		execPrefix := filepath.Dir(execEnv.BinaryPath)
+		require.Equal(t, []string{
+			"GIT_EXEC_PATH=" + execPrefix,
+		}, execEnv.EnvironmentVariables)
+
+		for _, binary := range []string{"git", "git-remote-http", "git-http-backend"} {
+			target, err := filepath.EvalSymlinks(filepath.Join(execPrefix, binary))
+			require.NoError(t, err)
+			require.Equal(t, filepath.Join(binDir, "gitaly-"+binary), target)
+		}
+	})
+
+	t.Run("cleanup removes temporary directory", func(t *testing.T) {
+		execEnv, cleanup, err := constructor.Construct(config.Cfg{
+			BinDir: seedDirWithExecutables(t, "gitaly-git", "gitaly-git-remote-http", "gitaly-git-http-backend"),
+			Git: config.Git{
+				UseBundledBinaries: true,
+			},
+		})
+		require.NoError(t, err)
+
+		execPrefix := filepath.Dir(execEnv.BinaryPath)
+		require.DirExists(t, execPrefix)
+
+		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{})
+		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{
+			BinDir: testhelper.TempDir(t),
+		})
+		require.Error(t, err)
+		require.Equal(t, err.Error(), "statting \"gitaly-git\": stat /does/not/exist/gitaly-git: no such file or directory")
+	})
+
+	t.Run("incomplete bundled Git environment fails", func(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{
+			BinDir: testhelper.TempDir(t),
+		})
+		require.Error(t, err)
+		require.Contains(t, err.Error(), "statting \"gitaly-git-http-backend\": ")
+	})
+
+	t.Run("complete bundled Git environment populates binary directory", func(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{
+			BinDir: testhelper.TempDir(t),
+		})
+		require.NoError(t, err)
+		defer cleanup()
+
+		require.Equal(t, "git", filepath.Base(execEnv.BinaryPath))
+		execPrefix := filepath.Dir(execEnv.BinaryPath)
+
+		require.Equal(t, []string{
+			"GIT_EXEC_PATH=" + execPrefix,
+		}, execEnv.EnvironmentVariables)
+
+		for _, binary := range []string{"git", "git-remote-http", "git-http-backend"} {
+			target, err := filepath.EvalSymlinks(filepath.Join(execPrefix, binary))
+			require.NoError(t, err)
+			require.Equal(t, filepath.Join(bundledGitPath, "gitaly-"+binary), target)
+		}
+	})
+}