Merge branch 'pks-config-pair-global-opt' into 'master'

git: Allow ConfigPair to be used as GlobalOption

See merge request gitlab-org/gitaly!2936

12 files changed, 178 insertions, 29 deletions

diff --git a/internal/git/hooks_options.go b/internal/git/hooks_options.go
index 642aa7f51..2f3390ddd 100644
--- a/internal/git/hooks_options.go
+++ b/internal/git/hooks_options.go
@@ -68,7 +68,7 @@ func (cc *cmdCfg) configureHooks(
 		fmt.Sprintf("%s=%s", log.GitalyLogDirEnvKey, cfg.Logging.Dir),
 	)
 
-	cc.globals = append(cc.globals, ValueFlag{"-c", fmt.Sprintf("core.hooksPath=%s", hooks.Path(cfg))})
+	cc.globals = append(cc.globals, ConfigPair{Key: "core.hooksPath", Value: hooks.Path(cfg)})
 	cc.hooksConfigured = true
 
 	return nil
diff --git a/internal/git/objectpool/fetch.go b/internal/git/objectpool/fetch.go
index 995f124a1..48f8c3df3 100644
--- a/internal/git/objectpool/fetch.go
+++ b/internal/git/objectpool/fetch.go
@@ -187,10 +187,10 @@ func rescueDanglingObjects(ctx context.Context, repo repository.GitRepo) error {
 
 func repackPool(ctx context.Context, pool repository.GitRepo) error {
 	repackArgs := []git.GlobalOption{
-		git.ValueFlag{"-c", "pack.island=" + sourceRefNamespace + "/he(a)ds"},
-		git.ValueFlag{"-c", "pack.island=" + sourceRefNamespace + "/t(a)gs"},
-		git.ValueFlag{"-c", "pack.islandCore=a"},
-		git.ValueFlag{"-c", "pack.writeBitmapHashCache=true"},
+		git.ConfigPair{Key: "pack.island", Value: sourceRefNamespace + "/he(a)ds"},
+		git.ConfigPair{Key: "pack.island", Value: sourceRefNamespace + "/t(a)gs"},
+		git.ConfigPair{Key: "pack.islandCore", Value: "a"},
+		git.ConfigPair{Key: "pack.writeBitmapHashCache", Value: "true"},
 	}
 
 	repackCmd, err := git.SafeCmd(ctx, pool, repackArgs, git.SubCmd{
diff --git a/internal/git/receivepack.go b/internal/git/receivepack.go
index 97fbc98f4..67390908d 100644
--- a/internal/git/receivepack.go
+++ b/internal/git/receivepack.go
@@ -9,7 +9,7 @@ func ReceivePackConfig() []GlobalOption {
 		// this by rigging core.alternateRefsCommand to produce no output.
 		// Because Git itself will append the pool repository directory, the
 		// command ends with a "#". The end result is that Git runs `/bin/sh -c 'exit 0 # /path/to/pool.git`.
-		ValueFlag{"-c", "core.alternateRefsCommand=exit 0 #"},
+		ConfigPair{Key: "core.alternateRefsCommand", Value: "exit 0 #"},
 
 		// In the past, there was a bug in git that caused users to
 		// create commits with invalid timezones. As a result, some
@@ -17,6 +17,6 @@ func ReceivePackConfig() []GlobalOption {
 		// fsck received packfiles by default, any push containing such
 		// a commit will be rejected. As this is a mostly harmless
 		// issue, we add the following flag to ignore this check.
-		ValueFlag{"-c", "receive.fsck.badTimezone=ignore"},
+		ConfigPair{Key: "receive.fsck.badTimezone", Value: "ignore"},
 	}
 }
diff --git a/internal/git/repository_test.go b/internal/git/repository_test.go
index d371f3dfa..ec7701936 100644
--- a/internal/git/repository_test.go
+++ b/internal/git/repository_test.go
@@ -571,7 +571,7 @@ func TestLocalRepository_FetchRemote(t *testing.T) {
 			ctx,
 			"source",
 			FetchOpts{
-				Global: []GlobalOption{ValueFlag{Name: "-c", Value: "fetch.prune=true"}},
+				Global: []GlobalOption{ConfigPair{Key: "fetch.prune", Value: "true"}},
 			}),
 		)
 
diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go
index 82a90658e..dfa50290b 100644
--- a/internal/git/safecmd.go
+++ b/internal/git/safecmd.go
@@ -175,16 +175,35 @@ type ConfigPair struct {
 	Scope string
 }
 
-var configKeyRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*\.(.+\.)*[[:alnum:]]+[-[:alnum:]]*$`)
+var (
+	configKeyOptionRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*\.(.+\.)*[[:alnum:]]+[-[:alnum:]]*$`)
+	// configKeyGlobalRegex is intended to verify config keys when used as
+	// global arguments. We're playing it safe here by disallowing lots of
+	// keys which git would parse just fine, but we only have a limited
+	// number of config entries anyway. Most importantly, we cannot allow
+	// `=` as part of the key as that would break parsing of `git -c`.
+	configKeyGlobalRegex = regexp.MustCompile(`^[[:alnum:]]+(\.[-/_a-zA-Z0-9]+)+$`)
+)
 
 // OptionArgs validates the config pair args
 func (cp ConfigPair) OptionArgs() ([]string, error) {
-	if !configKeyRegex.MatchString(cp.Key) {
+	if !configKeyOptionRegex.MatchString(cp.Key) {
 		return nil, fmt.Errorf("config key %q failed regexp validation: %w", cp.Key, ErrInvalidArg)
 	}
 	return []string{cp.Key, cp.Value}, nil
 }
 
+// GlobalArgs generates a git `-c <key>=<value>` flag. The key must pass
+// validation by containing only alphanumeric sections separated by dots.
+// No other characters are allowed for now as `git -c` may not correctly parse
+// them, most importantly when they contain equals signs.
+func (cp ConfigPair) GlobalArgs() ([]string, error) {
+	if !configKeyGlobalRegex.MatchString(cp.Key) {
+		return nil, fmt.Errorf("config key %q failed regexp validation: %w", cp.Key, ErrInvalidArg)
+	}
+	return []string{"-c", fmt.Sprintf("%s=%s", cp.Key, cp.Value)}, nil
+}
+
 // Flag is a single token optional command line argument that enables or
 // disables functionality (e.g. "-L")
 type Flag struct {
@@ -310,8 +329,8 @@ func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error {
 		return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookNotRequired)
 	}
 	if mayGeneratePackfiles(sc.Subcommand()) {
-		cc.globals = append(cc.globals, ValueFlag{
-			Name: "-c", Value: "pack.windowMemory=100m",
+		cc.globals = append(cc.globals, ConfigPair{
+			Key: "pack.windowMemory", Value: "100m",
 		})
 	}
 
diff --git a/internal/git/safecmd_test.go b/internal/git/safecmd_test.go
index f66eaed44..286c4f0fc 100644
--- a/internal/git/safecmd_test.go
+++ b/internal/git/safecmd_test.go
@@ -69,6 +69,136 @@ func TestFlagValidation(t *testing.T) {
 	}
 }
 
+func TestGlobalOption(t *testing.T) {
+	for _, tc := range []struct {
+		desc     string
+		option   GlobalOption
+		valid    bool
+		expected []string
+	}{
+		{
+			desc:     "single-letter flag",
+			option:   Flag{Name: "-k"},
+			valid:    true,
+			expected: []string{"-k"},
+		},
+		{
+			desc:     "long option flag",
+			option:   Flag{Name: "--asdf"},
+			valid:    true,
+			expected: []string{"--asdf"},
+		},
+		{
+			desc:     "multiple single-letter flags",
+			option:   Flag{Name: "-abc"},
+			valid:    true,
+			expected: []string{"-abc"},
+		},
+		{
+			desc:     "single-letter option with value",
+			option:   Flag{Name: "-a=value"},
+			valid:    true,
+			expected: []string{"-a=value"},
+		},
+		{
+			desc:     "long option with value",
+			option:   Flag{Name: "--asdf=value"},
+			valid:    true,
+			expected: []string{"--asdf=value"},
+		},
+		{
+			desc:   "flags without dashes are not allowed",
+			option: Flag{Name: "foo"},
+			valid:  false,
+		},
+		{
+			desc:   "leading spaces are not allowed",
+			option: Flag{Name: " -a"},
+			valid:  false,
+		},
+
+		{
+			desc:     "single-letter value flag",
+			option:   ValueFlag{Name: "-a", Value: "value"},
+			valid:    true,
+			expected: []string{"-a", "value"},
+		},
+		{
+			desc:     "long option value flag",
+			option:   ValueFlag{Name: "--foobar", Value: "value"},
+			valid:    true,
+			expected: []string{"--foobar", "value"},
+		},
+		{
+			desc:     "multiple single-letters for value flag",
+			option:   ValueFlag{Name: "-abc", Value: "value"},
+			valid:    true,
+			expected: []string{"-abc", "value"},
+		},
+		{
+			desc:     "value flag with empty value",
+			option:   ValueFlag{Name: "--key", Value: ""},
+			valid:    true,
+			expected: []string{"--key", ""},
+		},
+		{
+			desc:   "value flag without dashes are not allowed",
+			option: ValueFlag{Name: "foo", Value: "bar"},
+			valid:  false,
+		},
+		{
+			desc:   "value flag with empty key are not allowed",
+			option: ValueFlag{Name: "", Value: "bar"},
+			valid:  false,
+		},
+
+		{
+			desc:     "config pair with key and value",
+			option:   ConfigPair{Key: "foo.bar", Value: "value"},
+			valid:    true,
+			expected: []string{"-c", "foo.bar=value"},
+		},
+		{
+			desc:     "config pair with subsection",
+			option:   ConfigPair{Key: "foo.bar.baz", Value: "value"},
+			valid:    true,
+			expected: []string{"-c", "foo.bar.baz=value"},
+		},
+		{
+			desc:     "config pair without value",
+			option:   ConfigPair{Key: "foo.bar"},
+			valid:    true,
+			expected: []string{"-c", "foo.bar="},
+		},
+		{
+			desc:   "config pair with invalid section format",
+			option: ConfigPair{Key: "foo", Value: "value"},
+			valid:  false,
+		},
+		{
+			desc:   "config pair with leading whitespace",
+			option: ConfigPair{Key: " foo.bar", Value: "value"},
+			valid:  false,
+		},
+		{
+			desc:   "config pair with disallowed character in key",
+			option: ConfigPair{Key: "http.https://weak.example.com.sslVerify", Value: "false"},
+			valid:  false,
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			args, err := tc.option.GlobalArgs()
+			if tc.valid {
+				require.NoError(t, err)
+				require.Equal(t, tc.expected, args)
+			} else {
+				require.Error(t, err, "expected error, but args %v passed validation", args)
+				require.True(t, IsInvalidArgErr(err))
+			}
+		})
+	}
+}
+
 func TestSafeCmdInvalidArg(t *testing.T) {
 	for _, tt := range []struct {
 		globals []GlobalOption
diff --git a/internal/git/uploadpack.go b/internal/git/uploadpack.go
index c5c7d64c2..0a1c44948 100644
--- a/internal/git/uploadpack.go
+++ b/internal/git/uploadpack.go
@@ -4,9 +4,9 @@ package git
 // partial-clone filters.
 func UploadPackFilterConfig() []GlobalOption {
 	return []GlobalOption{
-		ValueFlag{"-c", "uploadpack.allowFilter=true"},
+		ConfigPair{Key: "uploadpack.allowFilter", Value: "true"},
 		// Enables the capability to request individual SHA1's from the
 		// remote repo.
-		ValueFlag{"-c", "uploadpack.allowAnySHA1InWant=true"},
+		ConfigPair{Key: "uploadpack.allowAnySHA1InWant", Value: "true"},
 	}
 }
diff --git a/internal/gitaly/service/diff/commit.go b/internal/gitaly/service/diff/commit.go
index ac137617d..d1180b5be 100644
--- a/internal/gitaly/service/diff/commit.go
+++ b/internal/gitaly/service/diff/commit.go
@@ -207,7 +207,7 @@ func validateRequest(in requestWithLeftRightCommitIds) error {
 
 func eachDiff(ctx context.Context, rpc string, repo *gitalypb.Repository, subCmd git.Cmd, limits diff.Limits, callback func(*diff.Diff) error) error {
 	diffArgs := []git.GlobalOption{
-		git.ValueFlag{"-c", "diff.noprefix=false"},
+		git.ConfigPair{Key: "diff.noprefix", Value: "false"},
 	}
 
 	cmd, err := git.SafeCmd(ctx, repo, diffArgs, subCmd)
diff --git a/internal/gitaly/service/repository/archive.go b/internal/gitaly/service/repository/archive.go
index 7c620e5a2..741b9c549 100644
--- a/internal/gitaly/service/repository/archive.go
+++ b/internal/gitaly/service/repository/archive.go
@@ -204,7 +204,7 @@ func handleArchive(p archiveParams) error {
 
 	if p.in.GetIncludeLfsBlobs() {
 		binary := filepath.Join(p.binDir, "gitaly-lfs-smudge")
-		globals = append(globals, git.ValueFlag{"-c", fmt.Sprintf("filter.lfs.smudge=%s", binary)})
+		globals = append(globals, git.ConfigPair{Key: "filter.lfs.smudge", Value: binary})
 	}
 
 	archiveCommand, err := git.SafeCmdWithEnv(p.ctx, env, p.in.GetRepository(), globals, git.SubCmd{
diff --git a/internal/gitaly/service/repository/create_from_url.go b/internal/gitaly/service/repository/create_from_url.go
index 15a28a36b..62d7d4415 100644
--- a/internal/gitaly/service/repository/create_from_url.go
+++ b/internal/gitaly/service/repository/create_from_url.go
@@ -24,7 +24,7 @@ func (s *server) cloneFromURLCommand(ctx context.Context, repo *gitalypb.Reposit
 	}
 
 	globalFlags := []git.GlobalOption{
-		git.ValueFlag{Name: "-c", Value: "http.followRedirects=false"},
+		git.ConfigPair{Key: "http.followRedirects", Value: "false"},
 	}
 
 	cloneFlags := []git.Option{
@@ -44,7 +44,7 @@ func (s *server) cloneFromURLCommand(ctx context.Context, repo *gitalypb.Reposit
 
 		u.User = nil
 		authHeader := fmt.Sprintf("Authorization: Basic %s", base64.StdEncoding.EncodeToString([]byte(creds)))
-		globalFlags = append(globalFlags, git.ValueFlag{Name: "-c", Value: fmt.Sprintf("http.extraHeader=%s", authHeader)})
+		globalFlags = append(globalFlags, git.ConfigPair{Key: "http.extraHeader", Value: authHeader})
 	}
 
 	return git.SafeBareCmd(ctx, nil, globalFlags,
diff --git a/internal/gitaly/service/repository/fetch_remote.go b/internal/gitaly/service/repository/fetch_remote.go
index 47f24072d..f443bffee 100644
--- a/internal/gitaly/service/repository/fetch_remote.go
+++ b/internal/gitaly/service/repository/fetch_remote.go
@@ -96,13 +96,13 @@ func (s *server) FetchRemote(ctx context.Context, req *gitalypb.FetchRemoteReque
 		}(ctx)
 
 		for _, refspec := range refspecs {
-			opts.Global = append(opts.Global, git.ValueFlag{Name: "-c", Value: "remote." + remoteName + ".fetch=" + refspec})
+			opts.Global = append(opts.Global, git.ConfigPair{Key: "remote." + remoteName + ".fetch", Value: refspec})
 		}
 
 		opts.Global = append(opts.Global,
-			git.ValueFlag{Name: "-c", Value: "remote." + remoteName + ".mirror=true"},
-			git.ValueFlag{Name: "-c", Value: "remote." + remoteName + ".prune=true"},
-			git.ValueFlag{Name: "-c", Value: "http.followRedirects=false"},
+			git.ConfigPair{Key: "remote." + remoteName + ".mirror", Value: "true"},
+			git.ConfigPair{Key: "remote." + remoteName + ".prune", Value: "true"},
+			git.ConfigPair{Key: "http.followRedirects", Value: "false"},
 		)
 
 		if params.GetHttpAuthorizationHeader() != "" {
diff --git a/internal/gitaly/service/repository/repack.go b/internal/gitaly/service/repository/repack.go
index 214cf482f..a3abd040a 100644
--- a/internal/gitaly/service/repository/repack.go
+++ b/internal/gitaly/service/repository/repack.go
@@ -72,17 +72,17 @@ func repackCommand(ctx context.Context, repo repository.GitRepo, bitmap bool, ar
 
 func repackConfig(ctx context.Context, bitmap bool) []git.GlobalOption {
 	args := []git.GlobalOption{
-		git.ValueFlag{"-c", "pack.island=r(e)fs/heads"},
-		git.ValueFlag{"-c", "pack.island=r(e)fs/tags"},
-		git.ValueFlag{"-c", "pack.islandCore=e"},
-		git.ValueFlag{"-c", "repack.useDeltaIslands=true"},
+		git.ConfigPair{Key: "pack.island", Value: "r(e)fs/heads"},
+		git.ConfigPair{Key: "pack.island", Value: "r(e)fs/tags"},
+		git.ConfigPair{Key: "pack.islandCore", Value: "e"},
+		git.ConfigPair{Key: "repack.useDeltaIslands", Value: "true"},
 	}
 
 	if bitmap {
-		args = append(args, git.ValueFlag{"-c", "repack.writeBitmaps=true"})
-		args = append(args, git.ValueFlag{"-c", "pack.writeBitmapHashCache=true"})
+		args = append(args, git.ConfigPair{Key: "repack.writeBitmaps", Value: "true"})
+		args = append(args, git.ConfigPair{Key: "pack.writeBitmapHashCache", Value: "true"})
 	} else {
-		args = append(args, git.ValueFlag{"-c", "repack.writeBitmaps=false"})
+		args = append(args, git.ConfigPair{Key: "repack.writeBitmaps", Value: "false"})
 	}
 
 	repackCounter.WithLabelValues(fmt.Sprint(bitmap)).Inc()