ruby: Fix diverging Git configuration in Go and Ruby

Right now, we have multiple sources of truth for how Git is configured.
In Go, most of the configuration is handled centrally by Gitaly itself,
includes mandatory configuration that must always be set. This config
supersedes and overrides any values in the global gitconfig that is
installed by both Omnibus and CNG. The goal here is that Gitaly becomes
the single source of truth for the Git configuration eventually and that
it starts ignoring any system- or global-level configuration that exists
in the filesystem. With this schema, we have a bunch of benefits:

    - We exactly know about the configuration that Git commands are in.

    - We don't need to rely on the repository's gitconfig anymore like
      we did in the past.

    - We have much tighter control over the execution environment and
      can also easily set up per-command configuration.

    - We can conditionally inject Git configuration based on the Git
      version that's currently running.

While we already live in this world for quite a while in our Go code
base, the Ruby sidecar is a different story. Here we don't use any of
the central configuration yet, but instead we still rely on the files
installed by both CNG and Omnibus. Which effectively means that we
cannot currently stop installing them, or otherwise some important
config entries that we do alread inject in Go might get lost.

Unfortunately, this has come to bite us just now where we're close to
removing the sidecar altogether: starting with Git v2.36.0, it prints a
warning when `core.fsyncObjectFiles` is set. This means that we're
forced to conditionally either inject this configuration when running
with an older Git version, or alternatively we must inject the new Git
configuration `core.fsync`. And while this is easy enough to do in Go
land, the sidecar can't do that at all because it is forced to rely on
the gitconfig files installed by distributions.

Let's fix this issue by injecting the Git configuration as set up by the
Git command factory in the Ruby sidecar via environment variables. This
ensures that Git commands spawned by it will share their configuration
with what Go uses, including `core.fsyncObjectFiles` or `core.fsync`
depending on the current Git version. Given that we now know that those
options are always set in the context of Ruby we can then strip away the
old setting that is causing warnings in both CNG and Omnibus.

Changelog: fixed

5 files changed, 168 insertions, 22 deletions

diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 41ee7a9a5..14f94ade8 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -34,6 +34,11 @@ type CommandFactory interface {
 	HooksPath(context.Context) string
 	// GitVersion returns the Git version used by the command factory.
 	GitVersion(context.Context) (Version, error)
+
+	// SidecarGitConfiguration returns the Git configuration is it should be used by the Ruby
+	// sidecar. This is a design wart and shouldn't ever be used outside of the context of the
+	// sidecar.
+	SidecarGitConfiguration(context.Context) ([]ConfigPair, error)
 }
 
 type execCommandFactoryConfig struct {
@@ -514,3 +519,47 @@ func (cf *ExecCommandFactory) globalConfiguration(ctx context.Context) ([]Global
 
 	return config, nil
 }
+
+// SidecarGitConfiguration assembles the Git configuration as required by the Ruby sidecar. This
+// includes global configuration, command-specific configuration for all commands executed in the
+// sidecar, as well as configuration that was configured by the administrator in Gitaly's config.
+//
+// This function should not be used for anything else but the Ruby sidecar.
+func (cf *ExecCommandFactory) SidecarGitConfiguration(ctx context.Context) ([]ConfigPair, error) {
+	// Collect the global configuration that is specific to the current Git version...
+	options, err := cf.globalConfiguration(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("getting global config: %w", err)
+	}
+
+	// ... as well as all configuration that exists for specific Git subcommands. The sidecar
+	// only executes git-fetch(1), git-symbolic-ref(1) and git-update-ref(1) nowadays, and this
+	// set of commands is not expected to grow anymore. So while really intimate with how the
+	// sidecar does this, it is good enough until we finally remove it.
+	options = append(options, commandDescriptions["fetch"].opts...)
+	options = append(options, commandDescriptions["symbolic-ref"].opts...)
+	options = append(options, commandDescriptions["update-ref"].opts...)
+
+	// Convert the `GlobalOption`s into `ConfigPair`s.
+	configPairs := make([]ConfigPair, 0, len(options)+len(cf.cfg.Git.Config))
+	for _, option := range options {
+		configPair, ok := option.(ConfigPair)
+		if !ok {
+			continue
+		}
+
+		configPairs = append(configPairs, configPair)
+	}
+
+	// Lastly, we also apply the Git configuration as set by the administrator in Gitaly's
+	// config. Note that we do not check for conflicts here: administrators should be able to
+	// override whatever is configured by Gitaly.
+	for _, configEntry := range cf.cfg.Git.Config {
+		configPairs = append(configPairs, ConfigPair{
+			Key:   configEntry.Key,
+			Value: configEntry.Value,
+		})
+	}
+
+	return configPairs, nil
+}
diff --git a/internal/git/command_factory_test.go b/internal/git/command_factory_test.go
index 6a053da1c..ab617297d 100644
--- a/internal/git/command_factory_test.go
+++ b/internal/git/command_factory_test.go
@@ -427,6 +427,11 @@ func TestExecCommandFactory_config(t *testing.T) {
 	ctx := testhelper.Context(t)
 	cfg := testcfg.Build(t)
 
+	// Create a repository and remove its gitconfig to bring us into a known state where there
+	// is no repo-level configuration that interferes with our test.
+	repo, repoDir := gittest.InitRepo(t, cfg, cfg.Storages[0])
+	require.NoError(t, os.Remove(filepath.Join(repoDir, "config")))
+
 	commonEnv := []string{
 		"gc.auto=0",
 		"core.autocrlf=input",
@@ -463,7 +468,7 @@ func TestExecCommandFactory_config(t *testing.T) {
 			}, gittest.WithInterceptedVersion())
 
 			var stdout bytes.Buffer
-			cmd, err := factory.NewWithDir(ctx, "/", git.SubCmd{
+			cmd, err := factory.New(ctx, repo, git.SubCmd{
 				Name: "config",
 				Flags: []git.Option{
 					git.Flag{Name: "--list"},
@@ -476,3 +481,66 @@ func TestExecCommandFactory_config(t *testing.T) {
 		})
 	}
 }
+
+func TestExecCommandFactory_SidecarGitConfiguration(t *testing.T) {
+	t.Parallel()
+
+	ctx := testhelper.Context(t)
+	cfg := testcfg.Build(t)
+
+	cfg.Git.Config = []config.GitConfig{
+		{Key: "custom.key", Value: "injected"},
+	}
+
+	commonHead := []git.ConfigPair{
+		{Key: "gc.auto", Value: "0"},
+		{Key: "core.autocrlf", Value: "input"},
+		{Key: "core.useReplaceRefs", Value: "false"},
+	}
+
+	commonTail := []git.ConfigPair{
+		{Key: "fetch.writeCommitGraph", Value: "false"},
+		{Key: "http.followRedirects", Value: "false"},
+		{Key: "fetch.recurseSubmodules", Value: "no"},
+		{Key: "fetch.fsckObjects", Value: "true"},
+		{Key: "fetch.fsck.badTimezone", Value: "ignore"},
+		{Key: "fetch.fsck.missingSpaceBeforeDate", Value: "ignore"},
+		{Key: "fetch.fsck.zeroPaddedFilemode", Value: "ignore"},
+		{Key: "custom.key", Value: "injected"},
+	}
+
+	for _, tc := range []struct {
+		desc           string
+		version        string
+		expectedConfig []git.ConfigPair
+	}{
+		{
+			desc:    "without support for core.fsync",
+			version: "2.35.0",
+			expectedConfig: append(append(commonHead,
+				git.ConfigPair{Key: "core.fsyncObjectFiles", Value: "true"},
+			), commonTail...),
+		},
+		{
+			desc:    "with support for core.fsync",
+			version: "2.36.0",
+			expectedConfig: append(append(commonHead,
+				git.ConfigPair{Key: "core.fsync", Value: "objects,derived-metadata,reference"},
+				git.ConfigPair{Key: "core.fsyncMethod", Value: "fsync"},
+			), commonTail...),
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			factory := gittest.NewInterceptingCommandFactory(ctx, t, cfg, func(git.ExecutionEnvironment) string {
+				return fmt.Sprintf(
+					`#!/usr/bin/env bash
+					echo "git version %s"
+				`, tc.version)
+			}, gittest.WithInterceptedVersion())
+
+			configPairs, err := factory.SidecarGitConfiguration(ctx)
+			require.NoError(t, err)
+			require.Equal(t, tc.expectedConfig, configPairs)
+		})
+	}
+}
diff --git a/internal/git/gittest/intercepting_command_factory.go b/internal/git/gittest/intercepting_command_factory.go
index f68a3bca4..c6c6f09a1 100644
--- a/internal/git/gittest/intercepting_command_factory.go
+++ b/internal/git/gittest/intercepting_command_factory.go
@@ -148,3 +148,9 @@ func (f *InterceptingCommandFactory) GitVersion(ctx context.Context) (git.Versio
 	}
 	return f.realCommandFactory.GitVersion(ctx)
 }
+
+// SidecarGitConfiguration returns the Ruby sidecar Git configuration as computed by the actual Git
+// command factory.
+func (f *InterceptingCommandFactory) SidecarGitConfiguration(ctx context.Context) ([]git.ConfigPair, error) {
+	return f.interceptingCommandFactory.SidecarGitConfiguration(ctx)
+}
diff --git a/internal/gitaly/rubyserver/rubyserver.go b/internal/gitaly/rubyserver/rubyserver.go
index 794178123..8db9c500d 100644
--- a/internal/gitaly/rubyserver/rubyserver.go
+++ b/internal/gitaly/rubyserver/rubyserver.go
@@ -37,14 +37,16 @@ func init() {
 	}
 }
 
-func setupEnv(cfg config.Cfg, gitCmdFactory git.CommandFactory) []string {
+func setupEnv(cfg config.Cfg, gitCmdFactory git.CommandFactory) ([]string, error) {
 	// Ideally, we'd pass in the RPC context to the Git command factory such that we can
 	// properly use feature flags to switch between different execution environments. But the
 	// Ruby server is precreated and thus cannot use feature flags here. So for now, we have to
 	// live with the fact that we cannot use feature flags for it.
-	gitExecEnv := gitCmdFactory.GetExecutionEnvironment(context.TODO())
-	// The same remark exists with our hooks path.
-	hooksPath := gitCmdFactory.HooksPath(context.TODO())
+	ctx, cancel := context.WithCancel(context.TODO())
+	defer cancel()
+
+	gitExecEnv := gitCmdFactory.GetExecutionEnvironment(ctx)
+	hooksPath := gitCmdFactory.HooksPath(ctx)
 
 	env := append(
 		command.AllowedEnvironment(os.Environ()),
@@ -65,6 +67,12 @@ func setupEnv(cfg config.Cfg, gitCmdFactory git.CommandFactory) []string {
 	env = appendEnvIfSet(env, "BUNDLE_USER_CONFIG")
 	env = appendEnvIfSet(env, "GEM_HOME")
 
+	gitConfig, err := gitCmdFactory.SidecarGitConfiguration(ctx)
+	if err != nil {
+		return nil, fmt.Errorf("getting Git configuration: %w", err)
+	}
+	env = append(env, git.ConfigPairsToGitEnvironment(gitConfig)...)
+
 	if dsn := cfg.Logging.RubySentryDSN; dsn != "" {
 		env = append(env, "SENTRY_DSN="+dsn)
 	}
@@ -73,7 +81,7 @@ func setupEnv(cfg config.Cfg, gitCmdFactory git.CommandFactory) []string {
 		env = append(env, "SENTRY_ENVIRONMENT="+sentryEnvironment)
 	}
 
-	return env
+	return env, nil
 }
 
 func appendEnvIfSet(envvars []string, key string) []string {
@@ -128,7 +136,10 @@ func (s *Server) start() error {
 	}
 
 	cfg := s.cfg
-	env := setupEnv(cfg, s.gitCmdFactory)
+	env, err := setupEnv(cfg, s.gitCmdFactory)
+	if err != nil {
+		return fmt.Errorf("setting up sidecar environment: %w", err)
+	}
 
 	gitalyRuby := filepath.Join(cfg.Ruby.Dir, "bin", "gitaly-ruby")
 
diff --git a/internal/gitaly/rubyserver/rubyserver_test.go b/internal/gitaly/rubyserver/rubyserver_test.go
index d852841a1..a98d0eeb4 100644
--- a/internal/gitaly/rubyserver/rubyserver_test.go
+++ b/internal/gitaly/rubyserver/rubyserver_test.go
@@ -96,6 +96,12 @@ func (mockGitCommandFactory) HooksPath(context.Context) string {
 	return "custom_hooks_path"
 }
 
+func (mockGitCommandFactory) SidecarGitConfiguration(context.Context) ([]git.ConfigPair, error) {
+	return []git.ConfigPair{
+		{Key: "core.gc", Value: "false"},
+	}, nil
+}
+
 func TestSetupEnv(t *testing.T) {
 	cfg := config.Cfg{
 		BinDir:     "/bin/dit",
@@ -113,19 +119,25 @@ func TestSetupEnv(t *testing.T) {
 		Ruby: config.Ruby{RuggedGitConfigSearchPath: "/bin/rugged"},
 	}
 
-	env := setupEnv(cfg, mockGitCommandFactory{})
-
-	require.Contains(t, env, "FOO=bar")
-	require.Contains(t, env, "GITALY_LOG_DIR=/log/dir")
-	require.Contains(t, env, "GITALY_RUBY_GIT_BIN_PATH=/something")
-	require.Contains(t, env, fmt.Sprintf("GITALY_RUBY_WRITE_BUFFER_SIZE=%d", streamio.WriteBufferSize))
-	require.Contains(t, env, fmt.Sprintf("GITALY_RUBY_MAX_COMMIT_OR_TAG_MESSAGE_SIZE=%d", helper.MaxCommitOrTagMessageSize))
-	require.Contains(t, env, "GITALY_RUBY_GITALY_BIN_DIR=/bin/dit")
-	require.Contains(t, env, "GITALY_VERSION="+version.GetVersion())
-	require.Contains(t, env, fmt.Sprintf("GITALY_SOCKET=%s", cfg.InternalSocketPath()))
-	require.Contains(t, env, "GITALY_TOKEN=paswd")
-	require.Contains(t, env, "GITALY_RUGGED_GIT_CONFIG_SEARCH_PATH=/bin/rugged")
-	require.Contains(t, env, "SENTRY_DSN=testDSN")
-	require.Contains(t, env, "SENTRY_ENVIRONMENT=testEnvironment")
-	require.Contains(t, env, "GITALY_GIT_HOOKS_DIR=custom_hooks_path")
+	env, err := setupEnv(cfg, mockGitCommandFactory{})
+	require.NoError(t, err)
+
+	require.Subset(t, env, []string{
+		"FOO=bar",
+		"GITALY_LOG_DIR=/log/dir",
+		"GITALY_RUBY_GIT_BIN_PATH=/something",
+		fmt.Sprintf("GITALY_RUBY_WRITE_BUFFER_SIZE=%d", streamio.WriteBufferSize),
+		fmt.Sprintf("GITALY_RUBY_MAX_COMMIT_OR_TAG_MESSAGE_SIZE=%d", helper.MaxCommitOrTagMessageSize),
+		"GITALY_RUBY_GITALY_BIN_DIR=/bin/dit",
+		"GITALY_VERSION=" + version.GetVersion(),
+		fmt.Sprintf("GITALY_SOCKET=%s", cfg.InternalSocketPath()),
+		"GITALY_TOKEN=paswd",
+		"GITALY_RUGGED_GIT_CONFIG_SEARCH_PATH=/bin/rugged",
+		"SENTRY_DSN=testDSN",
+		"SENTRY_ENVIRONMENT=testEnvironment",
+		"GITALY_GIT_HOOKS_DIR=custom_hooks_path",
+		"GIT_CONFIG_KEY_0=core.gc",
+		"GIT_CONFIG_VALUE_0=false",
+		"GIT_CONFIG_COUNT=1",
+	})
 }