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, 09:23 UTC
Message-ID: <20250802092331.GA3721454@coredump.intra.peff.net>
In-Reply-To: <CALnO6CAbTjuFrLbhC24VDmdy4Utx29g92M5eYV9BPahXK+o+1A@mail.gmail.com>

On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:

Show 7 quoted lines

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

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