On Sat, Aug 2, 2025 at 12:28 PM Jeff King <peff@xxxxxxxx> wrote: > > On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote: > > > > 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). Sensible. Will drop. > > > > 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@xxxxxxxxx/ > > 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". Ah, yep, that could be clearer (and it didn't occur to me that basing on a merge would cause application issues relative to basing directly on the branch). If it helps, the graph I have is pasted below (but beware GMail whitespace munging?) git log --graph --boundary ^origin/master @ * 3099d83cdf (HEAD -> help-all-tweaks) builtins: show help on "-h"/"--help-all" with more than 2 arguments left * 352fe87c80 builtin: also setup gently for --help-all * 56665594a8 parse-options: name flags passed to usage_with_options_internal * 852a4547af t1517: fixup for ua/t1517-short-help-tests * 14c7e9dddd Merge remote-tracking branch 'broken-out/ua/t1517-short-help-tests' into help-all-tweaks-tests |\ | * 344c7067e6 (broken-out/ua/t1517-short-help-tests) t5200: move `update-server-info -h` test from t1517 | * e552926bc4 t/t1517: automate `git subcmd -h` tests outside a repository | o 16bd9f20a4 (tag: v2.50.0) Git 2.50 o | e4ef0485fd (origin/master, origin/HEAD, broken-out/master, broken-out/HEAD) The fourteenth batch / I've also (now) made the series available on GitHub at https://github.com/benknoble/git/tree/help-all-tweaks > > -Peff
