Merge branch 'pks-git-inject-global-config' into 'master'

git: Add support for options which always get injected

See merge request gitlab-org/gitaly!3028

7 files changed, 46 insertions, 30 deletions

diff --git a/changelogs/unreleased/pks-git-inject-global-config.yml b/changelogs/unreleased/pks-git-inject-global-config.yml
new file mode 100644
index 000000000..3d9d30228
--- /dev/null
+++ b/changelogs/unreleased/pks-git-inject-global-config.yml
@@ -0,0 +1,5 @@
+---
+title: 'git: Add support for options which always get injected'
+merge_request: 3028
+author:
+type: added
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index d9ae526fd..015d7c7d9 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -14,6 +14,24 @@ import (
 	"gitlab.com/gitlab-org/gitaly/internal/storage"
 )
 
+var (
+	globalOptions = []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"},
+
+		// CRLF line endings will get replaced with LF line endings
+		// when writing blobs to the object database. No conversion is
+		// done when reading blobs from the object database. This is
+		// required for the web editor.
+		ConfigPair{Key: "core.autocrlf", Value: "input"},
+	}
+)
+
 // CommandFactory is designed to create and run git commands in a protected and fully managed manner.
 type CommandFactory interface {
 	// New creates a new command for the repo repository.
@@ -160,11 +178,13 @@ func combineArgs(globals []GlobalOption, sc Cmd, cc *cmdCfg) (_ []string, err er
 	// specific. This allows callsites to override options which would
 	// otherwise be set up automatically.
 	//
-	// 1. Globals which get set up by default for a given git command.
-	// 2. Globals passed via command options, e.g. as set up by
+	// 1. Globals which get set up by default for all git commands.
+	// 2. Globals which get set up by default for a given git command.
+	// 3. Globals passed via command options, e.g. as set up by
 	//    `WithReftxHook()`.
-	// 3. Globals passed directly to the command at the site of execution.
+	// 4. Globals passed directly to the command at the site of execution.
 	var combinedGlobals []GlobalOption
+	combinedGlobals = append(combinedGlobals, globalOptions...)
 	combinedGlobals = append(combinedGlobals, gitCommand.opts...)
 	combinedGlobals = append(combinedGlobals, cc.globals...)
 	combinedGlobals = append(combinedGlobals, globals...)
diff --git a/internal/git/command_test.go b/internal/git/command_test.go
index 81384bfbb..6ab090d11 100644
--- a/internal/git/command_test.go
+++ b/internal/git/command_test.go
@@ -337,19 +337,25 @@ func TestNewCommandValid(t *testing.T) {
 		t.Run(tt.desc, func(t *testing.T) {
 			opts := []CmdOpt{WithRefTxHook(ctx, &gitalypb.Repository{}, cfg)}
 
+			expectArgs := append([]string{
+				"-c", "core.fsyncObjectFiles=true",
+				"-c", "gc.auto=0",
+				"-c", "core.autocrlf=input",
+			}, tt.expectArgs...)
+
 			cmd, err := NewCommand(ctx, testRepo, tt.globals, tt.subCmd, opts...)
 			require.NoError(t, err)
 			// ignore first 3 indeterministic args (executable path and repo args)
-			require.Equal(t, tt.expectArgs, cmd.Args()[3:])
+			require.Equal(t, expectArgs, cmd.Args()[3:])
 
 			cmd, err = NewCommandWithoutRepo(ctx, tt.globals, tt.subCmd, opts...)
 			require.NoError(t, err)
 			// ignore first indeterministic arg (executable path)
-			require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+			require.Equal(t, expectArgs, cmd.Args()[1:])
 
 			cmd, err = gitCmdFactory.NewWithDir(ctx, testRepoPath, tt.globals, tt.subCmd, opts...)
 			require.NoError(t, err)
-			require.Equal(t, tt.expectArgs, cmd.Args()[1:])
+			require.Equal(t, expectArgs, cmd.Args()[1:])
 		})
 	}
 }
diff --git a/internal/git/objectpool/link.go b/internal/git/objectpool/link.go
index 36b4907be..662f5a7aa 100644
--- a/internal/git/objectpool/link.go
+++ b/internal/git/objectpool/link.go
@@ -10,7 +10,6 @@ import (
 	"path/filepath"
 	"strings"
 
-	"gitlab.com/gitlab-org/gitaly/internal/git"
 	"gitlab.com/gitlab-org/gitaly/internal/git/remote"
 	"gitlab.com/gitlab-org/gitaly/internal/git/repository"
 	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
@@ -180,18 +179,3 @@ func (o *ObjectPool) Unlink(ctx context.Context, repo *gitalypb.Repository) erro
 
 	return nil
 }
-
-// Config options setting will leak the key value pairs in the logs. This makes
-// this function not suitable for general usage, and scoped to this package.
-// To be corrected in: https://gitlab.com/gitlab-org/gitaly/issues/1430
-func (o *ObjectPool) setConfig(ctx context.Context, key, value string) error {
-	cmd, err := git.NewCommand(ctx, o, nil, git.SubCmd{
-		Name:  "config",
-		Flags: []git.Option{git.ConfigPair{Key: key, Value: value}},
-	})
-	if err != nil {
-		return err
-	}
-
-	return cmd.Wait()
-}
diff --git a/internal/git/objectpool/pool.go b/internal/git/objectpool/pool.go
index 9e6ad046c..a05fb2ef5 100644
--- a/internal/git/objectpool/pool.go
+++ b/internal/git/objectpool/pool.go
@@ -100,10 +100,6 @@ func (o *ObjectPool) Create(ctx context.Context, repo *gitalypb.Repository) (err
 		return fmt.Errorf("remove hooks: %v", err)
 	}
 
-	if err := o.setConfig(ctx, "gc.auto", "0"); err != nil {
-		return fmt.Errorf("config gc.auto: %v", err)
-	}
-
 	return nil
 }
 
diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go
index f17903115..2592947b5 100644
--- a/internal/git/subcommand.go
+++ b/internal/git/subcommand.go
@@ -126,6 +126,10 @@ var gitCommands = map[string]gitCommand{
 			// a commit will be rejected. As this is a mostly harmless
 			// issue, we add the following flag to ignore this check.
 			ConfigPair{Key: "receive.fsck.badTimezone", Value: "ignore"},
+
+			// Make git-receive-pack(1) advertise the push options
+			// capability to clients.
+			ConfigPair{Key: "receive.advertisePushOptions", Value: "true"},
 		},
 	},
 	"remote": gitCommand{
@@ -133,6 +137,11 @@ var gitCommands = map[string]gitCommand{
 	},
 	"repack": gitCommand{
 		flags: scNoRefUpdates | scGeneratesPackfiles,
+		opts: []GlobalOption{
+			// Write bitmap indices when packing objects, which
+			// speeds up packfile creation for fetches.
+			ConfigPair{Key: "repack.writeBitmaps", Value: "true"},
+		},
 	},
 	"rev-list": gitCommand{
 		flags: scReadOnly,
diff --git a/internal/gitaly/service/objectpool/create_test.go b/internal/gitaly/service/objectpool/create_test.go
index 68f6f15e5..86e405ce9 100644
--- a/internal/gitaly/service/objectpool/create_test.go
+++ b/internal/gitaly/service/objectpool/create_test.go
@@ -52,10 +52,6 @@ func TestCreate(t *testing.T) {
 	out := testhelper.MustRunCommand(t, nil, "git", "-C", pool.FullPath(), "cat-file", "-s", "55bc176024cfa3baaceb71db584c7e5df900ea65")
 	assert.Equal(t, "282\n", string(out))
 
-	// No automatic GC
-	gc := testhelper.MustRunCommand(t, nil, "git", "-C", pool.FullPath(), "config", "--get", "gc.auto")
-	assert.Equal(t, "0\n", string(gc))
-
 	// Making the same request twice, should result in an error
 	_, err = client.CreateObjectPool(ctx, poolReq)
 	require.Error(t, err)