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

From: Jeff King <peff@peff.net>
Date: Aug 2, 2025, 16:28 UTC
Message-ID: <20250802162845.GA1203943@coredump.intra.peff.net>
In-Reply-To: <CALnO6CD_=ksmXrzS-k5rQukYcMSDLm0HSLZ35xP-Y1neKc70Xw@mail.gmail.com>

On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote:

Show 32 quoted lines

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

I think I mostly share Junio's concern. The issue is that we want to detect the "user is asking for help" situation without having access to the option-parsing information for the actual sub-command. And so our strategy has been to make the rule for triggering "asking for help" to be fairly conservative.

If we loosened it now, even though it happens to work for all current commands, we'd later potentially have to re-tighten (which is awkward) or start carrying extra signals back to git.c (e.g., a HAVE_H_OPTION flag).

Show 9 quoted lines

> > 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?
> 
> 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/

Ah, hmm. I was trying on top of ua/t1517-short-help-tests, which still fails. But it works if I merge that branch to 'master'. I guess that's what you meant by "Merge that branch to a new topic branch".

-Peff