From 0c223f54ed46da406bc8b43a5806e0b227863b31 Mon Sep 17 00:00:00 2001
From: Kevin K <kbknapp@gmail.com>
Date: Tue, 24 Oct 2017 13:57:20 -0700
Subject: [PATCH] fix: when an argument requires a value and that value happens
 to match a subcommand name, its parsed as a value

**This commit contains a breaking change in order to fix a bug.**

The only users affected are those relying on the "bug." The bug is only in code that uses
subcommands, and parent commands with arguments accepting multiple values (positionals or options)
unrestrained *and* where a value may coincide with a subcommand's name.

Imagine a CLI with a positional argument `files` that accepts multiple values but no other conditions
or parameters (just `Arg::multiple(true)`), and a subcommand `log`

Prior to this commit, the following was valid:

```
$ prog file1 file2 file3 log
```

which would be parsed as:

 * files = file1, file2, file3
 * subcommand = log

However, if there was a file named `log` the subcommand isn't callable.

The CLI should be designed in a way that either limits number of values so that clap knows
when `files` is done and can then look for subcommands, or other AppSettings which change
the behavior.

Possible fixes are `Arg::number_of_values`, `Arg::max_values`,
`AppSettings::ArgsNegateSubcommands`, `Arg::require_equals`,
`AppSettings::SubcommandsNegateArgs`, and more.

This *does not* affect CLIs which contain other arguments between the unrestrained multiple
value args, and the any subcommands. Such as if our example above also had a flag `-f`

The following would be parsed the same before and after this commit.

```
$ prog file1 file2 file3 -f log
```

This is because upon reaching the flag `-f`, clap stops parsing `files`.

No other breaking changes were made.

---

When using args with `multiple(true)` on options or positionals (i.e. those args that
accept values) and subcommands, one needs to consider the posibility of an argument value
being the same as a valid subcommand. By default `clap` will parse the argument in question
as a value *only if* a value is possible at that moment. Otherwise it will be parsed as a
subcommand. In effect, this means using `multiple(true)` with no additional parameters and
a possible value that coincides with a subcommand name, the subcommand cannot be called
unless another argument is passed first.

As an example, consider a CLI with an option `--ui-paths=<paths>...` and subcommand `signer`

The following would be parsed as values to `--ui-paths`.

```
$ program --ui-paths path1 path2 signer
```

This is because `--ui-paths` accepts multiple values. `clap` will continue parsing values
until another argument is reached and it knows `--ui-paths` is done.

By adding additional parameters to `--ui-paths` we can solve this issue. Consider adding
`Arg::number_of_values(1)` as discussed above. The following are all valid, and `signer`
is parsed as both a subcommand and a value in the second case.

```
$ program --ui-paths path1 signer
$ program --ui-paths path1 --ui-paths signer signer
```

Closes #1031
---
 src/app/parser.rs | 25 +++++++++++++++----------
 1 file changed, 15 insertions(+), 10 deletions(-)

diff --git a/src/app/parser.rs b/src/app/parser.rs
index 9c72e4e8249..5af4f105a55 100644
--- a/src/app/parser.rs
+++ b/src/app/parser.rs
@@ -848,17 +848,22 @@ impl<'a, 'b> Parser<'a, 'b>
             if !self.is_set(AS::TrailingValues) {
                 // Does the arg match a subcommand name, or any of it's aliases (if defined)
                 {
-                    let (is_match, sc_name) = self.possible_subcommand(&arg_os);
-                    debugln!("Parser::get_matches_with: possible_sc={:?}, sc={:?}",
-                             is_match,
-                             sc_name);
-                    if is_match {
-                        let sc_name = sc_name.expect(INTERNAL_ERROR_MSG);
-                        if sc_name == "help" && self.is_set(AS::NeedsSubcommandHelp) {
-                            self.parse_help_subcommand(it)?;
+                    match needs_val_of {
+                        ParseResult::Opt(_) | ParseResult::Pos(_) =>(),
+                        _ => {
+                            let (is_match, sc_name) = self.possible_subcommand(&arg_os);
+                            debugln!("Parser::get_matches_with: possible_sc={:?}, sc={:?}",
+                                    is_match,
+                                    sc_name);
+                            if is_match {
+                                let sc_name = sc_name.expect(INTERNAL_ERROR_MSG);
+                                if sc_name == "help" && self.is_set(AS::NeedsSubcommandHelp) {
+                                    self.parse_help_subcommand(it)?;
+                                }
+                                subcmd_name = Some(sc_name.to_owned());
+                                break;
+                            }
                         }
-                        subcmd_name = Some(sc_name.to_owned());
-                        break;
                     }
                 }