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

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

On Fri, Sep 10 2021, Eric Sunshine wrote:

> On Wed, Sep 8, 2021 at 3:40 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> On Wed, Sep 08 2021, Eric Sunshine wrote:
>> > 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/
>
> 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/
>
>> 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.