Re: [PATCH 00/20] parse-options: handle subcommands

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Jul 25 2022, SZEDER Gábor wrote:

> Several Git commands have subcommands to implement mutually exclusive
> "operation modes", and they usually parse their subcommand argument
> with a bunch of if-else if statements.

On this series in general, quoting from 19/20:

> Note also that this change "hides" the '-h' option in 'git stash push
> -h' from the parse_option() call in cmd_stash(), as it comes after the
> subcommand.  Consequently, from now on it will emit the usage of the
> 'push' subcommand instead of the usage of 'git stash'.  We had a
> failing test for this case, which can now be flipped to expect
> success.

This is good!

>  	/* Assume 'stash push' */
>  	strvec_push(&args, "push");
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index 2a4c3fd61c..376cc8f4ab 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -25,7 +25,7 @@ test_expect_success 'usage on main command -h emits a summary of subcommands' '
>  	grep -F "or: git stash show" usage
>  '
>  
> -test_expect_failure 'usage for subcommands should emit subcommand usage' '
> +test_expect_success 'usage for subcommands should emit subcommand usage' '
>  	test_expect_code 129 git stash push -h >usage &&
>  	grep -F "usage: git stash [push" usage
>  '

This fixes the TODO test I left in ca7990cea5a (stash: don't show "git
stash push" usage on bad "git stash" usage, 2021-12-16), it probably
makes sense to link to that in the commit message, as it shows exactly
the output you're referring to.

After our recent off-list discussion about this topic I thought it would
make sense to lead with some testing of this behavior, so I had created
this WIP:
https://github.com/git/git/compare/master...avar:git:avar-szeder/add-usage-tests

Some of that is adding tests for the behavior of commands you're
modifying in this series, many of them suffer from the same caveat as
"git stash" did before this fix, and it would be nice to test for that.

But on the other hand maybe we're confident enough with this being a
standard API that it's just tested once, and we don't need to reapeat
for every command that uses it that "git commit-graph write -h" or
whatever returns usage that's a subset of "git commit-graph -h" etc.

I left some nits here & there, and would like to have more time to
review 10/20 in particlar (as well as others).

But this series is a bit winding, in that it's doing at least these N
unrelated things:

 * Fixes to parse-options docs
 * Tests for existing parse options behavior
 * Nits here & there, e.g. 08/20
 * The meaty change in 10/20
 * For 11-20 some of it's one-to-one with existing behavior, and some of
   it's changing behavior (but all in a good way, I think!), per the
   above some of which we don't have tests for.

Personally I'm fine with just bundling this all together, just some
reading notes from going over this...