From 464c242da875255f3bf8071d2d99c75f6bd6c0e3 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Fri, 8 May 2020 12:20:27 +0200 Subject: [PATCH 1/2] 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) Signed-off-by: Sebastiaan van Stijn --- command.go | 2 +- command_test.go | 65 +++++++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/command.go b/command.go index f02d3589ff..db528ae680 100644 --- a/command.go +++ b/command.go @@ -234,7 +234,7 @@ func reorderArgs(commandFlags []Flag, args []string) []string { break // checks if this arg is a value that should be re-ordered next to its associated flag - } else if nextIndexMayContainValue && !strings.HasPrefix(arg, "-") { + } else if nextIndexMayContainValue && !argIsFlag(commandFlags, arg) { nextIndexMayContainValue = false reorderedArgs = append(reorderedArgs, arg) diff --git a/command_test.go b/command_test.go index a59d368c46..34a78cbf3b 100644 --- a/command_test.go +++ b/command_test.go @@ -114,6 +114,71 @@ 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"}, + } + + 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"}, + }, + } + + 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{ From 3358e6ff2496b9b8e945e429124606000f8b6716 Mon Sep 17 00:00:00 2001 From: Sebastiaan van Stijn Date: Thu, 14 May 2020 14:28:00 +0200 Subject: [PATCH 2/2] 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 --- app_test.go | 6 ++---- command.go | 30 ++++++++++++++++++------------ command_test.go | 12 ++++++++++++ 3 files changed, 32 insertions(+), 16 deletions(-) diff --git a/app_test.go b/app_test.go index 77a2652011..b9aa34a2dd 100644 --- a/app_test.go +++ b/app_test.go @@ -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) { @@ -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) { diff --git a/command.go b/command.go index db528ae680..09fda1642f 100644 --- a/command.go +++ b/command.go @@ -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 && !argIsFlag(commandFlags, 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 @@ -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 - diff --git a/command_test.go b/command_test.go index 34a78cbf3b..90bcecc21f 100644 --- a/command_test.go +++ b/command_test.go @@ -140,6 +140,17 @@ func TestParseAndRunHyphenValues(t *testing.T) { {[]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 { @@ -161,6 +172,7 @@ func TestParseAndRunHyphenValues(t *testing.T) { }, Flags: []Flag{ StringFlag{Name: "opt"}, + StringFlag{Name: "opt2"}, }, }