diff --git a/app_regression_test.go b/app_regression_test.go new file mode 100644 index 0000000000..3c8681b993 --- /dev/null +++ b/app_regression_test.go @@ -0,0 +1,59 @@ +package cli + +import ( + "testing" +) + +// TestRegression tests a regression that was merged between versions 1.20.0 and 1.21.0 +// The included app.Run line worked in 1.20.0, and then was broken in 1.21.0. +// Relevant PR: https://github.com/urfave/cli/pull/872 +func TestVersionOneTwoOneRegression(t *testing.T) { + testData := []struct { + testCase string + appRunInput []string + skipArgReorder bool + }{ + { + testCase: "with_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, + }, + { + testCase: "with_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "--", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, + { + testCase: "without_dash_dash", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + }, + { + testCase: "without_dash_dash_and_skip_reorder", + appRunInput: []string{"cli", "command", "--flagone", "flagvalue", "docker", "image", "ls", "--no-trunc"}, + skipArgReorder: true, + }, + } + for _, test := range testData { + t.Run(test.testCase, func(t *testing.T) { + // setup + app := NewApp() + app.Commands = []Command{{ + Name: "command", + SkipArgReorder: test.skipArgReorder, + Flags: []Flag{ + StringFlag{ + Name: "flagone", + }, + }, + Action: func(c *Context) error { return nil }, + }} + + // logic under test + err := app.Run(test.appRunInput) + + // assertions + if err != nil { + t.Errorf("did not expected an error, but there was one: %s", err) + } + }) + } +} diff --git a/command.go b/command.go index b10e4d4045..e7cb97a6af 100644 --- a/command.go +++ b/command.go @@ -190,7 +190,7 @@ func (c *Command) parseFlags(args Args) (*flag.FlagSet, error) { } if !c.SkipArgReorder { - args = reorderArgs(args) + args = reorderArgs(c.Flags, args) } set, err := c.newFlagSet() @@ -219,34 +219,73 @@ func (c *Command) useShortOptionHandling() bool { return c.UseShortOptionHandling } -// reorderArgs moves all flags before arguments as this is what flag expects -func reorderArgs(args []string) []string { - var nonflags, flags []string +// reorderArgs moves all flags (via reorderedArgs) before the rest of +// the arguments (remainingArgs) as this is what flag expects. +func reorderArgs(commandFlags []Flag, args []string) []string { + var remainingArgs, reorderedArgs []string - readFlagValue := false + nextIndexMayContainValue := false for i, arg := range args { + + // dont reorder any args after a -- + // read about -- here: + // https://unix.stackexchange.com/questions/11376/what-does-double-dash-mean-also-known-as-bare-double-dash if arg == "--" { - nonflags = append(nonflags, args[i:]...) + remainingArgs = append(remainingArgs, args[i:]...) break - } - if readFlagValue && !strings.HasPrefix(arg, "-") && !strings.HasPrefix(arg, "--") { - readFlagValue = false - flags = append(flags, arg) - continue - } - readFlagValue = false + // checks if this arg is a value that should be re-ordered next to its associated flag + } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") { + nextIndexMayContainValue = false + reorderedArgs = append(reorderedArgs, arg) - if arg != "-" && strings.HasPrefix(arg, "-") { - flags = append(flags, arg) + // checks if this is an arg that should be re-ordered + } else if argIsFlag(commandFlags, arg) { + // we have determined that this is a flag that we should re-order + reorderedArgs = append(reorderedArgs, arg) + // if this arg does not contain a "=", then the next index may contain the value for this flag + nextIndexMayContainValue = !strings.Contains(arg, "=") - readFlagValue = !strings.Contains(arg, "=") + // simply append any remaining args } else { - nonflags = append(nonflags, arg) + remainingArgs = append(remainingArgs, arg) } } - return append(flags, nonflags...) + return append(reorderedArgs, remainingArgs...) +} + +// argIsFlag checks if an arg is one of our command flags +func argIsFlag(commandFlags []Flag, arg string) bool { + // checks if this is just a `-`, and so definitely not a flag + if arg == "-" { + return false + } + // flags always start with a - + if !strings.HasPrefix(arg, "-") { + return false + } + // this line turns `--flag` into `flag` + if strings.HasPrefix(arg, "--") { + arg = strings.Replace(arg, "-", "", 2) + } + // this line turns `-flag` into `flag` + if strings.HasPrefix(arg, "-") { + arg = strings.Replace(arg, "-", "", 1) + } + // this line turns `flag=value` into `flag` + arg = strings.Split(arg, "=")[0] + // look through all the flags, to see if the `arg` is one of our flags + for _, flag := range commandFlags { + for _, key := range strings.Split(flag.GetName(), ",") { + key := strings.TrimSpace(key) + if key == arg { + return true + } + } + } + // return false if this arg was not one of our flags + return false } // Names returns the names including short names and aliases. diff --git a/command_test.go b/command_test.go index 66859c07d8..bcfbee747e 100644 --- a/command_test.go +++ b/command_test.go @@ -18,7 +18,7 @@ func TestCommandFlagParsing(t *testing.T) { UseShortOptionHandling bool }{ // Test normal "not ignoring flags" flow - {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, errors.New("flag provided but not defined: -break"), false}, + {[]string{"test-cmd", "blah", "blah", "-break"}, false, false, nil, false}, // Test no arg reorder {[]string{"test-cmd", "blah", "blah", "-break"}, false, true, nil, false},