Re: [PATCH 4/4] builtins: show help on "-h"/"--help-all" with more than 2 arguments left

["D. Ben Knoble" <ben.knoble+github@gmail.com>]

On Sat, Aug 2, 2025 at 5:23 AM Jeff King <peff@peff.net> wrote:
>
> On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:
>
> > >  * "git rebase -h new-base" that shows help is probably a bug (think
> > >    what should happen with s/rebase/grep/) in the first place.
> >
> > And at least according to my tests, "git grep -h new-base" still greps
> > rather than shows help. Compare
> > - "git grep -h squash" (greps squash)
> > - "git rebase -h @{u}" (shows help)
>
> I was somewhat surprised that grep would still work, looking at the
> diff. The reason is that it does not call any of the touched functions,
> but instead relies on this line in parse-options to trigger help:
>
>   $ git grep -A2 'lone -h'
>   parse-options.c:                /* lone -h asks for help */
>   parse-options.c-                if (internal_help && ctx->total == 1 && !strcmp(arg + 1, "h"))
>   parse-options.c-                        goto show_usage;
>
> rather than any of the if_asked functions you touched. So I think there
> may be two problems:
>
>   1. You didn't touch this spot in the parse-options code. Would you
>      need to for it to be consistent with the non-parse-options callers
>      that use the if_asked functions?
>
>   2. We can only get here if we make it past the help check in
>      run_builtin(), that you do modify in your patch. That works for
>      git-grep because it does not use RUN_SETUP, and calls
>      parse_options() before checking whether we are in a repository.
>
>      So in run_builtin() we do set "help" to 1, but it does nothing
>      without the RUN_SETUP flag. But imagine a hypothetical git-foo that
>      takes a "-h" option and does require a repository. It would set the
>      RUN_SETUP flag, and then:
>
>        git foo -h bar
>
>      would show the help before we even get into cmd_foo() to parse the
>      options.

I think I need to consider both questions in parallel: as you point
out, this patch probably doesn't work for a hypothetical command that
both needs a repository and has a "-h" option. (I note that ls-remote
also is RUN_SETUP_GENTLY, like grep). Since no such command exists
today, we /could/ take some version of this patch and refine later if
a command needs both RUN_SETUP and a "-h" option. Or we could reject
this patch (assuming there's no workaround for now). Given Junio's
concern, I'm inclined to just drop the patch from the series…

…which moots question 1, I think. OTOH, if we keep the patch, it does
seem like we might want the parse-options API to be consistent.
Fortunately, I don't think this area needs adjusted for 3/4 based on
the tests.

>
> BTW, I applied your patch 4 manually to dig into this. I wasn't able to
> apply the whole series. It doesn't go on top of the current 'master',
> and applying with "am -3" mentions "sha1 information is lacking or
> useless". Did you build this on some other unpublished series?
>
> -Peff

The base is published and mentioned in the cover letter [1]; if I can
make that more explicit in any way going forward, please let me know!

[1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/