Merge branch 'pks-rev-list-pseudo-revs-as-args' into 'master'

blob: Convert LFS pointer search by revision to use pseudo-revs in args

See merge request gitlab-org/gitaly!3304

9 files changed, 407 insertions, 362 deletions

diff --git a/internal/git/command.go b/internal/git/command.go
index 1dcac3bb9..84304e9ba 100644
--- a/internal/git/command.go
+++ b/internal/git/command.go
@@ -3,12 +3,9 @@ package git
 import (
 	"errors"
 	"fmt"
-	"io"
 	"regexp"
-	"strings"
 
 	"github.com/prometheus/client_golang/prometheus"
-	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
 )
 
 var (
@@ -56,13 +53,13 @@ func (sc SubCmd) Subcommand() string { return sc.Name }
 func (sc SubCmd) CommandArgs() ([]string, error) {
 	var safeArgs []string
 
-	gitCommand, ok := gitCommands[sc.Name]
+	commandDescription, ok := commandDescriptions[sc.Name]
 	if !ok {
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Name, ErrInvalidArg)
 	}
 	safeArgs = append(safeArgs, sc.Name)
 
-	commandArgs, err := assembleCommandArgs(gitCommand, sc.Flags, sc.Args, sc.PostSepArgs)
+	commandArgs, err := commandDescription.args(sc.Flags, sc.Args, sc.PostSepArgs)
 	if err != nil {
 		return nil, err
 	}
@@ -71,51 +68,6 @@ func (sc SubCmd) CommandArgs() ([]string, error) {
 	return safeArgs, nil
 }
 
-func assembleCommandArgs(gitCommand gitCommand, flags []Option, args []string, postSepArgs []string) ([]string, error) {
-	var commandArgs []string
-
-	for _, o := range flags {
-		args, err := o.OptionArgs()
-		if err != nil {
-			return nil, err
-		}
-		commandArgs = append(commandArgs, args...)
-	}
-
-	for _, a := range args {
-		if err := validatePositionalArg(a); err != nil {
-			return nil, err
-		}
-		commandArgs = append(commandArgs, a)
-	}
-
-	if gitCommand.supportsEndOfOptions() {
-		commandArgs = append(commandArgs, "--end-of-options")
-	}
-
-	if len(postSepArgs) > 0 {
-		commandArgs = append(commandArgs, "--")
-	}
-
-	// post separator args do not need any validation
-	commandArgs = append(commandArgs, postSepArgs...)
-
-	return commandArgs, nil
-}
-
-// GlobalOption is an interface for all options which can be globally applied
-// to git commands. This is the command-inspecific part before the actual
-// command that's being run, e.g. the `-c` part in `git -c foo.bar=value
-// command`.
-type GlobalOption interface {
-	GlobalArgs() ([]string, error)
-}
-
-// Option is a git command line flag with validation logic
-type Option interface {
-	OptionArgs() ([]string, error)
-}
-
 // SubSubCmd is a positional argument that appears in the list of options for
 // a subcommand.
 type SubSubCmd struct {
@@ -143,7 +95,7 @@ var actionRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*$`)
 func (sc SubSubCmd) CommandArgs() ([]string, error) {
 	var safeArgs []string
 
-	gitCommand, ok := gitCommands[sc.Name]
+	commandDescription, ok := commandDescriptions[sc.Name]
 	if !ok {
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Name, ErrInvalidArg)
 	}
@@ -154,7 +106,7 @@ func (sc SubSubCmd) CommandArgs() ([]string, error) {
 	}
 	safeArgs = append(safeArgs, sc.Action)
 
-	commandArgs, err := assembleCommandArgs(gitCommand, sc.Flags, sc.Args, sc.PostSepArgs)
+	commandArgs, err := commandDescription.args(sc.Flags, sc.Args, sc.PostSepArgs)
 	if err != nil {
 		return nil, err
 	}
@@ -163,189 +115,7 @@ func (sc SubSubCmd) CommandArgs() ([]string, error) {
 	return safeArgs, nil
 }
 
-// ConfigPair is a sub-command option for use with commands like "git config"
-type ConfigPair struct {
-	Key   string
-	Value string
-	// Origin shows the origin type: file, standard input, blob, command line.
-	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---show-origin
-	Origin string
-	// Scope shows the scope of this config value: local, global, system, command.
-	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---show-scope
-	Scope string
-}
-
-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 !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 {
-	Name string
-}
-
-// GlobalArgs returns the arguments for the given flag, which should typically
-// only be the flag itself. It returns an error if the flag is not sanitary.
-func (f Flag) GlobalArgs() ([]string, error) {
-	return f.OptionArgs()
-}
-
-// OptionArgs returns an error if the flag is not sanitary
-func (f Flag) OptionArgs() ([]string, error) {
-	if !flagRegex.MatchString(f.Name) {
-		return nil, fmt.Errorf("flag %q failed regex validation: %w", f.Name, ErrInvalidArg)
-	}
-	return []string{f.Name}, nil
-}
-
-// ValueFlag is an optional command line argument that is comprised of pair of
-// tokens (e.g. "-n 50")
-type ValueFlag struct {
-	Name  string
-	Value string
-}
-
-// GlobalArgs returns the arguments for the given value flag, which should
-// typically be two arguments: the flag and its value. It returns an error if the value flag is not sanitary.
-func (vf ValueFlag) GlobalArgs() ([]string, error) {
-	return vf.OptionArgs()
-}
-
-// OptionArgs returns an error if the flag is not sanitary
-func (vf ValueFlag) OptionArgs() ([]string, error) {
-	if !flagRegex.MatchString(vf.Name) {
-		return nil, fmt.Errorf("value flag %q failed regex validation: %w", vf.Name, ErrInvalidArg)
-	}
-	return []string{vf.Name, vf.Value}, nil
-}
-
-var flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`)
-
 // IsInvalidArgErr relays if the error is due to an argument validation failure
 func IsInvalidArgErr(err error) bool {
 	return errors.Is(err, ErrInvalidArg)
 }
-
-func validatePositionalArg(arg string) error {
-	if strings.HasPrefix(arg, "-") {
-		return fmt.Errorf("positional arg %q cannot start with dash '-': %w", arg, ErrInvalidArg)
-	}
-	return nil
-}
-
-// ConvertGlobalOptions converts a protobuf message to a CmdOpt.
-func ConvertGlobalOptions(options *gitalypb.GlobalOptions) []CmdOpt {
-	if options != nil && options.GetLiteralPathspecs() {
-		return []CmdOpt{
-			WithEnv("GIT_LITERAL_PATHSPECS=1"),
-		}
-	}
-
-	return nil
-}
-
-// ConvertConfigOptions converts `<key>=<value>` config entries into `ConfigPairs`.
-func ConvertConfigOptions(options []string) ([]ConfigPair, error) {
-	configPairs := make([]ConfigPair, len(options))
-
-	for i, option := range options {
-		configPair := strings.SplitN(option, "=", 2)
-		if len(configPair) != 2 {
-			return nil, fmt.Errorf("cannot convert invalid config key: %q", option)
-		}
-
-		configPairs[i] = ConfigPair{Key: configPair[0], Value: configPair[1]}
-	}
-
-	return configPairs, nil
-}
-
-type cmdCfg struct {
-	env             []string
-	globals         []GlobalOption
-	stdin           io.Reader
-	stdout          io.Writer
-	stderr          io.Writer
-	hooksConfigured bool
-}
-
-// CmdOpt is an option for running a command
-type CmdOpt func(*cmdCfg) error
-
-// WithStdin sets the command's stdin. Pass `command.SetupStdin` to make the
-// command suitable for `Write()`ing to.
-func WithStdin(r io.Reader) CmdOpt {
-	return func(c *cmdCfg) error {
-		c.stdin = r
-		return nil
-	}
-}
-
-// WithStdout sets the command's stdout.
-func WithStdout(w io.Writer) CmdOpt {
-	return func(c *cmdCfg) error {
-		c.stdout = w
-		return nil
-	}
-}
-
-// WithStderr sets the command's stderr.
-func WithStderr(w io.Writer) CmdOpt {
-	return func(c *cmdCfg) error {
-		c.stderr = w
-		return nil
-	}
-}
-
-// WithEnv adds environment variables to the command.
-func WithEnv(envs ...string) CmdOpt {
-	return func(c *cmdCfg) error {
-		c.env = append(c.env, envs...)
-		return nil
-	}
-}
-
-// WithConfig adds git configuration entries to the command.
-func WithConfig(configPairs ...ConfigPair) CmdOpt {
-	return func(c *cmdCfg) error {
-		for _, configPair := range configPairs {
-			c.globals = append(c.globals, configPair)
-		}
-		return nil
-	}
-}
-
-// WithGlobalOption adds the global options to the command. These are universal options which work
-// across all git commands.
-func WithGlobalOption(opts ...GlobalOption) CmdOpt {
-	return func(c *cmdCfg) error {
-		c.globals = append(c.globals, opts...)
-		return nil
-	}
-}
diff --git a/internal/git/subcommand.go b/internal/git/command_description.go
index d68e1a988..72d4b837c 100644
--- a/internal/git/subcommand.go
+++ b/internal/git/command_description.go
@@ -1,5 +1,10 @@
 package git
 
+import (
+	"fmt"
+	"strings"
+)
+
 const (
 	// scNoRefUpdates denotes a command which will never update refs
 	scNoRefUpdates = 1 << iota
@@ -9,111 +14,112 @@ const (
 	scGeneratesPackfiles
 )
 
-type gitCommand struct {
-	flags uint
-	opts  []GlobalOption
+type commandDescription struct {
+	flags                  uint
+	opts                   []GlobalOption
+	validatePositionalArgs func([]string) error
 }
 
-// gitCommands is a curated list of Git command names for special git.ExecCommandFactory
-// validation logic
-var gitCommands = map[string]gitCommand{
-	"apply": gitCommand{
+// commandDescriptions is a curated list of Git command descriptions for special
+// git.ExecCommandFactory validation logic
+var commandDescriptions = map[string]commandDescription{
+	"apply": {
 		flags: scNoRefUpdates,
 	},
-	"archive": gitCommand{
+	"archive": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"blame": gitCommand{
+	"blame": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"bundle": gitCommand{
+	"bundle": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
 	},
-	"cat-file": gitCommand{
+	"cat-file": {
 		flags: scNoRefUpdates,
 	},
-	"check-ref-format": gitCommand{
+	"check-ref-format": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"checkout": gitCommand{
+	"checkout": {
 		flags: scNoEndOfOptions,
 	},
-	"clone": gitCommand{
+	"clone": {
 		flags: scNoEndOfOptions | scGeneratesPackfiles,
 	},
-	"commit": gitCommand{
+	"commit": {
 		flags: 0,
 	},
-	"commit-graph": gitCommand{
+	"commit-graph": {
 		flags: scNoRefUpdates,
 	},
-	"config": gitCommand{
+	"config": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"count-objects": gitCommand{
+	"count-objects": {
 		flags: scNoRefUpdates,
 	},
-	"diff": gitCommand{
+	"diff": {
 		flags: scNoRefUpdates,
 	},
-	"diff-tree": gitCommand{
+	"diff-tree": {
 		flags: scNoRefUpdates,
 	},
-	"fetch": gitCommand{
+	"fetch": {
 		flags: 0,
 	},
-	"for-each-ref": gitCommand{
+	"for-each-ref": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"format-patch": gitCommand{
+	"format-patch": {
 		flags: scNoRefUpdates,
 	},
-	"fsck": gitCommand{
+	"fsck": {
 		flags: scNoRefUpdates,
 	},
-	"gc": gitCommand{
+	"gc": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
 	},
-	"grep": gitCommand{
+	"grep": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"hash-object": gitCommand{
+	"hash-object": {
 		flags: scNoRefUpdates,
 	},
-	"init": gitCommand{
+	"init": {
 		flags: scNoRefUpdates,
 	},
-	"linguist": gitCommand{
+	"linguist": {
 		flags: scNoEndOfOptions,
 	},
-	"log": gitCommand{
+	"log": {
 		flags: scNoRefUpdates,
 	},
-	"ls-remote": gitCommand{
+	"ls-remote": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"ls-tree": gitCommand{
+	"ls-tree": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"merge-base": gitCommand{
+	"merge-base": {
 		flags: scNoRefUpdates,
 	},
-	"mktag": gitCommand{
+	"mktag": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"multi-pack-index": gitCommand{
+	"multi-pack-index": {
 		flags: scNoRefUpdates,
 	},
-	"pack-refs": gitCommand{
+	"pack-refs": {
 		flags: scNoRefUpdates,
 	},
-	"pack-objects": gitCommand{
+	"pack-objects": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
 	},
-	"push": gitCommand{
+	"push": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"receive-pack": gitCommand{
+	"receive-pack": {
 		flags: 0,
 		opts: []GlobalOption{
 			// In case the repository belongs to an object pool, we want to prevent
@@ -136,10 +142,10 @@ var gitCommands = map[string]gitCommand{
 			ConfigPair{Key: "receive.advertisePushOptions", Value: "true"},
 		},
 	},
-	"remote": gitCommand{
+	"remote": {
 		flags: scNoEndOfOptions,
 	},
-	"repack": gitCommand{
+	"repack": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
 		opts: []GlobalOption{
 			// Write bitmap indices when packing objects, which
@@ -147,31 +153,49 @@ var gitCommands = map[string]gitCommand{
 			ConfigPair{Key: "repack.writeBitmaps", Value: "true"},
 		},
 	},
-	"rev-list": gitCommand{
+	"rev-list": {
 		flags: scNoRefUpdates,
+		validatePositionalArgs: func(args []string) error {
+			for _, arg := range args {
+				// git-rev-list(1) supports pseudo-revision arguments which can be
+				// intermingled with normal positional arguments. Given that these
+				// pseudo-revisions have leading dashes, normal validation would
+				// refuse them as positional arguments. We thus override validation
+				// for two of these which we are using in our codebase. There are
+				// more, but we can add them at a later point if they're ever
+				// required.
+				if arg == "--all" || arg == "--not" {
+					continue
+				}
+				if err := validatePositionalArg(arg); err != nil {
+					return fmt.Errorf("rev-list: %w", err)
+				}
+			}
+			return nil
+		},
 	},
-	"rev-parse": gitCommand{
+	"rev-parse": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"show": gitCommand{
+	"show": {
 		flags: scNoRefUpdates,
 	},
-	"show-ref": gitCommand{
+	"show-ref": {
 		flags: scNoRefUpdates,
 	},
-	"symbolic-ref": gitCommand{
+	"symbolic-ref": {
 		flags: 0,
 	},
-	"tag": gitCommand{
+	"tag": {
 		flags: 0,
 	},
-	"update-ref": gitCommand{
+	"update-ref": {
 		flags: 0,
 	},
-	"upload-archive": gitCommand{
+	"upload-archive": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"upload-pack": gitCommand{
+	"upload-pack": {
 		flags: scNoRefUpdates | scGeneratesPackfiles,
 		opts: []GlobalOption{
 			ConfigPair{Key: "uploadpack.allowFilter", Value: "true"},
@@ -180,31 +204,77 @@ var gitCommands = map[string]gitCommand{
 			ConfigPair{Key: "uploadpack.allowAnySHA1InWant", Value: "true"},
 		},
 	},
-	"version": gitCommand{
+	"version": {
 		flags: scNoRefUpdates | scNoEndOfOptions,
 	},
-	"worktree": gitCommand{
+	"worktree": {
 		flags: 0,
 	},
 }
 
-// mayUpdateRef indicates if a gitCommand is known to update references.
+// mayUpdateRef indicates if a command is known to update references.
 // This is useful to determine if a command requires reference hook
 // configuration. A non-exhaustive list of commands is consulted to determine if
 // refs are updated. When unknown, true is returned to err on the side of
 // caution.
-func (c gitCommand) mayUpdateRef() bool {
+func (c commandDescription) mayUpdateRef() bool {
 	return c.flags&scNoRefUpdates == 0
 }
 
-// mayGeneratePackfiles indicates if a gitCommand is known to generate
+// mayGeneratePackfiles indicates if a command is known to generate
 // packfiles. This is used in order to inject packfile configuration.
-func (c gitCommand) mayGeneratePackfiles() bool {
+func (c commandDescription) mayGeneratePackfiles() bool {
 	return c.flags&scGeneratesPackfiles != 0
 }
 
 // supportsEndOfOptions indicates whether a command can handle the
 // `--end-of-options` option.
-func (c gitCommand) supportsEndOfOptions() bool {
+func (c commandDescription) supportsEndOfOptions() bool {
 	return c.flags&scNoEndOfOptions == 0
 }
+
+// args validates the given flags and arguments and, if valid, returns the complete command line.
+func (c commandDescription) args(flags []Option, args []string, postSepArgs []string) ([]string, error) {
+	var commandArgs []string
+
+	for _, o := range flags {
+		args, err := o.OptionArgs()
+		if err != nil {
+			return nil, err
+		}
+		commandArgs = append(commandArgs, args...)
+	}
+
+	if c.validatePositionalArgs != nil {
+		if err := c.validatePositionalArgs(args); err != nil {
+			return nil, err
+		}
+	} else {
+		for _, a := range args {
+			if err := validatePositionalArg(a); err != nil {
+				return nil, err
+			}
+		}
+	}
+	commandArgs = append(commandArgs, args...)
+
+	if c.supportsEndOfOptions() {
+		commandArgs = append(commandArgs, "--end-of-options")
+	}
+
+	if len(postSepArgs) > 0 {
+		commandArgs = append(commandArgs, "--")
+	}
+
+	// post separator args do not need any validation
+	commandArgs = append(commandArgs, postSepArgs...)
+
+	return commandArgs, nil
+}
+
+func validatePositionalArg(arg string) error {
+	if strings.HasPrefix(arg, "-") {
+		return fmt.Errorf("positional arg %q cannot start with dash '-': %w", arg, ErrInvalidArg)
+	}
+	return nil
+}
diff --git a/internal/git/command_description_test.go b/internal/git/command_description_test.go
new file mode 100644
index 000000000..a0915b9d5
--- /dev/null
+++ b/internal/git/command_description_test.go
@@ -0,0 +1,57 @@
+package git
+
+import (
+	"fmt"
+	"testing"
+
+	"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)
+	require.NotNil(t, revlist.validatePositionalArgs)
+
+	for _, tc := range []struct {
+		desc        string
+		args        []string
+		expectedErr error
+	}{
+		{
+			desc: "normal reference",
+			args: []string{
+				"master",
+			},
+		},
+		{
+			desc: "reference with leading dash",
+			args: []string{
+				"-master",
+			},
+			expectedErr: fmt.Errorf("rev-list: %w",
+				fmt.Errorf("positional arg \"-master\" cannot start with dash '-': %w", ErrInvalidArg),
+			),
+		},
+		{
+			desc: "revisions and pseudo-revisions",
+			args: []string{
+				"master --not --all",
+			},
+		},
+	} {
+		t.Run(tc.desc, func(t *testing.T) {
+			err := revlist.validatePositionalArgs(tc.args)
+			require.Equal(t, tc.expectedErr, err)
+		})
+	}
+}
diff --git a/internal/git/command_factory.go b/internal/git/command_factory.go
index f27308788..5497053ab 100644
--- a/internal/git/command_factory.go
+++ b/internal/git/command_factory.go
@@ -129,7 +129,7 @@ func (cf *ExecCommandFactory) newCommand(ctx context.Context, repo repository.Gi
 }
 
 func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error {
-	gitCommand, ok := gitCommands[sc.Subcommand()]
+	commandDescription, ok := commandDescriptions[sc.Subcommand()]
 	if !ok {
 		return fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg)
 	}
@@ -140,10 +140,10 @@ func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error {
 		}
 	}
 
-	if !cc.hooksConfigured && gitCommand.mayUpdateRef() {
+	if !cc.hooksConfigured && commandDescription.mayUpdateRef() {
 		return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrHookPayloadRequired)
 	}
-	if gitCommand.mayGeneratePackfiles() {
+	if commandDescription.mayGeneratePackfiles() {
 		cc.globals = append(cc.globals, ConfigPair{
 			Key: "pack.windowMemory", Value: "100m",
 		})
@@ -161,7 +161,7 @@ func combineArgs(gitConfig []config.GitConfig, sc Cmd, cc *cmdCfg) (_ []string,
 		}
 	}()
 
-	gitCommand, ok := gitCommands[sc.Subcommand()]
+	commandDescription, ok := commandDescriptions[sc.Subcommand()]
 	if !ok {
 		return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg)
 	}
@@ -185,7 +185,7 @@ func combineArgs(gitConfig []config.GitConfig, sc Cmd, cc *cmdCfg) (_ []string,
 		})
 	}
 	combinedGlobals = append(combinedGlobals, globalOptions...)
-	combinedGlobals = append(combinedGlobals, gitCommand.opts...)
+	combinedGlobals = append(combinedGlobals, commandDescription.opts...)
 	combinedGlobals = append(combinedGlobals, cc.globals...)
 
 	for _, global := range combinedGlobals {
diff --git a/internal/git/command_options.go b/internal/git/command_options.go
new file mode 100644
index 000000000..e3d07d138
--- /dev/null
+++ b/internal/git/command_options.go
@@ -0,0 +1,198 @@
+package git
+
+import (
+	"fmt"
+	"io"
+	"regexp"
+	"strings"
+
+	"gitlab.com/gitlab-org/gitaly/proto/go/gitalypb"
+)
+
+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]+)+$`)
+
+	flagRegex = regexp.MustCompile(`^(-|--)[[:alnum:]]`)
+)
+
+// GlobalOption is an interface for all options which can be globally applied
+// to git commands. This is the command-inspecific part before the actual
+// command that's being run, e.g. the `-c` part in `git -c foo.bar=value
+// command`.
+type GlobalOption interface {
+	GlobalArgs() ([]string, error)
+}
+
+// Option is a git command line flag with validation logic
+type Option interface {
+	OptionArgs() ([]string, error)
+}
+
+// ConfigPair is a sub-command option for use with commands like "git config"
+type ConfigPair struct {
+	Key   string
+	Value string
+	// Origin shows the origin type: file, standard input, blob, command line.
+	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---show-origin
+	Origin string
+	// Scope shows the scope of this config value: local, global, system, command.
+	// https://git-scm.com/docs/git-config#Documentation/git-config.txt---show-scope
+	Scope string
+}
+
+// OptionArgs validates the config pair args
+func (cp ConfigPair) OptionArgs() ([]string, error) {
+	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 {
+	Name string
+}
+
+// GlobalArgs returns the arguments for the given flag, which should typically
+// only be the flag itself. It returns an error if the flag is not sanitary.
+func (f Flag) GlobalArgs() ([]string, error) {
+	return f.OptionArgs()
+}
+
+// OptionArgs returns an error if the flag is not sanitary
+func (f Flag) OptionArgs() ([]string, error) {
+	if !flagRegex.MatchString(f.Name) {
+		return nil, fmt.Errorf("flag %q failed regex validation: %w", f.Name, ErrInvalidArg)
+	}
+	return []string{f.Name}, nil
+}
+
+// ValueFlag is an optional command line argument that is comprised of pair of
+// tokens (e.g. "-n 50")
+type ValueFlag struct {
+	Name  string
+	Value string
+}
+
+// GlobalArgs returns the arguments for the given value flag, which should
+// typically be two arguments: the flag and its value. It returns an error if the value flag is not sanitary.
+func (vf ValueFlag) GlobalArgs() ([]string, error) {
+	return vf.OptionArgs()
+}
+
+// OptionArgs returns an error if the flag is not sanitary
+func (vf ValueFlag) OptionArgs() ([]string, error) {
+	if !flagRegex.MatchString(vf.Name) {
+		return nil, fmt.Errorf("value flag %q failed regex validation: %w", vf.Name, ErrInvalidArg)
+	}
+	return []string{vf.Name, vf.Value}, nil
+}
+
+// ConvertGlobalOptions converts a protobuf message to a CmdOpt.
+func ConvertGlobalOptions(options *gitalypb.GlobalOptions) []CmdOpt {
+	if options != nil && options.GetLiteralPathspecs() {
+		return []CmdOpt{
+			WithEnv("GIT_LITERAL_PATHSPECS=1"),
+		}
+	}
+
+	return nil
+}
+
+// ConvertConfigOptions converts `<key>=<value>` config entries into `ConfigPairs`.
+func ConvertConfigOptions(options []string) ([]ConfigPair, error) {
+	configPairs := make([]ConfigPair, len(options))
+
+	for i, option := range options {
+		configPair := strings.SplitN(option, "=", 2)
+		if len(configPair) != 2 {
+			return nil, fmt.Errorf("cannot convert invalid config key: %q", option)
+		}
+
+		configPairs[i] = ConfigPair{Key: configPair[0], Value: configPair[1]}
+	}
+
+	return configPairs, nil
+}
+
+type cmdCfg struct {
+	env             []string
+	globals         []GlobalOption
+	stdin           io.Reader
+	stdout          io.Writer
+	stderr          io.Writer
+	hooksConfigured bool
+}
+
+// CmdOpt is an option for running a command
+type CmdOpt func(*cmdCfg) error
+
+// WithStdin sets the command's stdin. Pass `command.SetupStdin` to make the
+// command suitable for `Write()`ing to.
+func WithStdin(r io.Reader) CmdOpt {
+	return func(c *cmdCfg) error {
+		c.stdin = r
+		return nil
+	}
+}
+
+// WithStdout sets the command's stdout.
+func WithStdout(w io.Writer) CmdOpt {
+	return func(c *cmdCfg) error {
+		c.stdout = w
+		return nil
+	}
+}
+
+// WithStderr sets the command's stderr.
+func WithStderr(w io.Writer) CmdOpt {
+	return func(c *cmdCfg) error {
+		c.stderr = w
+		return nil
+	}
+}
+
+// WithEnv adds environment variables to the command.
+func WithEnv(envs ...string) CmdOpt {
+	return func(c *cmdCfg) error {
+		c.env = append(c.env, envs...)
+		return nil
+	}
+}
+
+// WithConfig adds git configuration entries to the command.
+func WithConfig(configPairs ...ConfigPair) CmdOpt {
+	return func(c *cmdCfg) error {
+		for _, configPair := range configPairs {
+			c.globals = append(c.globals, configPair)
+		}
+		return nil
+	}
+}
+
+// WithGlobalOption adds the global options to the command. These are universal options which work
+// across all git commands.
+func WithGlobalOption(opts ...GlobalOption) CmdOpt {
+	return func(c *cmdCfg) error {
+		c.globals = append(c.globals, opts...)
+		return nil
+	}
+}
diff --git a/internal/git/command_test.go b/internal/git/command_options_test.go
index e2be412ed..e2be412ed 100644
--- a/internal/git/command_test.go
+++ b/internal/git/command_options_test.go
diff --git a/internal/git/subcommand_test.go b/internal/git/subcommand_test.go
deleted file mode 100644
index e874e817b..000000000
--- a/internal/git/subcommand_test.go
+++ /dev/null
@@ -1,17 +0,0 @@
-package git
-
-import (
-	"testing"
-
-	"github.com/stretchr/testify/require"
-)
-
-func TestSubcommand_mayGeneratePackfiles(t *testing.T) {
-	gcCmd, ok := gitCommands["gc"]
-	require.True(t, ok)
-	require.True(t, gcCmd.mayGeneratePackfiles())
-
-	applyCmd, ok := gitCommands["apply"]
-	require.True(t, ok)
-	require.False(t, applyCmd.mayGeneratePackfiles())
-}
diff --git a/internal/gitaly/service/blob/lfs_pointers.go b/internal/gitaly/service/blob/lfs_pointers.go
index e8e8419ac..f710f2295 100644
--- a/internal/gitaly/service/blob/lfs_pointers.go
+++ b/internal/gitaly/service/blob/lfs_pointers.go
@@ -132,28 +132,16 @@ func (s *server) GetNewLFSPointers(in *gitalypb.GetNewLFSPointersRequest, stream
 	repo := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg)
 
 	var refs []string
-	var opts []git.Option
-
 	if in.NotInAll {
-		refs = []string{string(in.Revision)}
-		// We need to append another `--not` to cancel out the first one. Otherwise, we'd
-		// negate the user-provided revision.
-		opts = []git.Option{
-			git.Flag{"--not"}, git.Flag{"--all"}, git.Flag{"--not"},
-		}
+		refs = []string{string(in.Revision), "--not", "--all"}
 	} else {
-		refs = make([]string, len(in.NotInRefs)+1)
-		refs[0] = string(in.Revision)
-
-		// We cannot intermix references and flags because of safety guards of our git DSL.
-		// Instead, we thus manually negate all references by prefixing them with the caret
-		// symbol.
-		for i, notInRef := range in.NotInRefs {
-			refs[i+1] = "^" + string(notInRef)
+		refs = []string{string(in.Revision), "--not"}
+		for _, notInRef := range in.NotInRefs {
+			refs = append(refs, string(notInRef))
 		}
 	}
 
-	lfsPointers, err := findLFSPointersByRevisions(ctx, repo, s.gitCmdFactory, opts, int(in.Limit), refs...)
+	lfsPointers, err := findLFSPointersByRevisions(ctx, repo, s.gitCmdFactory, int(in.Limit), refs...)
 	if err != nil {
 		if errors.Is(err, errInvalidRevision) {
 			return status.Errorf(codes.InvalidArgument, err.Error())
@@ -225,9 +213,7 @@ func (s *server) GetAllLFSPointers(in *gitalypb.GetAllLFSPointersRequest, stream
 
 	repo := localrepo.New(s.gitCmdFactory, in.Repository, s.cfg)
 
-	lfsPointers, err := findLFSPointersByRevisions(ctx, repo, s.gitCmdFactory, []git.Option{
-		git.Flag{Name: "--all"},
-	}, 0)
+	lfsPointers, err := findLFSPointersByRevisions(ctx, repo, s.gitCmdFactory, 0, "--all")
 	if err != nil {
 		if errors.Is(err, errInvalidRevision) {
 			return status.Errorf(codes.InvalidArgument, err.Error())
@@ -284,15 +270,11 @@ func validateGetAllLFSPointersRequest(in *gitalypb.GetAllLFSPointersRequest) err
 }
 
 // findLFSPointersByRevisions will return all LFS objects reachable via the given set of revisions.
-// Revisions accept all syntax supported by git-rev-list(1). This function also accepts a set of
-// options accepted by git-rev-list(1). Note that because git.Commands do not accept dashed
-// positional arguments, it is currently not possible to mix options and revisions (e.g. "git
-// rev-list master --not feature").
+// Revisions accept all syntax supported by git-rev-list(1).
 func findLFSPointersByRevisions(
 	ctx context.Context,
 	repo *localrepo.Repo,
 	gitCmdFactory git.CommandFactory,
-	opts []git.Option,
 	limit int,
 	revisions ...string,
 ) (lfsPointers []*gitalypb.LFSPointer, returnErr error) {
@@ -311,7 +293,6 @@ func findLFSPointersByRevisions(
 	if featureflag.IsEnabled(ctx, featureflag.LFSPointersUseBitmapIndex) {
 		flags = append(flags, git.Flag{Name: "--use-bitmap-index"})
 	}
-	flags = append(flags, opts...)
 
 	// git-rev-list(1) currently does not have any way to list all reachable objects of a
 	// certain type.
diff --git a/internal/gitaly/service/blob/lfs_pointers_test.go b/internal/gitaly/service/blob/lfs_pointers_test.go
index 5d0d083bc..e2f616968 100644
--- a/internal/gitaly/service/blob/lfs_pointers_test.go
+++ b/internal/gitaly/service/blob/lfs_pointers_test.go
@@ -510,7 +510,6 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 
 	for _, tc := range []struct {
 		desc                string
-		opts                []git.Option
 		revs                []string
 		limit               int
 		expectedErr         error
@@ -518,9 +517,7 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 	}{
 		{
 			desc: "--all",
-			opts: []git.Option{
-				git.Flag{Name: "--all"},
-			},
+			revs: []string{"--all"},
 			expectedLFSPointers: []*gitalypb.LFSPointer{
 				lfsPointers[lfsPointer1],
 				lfsPointers[lfsPointer2],
@@ -531,10 +528,8 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 			},
 		},
 		{
-			desc: "--all with high limit",
-			opts: []git.Option{
-				git.Flag{Name: "--all"},
-			},
+			desc:  "--all with high limit",
+			revs:  []string{"--all"},
 			limit: 7,
 			expectedLFSPointers: []*gitalypb.LFSPointer{
 				lfsPointers[lfsPointer1],
@@ -546,10 +541,8 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 			},
 		},
 		{
-			desc: "--all with truncating limit",
-			opts: []git.Option{
-				git.Flag{Name: "--all"},
-			},
+			desc:  "--all with truncating limit",
+			revs:  []string{"--all"},
 			limit: 3,
 			expectedLFSPointers: []*gitalypb.LFSPointer{
 				lfsPointers[lfsPointer1],
@@ -559,10 +552,7 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 		},
 		{
 			desc: "--not --all",
-			opts: []git.Option{
-				git.Flag{Name: "--not"},
-				git.Flag{Name: "--all"},
-			},
+			revs: []string{"--not", "--all"},
 		},
 		{
 			desc: "initial commit",
@@ -597,7 +587,7 @@ func TestFindLFSPointersByRevisions(t *testing.T) {
 	} {
 		t.Run(tc.desc, func(t *testing.T) {
 			actualLFSPointers, err := findLFSPointersByRevisions(
-				ctx, repo, gitCmdFactory, tc.opts, tc.limit, tc.revs...)
+				ctx, repo, gitCmdFactory, tc.limit, tc.revs...)
 			if tc.expectedErr == nil {
 				require.NoError(t, err)
 			} else {
@@ -621,16 +611,12 @@ func BenchmarkFindLFSPointers(b *testing.B) {
 	defer cancel()
 
 	b.Run("limitless", func(b *testing.B) {
-		_, err := findLFSPointersByRevisions(ctx, repo, gitCmdFactory, []git.Option{
-			git.Flag{"--all"},
-		}, 0)
+		_, err := findLFSPointersByRevisions(ctx, repo, gitCmdFactory, 0, "--all")
 		require.NoError(b, err)
 	})
 
 	b.Run("limit", func(b *testing.B) {
-		lfsPointer, err := findLFSPointersByRevisions(ctx, repo, gitCmdFactory, []git.Option{
-			git.Flag{"--all"},
-		}, 1)
+		lfsPointer, err := findLFSPointersByRevisions(ctx, repo, gitCmdFactory, 1, "--all")
 		require.NoError(b, err)
 		require.Len(b, lfsPointer, 1)
 	})