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

On Wed, Sep 08 2021, Eric Sunshine wrote:

On Wed, Sep 8, 2021 at 11:24 AM Ævar Arnfjörð Bjarmason
[off-list ref] wrote:

quoted

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 <redacted>
---

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/ (local)