1255470 - Don't assume we're trying to run a subcommand just because there are args

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Andrew Halberstadt [:ahal]]

It's not currently possible to have a command with subcommands as well as args. This we raise if args[0] is not in subcommand_handlers:
https://dxr.mozilla.org/mozilla-central/source/python/mach/mach/dispatcher.py#164

Instead we should continue running the parent command with all args.

Comment 1

[Andrew Halberstadt [:ahal]]

Attachment: MozReview Request: Bug 1255470 - [mach] don't raise if a subcommand exists and but wasn't found in args, r?gps

This is currently preventing a command from having both args and subcommands at the same
time.

Review commit: https://reviewboard.mozilla.org/r/39227/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/39227/

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8729058 [details]
MozReview Request: Bug 1255470 - [mach] don't raise if a subcommand exists and but wasn't found in args, r?gps

https://reviewboard.mozilla.org/r/39227/#review36061

::: python/mach/mach/dispatcher.py
(Diff revision 1)
> -                if subcommand[0] == '-':

I wonder why I added this when I wrote this code. Sadly, there is no inline comment, nothing in the commit message, and nothing in the code review.

I have a bad feeling this is here for a reason. But without any evidence of its importance, I have no reason to cling to it.

::: python/mach/mach/dispatcher.py
(Diff revision 1)
> -                if subcommand not in handler.subcommand_handlers:
> -                    raise UnknownCommandError(subcommand, 'run',
> -                        handler.subcommand_handlers.keys())
> -
>                  handler = handler.subcommand_handlers[subcommand]

The last line will raise a KeyError if subcommand isn't present. So why are you removing the check that raises a better exception type?

Comment 3

[Andrew Halberstadt [:ahal]]

https://reviewboard.mozilla.org/r/39227/#review36061

> I wonder why I added this when I wrote this code. Sadly, there is no inline comment, nothing in the commit message, and nothing in the code review.
> 
> I have a bad feeling this is here for a reason. But without any evidence of its importance, I have no reason to cling to it.

I presume it's here to enforce:
./mach command subcommand --argument

and prevent:
./mach command --argument subcommand

But this check assumed that commands with a subcommand cannot have arguments themselves. What this patch is trying to accomplish is to support both:
./mach command arg1 arg2

and:
./mach command subcommand arg1 arg2

So the side effect you're worried about is that:
./mach command --argument subcommand

will no longer be an error, and the subcommand handler will not be used. This patch was the simplest way forward, but if you're worried I could refactor this a bit more heavily and tackle all edge cases. Fwiw, |mach file-info| is the only command currently using SubCommands.

> The last line will raise a KeyError if subcommand isn't present. So why are you removing the check that raises a better exception type?

If subcommand isn't present, then we wouldn't be in this if block in the first place so that `raise UnknownCommandError` would be dead code.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 8729058 [details]
MozReview Request: Bug 1255470 - [mach] don't raise if a subcommand exists and but wasn't found in args, r?gps

https://reviewboard.mozilla.org/r/39227/#review36235

Thanks for the explanation. r+

Comment 5

[Pulsebot]

https://hg.mozilla.org/integration/mozilla-inbound/rev/5c9c719705b1

Comment 6

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/5c9c719705b1