git: Extract construction of global Git configuration

Extract the construction of global Git configuration that applies to
every spawned Git command into its own function. This is a preparatory
step for implementing a function that assembles the Git configuration as
required for the Ruby sidecar.

2 files changed, 96 insertions, 28 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 211f40dbd..41ee7a9a5 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -421,6 +421,39 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg)
 	}
 
+	combinedGlobals, err := cf.globalConfiguration(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("getting global Git configuration: %w", err)
+	}
+
+	combinedGlobals = append(combinedGlobals, commandDescription.opts...)
+	combinedGlobals = append(combinedGlobals, cc.globals...)
+	for _, configPair := range gitConfig {
+		combinedGlobals = append(combinedGlobals, ConfigPair{
+			Key:   configPair.Key,
+			Value: configPair.Value,
+		})
+	}
+
+	for _, global := range combinedGlobals {
+		globalArgs, err := global.GlobalArgs()
+		if err != nil {
+			return nil, err
+		}
+		args = append(args, globalArgs...)
+	}
+
+	scArgs, err := sc.CommandArgs()
+	if err != nil {
+		return nil, err
+	}
+
+	return append(args, scArgs...), nil
+}
+
+// globalConfiguration returns the global Git configuration that should be applied to every Git
+// command.
+func (cf *ExecCommandFactory) globalConfiguration(ctx context.Context) ([]GlobalOption, error) {
 	// It's fine to ask for the Git version whenever we spawn a command: the value is cached
 	// nowadays, so this would typically only boil down to a single stat(3P) call to determine
 	// whether the cache is stale or not.
@@ -441,7 +474,7 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 	// 3. Globals passed via command options, e.g. as set up by
 	//    `WithReftxHook()`.
 	// 4. Configuration as provided by the admin in Gitaly's config.toml.
-	combinedGlobals := []GlobalOption{
+	config := []GlobalOption{
 		// Disable automatic garbage collection as we handle scheduling
 		// of it ourselves.
 		ConfigPair{Key: "gc.auto", Value: "0"},
@@ -463,8 +496,8 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 	// Git v2.36.0 introduced new fine-grained configuration for what data should be fsynced and
 	// how that should happen.
 	if gitVersion.HasGranularFsyncConfig() {
-		combinedGlobals = append(
-			combinedGlobals,
+		config = append(
+			config,
 			// This is the same as below, but in addition we're also syncing packed-refs
 			// and loose refs to disk. This fixes a long-standing issue we've had where
 			// hard reboots of a server could end up corrupting loose references.
@@ -474,32 +507,10 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 	} else {
 		// Synchronize object files to lessen the likelihood of
 		// repository corruption in case the server crashes.
-		combinedGlobals = append(
-			combinedGlobals, ConfigPair{Key: "core.fsyncObjectFiles", Value: "true"},
+		config = append(
+			config, ConfigPair{Key: "core.fsyncObjectFiles", Value: "true"},
 		)
 	}
 
-	combinedGlobals = append(combinedGlobals, commandDescription.opts...)
-	combinedGlobals = append(combinedGlobals, cc.globals...)
-	for _, configPair := range gitConfig {
-		combinedGlobals = append(combinedGlobals, ConfigPair{
-			Key:   configPair.Key,
-			Value: configPair.Value,
-		})
-	}
-
-	for _, global := range combinedGlobals {
-		globalArgs, err := global.GlobalArgs()
-		if err != nil {
-			return nil, err
-		}
-		args = append(args, globalArgs...)
-	}
-
-	scArgs, err := sc.CommandArgs()
-	if err != nil {
-		return nil, err
-	}
-
-	return append(args, scArgs...), nil
+	return config, nil
 }
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index b1ffe5823..6a053da1c 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -10,6 +10,7 @@ import (
 	"os"
 	"os/exec"
 	"path/filepath"
+	"strings"
 	"testing"
 
 	"github.com/stretchr/testify/require"
@@ -419,3 +420,59 @@ func TestExecCommandFactory_GitVersion(t *testing.T) {
 		require.Equal(t, "2.34.1", version.String())
 	})
 }
+
+func TestExecCommandFactory_config(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
+
+	commonEnv := []string{
+		"gc.auto=0",
+		"core.autocrlf=input",
+		"core.usereplacerefs=false",
+	}
+
+	for _, tc := range []struct {
+		desc        string
+		version     string
+		expectedEnv []string
+	}{
+		{
+			desc:        "without support for core.fsync",
+			version:     "2.35.0",
+			expectedEnv: append(commonEnv, "core.fsyncobjectfiles=true"),
+		},
+		{
+			desc:        "with support for core.fsync",
+			version:     "2.36.0",
+			expectedEnv: append(commonEnv, "core.fsync=objects,derived-metadata,reference", "core.fsyncmethod=fsync"),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			factory := gittest.NewInterceptingCommandFactory(ctx, t, cfg, func(execEnv git.ExecutionEnvironment) string {
+				return fmt.Sprintf(
+					`#!/usr/bin/env bash
+					if test "$1" = "version"
+					then
+						echo "git version %s"
+						exit 0
+					fi
+					exec %q "$@"
+				`, tc.version, execEnv.BinaryPath)
+			}, gittest.WithInterceptedVersion())
+
+			var stdout bytes.Buffer
+			cmd, err := factory.NewWithDir(ctx, "/", git.SubCmd{
+				Name: "config",
+				Flags: []git.Option{
+					git.Flag{Name: "--list"},
+				},
+			}, git.WithStdout(&stdout))
+			require.NoError(t, err)
+
+			require.NoError(t, cmd.Wait())
+			require.Equal(t, tc.expectedEnv, strings.Split(strings.TrimSpace(stdout.String()), "\n"))
+		})
+	}
+}