git: Move handling of Git-specific envvars into command factory

While almost all parts of spawned Git commands get assembled in our
command factory, we do have a set of Git-specific environment variables
that are instead set up by our `command` package. This is a violation of
our abstractions.

Move the assembly of Git-specific environment variables into the command
factory to fix this layering violation. We now append those environment
variables to the execution environments: all sites where we directly or
indirectly spawn Git commands must inject environment variables of the
Git execution environment anyway.

This change also prepares for an upcoming change where we override
`GIT_CONFIG_GLOBAL` and `GIT_CONFIG_SYSTEM` when a Gitaly configuration
is set that asks us to do this.

6 files changed, 51 insertions, 47 deletions

diff --git a/internal/command/command.go b/internal/command/command.go
index 4992c6d5b..6dc524e57 100644
--- a/internal/command/command.go
+++ b/internal/command/command.go
@@ -23,20 +23,6 @@ import (
 	"gitlab.com/gitlab-org/labkit/tracing"
 )
 
-func init() {
-	// Prevent the environment from affecting git calls by ignoring the configuration files.
-	//
-	// This should be done always but we have to wait until 15.0 due to backwards compatibility
-	// concerns. To fix tests ahead to 15.0, we ignore the global configuration when the package
-	// has been built under tests. `go test` uses a `.test` suffix on the test binaries. We use
-	// that to check whether to ignore the globals or not.
-	//
-	// See https://gitlab.com/gitlab-org/gitaly/-/issues/3617.
-	if strings.HasSuffix(os.Args[0], ".test") {
-		GitEnv = append(GitEnv, "GIT_CONFIG_GLOBAL=/dev/null", "GIT_CONFIG_SYSTEM=/dev/null")
-	}
-}
-
 var (
 	cpuSecondsTotal = promauto.NewCounterVec(
 		prometheus.CounterOpts{
@@ -89,18 +75,6 @@ var (
 	)
 )
 
-// GitEnv contains the ENV variables for git commands
-var GitEnv = []string{
-	// Force english locale for consistency on the output messages
-	"LANG=en_US.UTF-8",
-
-	//
-	// PLEASE NOTE: the init of this package adds rules to ignore global git configuration in
-	// tests. This should really be done always but we can't do this before 15.0 due to backwards
-	// compatibility concerns. See https://gitlab.com/gitlab-org/gitaly/-/issues/3617.
-	//
-}
-
 // exportedEnvVars contains a list of environment variables
 // that are always exported to child processes on spawn
 var exportedEnvVars = []string{
@@ -275,11 +249,11 @@ func New(ctx context.Context, cmd *exec.Cmd, stdin io.Reader, stdout, stderr io.
 		span:      span,
 	}
 
-	// Explicitly set the environment for the command
-	env = append(env, "GIT_TERMINAL_PROMPT=0")
-
-	// Export env vars
-	cmd.Env = append(AllowedEnvironment(os.Environ()), env...)
+	// Export allowed environment variables as set in the Gitaly process.
+	cmd.Env = AllowedEnvironment(os.Environ())
+	// Append environment variables explicitly requested by thecaller.
+	cmd.Env = append(cmd.Env, env...)
+	// And finally inject environment variables required for tracing into the command.
 	cmd.Env = envInjector(ctx, cmd.Env)
 
 	// Start the command in its own process group (nice for signalling)
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index a5d127019..83e9a1060 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -7,6 +7,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"sync"
 
 	"github.com/prometheus/client_golang/prometheus"
@@ -147,9 +148,32 @@ func NewExecCommandFactory(cfg config.Cfg, opts ...ExecCommandFactoryOption) (_
 // setupGitExecutionEnvironments assembles a Git execution environment that can be used to run Git
 // commands. It warns if no path was specified in the configuration.
 func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactoryConfig) ([]ExecutionEnvironment, func(), error) {
+	sharedEnvironment := []string{
+		// Force English locale for consistency on output messages and to help us debug in
+		// case we get bug reports from customers whose system-locale would be different.
+		"LANG=en_US.UTF-8",
+		// Ask Git to never prompt us for any information like e.g. credentials.
+		"GIT_TERMINAL_PROMPT=0",
+	}
+
+	// Prevent the environment from affecting git calls by ignoring the configuration files.
+	//
+	// This should be done always but we have to wait until 15.0 due to backwards compatibility
+	// concerns. To fix tests ahead to 15.0, we ignore the global configuration when the package
+	// has been built under tests. `go test` uses a `.test` suffix on the test binaries. We use
+	// that to check whether to ignore the globals or not.
+	//
+	// See https://gitlab.com/gitlab-org/gitaly/-/issues/3617.
+	if strings.HasSuffix(os.Args[0], ".test") {
+		sharedEnvironment = append(sharedEnvironment,
+			"GIT_CONFIG_GLOBAL=/dev/null",
+			"GIT_CONFIG_SYSTEM=/dev/null",
+		)
+	}
+
 	if factoryCfg.gitBinaryPath != "" {
 		return []ExecutionEnvironment{
-			{BinaryPath: factoryCfg.gitBinaryPath},
+			{BinaryPath: factoryCfg.gitBinaryPath, EnvironmentVariables: sharedEnvironment},
 		}, func() {}, nil
 	}
 
@@ -168,6 +192,8 @@ func setupGitExecutionEnvironments(cfg config.Cfg, factoryCfg execCommandFactory
 			return nil, nil, fmt.Errorf("constructing Git environment: %w", err)
 		}
 
+		execEnv.EnvironmentVariables = append(execEnv.EnvironmentVariables, sharedEnvironment...)
+
 		execEnvs = append(execEnvs, execEnv)
 	}
 
@@ -369,7 +395,6 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 
 	execEnv := cf.GetExecutionEnvironment(ctx)
 
-	env = append(env, command.GitEnv...)
 	env = append(env, execEnv.EnvironmentVariables...)
 
 	execCommand := exec.Command(execEnv.BinaryPath, args...)
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 06d3f0762..45ff1c9ce 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -170,6 +170,12 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 			Git: config.Git{BinPath: "/path/to/myGit"},
 		}, git.ExecutionEnvironment{
 			BinaryPath: "/path/to/myGit",
+			EnvironmentVariables: []string{
+				"LANG=en_US.UTF-8",
+				"GIT_TERMINAL_PROMPT=0",
+				"GIT_CONFIG_GLOBAL=/dev/null",
+				"GIT_CONFIG_SYSTEM=/dev/null",
+			},
 		})
 	})
 
@@ -178,6 +184,12 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 
 		assertExecEnv(t, config.Cfg{Git: config.Git{}}, git.ExecutionEnvironment{
 			BinaryPath: "/path/to/env_git",
+			EnvironmentVariables: []string{
+				"LANG=en_US.UTF-8",
+				"GIT_TERMINAL_PROMPT=0",
+				"GIT_CONFIG_GLOBAL=/dev/null",
+				"GIT_CONFIG_SYSTEM=/dev/null",
+			},
 		})
 	})
 
@@ -271,6 +283,12 @@ func TestCommandFactory_ExecutionEnvironment(t *testing.T) {
 
 		assertExecEnv(t, config.Cfg{}, git.ExecutionEnvironment{
 			BinaryPath: resolvedPath,
+			EnvironmentVariables: []string{
+				"LANG=en_US.UTF-8",
+				"GIT_TERMINAL_PROMPT=0",
+				"GIT_CONFIG_GLOBAL=/dev/null",
+				"GIT_CONFIG_SYSTEM=/dev/null",
+			},
 		})
 	})
 
diff --git a/internal/git/gittest/command.go b/internal/git/gittest/command.go
index 751f4ee54..b31e57cd3 100644
--- a/internal/git/gittest/command.go
+++ b/internal/git/gittest/command.go
@@ -62,7 +62,6 @@ func createCommand(t testing.TB, cfg config.Cfg, execCfg ExecConfig, args ...str
 
 	cmd := exec.CommandContext(ctx, execEnv.BinaryPath, args...)
 	cmd.Env = command.AllowedEnvironment(os.Environ())
-	cmd.Env = append(command.GitEnv, cmd.Env...)
 	cmd.Env = append(cmd.Env,
 		"GIT_AUTHOR_DATE=1572776879 +0100",
 		"GIT_COMMITTER_DATE=1572776879 +0100",
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 1ae315cb3..5ac213e72 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -61,7 +61,6 @@ func setupEnv(cfg config.Cfg, gitCmdFactory git.CommandFactory) ([]string, error
 		"GITALY_TOKEN="+cfg.Auth.Token,
 		"GITALY_RUGGED_GIT_CONFIG_SEARCH_PATH="+cfg.Ruby.RuggedGitConfigSearchPath,
 	)
-	environment = append(environment, command.GitEnv...)
 	environment = append(environment, gitExecEnv.EnvironmentVariables...)
 	environment = append(environment, env.AllowedRubyEnvironment(os.Environ())...)
 
diff --git a/internal/testhelper/testcfg/build.go b/internal/testhelper/testcfg/build.go
index a39a409f4..59ab9ee7a 100644
--- a/internal/testhelper/testcfg/build.go
+++ b/internal/testhelper/testcfg/build.go
@@ -100,18 +100,7 @@ func BuildBinary(t testing.TB, targetDir, sourcePath string) string {
 		// We need to filter out some environments we set globally in our tests which would
 		// cause Git to not operate correctly.
 		for _, env := range os.Environ() {
-			shouldExclude := false
-			for _, prefix := range []string{
-				"GIT_DIR=",
-				"GIT_CONFIG_GLOBAL=",
-				"GIT_CONFIG_SYSTEM=",
-			} {
-				if strings.HasPrefix(env, prefix) {
-					shouldExclude = true
-					break
-				}
-			}
-			if !shouldExclude {
+			if !strings.HasPrefix(env, "GIT_DIR=") {
 				gitEnvironment = append(gitEnvironment, env)
 			}
 		}