diff options
author | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-01-06 11:53:23 +0300 |
---|---|---|
committer | Patrick Steinhardt <psteinhardt@gitlab.com> | 2021-01-06 12:46:58 +0300 |
commit | 94979b06d074bf2cf7e5d2902230edbe6cc1a02d (patch) | |
tree | b5701dd8230b3e4ac758f3ad37b5317409563b3f | |
parent | 7402234c9f0a003f84b9c650e2258f16c0c35944 (diff) |
git: Convert subcommands to use a struct
The map of subcommands is currently a simple bitfield used for flags.
While this has served its purpose, it's inflexible and doesn't allow us
to further customize specific commands. While one could do so via flags
as well, it is cumbersome and rather indirect.
Let's refactor the map to use a struct instead. While it currently only
has a `flags` member, it'll get extended in a subsequent commit to also
carry command-specific options.
-rw-r--r-- | internal/git/safecmd.go | 25 | ||||
-rw-r--r-- | internal/git/subcommand.go | 200 | ||||
-rw-r--r-- | internal/git/subcommand_test.go | 10 |
3 files changed, 160 insertions, 75 deletions
diff --git a/internal/git/safecmd.go b/internal/git/safecmd.go index 9c696b8a1..7f559623d 100644 --- a/internal/git/safecmd.go +++ b/internal/git/safecmd.go @@ -62,12 +62,13 @@ func (sc SubCmd) Subcommand() string { return sc.Name } func (sc SubCmd) CommandArgs() ([]string, error) { var safeArgs []string - if _, ok := subcommands[sc.Name]; !ok { + gitCommand, ok := gitCommands[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(sc.Name, sc.Flags, sc.Args, sc.PostSepArgs) + commandArgs, err := assembleCommandArgs(gitCommand, sc.Flags, sc.Args, sc.PostSepArgs) if err != nil { return nil, err } @@ -76,7 +77,7 @@ func (sc SubCmd) CommandArgs() ([]string, error) { return safeArgs, nil } -func assembleCommandArgs(command string, flags []Option, args []string, postSepArgs []string) ([]string, error) { +func assembleCommandArgs(gitCommand gitCommand, flags []Option, args []string, postSepArgs []string) ([]string, error) { var commandArgs []string for _, o := range flags { @@ -94,7 +95,7 @@ func assembleCommandArgs(command string, flags []Option, args []string, postSepA commandArgs = append(commandArgs, a) } - if supportsEndOfOptions(command) { + if gitCommand.supportsEndOfOptions() { commandArgs = append(commandArgs, "--end-of-options") } @@ -144,7 +145,8 @@ var actionRegex = regexp.MustCompile(`^[[:alnum:]]+[-[:alnum:]]*$`) func (sc SubSubCmd) CommandArgs() ([]string, error) { var safeArgs []string - if _, ok := subcommands[sc.Name]; !ok { + gitCommand, ok := gitCommands[sc.Name] + if !ok { return nil, fmt.Errorf("invalid sub command name %q: %w", sc.Name, ErrInvalidArg) } safeArgs = append(safeArgs, sc.Name) @@ -154,7 +156,7 @@ func (sc SubSubCmd) CommandArgs() ([]string, error) { } safeArgs = append(safeArgs, sc.Action) - commandArgs, err := assembleCommandArgs(sc.Name, sc.Flags, sc.Args, sc.PostSepArgs) + commandArgs, err := assembleCommandArgs(gitCommand, sc.Flags, sc.Args, sc.PostSepArgs) if err != nil { return nil, err } @@ -316,19 +318,24 @@ var ( ) func handleOpts(ctx context.Context, sc Cmd, cc *cmdCfg, opts []CmdOpt) error { + gitCommand, ok := gitCommands[sc.Subcommand()] + if !ok { + return fmt.Errorf("invalid sub command name %q: %w", sc.Subcommand(), ErrInvalidArg) + } + for _, opt := range opts { if err := opt(cc); err != nil { return err } } - if !cc.hooksConfigured && mayUpdateRef(sc.Subcommand()) { + if !cc.hooksConfigured && gitCommand.mayUpdateRef() { return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookRequired) } - if cc.hooksConfigured && !mayUpdateRef(sc.Subcommand()) { + if cc.hooksConfigured && !gitCommand.mayUpdateRef() { return fmt.Errorf("subcommand %q: %w", sc.Subcommand(), ErrRefHookNotRequired) } - if mayGeneratePackfiles(sc.Subcommand()) { + if gitCommand.mayGeneratePackfiles() { cc.globals = append(cc.globals, ConfigPair{ Key: "pack.windowMemory", Value: "100m", }) diff --git a/internal/git/subcommand.go b/internal/git/subcommand.go index 527a04a0e..09404ba76 100644 --- a/internal/git/subcommand.go +++ b/internal/git/subcommand.go @@ -11,81 +11,155 @@ const ( scGeneratesPackfiles ) -// subcommands is a curated list of Git command names for special git.SafeCmd +type gitCommand struct { + flags uint +} + +// gitCommands is a curated list of Git command names for special git.SafeCmd // validation logic -var subcommands = map[string]uint{ - "apply": scNoRefUpdates, - "archive": scReadOnly | scNoEndOfOptions, - "blame": scReadOnly | scNoEndOfOptions, - "bundle": scReadOnly | scGeneratesPackfiles, - "cat-file": scReadOnly, - "checkout": scNoEndOfOptions, - "clone": scNoEndOfOptions | scGeneratesPackfiles, - "commit": 0, - "commit-graph": scNoRefUpdates, - "config": scNoRefUpdates | scNoEndOfOptions, - "count-objects": scReadOnly, - "diff": scReadOnly, - "diff-tree": scReadOnly, - "fetch": 0, - "for-each-ref": scReadOnly | scNoEndOfOptions, - "format-patch": scReadOnly, - "fsck": scReadOnly, - "gc": scNoRefUpdates | scGeneratesPackfiles, - "grep": scReadOnly | scNoEndOfOptions, - "hash-object": scNoRefUpdates, - "init": scNoRefUpdates, - "linguist": scNoEndOfOptions, - "log": scReadOnly, - "ls-remote": scReadOnly, - "ls-tree": scReadOnly | scNoEndOfOptions, - "merge-base": scReadOnly, - "multi-pack-index": scNoRefUpdates, - "pack-refs": scNoRefUpdates, - "receive-pack": 0, - "remote": scNoEndOfOptions, - "repack": scNoRefUpdates | scGeneratesPackfiles, - "rev-list": scReadOnly, - "rev-parse": scReadOnly | scNoEndOfOptions, - "show": scReadOnly, - "show-ref": scReadOnly, - "symbolic-ref": 0, - "tag": 0, - "update-ref": 0, - "upload-archive": scReadOnly | scNoEndOfOptions, - "upload-pack": scReadOnly | scGeneratesPackfiles, - "worktree": 0, +var gitCommands = map[string]gitCommand{ + "apply": gitCommand{ + flags: scNoRefUpdates, + }, + "archive": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "blame": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "bundle": gitCommand{ + flags: scReadOnly | scGeneratesPackfiles, + }, + "cat-file": gitCommand{ + flags: scReadOnly, + }, + "checkout": gitCommand{ + flags: scNoEndOfOptions, + }, + "clone": gitCommand{ + flags: scNoEndOfOptions | scGeneratesPackfiles, + }, + "commit": gitCommand{ + flags: 0, + }, + "commit-graph": gitCommand{ + flags: scNoRefUpdates, + }, + "config": gitCommand{ + flags: scNoRefUpdates | scNoEndOfOptions, + }, + "count-objects": gitCommand{ + flags: scReadOnly, + }, + "diff": gitCommand{ + flags: scReadOnly, + }, + "diff-tree": gitCommand{ + flags: scReadOnly, + }, + "fetch": gitCommand{ + flags: 0, + }, + "for-each-ref": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "format-patch": gitCommand{ + flags: scReadOnly, + }, + "fsck": gitCommand{ + flags: scReadOnly, + }, + "gc": gitCommand{ + flags: scNoRefUpdates | scGeneratesPackfiles, + }, + "grep": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "hash-object": gitCommand{ + flags: scNoRefUpdates, + }, + "init": gitCommand{ + flags: scNoRefUpdates, + }, + "linguist": gitCommand{ + flags: scNoEndOfOptions, + }, + "log": gitCommand{ + flags: scReadOnly, + }, + "ls-remote": gitCommand{ + flags: scReadOnly, + }, + "ls-tree": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "merge-base": gitCommand{ + flags: scReadOnly, + }, + "multi-pack-index": gitCommand{ + flags: scNoRefUpdates, + }, + "pack-refs": gitCommand{ + flags: scNoRefUpdates, + }, + "receive-pack": gitCommand{ + flags: 0, + }, + "remote": gitCommand{ + flags: scNoEndOfOptions, + }, + "repack": gitCommand{ + flags: scNoRefUpdates | scGeneratesPackfiles, + }, + "rev-list": gitCommand{ + flags: scReadOnly, + }, + "rev-parse": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "show": gitCommand{ + flags: scReadOnly, + }, + "show-ref": gitCommand{ + flags: scReadOnly, + }, + "symbolic-ref": gitCommand{ + flags: 0, + }, + "tag": gitCommand{ + flags: 0, + }, + "update-ref": gitCommand{ + flags: 0, + }, + "upload-archive": gitCommand{ + flags: scReadOnly | scNoEndOfOptions, + }, + "upload-pack": gitCommand{ + flags: scReadOnly | scGeneratesPackfiles, + }, + "worktree": gitCommand{ + flags: 0, + }, } -// mayUpdateRef indicates if a subcommand is known to update references. +// mayUpdateRef indicates if a gitCommand 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 mayUpdateRef(subcmd string) bool { - flags, ok := subcommands[subcmd] - if !ok { - return true - } - return flags&(scReadOnly|scNoRefUpdates) == 0 +func (c gitCommand) mayUpdateRef() bool { + return c.flags&(scReadOnly|scNoRefUpdates) == 0 } -// mayGeneratePackfiles indicates if a subcommand is known to generate +// mayGeneratePackfiles indicates if a gitCommand is known to generate // packfiles. This is used in order to inject packfile configuration. -func mayGeneratePackfiles(subcmd string) bool { - flags, ok := subcommands[subcmd] - if !ok { - return false - } - return flags&scGeneratesPackfiles != 0 +func (c gitCommand) mayGeneratePackfiles() bool { + return c.flags&scGeneratesPackfiles != 0 } // supportsEndOfOptions indicates whether a command can handle the // `--end-of-options` option. -func supportsEndOfOptions(subcmd string) bool { - flags, ok := subcommands[subcmd] - if !ok { - return true - } - return flags&scNoEndOfOptions == 0 +func (c gitCommand) supportsEndOfOptions() bool { + return c.flags&scNoEndOfOptions == 0 } diff --git a/internal/git/subcommand_test.go b/internal/git/subcommand_test.go index 78dc39f79..e874e817b 100644 --- a/internal/git/subcommand_test.go +++ b/internal/git/subcommand_test.go @@ -7,7 +7,11 @@ import ( ) func TestSubcommand_mayGeneratePackfiles(t *testing.T) { - require.True(t, mayGeneratePackfiles("gc")) - require.False(t, mayGeneratePackfiles("apply")) - require.False(t, mayGeneratePackfiles("nonexistent")) + 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()) } |