Print the help when --help or -h is unexpectedly encountered.

[iwanders]

Most binaries used on Linux support the '--help' argument, this is a convention specified by the GNU project, as well as in the clig.

Currently, adding --help after a failing command gives an error, following by an note at the end on how to get the help. Actually getting the help usually involves pressing the up arrow, removing the erroneous --help at the end, moving the cursor to after picotool to insert help and then succesfully printing out the help.

This commit adds a new error type to the parser to distinguish between parse errors, and errors that can be identified as an attempt to obtain the help. This help error is raised only if an unexpected argument called --help is encountered, or an unexpected option -h.

In the main.cpp this specific error is now caught and handled on appropriately, still relying on the outer catch problematic parse errors, like using command names that don't exist.

---

Above the line is the commit message, I considered an example too long to be included in it, but before this commit the behaviour is as follows:

$ picotool partition info --help

ERROR: unexpected option: --help

SYNOPSIS:
    picotool partition info [-m <family_id>] [device-selection]

Use "picotool help partition info" for more info

With this commit, it instead does:

$ ./picotool partition info --help
ERROR: unexpected option: --help, this looks like you want the help:
INFO:
    Print the device's partition table.

SYNOPSIS:
    picotool partition info [-m <family_id>] [device-selection]

OPTIONS:
        -m <family_id>
            family ID (will show target partition for said family)
        ... more options.

Behaviour when subcommands don't exist is unmodified:

$ ./picotool this_doesnt_exist --help

ERROR: Unknown command: this_doesnt_exist

COMMANDS:
    info        Display information from the target device(s) or file.

Neither is behaviour when a secondary subcommand is not specified, so in case of picotool partition --help, the behaviour is unmodified as that situation already prints the available subcommands. It's really only handling --help and -h at the end of the commandline, which is muscle memory for many people that live in their terminals.

I at first tried to modify the signature of match to use an outparam, but that didn't work as any throws would still end up being handled by the outer throw. I didn't want to return a boolean return to indicate the help is desired, so instead I decided to subclass from the parse_error and make a more specific flavour of that.

I've only tested this on Debian GNU/Linux 12, I hope it works with MSVC, I did not find a CONTRIBUTING.md file or .clang-format file, so not sure if there's any tests I should/can run.

[iwanders]

While using the new functionality I discovered two deficiencies my initial implementation, hence the force pushes. It's now greedily searching over the remaining arguments for -h or --help, instead of only checking the first argument that remains. I ran into this through:

$picotool partition create partitions.json  --verbose  /tmp/foo.uf2
# Cool, there's an uf2 file at /tmp/foo.uf2
$picotool --help
...
COMMANDS:
    info        Display information from the target device(s) or file. 
# Nice, lets call info on that file we just created.
$picotool uf2 info /tmp/foo.uf2
ERROR: unexpected argument: /tmp/foo.uf2
$ picotool uf2 info /tmp/foo.uf2  --help
ERROR: unexpected argument: /tmp/foo.uf2

Because the parse error happens on the first unmatched argument.

With the latest version, we get (for the last command):

picotool uf2 info /tmp/foo.uf2 --help
ERROR: unexpected argument: --help, this looks like you want the help:
INFO:
    Print info about UF2 download.

Which unfortunately doesn't print anything on how to run it on a file, I think that's a bug, but unrelated to this PR, I'm going to try to flash the file instead of pursuing this unrelated bug and try to get the wifi firmware into a partition :)

[lurch]

I've no opinion on whether we should include this change or not (that's @will-v-pi 's decision to make), but I wonder if you ought to remove the ERROR: unexpected option: --help, this looks like you want the help: message? If I run e.g. git help commit or git commit --help, then both commands appear to show identical output.

[will-v-pi]

Which unfortunately doesn't print anything on how to run it on a file, I think that's a bug, but unrelated to this PR, I'm going to try to flash the file instead of pursuing this unrelated bug and try to get the wifi firmware into a partition :)

You can't run uf2 info on a file - it prints info about the last UF2 download to a device if it failed. To get info from a file just use the info command.

[will-v-pi]

Also, please target all PRs at the develop branch, not master

[iwanders]

I've no opinion on whether we should include this change or not

As always; maintainer's choice, I ran into something that I thought was worth fixing for myself :)

but I wonder if you ought to remove the ERROR: unexpected option: --help, this looks like you want the help: message? If I run e.g. git help commit or git commit --help, then both commands appear to show identical output.

Yeah, maybe we should. Initially I didn't in case someone typo'd a -h command or something, but you are right that it probably makes more sense to not print this. I've removed it, output is now:

./picotool partition info --help
INFO:
    Print the device's partition table.

SYNOPSIS:
    picotool partition info [-m <family_id>] [device-selection]

OPTIONS:
        -m <family_id>
            family ID (will show target partition for said family)

To get info from a file just use the info command.

Ah, perfect, thanks that's indeed what I needed.

Also, please target all PRs at the develop branch, not master

I've changed the target branch. It would be good to include this in the pull request template, that way anyone filing a PR will read this requirement.

[iwanders]

I've now also rebased my branch on the develop branch, that should hopefully make the previous pipeline failures go away. In my previous comment I merely changed the target branch but didn't rebase the source completely, I thought it would be fine, but the pipeline failed spectacularly... this should do it 🤞

[lurch]

I've changed the target branch. It would be good to include this in the pull request template, that way anyone filing a PR will read this requirement.

When dcbeb00 eventually gets merged to the master branch, it'll automatically reject any PRs targeting master 🙂

[will-v-pi]

I was having a go at testing this PR, and it doesn't work for these commands (and possibly others):

• version - prints ERROR: Version -h not compatible with this software
• otp list - prints nothing, because it filters for OTP rows -h or --help which don't exist
• otp get - same as list

[iwanders]

it doesn't work for these commands (and possibly others):
version - prints ERROR: Version -h not compatible with this software
otp list - prints nothing, because it filters for OTP rows -h or --help which don't exist
otp get - same as list

It works exactly as intended though? In these examples you provide the -h is not considered an unexpected argument, it is considered valid positional argument by the parser?

Against the master branch (de8ae5a);

$ ./picotool otp list -h
$ ./picotool version -h
picotool v2.1.1 (Linux, GNU-14.2.0, Release)
ERROR: Version -h not compatible with this software

$ ./picotool otp get -h
No accessible RP2350 devices in BOOTSEL mode were found.

I did not state this PR would introduce blanket support for --help or -h printing the help; the title stated we only do so when the argument is unexpectedly encountered while parsing. In that situation previously the parser bailed and printed that the user made a mistake and informed them how to actually get the help, we now print the help directly? It's not a change that exhaustively matches -h or --help, and it was never intended to be. For me it was about making the failure mode more user friendly.

[iwanders]

Alternatively, a simple implementation to fix this by handling all --help or -h as if the help subcommand was used is available in d681d0f, the diff is super small, because we can just check if the args vector contains -h or --help anywhere and if so prepend the help subcommand, then relying on all the existing logic to handle the printing of the help. This prevents --help and -h from ever being matched as a positional argument... grep -P "option\('h'" main.cpp is empty though, so maybe nothing uses -h right now and this is actually the best way forward? Unless someone actually used -h as the value of a positional argument, I'm not sure what options and values all exist. Maybe the best compromise is to only handle --help, using the method proposed in this comment? That's probably least surprising since it is consistent (not only on the unexpected argument path) and is very unlikely to overzealously match?

With that commit:

$ ./picotool version -h
VERSION:
    Display picotool version

SYNOPSIS:
    picotool version [-s] [<version>]

OPTIONS:
        -s, --semantic
            Output semantic version number only
        <version>
            Check compatibility with version
$ ./picotool otp get -h
OTP GET:
    Get the value of one or more OTP registers/fields

SYNOPSIS:
    picotool otp get [-c <copies>] [-r] [-e] [-n] [-i <filename>] [device-selection] [-z] [<selector>..]

OPTIONS:
...

The approach here breaks if anyone ever introduces the -- convention in the make_args parser, since it would match the --help after the double hyphen. Though I don't think this commandline tool will need that anytime soon.

[will-v-pi]

Alternatively, a simple implementation to fix this by handling all --help or -h as if the help subcommand was used is available in d681d0f, the diff is super small, because we can just check if the args vector contains -h or --help anywhere and if so prepend the help subcommand, then relying on all the existing logic to handle the printing of the help. This prevents --help and -h from ever being matched as a positional argument... grep -P "option\('h'" main.cpp is empty though, so maybe nothing uses -h right now and this is actually the best way forward? Unless someone actually used -h as the value of a positional argument, I'm not sure what options and values all exist. Maybe the best compromise is to only handle --help, using the method proposed in this comment? That's probably least surprising since it is consistent (not only on the unexpected argument path) and is very unlikely to overzealously match?

Thanks, I think this option is better - if you could push this commit, I'm happy to merge it

I think matching both -h and --help is fine, as no current commands use the -h parameter, so it should be fine to ensure future commands also don't have a -h parameter

[iwanders]

Thanks, I think this option is better

Sounds good, maybe my original approach was unnecessarily conservative.

if you could push this commit, I'm happy to merge it

I've rebased the commit on the latest upstream master and pushed it to this branch.