New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix regression in handling flag-values starting with hyphen #1135
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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, "-") { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is still not quite correct if you wanted to do something like:
Where it would still re-order things. The least-ugly way I can think of solving this is that we change I also hasten to point out that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. A possible patch to do the above might look like the following. This change does pass all of the existing tests... diff --git a/command.go b/command.go
index f02d3589ff3a..3c8e971a1c53 100644
--- a/command.go
+++ b/command.go
@@ -223,7 +223,7 @@ func (c *Command) useShortOptionHandling() bool {
func reorderArgs(commandFlags []Flag, args []string) []string {
var remainingArgs, reorderedArgs []string
- nextIndexMayContainValue := false
+ nextIndexIsValue := false
for i, arg := range args {
// dont reorder any args after a --
@@ -233,17 +233,29 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
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, "-") {
- nextIndexMayContainValue = false
+ // if this argument is a value then it isn't a flag and should be
+ // kept with its associated flag (which was the last flag
+ // reordered)
+ } else if nextIndexIsValue {
+ nextIndexIsValue = false
reorderedArgs = append(reorderedArgs, arg)
// checks if this is an arg that should be re-ordered
- } else if argIsFlag(commandFlags, arg) {
+ } else if flag := findFlagForArg(commandFlags, arg); flag != nil {
// 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, "=")
+
+ // if the flag takes an argument and doesn't contain a "=" then the
+ // next index contains a value for this flag -- unfortunately we
+ // have to depend on the Flag implementing DocGenerationFlag for
+ // TakesValue().
+ if docFlag, ok := (*flag).(DocGenerationFlag); ok {
+ nextIndexIsValue = docFlag.TakesValue() && !strings.Contains(arg, "=")
+ } else {
+ // assume that flags take arguments
+ // XXX: This is quite ugly and I'm not sure what we should do here.
+ nextIndexIsValue = !strings.Contains(arg, "=")
+ }
// simply append any remaining args
} else {
@@ -254,15 +266,16 @@ func reorderArgs(commandFlags []Flag, args []string) []string {
return append(reorderedArgs, remainingArgs...)
}
-// argIsFlag checks if an arg is one of our command flags
-func argIsFlag(commandFlags []Flag, arg string) bool {
+// findFlagForArg checks if an arg is one of our command flags and returns the
+// Flag if the arg is a flag (nil otherwise).
+func findFlagForArg(commandFlags []Flag, arg string) *Flag {
// checks if this is just a `-`, and so definitely not a flag
if arg == "-" {
- return false
+ return nil
}
// flags always start with a -
if !strings.HasPrefix(arg, "-") {
- return false
+ return nil
}
// this line turns `--flag` into `flag`
if strings.HasPrefix(arg, "--") {
@@ -279,12 +292,12 @@ func argIsFlag(commandFlags []Flag, arg string) bool {
for _, key := range strings.Split(flag.GetName(), ",") {
key := strings.TrimSpace(key)
if key == arg {
- return true
+ return &flag
}
}
}
- // return false if this arg was not one of our flags
- return false
+ // couldn't find the flag
+ return nil
}
// Names returns the names including short names and aliases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @cyphar I see you commented on the first commit; did you check with the second commit as well? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. (I was looking at the requested changes to preserve some of the existing behaviour) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No, this was just talking about the behaviour of the first change (and not the fun I think the There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So, in your example case ( There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The former (
(Well, for umoci it's more like Should mean that The patch I posted above should make it so that
I disagree (especially if we take the interpretation that
But also it's the obvious behaviour if you just inject
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @thaJeztah PTAL? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ping @thaJeztah 🙏 |
||
// 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 - | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could use extra eyes on this change; it's a result of the latest commit / fix, but could potentially be a breaking change for users (so at least should be called out in the release notes); I think I interpreted the POSIX spec correctly with my patch, but specs can sometimes be ambiguous, so could use the extra eyes.
I also haven't checked the
v2
branch/version yet (possibly the fix is needed there as well, or in another form)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So far, the twitter-verse seems to consolidate to my interpretation being correct 😂 https://twitter.com/thaJeztah/status/1260921988102656002?s=20
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm having a hard time imagining if this is [ a mundane change, or if it'll cause mass chaos for somebody somewhere ]. Is there a way to keep this behavior as-is???