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

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

quoted

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

quoted

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/ (local)

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