git: Fix fsyncing config incompatibility with Git v2.36.0

Git v2.36.0 has introduced new infrastructure to allow administrators
more fine-grained control over what files Git will synchronize to disks.
This new infrastructure replaces the old `core.fsyncObjectFiles` config,
which we are currently injecting unconditionally. Unfortunately, the new
Git version has started to emit a warning in that case, which would both
be presented to the user and furthermore it would break a bunch of our
tests.

We're thus forced to conditionally inject either the old or the new
config depending on which Git version we're running. Let's do so by
injecting `core.fsync=objects,derived-metadata` instead when running Git
v2.36.0 or newer. This has the same semantics as what we had previously
with `core.fsyncObjectFiles=true`:

    - `default` is what Git did by default previously, and expands to
      `objects,derived-metadata,-loose-object`. Note that the current
      documentation in gitconfig(1) is wrong as it pretends that it
      instead expands to `committed,-loose-object`.

    - Because we want to also synchronize loose objects though we can
      drop the `-loose-object` part.

This fixes compatibility witih v2.36.0 while continuing to do the same
as we did before.

Changelog: fixed

2 files changed, 37 insertions, 5 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index b1fc31251..a328e1415 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -421,6 +421,14 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg)
 	}
 
+	// 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.
+	gitVersion, err := cf.GitVersion(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("determining Git version: %w", err)
+	}
+
 	// As global options may cancel out each other, we have a clearly defined order in which
 	// globals get applied. The order is similar to how git handles configuration options from
 	// most general to most specific. This allows callsites to override options which would
@@ -434,10 +442,6 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 	//    `WithReftxHook()`.
 	// 4. Configuration as provided by the admin in Gitaly's config.toml.
 	combinedGlobals := []GlobalOption{
-		// Synchronize object files to lessen the likelihood of
-		// repository corruption in case the server crashes.
-		ConfigPair{Key: "core.fsyncObjectFiles", Value: "true"},
-
 		// Disable automatic garbage collection as we handle scheduling
 		// of it ourselves.
 		ConfigPair{Key: "gc.auto", Value: "0"},
@@ -456,6 +460,25 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 		ConfigPair{Key: "core.useReplaceRefs", Value: "false"},
 	}
 
+	// 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,
+			// For now, we only fsync what Git versions previous to v2.36.0 have fsynced
+			// when `core.fsyncObjectFiles=true`. Later, we'll want to expand this to
+			// also sync references to disk to fix a long-standing issue.
+			ConfigPair{Key: "core.fsync", Value: "objects,derived-metadata"},
+			ConfigPair{Key: "core.fsyncMethod", Value: "fsync"},
+		)
+	} 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"},
+		)
+	}
+
 	combinedGlobals = append(combinedGlobals, commandDescription.opts...)
 	combinedGlobals = append(combinedGlobals, cc.globals...)
 	for _, configPair := range gitConfig {
diff --git a/internal/middleware/commandstatshandler/commandstatshandler_test.go b/internal/middleware/commandstatshandler/commandstatshandler_test.go
index 9289eed11..9369cbb8e 100644
--- a/internal/middleware/commandstatshandler/commandstatshandler_test.go
+++ b/internal/middleware/commandstatshandler/commandstatshandler_test.go
@@ -99,7 +99,16 @@ func TestInterceptor(t *testing.T) {
 				require.NoError(t, err)
 			},
 			expectedLogData: map[string]interface{}{
-				"command.count": 1,
+				// Until we bump our minimum required Git version to v2.36.0 we are
+				// forced to carry a version check whenever we spawn Git commands.
+				// The command count here is thus 2 because of the additional
+				// git-version(1) command. Note that the next command count remains
+				// 1 though: the version is cached, and consequentially we don't
+				// re-run git-version(1).
+				//
+				// This test will break again as soon as we bump the minimum Git
+				// version and thus remove the version check.
+				"command.count": 2,
 			},
 		},
 		{