Merge branch 'pks-git-pack-configuration' into 'master'

git: Simplify setup of pack-generation configuration

See merge request gitlab-org/gitaly!4361

3 files changed, 22 insertions, 39 deletions

diff --git a/internal/git/command_description.go b/internal/git/command_description.go
index a1a33c037..3efad9c5c 100644
--- a/internal/git/command_description.go
+++ b/internal/git/command_description.go
@@ -11,8 +11,6 @@ const (
 	scNoRefUpdates = 1 << iota
 	// scNoEndOfOptions denotes a command which doesn't know --end-of-options
 	scNoEndOfOptions
-	// scGeneratesPackfiles denotes a command which may generate packfiles
-	scGeneratesPackfiles
 )
 
 type commandDescription struct {
@@ -37,7 +35,8 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
 	"bundle": {
-		flags: scNoRefUpdates | scGeneratesPackfiles,
+		flags: scNoRefUpdates,
+		opts:  packConfiguration(),
 	},
 	"cat-file": {
 		flags: scNoRefUpdates,
@@ -53,13 +52,12 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoEndOfOptions,
 	},
 	"clone": {
-		flags: scGeneratesPackfiles,
-		opts: []GlobalOption{
+		opts: append([]GlobalOption{
 			// See "init" for why we set the template directory to the empty string.
 			ConfigPair{Key: "init.templateDir", Value: ""},
 			// See "fetch" for why we disable following redirects.
 			ConfigPair{Key: "http.followRedirects", Value: "false"},
-		},
+		}, packConfiguration()...),
 	},
 	"commit": {
 		flags: 0,
@@ -112,7 +110,8 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoRefUpdates,
 	},
 	"gc": {
-		flags: scNoRefUpdates | scGeneratesPackfiles,
+		flags: scNoRefUpdates,
+		opts:  packConfiguration(),
 	},
 	"grep": {
 		// git-grep(1) does not support disambiguating options from paths from
@@ -174,7 +173,8 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoRefUpdates,
 	},
 	"pack-objects": {
-		flags: scNoRefUpdates | scGeneratesPackfiles,
+		flags: scNoRefUpdates,
+		opts:  packConfiguration(),
 	},
 	"prune": {
 		flags: scNoRefUpdates,
@@ -211,12 +211,12 @@ var commandDescriptions = map[string]commandDescription{
 		},
 	},
 	"repack": {
-		flags: scNoRefUpdates | scGeneratesPackfiles,
-		opts: []GlobalOption{
+		flags: scNoRefUpdates,
+		opts: append([]GlobalOption{
 			// Write bitmap indices when packing objects, which
 			// speeds up packfile creation for fetches.
 			ConfigPair{Key: "repack.writeBitmaps", Value: "true"},
-		},
+		}, packConfiguration()...),
 	},
 	"rev-list": {
 		// We cannot use --end-of-options here because pseudo revisions like `--all`
@@ -267,13 +267,13 @@ var commandDescriptions = map[string]commandDescription{
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
 	"upload-pack": {
-		flags: scNoRefUpdates | scGeneratesPackfiles,
-		opts: []GlobalOption{
+		flags: scNoRefUpdates,
+		opts: append([]GlobalOption{
 			ConfigPair{Key: "uploadpack.allowFilter", Value: "true"},
 			// Enables the capability to request individual SHA1's from the
 			// remote repo.
 			ConfigPair{Key: "uploadpack.allowAnySHA1InWant", Value: "true"},
-		},
+		}, packConfiguration()...),
 	},
 	"version": {
 		flags: scNoRefUpdates,
@@ -320,12 +320,6 @@ func (c commandDescription) mayUpdateRef() bool {
 	return c.flags&scNoRefUpdates == 0
 }
 
-// mayGeneratePackfiles indicates if a command is known to generate
-// packfiles. This is used in order to inject packfile configuration.
-func (c commandDescription) mayGeneratePackfiles() bool {
-	return c.flags&scGeneratesPackfiles != 0
-}
-
 // supportsEndOfOptions indicates whether a command can handle the
 // `--end-of-options` option.
 func (c commandDescription) supportsEndOfOptions() bool {
@@ -426,3 +420,10 @@ func fsckConfiguration(prefix string) []GlobalOption {
 
 	return configPairs
 }
+
+func packConfiguration() []GlobalOption {
+	return []GlobalOption{
+		ConfigPair{Key: "pack.windowMemory", Value: "100m"},
+		ConfigPair{Key: "pack.writeReverseIndex", Value: "true"},
+	}
+}
diff --git a/internal/git/command_description_test.go b/internal/git/command_description_test.go
index a0915b9d5..09209df15 100644
--- a/internal/git/command_description_test.go
+++ b/internal/git/command_description_test.go
@@ -7,16 +7,6 @@ import (
 	"github.com/stretchr/testify/require"
 )
 
-func TestCommandDescriptions_mayGeneratePackfiles(t *testing.T) {
-	gcDescription, ok := commandDescriptions["gc"]
-	require.True(t, ok)
-	require.True(t, gcDescription.mayGeneratePackfiles())
-
-	applyDescription, ok := commandDescriptions["apply"]
-	require.True(t, ok)
-	require.False(t, applyDescription.mayGeneratePackfiles())
-}
-
 func TestCommandDescriptions_revListPositionalArgs(t *testing.T) {
 	revlist, ok := commandDescriptions["rev-list"]
 	require.True(t, ok)
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index 19dc1537a..2ffe61596 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -565,14 +565,6 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg)
 	}
 
-	commandSpecificOptions := commandDescription.opts
-	if commandDescription.mayGeneratePackfiles() {
-		commandSpecificOptions = append(commandSpecificOptions,
-			ConfigPair{Key: "pack.windowMemory", Value: "100m"},
-			ConfigPair{Key: "pack.writeReverseIndex", Value: "true"},
-		)
-	}
-
 	// 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
@@ -587,7 +579,7 @@ func (cf *ExecCommandFactory) combineArgs(ctx context.Context, gitConfig []confi
 	// 4. Configuration as provided by the admin in Gitaly's config.toml.
 	var combinedGlobals []GlobalOption
 	combinedGlobals = append(combinedGlobals, globalOptions...)
-	combinedGlobals = append(combinedGlobals, commandSpecificOptions...)
+	combinedGlobals = append(combinedGlobals, commandDescription.opts...)
 	combinedGlobals = append(combinedGlobals, cc.globals...)
 	for _, configPair := range gitConfig {
 		combinedGlobals = append(combinedGlobals, ConfigPair{