On Sat, Aug 2, 2025 at 5:23 AM Jeff King <peff@xxxxxxxx> 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@xxxxxxxxx/
