Follow improve code quality (#21465)

After some discussion, introduce a new slice `brokenArgs` to make
`gitCmd.Run()` return errors if any dynamic argument is invalid.

Co-authored-by: delvh <dev.lh@web.de>
This commit is contained in:
wxiaoguang 2022-10-15 20:18:31 +08:00 committed by GitHub
parent d98c5db58f
commit 677af6ac57
No known key found for this signature in database
GPG key ID: 4AEE18F83AFDEB23
2 changed files with 23 additions and 11 deletions

View file

@ -40,6 +40,7 @@ type Command struct {
parentContext context.Context parentContext context.Context
desc string desc string
globalArgsLength int globalArgsLength int
brokenArgs []string
} }
func (c *Command) String() string { func (c *Command) String() string {
@ -50,6 +51,7 @@ func (c *Command) String() string {
} }
// NewCommand creates and returns a new Git Command based on given command and arguments. // NewCommand creates and returns a new Git Command based on given command and arguments.
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommand(ctx context.Context, args ...string) *Command { func NewCommand(ctx context.Context, args ...string) *Command {
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it // Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
cargs := make([]string, len(globalCommandArgs)) cargs := make([]string, len(globalCommandArgs))
@ -63,11 +65,13 @@ func NewCommand(ctx context.Context, args ...string) *Command {
} }
// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args // NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandNoGlobals(args ...string) *Command { func NewCommandNoGlobals(args ...string) *Command {
return NewCommandContextNoGlobals(DefaultContext, args...) return NewCommandContextNoGlobals(DefaultContext, args...)
} }
// NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args // NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command { func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command {
return &Command{ return &Command{
name: GitExecutable, name: GitExecutable,
@ -89,20 +93,24 @@ func (c *Command) SetDescription(desc string) *Command {
return c return c
} }
// AddArguments adds new argument(s) to the command. // AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted.
// User-provided arguments should be passed to AddDynamicArguments instead.
func (c *Command) AddArguments(args ...string) *Command { func (c *Command) AddArguments(args ...string) *Command {
c.args = append(c.args, args...) c.args = append(c.args, args...)
return c return c
} }
// AddDynamicArguments adds new dynamic argument(s) to the command. // AddDynamicArguments adds new dynamic argument(s) to the command.
// If the argument is invalid (it shouldn't happen in real life), it panics to caller // The arguments may come from user input and can not be trusted, so no leading '-' is allowed to avoid passing options
func (c *Command) AddDynamicArguments(args ...string) *Command { func (c *Command) AddDynamicArguments(args ...string) *Command {
for _, arg := range args { for _, arg := range args {
if arg != "" && arg[0] == '-' { if arg != "" && arg[0] == '-' {
panic("invalid argument: " + arg) c.brokenArgs = append(c.brokenArgs, arg)
} }
} }
if len(c.brokenArgs) != 0 {
return c
}
c.args = append(c.args, args...) c.args = append(c.args, args...)
return c return c
} }
@ -150,8 +158,14 @@ func CommonCmdServEnvs() []string {
return commonBaseEnvs() return commonBaseEnvs()
} }
var ErrBrokenCommand = errors.New("git command is broken")
// Run runs the command with the RunOpts // Run runs the command with the RunOpts
func (c *Command) Run(opts *RunOpts) error { func (c *Command) Run(opts *RunOpts) error {
if len(c.brokenArgs) != 0 {
log.Error("git command is broken: %s, broken args: %s", c.String(), strings.Join(c.brokenArgs, " "))
return ErrBrokenCommand
}
if opts == nil { if opts == nil {
opts = &RunOpts{} opts = &RunOpts{}
} }

View file

@ -27,15 +27,13 @@ func TestRunWithContextStd(t *testing.T) {
assert.Empty(t, stdout) assert.Empty(t, stdout)
} }
assert.Panics(t, func() {
cmd = NewCommand(context.Background()) cmd = NewCommand(context.Background())
cmd.AddDynamicArguments("-test") cmd.AddDynamicArguments("-test")
}) assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand)
assert.Panics(t, func() {
cmd = NewCommand(context.Background()) cmd = NewCommand(context.Background())
cmd.AddDynamicArguments("--test") cmd.AddDynamicArguments("--test")
}) assert.ErrorIs(t, cmd.Run(&RunOpts{}), ErrBrokenCommand)
subCmd := "version" subCmd := "version"
cmd = NewCommand(context.Background()).AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production cmd = NewCommand(context.Background()).AddDynamicArguments(subCmd) // for test purpose only, the sub-command should never be dynamic for production