Skip to content

Commit

Permalink
Merging 1135 fix flag parsing (#1356)
Browse files Browse the repository at this point in the history 
* Fix regression in handling flag-values starting with hyphen

Fix for a regression between v1.22.1 and v1.22.2, where
flag values starting with a hyphen would be parsed as a flag:

    runc update test_update --memory-swap -1
    Incorrect Usage: flag provided but not defined: -1

This problem was caused by `reorderArgs()` not properly checking
if the next arg in the list was a value for the flag (and not
the next flag);

Before this patch:

    args before reordering: --opt,--opt-value,arg1
    args after reordering:  --opt,arg1,--opt-value
    args before reordering: --opt,--opt-value,arg1,arg2
    args after reordering:  --opt,arg1,--opt-value,arg2
    args before reordering: arg1,--opt,--opt-value,arg2
    args after reordering:  --opt,arg2,arg1,--opt-value

After this patch is applied:

    args before reordering: --opt,--opt-value,arg1
    args after reordering:  --opt,--opt-value,arg1
    args before reordering: --opt,--opt-value,arg1,arg2
    args after reordering:  --opt,--opt-value,arg1,arg2
    args before reordering: arg1,--opt,--opt-value,arg2
    args after reordering:  --opt,--opt-value,arg1,arg2

Co-authored-by: lynn (they) <[email protected]>
Signed-off-by: Sebastiaan van Stijn <[email protected]>

* Fix handling of -- delimiter

If a `--` delimiter is encountered, any remaining _non-option-value_
arguments must be handled as argument.

From the POSIX spec (emphasis mine)
https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_0):

> The first -- argument that *is not an option-argument* should be accepted
> as a delimiter indicating the end of options. Any following arguments
> should be treated as operands, even if they begin with the '-' character.

There was a corner-case scenario, where a `--flag` followed by a `--` did
not use `--` as value for the flag, but instead considered it as delimiter.

As a result;

    foo test arg1 --opt -- arg2

Would be reordered as:

   foo test --opt arg1 arg2

Which caused `arg1` to be used as value for `--opt`, instead of `--`,
which is the actual value.

Signed-off-by: Sebastiaan van Stijn <[email protected]>

Co-authored-by: Sebastiaan van Stijn <[email protected]>
Co-authored-by: lynn (they) <[email protected]>
  • Loading branch information
@meatballhat @thaJeztah @coilysiren
3 people authored Apr 21, 2022
1 parent 8d987df commit b963ddc
Show file tree
Hide file tree
Showing 3 changed files with 97 additions and 16 deletions.
6 changes: 2 additions & 4 deletions app_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -546,8 +546,7 @@ func TestApp_CommandWithFlagBeforeTerminator(t *testing.T) {

expect(t, parsedOption, "my-option")
expect(t, args[0], "my-arg")
expect(t, args[1], "--")
expect(t, args[2], "--notARealFlag")
expect(t, args[1], "--notARealFlag")
}

func TestApp_CommandWithDash(t *testing.T) {
Expand Down Expand Up @@ -585,8 +584,7 @@ func TestApp_CommandWithNoFlagBeforeTerminator(t *testing.T) {
_ = app.Run([]string{"", "cmd", "my-arg", "--", "notAFlagAtAll"})

expect(t, args[0], "my-arg")
expect(t, args[1], "--")
expect(t, args[2], "notAFlagAtAll")
expect(t, args[1], "notAFlagAtAll")
}

func TestApp_VisibleCommands(t *testing.T) {
Expand Down
30 changes: 18 additions & 12 deletions command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -226,18 +226,23 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
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 == "--" {
remainingArgs = append(remainingArgs, args[i:]...)
break

// checks if this arg is a value that should be re-ordered next to its associated flag
} else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") {
// if we're expecting an option-value, check if this arg is a value, in
// which case it should be re-ordered next to its associated flag
if nextIndexMayContainValue && !argIsFlag(commandFlags, arg) {
nextIndexMayContainValue = false
reorderedArgs = append(reorderedArgs, arg)

} else if arg == "--" {
// don't reorder any args after the -- delimiter As described in the POSIX spec:
// https://pubs.opengroup.org/onlinepubs/9699919799/basedefs/V1_chap12.html#tag_12_02
// > Guideline 10:
// > The first -- argument that is not an option-argument should be accepted
// > as a delimiter indicating the end of options. Any following arguments
// > should be treated as operands, even if they begin with the '-' character.

// make sure the "--" delimiter itself is at the start
remainingArgs = append([]string{"--"}, remainingArgs...)
remainingArgs = append(remainingArgs, args[i+1:]...)
break
// 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
Expand All @@ -256,8 +261,9 @@ func reorderArgs(commandFlags []Flag, args []string) []string {

// 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 == "-" {
if arg == "-" || arg == "--"{
// `-` is never a flag
// `--` is an option-value when following a flag, and a delimiter indicating the end of options in other cases.
return false
}
// flags always start with a -
Expand Down
77 changes: 77 additions & 0 deletions command_test.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -114,6 +114,83 @@ func TestParseAndRunShortOpts(t *testing.T) {
}
}

// test-case for https://github.com/urfave/cli/issues/1092
func TestParseAndRunHyphenValues(t *testing.T) {
cases := []struct {
testArgs []string
expectedArgs []string
expectedOpt string
}{
{[]string{"foo", "test", "argz"}, []string{"argz"}, ""},
{[]string{"foo", "test", "argz", "arga"}, []string{"argz", "arga"}, ""},
{[]string{"foo", "test", "--opt", "opt-value"}, []string{}, "opt-value"},
{[]string{"foo", "test", "--opt", "opt-value", "argz"}, []string{"argz"}, "opt-value"},
{[]string{"foo", "test", "--opt", "opt-value", "argz", "arga"}, []string{"argz", "arga"}, "opt-value"},
{[]string{"foo", "test", "argz", "--opt", "opt-value"}, []string{"argz"}, "opt-value"},
{[]string{"foo", "test", "argz", "--opt", "opt-value", "arga"}, []string{"argz", "arga"}, "opt-value"},

{[]string{"foo", "test", "--opt", "-opt-value"}, []string{}, "-opt-value"},
{[]string{"foo", "test", "--opt", "-opt-value", "argz"}, []string{"argz"}, "-opt-value"},
{[]string{"foo", "test", "--opt", "-opt-value", "argz", "arga"}, []string{"argz", "arga"}, "-opt-value"},
{[]string{"foo", "test", "argz", "--opt", "-opt-value"}, []string{"argz"}, "-opt-value"},
{[]string{"foo", "test", "argz", "--opt", "-opt-value", "arga"}, []string{"argz", "arga"}, "-opt-value"},

{[]string{"foo", "test", "--opt", "--opt-value"}, []string{}, "--opt-value"},
{[]string{"foo", "test", "--opt", "--opt-value", "argz"}, []string{"argz"}, "--opt-value"},
{[]string{"foo", "test", "--opt", "--opt-value", "argz", "arga"}, []string{"argz", "arga"}, "--opt-value"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value"}, []string{"argz"}, "--opt-value"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value", "arga"}, []string{"argz", "arga"}, "--opt-value"},

// Tests below test handling of the "--" delimiter
{[]string{"foo", "test", "--", "--opt", "--opt-value", "argz"}, []string{"--opt", "--opt-value", "argz"}, ""},
{[]string{"foo", "test", "--", "argz", "--opt", "--opt-value", "arga"}, []string{"argz", "--opt", "--opt-value", "arga"}, ""},
{[]string{"foo", "test", "argz", "--", "--opt", "--opt-value", "arga"}, []string{"argz", "--opt", "--opt-value", "arga"}, ""},
{[]string{"foo", "test", "argz", "--opt", "--", "--opt-value", "arga"}, []string{"argz", "--opt-value", "arga"}, "--"},

// first "--" is an option-value for "--opt", second "--" is the delimiter indicating the end of options.
{[]string{"foo", "test", "argz", "--opt", "--", "--", "--opt2", "--opt-value", "arga"}, []string{"argz", "--opt2", "--opt-value", "arga"}, "--"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value", "--", "arga"}, []string{"argz", "arga"}, "--opt-value"},
{[]string{"foo", "test", "argz", "--opt", "--opt-value", "arga", "--"}, []string{"argz", "arga"}, "--opt-value"},
}

for _, tc := range cases {
tc := tc
t.Run(strings.Join(tc.testArgs, "_"), func(t *testing.T){
var (
args []string
opt string
)

cmd := Command{
Name: "test",
Usage: "this is for testing",
Description: "testing",
Action: func(c *Context) error {
args = c.Args()
opt = c.String("opt")
return nil
},
Flags: []Flag{
StringFlag{Name: "opt"},
StringFlag{Name: "opt2"},
},
}

app := NewApp()
app.Writer = ioutil.Discard
app.ErrWriter = ioutil.Discard
app.Commands = []Command{cmd}

err := app.Run(tc.testArgs)

expect(t, err, nil)
expect(t, args, tc.expectedArgs)
expect(t, opt, tc.expectedOpt)
})

}
}

func TestCommand_Run_DoesNotOverwriteErrorFromBefore(t *testing.T) {
app := NewApp()
app.Commands = []Command{
Expand Down

0 comments on commit b963ddc

Please sign in to comment.