Re: [PATCH 5/6] help: correct logic error in combining --all and --config

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

On Wed, Sep 08 2021, Eric Sunshine wrote:

> On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> wrote:
>> Fix a bug in the --config option that's been there ever since its
>> introduction in 3ac68a93fd2 (help: add --config to list all available
>> config, 2018-05-26). Die when --all and --config are combined,
>> combining them doesn't make sense.
>>
>> The code for the --config option when combined with an earlier
>> refactoring done to support the --guide option in
>> 65f98358c0c (builtin/help.c: add --guide option, 2013-04-02) would
>> cause us to take the "--all" branch early and ignore the --config
>> option.
>>
>> Let's instead list these as incompatible, both in the synopsis and
>> help output, and enforce it in the code itself.
>>
>> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
>> ---
>> diff --git a/builtin/help.c b/builtin/help.c
>> @@ -549,18 +550,26 @@ int cmd_help(int argc, const char **argv, const char *prefix)
>> +       /* Incompatible options */
>> +       if (show_all + !!show_config + show_guides > 1)
>> +               usage_with_options(builtin_help_usage, builtin_help_options);
>
> I personally find it highly frustrating when a program merely dumps
> the usage statement without any explanation of what exactly it doesn't
> like about the command-line. Printing out a simple:
>
>     --all, --guides, --config are mutually exclusive
>
> message would go a long way toward reducing the frustration level.
>
> (Aside: I also find it more hostile than helpful when programs dump
> the usage statement for a command-line invocation error -- even if
> preceded by an explanation of the error -- since the explanation
> usually gets drowned-out by the often multi-page usage text, and the
> user has to go spelunking around the wall of output to try to figure
> out what actually went wrong. It's much more helpful and easy to
> figure out what went wrong with the invocation when only a simple
> error message is printed -- without usage statement. However, that's a
> separate battle, as Git already has plenty of places which dump the
> usage statement in response to an invocation error.)

I'll make it emit something more helpful.

More generally I've got quite a bit of parse_options() improvements
queued up locally that I've been trying to trickle in at the rate I can
get them through the list, review over at [1] would be much appreciated.

I wonder if we can do this automatically, we already have the
builtin_help_usage, we could parse that in the general case and find
that certain options are mutually exclusive per the examples there.

We'd then discover what option we parsed when usage_with_options() was
called, and automatically emit a useful message in these sorts of cases.

Of course the usage strings might be incomplete or wrong, but part of
the point would be to find those cases & a test mode to die() if a
command was called with some option combinations not suggested as
working according to its documented usage...

https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/