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

On Fri, Sep 10 2021, Eric Sunshine wrote:

On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason [off-list ref] wrote:

quoted

On Wed, Sep 08 2021, Eric Sunshine wrote:

quoted

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.

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.
https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/ (local)

My review time is very limited these days (which is why most of my
review comments lately are superficial), but I set aside some time to
review that series for you[1]. Most of my review is subjective, but I
did identify one lurking bug (assuming I understand the code
correctly).

Thanks, I'll try to take a look at that in detail & re-roll soon.

[1]: https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/ (local)

quoted

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

Perhaps, though I imagine it would have to employ some, um,
"interesting" heuristics, and be quite hit-and-miss at first, at least
until people get around to formalizing existing and new usage strings
with the specific goal of supporting that feature.

Speaking of heuristics and wishful thinking, when I read the cover
letter of your series which I just reviewed, I thought at first that
the end-goal would be to ignore whatever indentation the caller
provided following each embedded newline, and instead insert the
correct computed indentation automatically. This approach would
obviate the need for the [1/2] indentation cleanup patch. However,
doing so would require heuristics or at least manual help from the
caller to indicate the proper indentation width. I also thought
perhaps the intention of the series was to do the line-wrapping
automatically (ignoring any caller-provided embedded newlines), thus
ensuring that the lines were indented correctly _and_ fit the terminal
width properly regardless, but that's a somewhat more substantial
change.

Yeah, I considered it but decided not to mainly not for the heuristics
reason, but that any such thing means that you won't have the same
indentation in the C code. I.e. instead of:

    "git foo [--some-option]\n"
    "        [-even -more options]"

You'd have:

    "git foo [--some-option]\n"
    "[--even --more options]"

Which I think is just not as legible, but also in the general case you
get into the quagmire of how do align a thing like:

    "git [-c foo=bar] something [--option]\n"
    "[--more-options]"

Are we doing to align on the "git " or "git [-c foo=bar] something ", or
is it a "git something" where "something" is the name of a sub-command,
or is that a filename?

We control the source code so we can do it, but I thought it would be
nasty, and in any case any working solution wouldn't give you alignment
in the source code, so I dropped the idea.