{"thread":{"id":"63861","subject":"[PATCH 0/4] permit -h/--help-all in more scenarios","startedAt":"2025-07-26T16:53:31Z","lastAt":"2025-08-05T01:29:11Z","messageCount":30,"participants":["D. Ben Knoble","Usman Akinyemi","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"522804","messageId":"20250726165320.4039-1-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":null,"subject":"[PATCH 0/4] permit -h/--help-all in more scenarios","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-26T16:53:10Z","receivedAt":"2025-07-26T16:53:31Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"This series depends on ua/t1517-short-help-tests with some fixes, which show up\nin the first patch. Merge that branch to a new topic branch, then apply this\nseries.\n\nThis series enables --help-all outside of repository contexts, and\nallows -h with other arguments (without breaking existing ls-remote/grep\nusage).\n\nIt consists of preparatory steps (fixes for a dependency branch;\nrefactoring to make an internal helper's arguments clearer) followed by\nthe main commits.\n\nD. Ben Knoble (4):\n t1517: fixup for ua/t1517-short-help-tests\n parse-options: name flags passed to usage_with_options_internal\n builtin: also setup gently for --help-all\n builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments\n left\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 41 +++++++++++++++++++++++++----------\n t/t1517-outside-repo.sh | 9 +++++---\n t/t5200-update-server-info.sh | 2 +-\n usage.c | 3 ++-\n 6 files changed, 41 insertions(+), 19 deletions(-)\n\n\nbase-commit: e4ef0485fd78fcb05866ea78df35796b904e4a8e\nprerequisite-patch-id: ffce2dd036e61c8d36485a17321f858e454db874\nprerequisite-patch-id: 52539022c824997adfc1be0bed8de6b1851d2187\n-- \n2.48.1\n\n"},{"id":"522805","messageId":"20250726165320.4039-2-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-26T16:53:11Z","receivedAt":"2025-07-26T16:53:46Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"- drop spurious message during test\n- fix known breakages that actually work\n- fix new t5200 test\n\nSigned-off-by: D. Ben Knoble \n---\n\nI expect this and other fixes to get squashed into the upstream branch, but I'm\nincluding it here so it's easy to create a clean build.\n\n t/t1517-outside-repo.sh | 5 ++---\n t/t5200-update-server-info.sh | 2 +-\n 2 files changed, 3 insertions(+), 4 deletions(-)\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 9443c0284f..e235ecccde 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -115,8 +115,8 @@\n \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n \thttp-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n-\tmktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \\\n-\tremote-http | remote-https | replay | request-pull | send-email | \\\n+\tmktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n+\tremote-http | remote-https | replay | send-email | \\\n \tsh-i18n--envsubst | shell | show | stage | submodule | svn | \\\n \tupload-archive--writer | upload-pack | web--browse | whatchanged)\n \t\texpect_outcome=expect_failure ;;\n@@ -125,7 +125,6 @@\n \tesac\n \ttest_$expect_outcome \"'git $cmd -h' outside a repository\" '\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n-\t\techo \"Hello\" &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n done\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nindex a1f129db4e..a551e955b5 100755\n--- a/t/t5200-update-server-info.sh\n+++ b/t/t5200-update-server-info.sh\n@@ -48,7 +48,7 @@\n \n test_expect_success 'update-server-info does not crash with -h' '\n \ttest_expect_code 129 git update-server-info -h >usage &&\n-\ttest_grep \"[Uu]sage: git update-server-info \" usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"522806","messageId":"20250726165320.4039-3-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH 2/4] parse-options: name flags passed to usage_with_options_internal","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-26T16:53:12Z","receivedAt":"2025-07-26T16:53:56Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"When reading or editing calls to usage_with_options_internal, it is\ndifficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n(NB there is never a \"1, 1\" case).\n\nGive the flags readable names to improve call-sites.\n\nSigned-off-by: D. Ben Knoble \n---\n parse-options.c | 32 ++++++++++++++++++++++----------\n 1 file changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 5224203ffe..c3222cc9bb 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n+enum usage_style {\n+\tstyle_normal = 0,\n+\tstyle_full = 1,\n+};\n+\n+enum usage_output {\n+\tto_out = 0,\n+\tto_err = 1,\n+};\n+\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t\t\t\t const char * const *,\n \t\t\t\t\t\t\t const struct option *,\n-\t\t\t\t\t\t\t int, int);\n+\t\t\t\t\t\t\t enum usage_style,\n+\t\t\t\t\t\t\t enum usage_output);\n \n enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *options,\n@@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t}\n \n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n-\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n+\t\t\treturn usage_with_options_internal(ctx, usagestr, options, style_full, to_out);\n \t\tif (internal_help && !strcmp(arg + 2, \"help\"))\n \t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n@@ -1129,7 +1140,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \treturn PARSE_OPT_DONE;\n \n show_usage:\n-\treturn usage_with_options_internal(ctx, usagestr, options, 0, 0);\n+\treturn usage_with_options_internal(ctx, usagestr, options, style_normal, to_out);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n@@ -1278,10 +1289,11 @@ static const struct option *find_option_by_long_name(const struct option *opts,\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t\t\t const char * const *usagestr,\n \t\t\t\t\t\t\t const struct option *opts,\n-\t\t\t\t\t\t\t int full, int err)\n+\t\t\t\t\t\t\t enum usage_style help_style,\n+\t\t\t\t\t\t\t enum usage_output to_where)\n {\n \tconst struct option *all_opts = opts;\n-\tFILE *outfile = err ? stderr : stdout;\n+\tFILE *outfile = to_where == to_err ? stderr : stdout;\n \tint need_newline;\n \n \tconst char *usage_prefix = _(\"usage: %s\");\n@@ -1327,7 +1339,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n-\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n+\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n \twhile (*usagestr) {\n@@ -1373,7 +1385,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t\t\t\tfprintf(outfile, \"%s\\n\", _(opts->help));\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!full && (opts->flags & PARSE_OPT_HIDDEN))\n+\t\tif (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))\n \t\t\tcontinue;\n \n \t\tif (need_newline) {\n@@ -1435,7 +1447,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n \t}\n \tfputc('\\n', outfile);\n \n-\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n+\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfputs(\"EOF\\n\", outfile);\n \n \treturn PARSE_OPT_HELP;\n@@ -1444,7 +1456,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n void NORETURN usage_with_options(const char * const *usagestr,\n \t\t\tconst struct option *opts)\n {\n-\tusage_with_options_internal(NULL, usagestr, opts, 0, 1);\n+\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_err);\n \texit(129);\n }\n \n@@ -1453,7 +1465,7 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const struct option *opts)\n {\n \tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n \t\texit(129);\n \t}\n }\n-- \n2.48.1\n\n"},{"id":"522807","messageId":"20250726165320.4039-4-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH 3/4] builtin: also setup gently for --help-all","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-26T16:53:13Z","receivedAt":"2025-07-26T16:54:04Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Git experts often check the help summary of a command to make sure they\nspell options right when suggesting advice to colleagues. Further, they\nmight check hidden options when responding to queries about deprecated\noptions like git-rebase(1)'s \"preserve merges\" option. But some commands\ndon't support \"--help-all\" outside of a git directory. Running (for\nexample)\n\n git rebase --help-all\n\noutside a directory fails in \"setup_git_directory\", erroring with the\nlocalized form of\n\n fatal: not a git repository (or any of the parent directories): .git\n\nLike 99caeed05d (Let 'git -h' show usage without a git dir,\n2009-11-09), we want to show the \"--help-all\" output even without a git\ndir. Make \"--help-all\" where we expect \"-h\" to mean\n\"setup_git_directory_gently\", and interpose early in the natural place\n(\"show_usage_with_options_if_asked\").\n\nDo the same for usage callers with show_usage_if_asked.\n\nThe exception is merge-recursive, whose help block doesn't use newer\nAPIs.\n\nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n On interposition points:\n \n I originally considered leaving out the changes to\n show_usage_with_options_if_asked and relying on the parse-options API to\n do the right thing. Unfortunately, most commands can't make it all the\n way to parse-options when setting up gently, and trying to parse options\n without a repo creates myriad dependency problems (like: we might read\n config after parsing CLI options, so we have to make sure the parsed\n options overrride config).\n \n Some usage.c callers, like check-ref-format, probably deserve to be\n ported to parse-options at this point.\n \n I opted not to do anything too invasive with merge-recursive (like a\n prepatory \"migrate to newer usage APIs\") since I think it's going the\n way of the dodo?\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 11 ++++++++---\n t/t1517-outside-repo.sh | 4 ++++\n usage.c | 3 ++-\n 5 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 03b5100cfa..17aa4db37a 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\")) {\n+\tif (argc == 2 && (!strcmp(argv[1], \"-h\") ||\n+\t\t\t !strcmp(argv[1], \"--help-all\"))) {\n \t\tstruct strbuf msg = STRBUF_INIT;\n \t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\n \t\tshow_usage_if_asked(argc, argv, msg.buf);\ndiff --git a/git.c b/git.c\nindex 07a5fe39fb..40d3df1b76 100644\n--- a/git.c\n+++ b/git.c\n@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n \n-\thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n+\thelp = argc == 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n \tif (help && (run_setup & RUN_SETUP))\n \t\t/* demote to GENTLY to allow 'git cmd -h' outside repo */\n \t\trun_setup = RUN_SETUP_GENTLY;\ndiff --git a/parse-options.c b/parse-options.c\nindex c3222cc9bb..e3ed42f709 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1464,9 +1464,14 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *opts)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n-\t\texit(129);\n+\tif (ac == 2) {\n+\t\tif (!strcmp(av[1], \"-h\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n+\t\t\texit(129);\n+\t\t} else if (!strcmp(av[1], \"--help-all\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_full, to_out);\n+\t\t\texit(129);\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex e235ecccde..b26a03d8a0 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -127,6 +127,10 @@\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n+\ttest_$expect_outcome \"'git $cmd --help-all' outside a repository\" '\n+\t\ttest_expect_code 129 nongit git $cmd --help-all >usage &&\n+\t\ttest_grep \"[Uu]sage: git $cmd \" usage\n+\t'\n done\n \n test_expect_success 'prune does not crash with -h' '\ndiff --git a/usage.c b/usage.c\nindex 81913236a4..4c245ba0cb 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)\n \n void show_usage_if_asked(int ac, const char **av, const char *err)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\"))\n+\tif (ac == 2 && (!strcmp(av[1], \"-h\") ||\n+\t\t\t!strcmp(av[1], \"--help-all\")))\n \t\tshow_usage_if_asked_helper(err);\n }\n \n-- \n2.48.1\n\n"},{"id":"522808","messageId":"20250726165320.4039-5-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-26T16:53:14Z","receivedAt":"2025-07-26T16:54:08Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"When asking for short help on a previous command, the user may use their\nshell history to recall a command like\n\n git rebase new-base\n\nThen inserting \"-h\" after \"rebase\" doesn't yield the help; make it so.\n\nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n I reviewed the following results\n \n git grep '[^[:alpha:]-]-h' 'Documentation/**adoc' ':(exclude)Documentation/RelNotes'\n \n The 2 commands that accept \"-h\" to mean other things are git-grep(1) and\n git-ls-remote(1).\n \n - \"git grep -h 123\" gives results sans filenames, while a lone \"-h\"\n gives help. Behavior is unchanged from 2.48.1 (my local install).\n - \"git ls-remote -h\" gives help; \"git ls-remote -h origin\" lists heads.\n Behavior is unchanged from 2.48.1.\n \n But I'd welcome a more thorough check to make sure I didn't overlook\n things. This is probably the most controversial patch, and if it's\n dropped, that would be alright (though it is helpful for me to reduce\n typing).\n\n builtin/merge-recursive.c | 2 +-\n git.c | 2 +-\n parse-options.c | 2 +-\n usage.c | 2 +-\n 4 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 17aa4db37a..c433d26bdf 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,7 +38,7 @@ int cmd_merge_recursive(int argc,\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \n-\tif (argc == 2 && (!strcmp(argv[1], \"-h\") ||\n+\tif (argc >= 2 && (!strcmp(argv[1], \"-h\") ||\n \t\t\t !strcmp(argv[1], \"--help-all\"))) {\n \t\tstruct strbuf msg = STRBUF_INIT;\n \t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\ndiff --git a/git.c b/git.c\nindex 40d3df1b76..7fe4e15e2d 100644\n--- a/git.c\n+++ b/git.c\n@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n \n-\thelp = argc == 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n+\thelp = argc >= 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n \tif (help && (run_setup & RUN_SETUP))\n \t\t/* demote to GENTLY to allow 'git cmd -h' outside repo */\n \t\trun_setup = RUN_SETUP_GENTLY;\ndiff --git a/parse-options.c b/parse-options.c\nindex e3ed42f709..b9a31c261c 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1464,7 +1464,7 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *opts)\n {\n-\tif (ac == 2) {\n+\tif (ac >= 2) {\n \t\tif (!strcmp(av[1], \"-h\")) {\n \t\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n \t\t\texit(129);\ndiff --git a/usage.c b/usage.c\nindex 4c245ba0cb..244e8d37ed 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -192,7 +192,7 @@ static void show_usage_if_asked_helper(const char *err, ...)\n \n void show_usage_if_asked(int ac, const char **av, const char *err)\n {\n-\tif (ac == 2 && (!strcmp(av[1], \"-h\") ||\n+\tif (ac >= 2 && (!strcmp(av[1], \"-h\") ||\n \t\t\t!strcmp(av[1], \"--help-all\")))\n \t\tshow_usage_if_asked_helper(err);\n }\n-- \n2.48.1\n\n"},{"id":"522814","messageId":"CAPSxiM83fpNXsfBrtG4xpRt=yYaU7e0OFkAHK8dkLGpF7jtDxg@mail.gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-2-ben.knoble+github@gmail.com","subject":"Re: [PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests","fromName":"Usman Akinyemi","fromEmail":"usmanakinyemi202@gmail.com","sentAt":"2025-07-26T21:57:28Z","receivedAt":"2025-07-26T21:57:40Z","isPatch":true,"sender":{"key":"usmanakinyemi202@gmail.com","avatar":"https://avatars.githubusercontent.com/u/86585626?v=4"},"body":"> - fix known breakages that actually work\n> - fix new t5200 test\nThanks for fixing this, I do not have to send the update version.\n>\n> Signed-off-by: D. Ben Knoble \n> ---\n>\n> I expect this and other fixes to get squashed into the upstream branch, but I'm\n> including it here so it's easy to create a clean build.\n>\n\n> difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n> http-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n> merge-octopus | merge-one-file | merge-resolve | mergetool | \\\n> - mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \\\n> - remote-http | remote-https | replay | request-pull | send-email | \\\n> + mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n> + remote-http | remote-https | replay | send-email | \\\nThanks\n> sh-i18n--envsubst | shell | show | stage | submodule | svn | \\\n> upload-archive--writer | upload-pack | web--browse | whatchanged)\n> expect_outcome=expect_failure ;;\n> @@ -125,7 +125,6 @@\n\n>\n> test_expect_success 'update-server-info does not crash with -h' '\n> test_expect_code 129 git update-server-info -h >usage &&\n> - test_grep \"[Uu]sage: git update-server-info \" usage &&\n> + test_grep \"[Uu]sage: git update-server-info \" usage\n> '\nLooks good to me.\nThanks.\n>\n"},{"id":"522820","messageId":"xmqqms8qzdd0.fsf@gitster.g","threadId":"63861","inReplyTo":"20250726165320.4039-5-ben.knoble+github@gmail.com","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-27T00:28:11Z","receivedAt":"2025-07-27T00:28:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> When asking for short help on a previous command, the user may use their\n> shell history to recall a command like\n>\n> git rebase new-base\n>\n> Then inserting \"-h\" after \"rebase\" doesn't yield the help; make it so.\n\nI doubt this is a good idea for at least two reasons.\n\n * As \"git help cli\" says, we should be discouraging, not\n encouraging peope to say \"git rebase new-base -h\".\n\n * \"git rebase -h new-base\" that shows help is probably a bug (think\n what should happen with s/rebase/grep/) in the first place.\n\nIf anything, we probably should fix the \"-h\" codepath to\n\n - react and do the short-help only when \"-h\" is the only command\n line option; with argument, it should probably barf, saying \"-h\n does not take an argument\".\n\n - if \"-h\" resulted in reported an alias, it should stop there.\n E.g. \"git -c alias.x=ls-files x -h\" would currently invoke \"git\n ls-files -h\" after reporting that 'x' is aliased to 'ls-files'.\n If the alias is to one of our commands, it is not too risky, but\n otherwise we should not assume it is safe to append \"-h\" to the\n underlying command and run it. Imagine\n\n $ git -c alias.x='!echo rm -rf .' x -h\n\n and worse yet, if your alias did not have \"echo\" in it ;-)???\n\nThe only end-user expectation we can safely assume is when they\nsay,\n \n $ git frotz -h\n\nis that they would get a help on 'frotz' without doing any harm. \nIf frotz is an alias to some external command, for which we have no\nidea what it would do when we run it with \"-h\" appended to the\ncommand line, the user would be in a lot of pain if the aliased\noperation is destructive.\n\n"},{"id":"522856","messageId":"xmqqjz3sxro3.fsf@gitster.g","threadId":"63861","inReplyTo":"20250726165320.4039-3-ben.knoble+github@gmail.com","subject":"Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T15:26:36Z","receivedAt":"2025-07-28T15:26:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> When reading or editing calls to usage_with_options_internal, it is\n> difficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n> (NB there is never a \"1, 1\" case).\n>\n> Give the flags readable names to improve call-sites.\n\nIt is a good idea to explicitly say that this step introduces no\nchange in behaviour, and only changes the way how these 0/1 are\nspelled.\n\n> Signed-off-by: D. Ben Knoble \n> ---\n> parse-options.c | 32 ++++++++++++++++++++++----------\n> 1 file changed, 22 insertions(+), 10 deletions(-)\n>\n> diff --git a/parse-options.c b/parse-options.c\n> index 5224203ffe..c3222cc9bb 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)\n> \tfree(options);\n> }\n> \n> +enum usage_style {\n> +\tstyle_normal = 0,\n> +\tstyle_full = 1,\n> +};\n> +\n> +enum usage_output {\n> +\tto_out = 0,\n> +\tto_err = 1,\n> +};\n\nThese are very much internal implementation detail, so I am not sure\nif this churn is a good thing, though.\n\nFor example, it ought to be sufficient, for the purpose of improved\nreadability, to instead doing this\n\n> static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n> \t\t\t\t\t\t\t const char * const *,\n> \t\t\t\t\t\t\t const struct option *,\n> -\t\t\t\t\t\t\t int, int);\n> +\t\t\t\t\t\t\t enum usage_style,\n> +\t\t\t\t\t\t\t enum usage_output);\n\njust do\n\n\t\tint full_usage,\n\t\tint usage_to_stderr);\n\nhere. Dropping the parameter names in the function prototype is\nallowed, and we encourage to do so in our codebase but _only_ when\nthe meaning of each parameter is obvious from their type. The first\n3 parameters we see above are of distinct types and except for the\nsecond one being the usage string given to the users, they should be\nobvious. But the last two unnamed integers are not obvious and they\nshould have been spelled out---otherwise a developer who is adding\na new callsite cannot work from the prototype alone and has to go to\nthe implementation to figure out what to pass.\n\nAdding two enums for this is a bit overkill, but is OK here locally.\n\n\n> @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n> \t\t}\n> \n> \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n> -\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n> +\t\t\treturn usage_with_options_internal(ctx, usagestr, options, style_full, to_out);\n\nBut this is not an improvement as-is. Wrap long lines or the result\nis even harder to read.\n\n> @@ -1278,10 +1289,11 @@ static const struct option *find_option_by_long_name(const struct option *opts,\n> static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n> \t\t\t\t\t\t\t const char * const *usagestr,\n> \t\t\t\t\t\t\t const struct option *opts,\n> -\t\t\t\t\t\t\t int full, int err)\n> +\t\t\t\t\t\t\t enum usage_style help_style,\n> +\t\t\t\t\t\t\t enum usage_output to_where)\n> {\n> \tconst struct option *all_opts = opts;\n\n> -\tFILE *outfile = err ? stderr : stdout;\n> +\tFILE *outfile = to_where == to_err ? stderr : stdout;\n\nThis one ...\n\n\n> @@ -1327,7 +1339,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n> -\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n> +\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n> \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n\n... and this one become markedly harder to read. I think the\nprimary reason is because unlike the original, the parameter names\nare not biased. \"If we are doing full usage, do this\" is far easier\nto grok than \"If the \"style\" we are told to use is the \"style_full\",\nthen do this\", but use of \"enum\" inherently is about not making the\nvariables and parameters of that enum type unbiased.\n\n> @@ -1373,7 +1385,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n> \t\t\t\tfprintf(outfile, \"%s\\n\", _(opts->help));\n> \t\t\tcontinue;\n> \t\t}\n> -\t\tif (!full && (opts->flags & PARSE_OPT_HIDDEN))\n> +\t\tif (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))\n> \t\t\tcontinue;\n\nDitto.\n\n> \t\tif (need_newline) {\n> @@ -1435,7 +1447,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n> \t}\n> \tfputc('\\n', outfile);\n> \n> -\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n> +\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n> \t\tfputs(\"EOF\\n\", outfile);\n\nDitto.\n\nOne way to reduce this churn is to do\n\n\tint err = (to_where == to_stderr);\n\tint full = (help_style == style_full);\n\nat the very beginning of the function. Then you do not have to\nchange the body of the function harder to read at all.\n\nThanks.\n"},{"id":"522857","messageId":"xmqqcy9kxrbw.fsf@gitster.g","threadId":"63861","inReplyTo":"20250726165320.4039-4-ben.knoble+github@gmail.com","subject":"Re: [PATCH 3/4] builtin: also setup gently for --help-all","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T15:33:55Z","receivedAt":"2025-07-28T15:33:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> I originally considered leaving out the changes to\n> show_usage_with_options_if_asked and relying on the parse-options API to\n> do the right thing. Unfortunately, most commands can't make it all the\n> way to parse-options when setting up gently, and trying to parse options\n> without a repo creates myriad dependency problems (like: we might read\n> config after parsing CLI options, so we have to make sure the parsed\n> options overrride config).\n> \n> Some usage.c callers, like check-ref-format, probably deserve to be\n> ported to parse-options at this point.\n\nIt is unclear what you mean by all of the above, but hopefully it\nwould become clear as we read the code changes.\n\n> I opted not to do anything too invasive with merge-recursive (like a\n> prepatory \"migrate to newer usage APIs\") since I think it's going the\n> way of the dodo?\n\nI think this is fine.\n\n> diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\n> index 03b5100cfa..17aa4db37a 100644\n> --- a/builtin/merge-recursive.c\n> +++ b/builtin/merge-recursive.c\n> @@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,\n> \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n> \t\to.subtree_shift = \"\";\n> \n> -\tif (argc == 2 && !strcmp(argv[1], \"-h\")) {\n> +\tif (argc == 2 && (!strcmp(argv[1], \"-h\") ||\n> +\t\t\t !strcmp(argv[1], \"--help-all\"))) {\n> \t\tstruct strbuf msg = STRBUF_INIT;\n> \t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\n> \t\tshow_usage_if_asked(argc, argv, msg.buf);\n\n;-)\n\n> diff --git a/git.c b/git.c\n> index 07a5fe39fb..40d3df1b76 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n> \tconst char *prefix;\n> \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n> \n> -\thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n> +\thelp = argc == 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n> \tif (help && (run_setup & RUN_SETUP))\n> \t\t/* demote to GENTLY to allow 'git cmd -h' outside repo */\n> \t\trun_setup = RUN_SETUP_GENTLY;\n\nOK. That's obvious and straight-forward. Behave as closely as the\ncase when \"-h\" is given, and let the lower-layer deal with the\ndifferences between \"-h\" and \"--help-all\".\n\n> diff --git a/parse-options.c b/parse-options.c\n> index c3222cc9bb..e3ed42f709 100644\n> --- a/parse-options.c\n> +++ b/parse-options.c\n> @@ -1464,9 +1464,14 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n> \t\t\t\t const char * const *usagestr,\n> \t\t\t\t const struct option *opts)\n> {\n> -\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n> -\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n> -\t\texit(129);\n> +\tif (ac == 2) {\n> +\t\tif (!strcmp(av[1], \"-h\")) {\n> +\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n> +\t\t\texit(129);\n> +\t\t} else if (!strcmp(av[1], \"--help-all\")) {\n> +\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_full, to_out);\n> +\t\t\texit(129);\n> +\t\t}\n> \t}\n> }\n\nAgain, that is obvious and straight-forward.\n\n> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\n> index e235ecccde..b26a03d8a0 100755\n> --- a/t/t1517-outside-repo.sh\n> +++ b/t/t1517-outside-repo.sh\n> @@ -127,6 +127,10 @@\n> \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n> \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n> \t'\n> +\ttest_$expect_outcome \"'git $cmd --help-all' outside a repository\" '\n> +\t\ttest_expect_code 129 nongit git $cmd --help-all >usage &&\n> +\t\ttest_grep \"[Uu]sage: git $cmd \" usage\n> +\t'\n> done\n> \n> test_expect_success 'prune does not crash with -h' '\n> diff --git a/usage.c b/usage.c\n> index 81913236a4..4c245ba0cb 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)\n> \n> void show_usage_if_asked(int ac, const char **av, const char *err)\n> {\n> -\tif (ac == 2 && !strcmp(av[1], \"-h\"))\n> +\tif (ac == 2 && (!strcmp(av[1], \"-h\") ||\n> +\t\t\t!strcmp(av[1], \"--help-all\")))\n> \t\tshow_usage_if_asked_helper(err);\n> }\n\nThis looks good.\n\nI don't understand your \"I originally considered leaving out ...\" at\nall. We are special casing a lone \"-h\" here already because we know\nthis is where we should stop without exercising unnecessary code\nbecause we may not even have repo!=NULL and that is the reason why\nthis function exists in the first place. It is obvious to me that\nwe need the same special casing for \"--help-all\".\n\nIn other words, I think all the above changes are good.\n\nThanks.\n"},{"id":"522859","messageId":"xmqq8qk8xr9j.fsf@gitster.g","threadId":"63861","inReplyTo":"CAPSxiM83fpNXsfBrtG4xpRt=yYaU7e0OFkAHK8dkLGpF7jtDxg@mail.gmail.com","subject":"Re: [PATCH 1/4] t1517: fixup for ua/t1517-short-help-tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T15:35:20Z","receivedAt":"2025-07-28T15:35:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Usman Akinyemi writes:\n\n>> I expect this and other fixes to get squashed into the upstream branch, but I'm\n>> including it here so it's easy to create a clean build.\n>>\n>\n>> difftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n>> http-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n>> merge-octopus | merge-one-file | merge-resolve | mergetool | \\\n>> - mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \\\n>> - remote-http | remote-https | replay | request-pull | send-email | \\\n>> + mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n>> + remote-http | remote-https | replay | send-email | \\\n> Thanks\n>> sh-i18n--envsubst | shell | show | stage | submodule | svn | \\\n>> upload-archive--writer | upload-pack | web--browse | whatchanged)\n>> expect_outcome=expect_failure ;;\n>> @@ -125,7 +125,6 @@\n>\n>>\n>> test_expect_success 'update-server-info does not crash with -h' '\n>> test_expect_code 129 git update-server-info -h >usage &&\n>> - test_grep \"[Uu]sage: git update-server-info \" usage &&\n>> + test_grep \"[Uu]sage: git update-server-info \" usage\n>> '\n> Looks good to me.\n> Thanks.\n\nSo, can I ignore this step from the series and expect the fixes to\nbe already in your updated series we will see in the future?\n\nThanks.\n\n"},{"id":"522871","messageId":"xmqqseiguqig.fsf@gitster.g","threadId":"63861","inReplyTo":"xmqqjz3sxro3.fsf@gitster.g","subject":"Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-07-28T18:19:51Z","receivedAt":"2025-07-28T18:19:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> For example, it ought to be sufficient, for the purpose of improved\n> readability, to instead doing this\n>\n>> static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n>> \t\t\t\t\t\t\t const char * const *,\n>> \t\t\t\t\t\t\t const struct option *,\n>> -\t\t\t\t\t\t\t int, int);\n>> +\t\t\t\t\t\t\t enum usage_style,\n>> +\t\t\t\t\t\t\t enum usage_output);\n>\n> just do\n>\n> \t\tint full_usage,\n> \t\tint usage_to_stderr);\n>\n> here. Dropping the parameter names in the function prototype is\n> allowed, and we encourage to do so in our codebase but _only_ when\n> the meaning of each parameter is obvious from their type. The first\n> 3 parameters we see above are of distinct types and except for the\n> second one being the usage string given to the users, they should be\n> obvious. But the last two unnamed integers are not obvious and they\n> should have been spelled out---otherwise a developer who is adding\n> a new callsite cannot work from the prototype alone and has to go to\n> the implementation to figure out what to pass.\n>\n> Adding two enums for this is a bit overkill, but is OK here locally.\n\n\nAnd with that lessor impact change, you could still add a smaller\nchange to help callers (you do not need any change to the callee,\nwhich uses biased parameter names for these two), if you wanted to\n(though as I said this is internal implementation detail of the\nparse_options API, so you do not have to). For example, you could\ndo\n\n#define USAGE_FULL 1\n#define USAGE_TO_STDERR 1\n\nwithout anything else and do\n\n>> @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n>> \t\t}\n>> \n>> \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n>> -\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n>> +\t\t\treturn usage_with_options_internal(ctx, usagestr, options, style_full, to_out);\n\n\t\t\treturn usage_with_options_internal(ctx,\tusagestr, options,\n\t\t\t\t\t\t\t USAGE_FULL, 0);\n\nwhich may make it easier to follow. The point is that you be more\nverbose only when you do a non-standard thing.\n\nAnd without enum, of course you do not need any change like below.\n\n> One way to reduce this churn is to do\n>\n> \tint err = (to_where == to_stderr);\n> \tint full = (help_style == style_full);\n>\n> at the very beginning of the function. Then you do not have to\n> change the body of the function harder to read at all.\n"},{"id":"523038","messageId":"CALnO6CAbTjuFrLbhC24VDmdy4Utx29g92M5eYV9BPahXK+o+1A@mail.gmail.com","threadId":"63861","inReplyTo":"xmqqms8qzdd0.fsf@gitster.g","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-30T21:55:32Z","receivedAt":"2025-07-30T21:55:45Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Jul 26, 2025 at 8:28 PM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > When asking for short help on a previous command, the user may use their\n> > shell history to recall a command like\n> >\n> > git rebase new-base\n> >\n> > Then inserting \"-h\" after \"rebase\" doesn't yield the help; make it so.\n>\n> I doubt this is a good idea for at least two reasons.\n\nAs I said in the cover letter, I think this is the most controversial\nand can be dropped. However…\n\n> * As \"git help cli\" says, we should be discouraging, not\n> encouraging peope to say \"git rebase new-base -h\".\n\n…that's not what I'm encouraging here. Instead, it's more like below:\n\n> * \"git rebase -h new-base\" that shows help is probably a bug (think\n> what should happen with s/rebase/grep/) in the first place.\n\nAnd at least according to my tests, \"git grep -h new-base\" still greps\nrather than shows help. Compare\n- \"git grep -h squash\" (greps squash)\n- \"git rebase -h @{u}\" (shows help)\n\n> If anything, we probably should fix the \"-h\" codepath to\n>\n> - react and do the short-help only when \"-h\" is the only command\n> line option; with argument, it should probably barf, saying \"-h\n> does not take an argument\".\n\nI think we have the first half already (\"argc == 2\" in the preimage).\nI'm not interested in writing the second half right now, personally,\nif we end up dropping this patch, so someone else could take that up.\n\n> - if \"-h\" resulted in reported an alias, it should stop there.\n> E.g. \"git -c alias.x=ls-files x -h\" would currently invoke \"git\n> ls-files -h\" after reporting that 'x' is aliased to 'ls-files'.\n> If the alias is to one of our commands, it is not too risky, but\n> otherwise we should not assume it is safe to append \"-h\" to the\n> underlying command and run it. Imagine\n>\n> $ git -c alias.x='!echo rm -rf .' x -h\n>\n> and worse yet, if your alias did not have \"echo\" in it ;-)???\n>\n> The only end-user expectation we can safely assume is when they\n> say,\n>\n> $ git frotz -h\n>\n> is that they would get a help on 'frotz' without doing any harm.\n> If frotz is an alias to some external command, for which we have no\n> idea what it would do when we run it with \"-h\" appended to the\n> command line, the user would be in a lot of pain if the aliased\n> operation is destructive.\n>\n\nI think this is being discussed on another series, and I'll leave it there :)\n"},{"id":"523039","messageId":"CALnO6CB+Hh46A=hLLNDYc2dJB=DbfdbethuGk0rzX3dA2ptO8g@mail.gmail.com","threadId":"63861","inReplyTo":"xmqqcy9kxrbw.fsf@gitster.g","subject":"Re: [PATCH 3/4] builtin: also setup gently for --help-all","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-30T22:00:35Z","receivedAt":"2025-07-30T22:00:48Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Jul 28, 2025 at 11:33 AM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > I originally considered leaving out the changes to\n> > show_usage_with_options_if_asked and relying on the parse-options API to\n> > do the right thing. Unfortunately, most commands can't make it all the\n> > way to parse-options when setting up gently, and trying to parse options\n> > without a repo creates myriad dependency problems (like: we might read\n> > config after parsing CLI options, so we have to make sure the parsed\n> > options overrride config).\n> >\n> > Some usage.c callers, like check-ref-format, probably deserve to be\n> > ported to parse-options at this point.\n>\n> It is unclear what you mean by all of the above, but hopefully it\n> would become clear as we read the code changes.\n>\n> [snip]\n>\n> I don't understand your \"I originally considered leaving out ...\" at\n> all. We are special casing a lone \"-h\" here already because we know\n> this is where we should stop without exercising unnecessary code\n> because we may not even have repo!=NULL and that is the reason why\n> this function exists in the first place. It is obvious to me that\n> we need the same special casing for \"--help-all\".\n>\n> In other words, I think all the above changes are good.\n>\n> Thanks.\n\nI'm glad it was obvious to you! I spent a lot of time trying to get\ncommands to call parse_options early, before touching any repository\nvariables, since that was the only place where I knew \"--help-all\" was\nhandled. This turned out to be a terrible mess, so I looked around for\na different path forward (the one here now), and that turned out to\nwork much better.\n\nIn that old world, parse-options doesn't change and the builtins need\na lot of surgery (and probably still aren't completely right)—I can\nsee how that note is confusing without that context, though, and I'll\ndrop it in future rounds (esp. now we've discussed this version).\n"},{"id":"523040","messageId":"CALnO6CDg5tJXNG2V1qXb_Qds+3tDM6cUgbiDO7i3Fy_wc0=FPw@mail.gmail.com","threadId":"63861","inReplyTo":"xmqqjz3sxro3.fsf@gitster.g","subject":"Re: [PATCH 2/4] parse-options: name flags passed to usage_with_options_internal","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-07-30T22:05:39Z","receivedAt":"2025-07-30T22:05:52Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Jul 28, 2025 at 11:26 AM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > When reading or editing calls to usage_with_options_internal, it is\n> > difficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n> > (NB there is never a \"1, 1\" case).\n> >\n> > Give the flags readable names to improve call-sites.\n>\n> It is a good idea to explicitly say that this step introduces no\n> change in behaviour, and only changes the way how these 0/1 are\n> spelled.\n\nWoops; definitely meant for that to be clearer. Will touch up.\n\n>\n> > Signed-off-by: D. Ben Knoble \n> > ---\n> > parse-options.c | 32 ++++++++++++++++++++++----------\n> > 1 file changed, 22 insertions(+), 10 deletions(-)\n> >\n> > diff --git a/parse-options.c b/parse-options.c\n> > index 5224203ffe..c3222cc9bb 100644\n> > --- a/parse-options.c\n> > +++ b/parse-options.c\n> > @@ -953,10 +953,21 @@ static void free_preprocessed_options(struct option *options)\n> > free(options);\n> > }\n> >\n> > +enum usage_style {\n> > + style_normal = 0,\n> > + style_full = 1,\n> > +};\n> > +\n> > +enum usage_output {\n> > + to_out = 0,\n> > + to_err = 1,\n> > +};\n>\n> These are very much internal implementation detail, so I am not sure\n> if this churn is a good thing, though.\n>\n> For example, it ought to be sufficient, for the purpose of improved\n> readability, to instead doing this\n>\n> > static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n> > const char * const *,\n> > const struct option *,\n> > - int, int);\n> > + enum usage_style,\n> > + enum usage_output);\n>\n> just do\n>\n> int full_usage,\n> int usage_to_stderr);\n>\n> here. Dropping the parameter names in the function prototype is\n> allowed, and we encourage to do so in our codebase but _only_ when\n> the meaning of each parameter is obvious from their type. The first\n> 3 parameters we see above are of distinct types and except for the\n> second one being the usage string given to the users, they should be\n> obvious. But the last two unnamed integers are not obvious and they\n> should have been spelled out---otherwise a developer who is adding\n> a new callsite cannot work from the prototype alone and has to go to\n> the implementation to figure out what to pass.\n\nYeah, but that relies on folks reading the prototype, no? I wanted it\nto be easier to read at the call sites (_without_ special tooling,\npreferably).\n\nI'll snip the rest, though, due to your downthread suggestion to use\n#define's instead, which I think gives the result I want without extra\nchurn in other places.\n\n\n> > @@ -1088,7 +1099,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n> > }\n> >\n> > if (internal_help && !strcmp(arg + 2, \"help-all\"))\n> > - return usage_with_options_internal(ctx, usagestr, options, 1, 0);\n> > + return usage_with_options_internal(ctx, usagestr, options, style_full, to_out);\n>\n> But this is not an improvement as-is. Wrap long lines or the result\n> is even harder to read.\n\nAh, indeed: I wasn't sure where to draw the line there.\n"},{"id":"523264","messageId":"20250802092331.GA3721454@coredump.intra.peff.net","threadId":"63861","inReplyTo":"CALnO6CAbTjuFrLbhC24VDmdy4Utx29g92M5eYV9BPahXK+o+1A@mail.gmail.com","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T09:23:31Z","receivedAt":"2025-08-02T09:23:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:\n\n> > * \"git rebase -h new-base\" that shows help is probably a bug (think\n> > what should happen with s/rebase/grep/) in the first place.\n> \n> And at least according to my tests, \"git grep -h new-base\" still greps\n> rather than shows help. Compare\n> - \"git grep -h squash\" (greps squash)\n> - \"git rebase -h @{u}\" (shows help)\n\nI was somewhat surprised that grep would still work, looking at the\ndiff. The reason is that it does not call any of the touched functions,\nbut instead relies on this line in parse-options to trigger help:\n\n $ git grep -A2 'lone -h'\n parse-options.c: /* lone -h asks for help */\n parse-options.c- if (internal_help && ctx->total == 1 && !strcmp(arg + 1, \"h\"))\n parse-options.c- goto show_usage;\n\nrather than any of the if_asked functions you touched. So I think there\nmay be two problems:\n\n 1. You didn't touch this spot in the parse-options code. Would you\n need to for it to be consistent with the non-parse-options callers\n that use the if_asked functions?\n\n 2. We can only get here if we make it past the help check in\n run_builtin(), that you do modify in your patch. That works for\n git-grep because it does not use RUN_SETUP, and calls\n parse_options() before checking whether we are in a repository.\n\n So in run_builtin() we do set \"help\" to 1, but it does nothing\n without the RUN_SETUP flag. But imagine a hypothetical git-foo that\n takes a \"-h\" option and does require a repository. It would set the\n RUN_SETUP flag, and then:\n\n git foo -h bar\n\n would show the help before we even get into cmd_foo() to parse the\n options.\n\nBTW, I applied your patch 4 manually to dig into this. I wasn't able to\napply the whole series. It doesn't go on top of the current 'master',\nand applying with \"am -3\" mentions \"sha1 information is lacking or\nuseless\". Did you build this on some other unpublished series?\n\n-Peff\n"},{"id":"523273","messageId":"CALnO6CD_=ksmXrzS-k5rQukYcMSDLm0HSLZ35xP-Y1neKc70Xw@mail.gmail.com","threadId":"63861","inReplyTo":"20250802092331.GA3721454@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-02T16:10:17Z","receivedAt":"2025-08-02T16:10:30Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Aug 2, 2025 at 5:23 AM Jeff King wrote:\n>\n> On Wed, Jul 30, 2025 at 05:55:32PM -0400, D. Ben Knoble wrote:\n>\n> > > * \"git rebase -h new-base\" that shows help is probably a bug (think\n> > > what should happen with s/rebase/grep/) in the first place.\n> >\n> > And at least according to my tests, \"git grep -h new-base\" still greps\n> > rather than shows help. Compare\n> > - \"git grep -h squash\" (greps squash)\n> > - \"git rebase -h @{u}\" (shows help)\n>\n> I was somewhat surprised that grep would still work, looking at the\n> diff. The reason is that it does not call any of the touched functions,\n> but instead relies on this line in parse-options to trigger help:\n>\n> $ git grep -A2 'lone -h'\n> parse-options.c: /* lone -h asks for help */\n> parse-options.c- if (internal_help && ctx->total == 1 && !strcmp(arg + 1, \"h\"))\n> parse-options.c- goto show_usage;\n>\n> rather than any of the if_asked functions you touched. So I think there\n> may be two problems:\n>\n> 1. You didn't touch this spot in the parse-options code. Would you\n> need to for it to be consistent with the non-parse-options callers\n> that use the if_asked functions?\n>\n> 2. We can only get here if we make it past the help check in\n> run_builtin(), that you do modify in your patch. That works for\n> git-grep because it does not use RUN_SETUP, and calls\n> parse_options() before checking whether we are in a repository.\n>\n> So in run_builtin() we do set \"help\" to 1, but it does nothing\n> without the RUN_SETUP flag. But imagine a hypothetical git-foo that\n> takes a \"-h\" option and does require a repository. It would set the\n> RUN_SETUP flag, and then:\n>\n> git foo -h bar\n>\n> would show the help before we even get into cmd_foo() to parse the\n> options.\n\nI think I need to consider both questions in parallel: as you point\nout, this patch probably doesn't work for a hypothetical command that\nboth needs a repository and has a \"-h\" option. (I note that ls-remote\nalso is RUN_SETUP_GENTLY, like grep). Since no such command exists\ntoday, we /could/ take some version of this patch and refine later if\na command needs both RUN_SETUP and a \"-h\" option. Or we could reject\nthis patch (assuming there's no workaround for now). Given Junio's\nconcern, I'm inclined to just drop the patch from the series…\n\n…which moots question 1, I think. OTOH, if we keep the patch, it does\nseem like we might want the parse-options API to be consistent.\nFortunately, I don't think this area needs adjusted for 3/4 based on\nthe tests.\n\n>\n> BTW, I applied your patch 4 manually to dig into this. I wasn't able to\n> apply the whole series. It doesn't go on top of the current 'master',\n> and applying with \"am -3\" mentions \"sha1 information is lacking or\n> useless\". Did you build this on some other unpublished series?\n>\n> -Peff\n\nThe base is published and mentioned in the cover letter [1]; if I can\nmake that more explicit in any way going forward, please let me know!\n\n[1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/\n"},{"id":"523278","messageId":"20250802162845.GA1203943@coredump.intra.peff.net","threadId":"63861","inReplyTo":"CALnO6CD_=ksmXrzS-k5rQukYcMSDLm0HSLZ35xP-Y1neKc70Xw@mail.gmail.com","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-08-02T16:28:45Z","receivedAt":"2025-08-02T16:35:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote:\n\n> > 1. You didn't touch this spot in the parse-options code. Would you\n> > need to for it to be consistent with the non-parse-options callers\n> > that use the if_asked functions?\n> >\n> > 2. We can only get here if we make it past the help check in\n> > run_builtin(), that you do modify in your patch. That works for\n> > git-grep because it does not use RUN_SETUP, and calls\n> > parse_options() before checking whether we are in a repository.\n> >\n> > So in run_builtin() we do set \"help\" to 1, but it does nothing\n> > without the RUN_SETUP flag. But imagine a hypothetical git-foo that\n> > takes a \"-h\" option and does require a repository. It would set the\n> > RUN_SETUP flag, and then:\n> >\n> > git foo -h bar\n> >\n> > would show the help before we even get into cmd_foo() to parse the\n> > options.\n> \n> I think I need to consider both questions in parallel: as you point\n> out, this patch probably doesn't work for a hypothetical command that\n> both needs a repository and has a \"-h\" option. (I note that ls-remote\n> also is RUN_SETUP_GENTLY, like grep). Since no such command exists\n> today, we /could/ take some version of this patch and refine later if\n> a command needs both RUN_SETUP and a \"-h\" option. Or we could reject\n> this patch (assuming there's no workaround for now). Given Junio's\n> concern, I'm inclined to just drop the patch from the series…\n> \n> …which moots question 1, I think. OTOH, if we keep the patch, it does\n> seem like we might want the parse-options API to be consistent.\n> Fortunately, I don't think this area needs adjusted for 3/4 based on\n> the tests.\n\nI think I mostly share Junio's concern. The issue is that we want to\ndetect the \"user is asking for help\" situation without having access to\nthe option-parsing information for the actual sub-command. And so our\nstrategy has been to make the rule for triggering \"asking for help\" to\nbe fairly conservative.\n\nIf we loosened it now, even though it happens to work for all current\ncommands, we'd later potentially have to re-tighten (which is awkward)\nor start carrying extra signals back to git.c (e.g., a HAVE_H_OPTION\nflag).\n\n> > BTW, I applied your patch 4 manually to dig into this. I wasn't able to\n> > apply the whole series. It doesn't go on top of the current 'master',\n> > and applying with \"am -3\" mentions \"sha1 information is lacking or\n> > useless\". Did you build this on some other unpublished series?\n> \n> The base is published and mentioned in the cover letter [1]; if I can\n> make that more explicit in any way going forward, please let me know!\n> \n> [1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/\n\nAh, hmm. I was trying on top of ua/t1517-short-help-tests, which still\nfails. But it works if I merge that branch to 'master'. I guess that's\nwhat you meant by \"Merge that branch to a new topic branch\".\n\n-Peff\n"},{"id":"523281","messageId":"CALnO6CCWo6t-SX22XZ62k8jrGdH0gxC=w6B9D=6BB1iLEyc1AA@mail.gmail.com","threadId":"63861","inReplyTo":"20250802162845.GA1203943@coredump.intra.peff.net","subject":"Re: [PATCH 4/4] builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-02T17:05:34Z","receivedAt":"2025-08-02T17:05:49Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sat, Aug 2, 2025 at 12:28 PM Jeff King wrote:\n>\n> On Sat, Aug 02, 2025 at 12:10:17PM -0400, D. Ben Knoble wrote:\n>\n> > > 1. You didn't touch this spot in the parse-options code. Would you\n> > > need to for it to be consistent with the non-parse-options callers\n> > > that use the if_asked functions?\n> > >\n> > > 2. We can only get here if we make it past the help check in\n> > > run_builtin(), that you do modify in your patch. That works for\n> > > git-grep because it does not use RUN_SETUP, and calls\n> > > parse_options() before checking whether we are in a repository.\n> > >\n> > > So in run_builtin() we do set \"help\" to 1, but it does nothing\n> > > without the RUN_SETUP flag. But imagine a hypothetical git-foo that\n> > > takes a \"-h\" option and does require a repository. It would set the\n> > > RUN_SETUP flag, and then:\n> > >\n> > > git foo -h bar\n> > >\n> > > would show the help before we even get into cmd_foo() to parse the\n> > > options.\n> >\n> > I think I need to consider both questions in parallel: as you point\n> > out, this patch probably doesn't work for a hypothetical command that\n> > both needs a repository and has a \"-h\" option. (I note that ls-remote\n> > also is RUN_SETUP_GENTLY, like grep). Since no such command exists\n> > today, we /could/ take some version of this patch and refine later if\n> > a command needs both RUN_SETUP and a \"-h\" option. Or we could reject\n> > this patch (assuming there's no workaround for now). Given Junio's\n> > concern, I'm inclined to just drop the patch from the series…\n> >\n> > …which moots question 1, I think. OTOH, if we keep the patch, it does\n> > seem like we might want the parse-options API to be consistent.\n> > Fortunately, I don't think this area needs adjusted for 3/4 based on\n> > the tests.\n>\n> I think I mostly share Junio's concern. The issue is that we want to\n> detect the \"user is asking for help\" situation without having access to\n> the option-parsing information for the actual sub-command. And so our\n> strategy has been to make the rule for triggering \"asking for help\" to\n> be fairly conservative.\n>\n> If we loosened it now, even though it happens to work for all current\n> commands, we'd later potentially have to re-tighten (which is awkward)\n> or start carrying extra signals back to git.c (e.g., a HAVE_H_OPTION\n> flag).\n\nSensible. Will drop.\n\n>\n> > > BTW, I applied your patch 4 manually to dig into this. I wasn't able to\n> > > apply the whole series. It doesn't go on top of the current 'master',\n> > > and applying with \"am -3\" mentions \"sha1 information is lacking or\n> > > useless\". Did you build this on some other unpublished series?\n> >\n> > The base is published and mentioned in the cover letter [1]; if I can\n> > make that more explicit in any way going forward, please let me know!\n> >\n> > [1]: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/\n>\n> Ah, hmm. I was trying on top of ua/t1517-short-help-tests, which still\n> fails. But it works if I merge that branch to 'master'. I guess that's\n> what you meant by \"Merge that branch to a new topic branch\".\n\nAh, yep, that could be clearer (and it didn't occur to me that basing\non a merge would cause application issues relative to basing directly\non the branch). If it helps, the graph I have is pasted below (but\nbeware GMail whitespace munging?)\n\n git log --graph --boundary ^origin/master @\n\n* 3099d83cdf (HEAD -> help-all-tweaks) builtins: show help on\n\"-h\"/\"--help-all\" with more than 2 arguments left\n* 352fe87c80 builtin: also setup gently for --help-all\n* 56665594a8 parse-options: name flags passed to usage_with_options_internal\n* 852a4547af t1517: fixup for ua/t1517-short-help-tests\n* 14c7e9dddd Merge remote-tracking branch\n'broken-out/ua/t1517-short-help-tests' into help-all-tweaks-tests\n|\\\n| * 344c7067e6 (broken-out/ua/t1517-short-help-tests) t5200: move\n`update-server-info -h` test from t1517\n| * e552926bc4 t/t1517: automate `git subcmd -h` tests outside a repository\n| o 16bd9f20a4 (tag: v2.50.0) Git 2.50\no | e4ef0485fd (origin/master, origin/HEAD, broken-out/master,\nbroken-out/HEAD) The fourteenth batch\n /\n\nI've also (now) made the series available on GitHub at\n\n https://github.com/benknoble/git/tree/help-all-tweaks\n\n>\n> -Peff\n"},{"id":"523298","messageId":"20250803012613.54086-1-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH v2 0/3] permit -h/--help-all in more scenarios","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T01:26:01Z","receivedAt":"2025-08-03T01:26:25Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v1:\n- tweak refactor commit message to indicate no behavior is changed;\n- use #define'd constants instead of an enum for internal flags;\n- drop controversial [4/4]\n\nQQ: Is there a better way to add --cc's to format-patch than grabbing the right\nfolks off threads from lore.kernel.org/git? (I could also grab them out of my\nmail client.) I assume this is where using tools like b4 starts to really shine?\nTo be clear: I mean CC's _in addition to_ the use of\n\n sendemail.ccCmd = perl contrib/contacts/git-contacts\n\nto keep folks that participated in prior rounds of discussion abreast of the\nnext version.\n\n--------\nOriginal\n--------\n\nThis series depends on ua/t1517-short-help-tests with some fixes, which\nshow up in the first patch. Merge that branch to a new topic branch:\n\n git switch -c topic v2.50.0 # or origin/master\n git merge gitster/ua/t1517-short-help-tests\n\nthen apply this series.\n\nThis series enables --help-all outside of repository contexts, and\nallows -h with other arguments (without breaking existing ls-remote/grep\nusage).\n\nIt consists of preparatory steps (fixes for a dependency branch;\nrefactoring to make an internal helper's arguments clearer) followed by\nthe main commits.\n\nv1: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/git/tree/help-all-tweaks\n\nD. Ben Knoble (3):\n t1517: fixup for ua/t1517-short-help-tests\n parse-options: refactor flags for usage_with_options_internal\n builtin: also setup gently for --help-all\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 30 +++++++++++++++++++++++-------\n t/t1517-outside-repo.sh | 11 +++++++----\n t/t5200-update-server-info.sh | 2 +-\n usage.c | 3 ++-\n 6 files changed, 36 insertions(+), 15 deletions(-)\n\nDiff-intervalle contre v1 :\n1: 852a4547af ! 1: 7a3e0a601d t1517: fixup for ua/t1517-short-help-tests\n @@ Commit message\n \n - drop spurious message during test\n - fix known breakages that actually work\n + - fix instaweb marker for known failure\n - fix new t5200 test\n \n \n @@ Notes\n \n ## t/t1517-outside-repo.sh ##\n @@\n + \tcase \"$cmd\" in\n + \tarchimport | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n - \thttp-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n +-\thttp-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n ++\thttp-backend | http-fetch | http-push | init-db | \\\n \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n -\tmktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \\\n -\tremote-http | remote-https | replay | request-pull | send-email | \\\n2: 56665594a8 ! 2: db74b1eff7 parse-options: name flags passed to usage_with_options_internal\n @@ Metadata\n Author: D. Ben Knoble \n \n ## Commit message ##\n - parse-options: name flags passed to usage_with_options_internal\n + parse-options: refactor flags for usage_with_options_internal\n \n When reading or editing calls to usage_with_options_internal, it is\n difficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n (NB there is never a \"1, 1\" case).\n \n - Give the flags readable names to improve call-sites.\n + Give the flags readable names to improve call-sites without changing any\n + behavior.\n \n ## parse-options.c ##\n @@ parse-options.c: static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n -+enum usage_style {\n -+\tstyle_normal = 0,\n -+\tstyle_full = 1,\n -+};\n -+\n -+enum usage_output {\n -+\tto_out = 0,\n -+\tto_err = 1,\n -+};\n ++#define USAGE_NORMAL 0\n ++#define USAGE_FULL 1\n ++#define USAGE_TO_STDOUT 0\n ++#define USAGE_TO_STDERR 1\n +\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t\t\t\t const char * const *,\n \t\t\t\t\t\t\t const struct option *,\n -\t\t\t\t\t\t\t int, int);\n -+\t\t\t\t\t\t\t enum usage_style,\n -+\t\t\t\t\t\t\t enum usage_output);\n ++\t\t\t\t\t\t\t int full_usage,\n ++\t\t\t\t\t\t\t int usage_to_stderr);\n \n enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *options,\n @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t\n \n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n -\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n -+\t\t\treturn usage_with_options_internal(ctx, usagestr, options, style_full, to_out);\n ++\t\t\treturn usage_with_options_internal(ctx, usagestr, options,\n ++\t\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n \t\tif (internal_help && !strcmp(arg + 2, \"help\"))\n \t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n @@ parse-options.c: enum parse_opt_result parse_options_step(struct parse_opt_ctx_t\n \n show_usage:\n -\treturn usage_with_options_internal(ctx, usagestr, options, 0, 0);\n -+\treturn usage_with_options_internal(ctx, usagestr, options, style_normal, to_out);\n ++\treturn usage_with_options_internal(ctx, usagestr, options,\n ++\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n -@@ parse-options.c: static const struct option *find_option_by_long_name(const struct option *opts,\n - static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n - \t\t\t\t\t\t\t const char * const *usagestr,\n - \t\t\t\t\t\t\t const struct option *opts,\n --\t\t\t\t\t\t\t int full, int err)\n -+\t\t\t\t\t\t\t enum usage_style help_style,\n -+\t\t\t\t\t\t\t enum usage_output to_where)\n - {\n - \tconst struct option *all_opts = opts;\n --\tFILE *outfile = err ? stderr : stdout;\n -+\tFILE *outfile = to_where == to_err ? stderr : stdout;\n - \tint need_newline;\n - \n - \tconst char *usage_prefix = _(\"usage: %s\");\n -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n - \tif (!usagestr)\n - \t\treturn PARSE_OPT_HELP;\n - \n --\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n -+\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n - \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n - \n - \twhile (*usagestr) {\n -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n - \t\t\t\tfprintf(outfile, \"%s\\n\", _(opts->help));\n - \t\t\tcontinue;\n - \t\t}\n --\t\tif (!full && (opts->flags & PARSE_OPT_HIDDEN))\n -+\t\tif (help_style != style_full && (opts->flags & PARSE_OPT_HIDDEN))\n - \t\t\tcontinue;\n - \n - \t\tif (need_newline) {\n -@@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n - \t}\n - \tfputc('\\n', outfile);\n - \n --\tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n -+\tif (to_where != to_err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n - \t\tfputs(\"EOF\\n\", outfile);\n - \n - \treturn PARSE_OPT_HELP;\n @@ parse-options.c: static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n void NORETURN usage_with_options(const char * const *usagestr,\n \t\t\tconst struct option *opts)\n {\n -\tusage_with_options_internal(NULL, usagestr, opts, 0, 1);\n -+\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_err);\n ++\tusage_with_options_internal(NULL, usagestr, opts,\n ++\t\t\t\t USAGE_NORMAL, USAGE_TO_STDERR);\n \texit(129);\n }\n \n @@ parse-options.c: void show_usage_with_options_if_asked(int ac, const char **av,\n {\n \tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n -\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n -+\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n ++\t\tusage_with_options_internal(NULL, usagestr, opts,\n ++\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n \t\texit(129);\n \t}\n }\n3: 352fe87c80 ! 3: cb4113f77d builtin: also setup gently for --help-all\n @@ Commit message\n The exception is merge-recursive, whose help block doesn't use newer\n APIs.\n \n + Best-viewed-with: --ignore-space-change\n +\n \n ## Notes ##\n Some usage.c callers, like check-ref-format, probably deserve to be\n @@ parse-options.c: void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const struct option *opts)\n {\n -\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n --\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n +-\t\tusage_with_options_internal(NULL, usagestr, opts,\n +-\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n -\t\texit(129);\n +\tif (ac == 2) {\n +\t\tif (!strcmp(av[1], \"-h\")) {\n -+\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_normal, to_out);\n ++\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n ++\t\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n +\t\t\texit(129);\n +\t\t} else if (!strcmp(av[1], \"--help-all\")) {\n -+\t\t\tusage_with_options_internal(NULL, usagestr, opts, style_full, to_out);\n ++\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n ++\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n +\t\t\texit(129);\n +\t\t}\n \t}\n4: 3099d83cdf < -: ---------- builtins: show help on \"-h\"/\"--help-all\" with more than 2 arguments left\n\nbase-commit: e4ef0485fd78fcb05866ea78df35796b904e4a8e\nprerequisite-patch-id: ffce2dd036e61c8d36485a17321f858e454db874\nprerequisite-patch-id: 52539022c824997adfc1be0bed8de6b1851d2187\n-- \n2.48.1\n\n"},{"id":"523299","messageId":"20250803012613.54086-2-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH v2 1/3] t1517: fixup for ua/t1517-short-help-tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T01:26:02Z","receivedAt":"2025-08-03T01:26:37Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"- drop spurious message during test\n- fix known breakages that actually work\n- fix instaweb marker for known failure: it is not a failure, but should not\n have .sh suffix\n- fix new t5200 test\n\nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n I expect this and other fixes to get squashed into the upstream branch,\n but I'm including it here so it's easy to create a clean build.\n\n t/t1517-outside-repo.sh | 7 +++----\n t/t5200-update-server-info.sh | 2 +-\n 2 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 9443c0284f..deb72da66e 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -113,10 +113,10 @@\n \tcase \"$cmd\" in\n \tarchimport | cvsexportcommit | cvsimport | cvsserver | daemon | \\\n \tdifftool--helper | filter-branch | fsck-objects | get-tar-commit-id | \\\n-\thttp-backend | http-fetch | http-push | init-db | instaweb.sh | \\\n+\thttp-backend | http-fetch | http-push | init-db | \\\n \tmerge-octopus | merge-one-file | merge-resolve | mergetool | \\\n-\tmktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \\\n-\tremote-http | remote-https | replay | request-pull | send-email | \\\n+\tmktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \\\n+\tremote-http | remote-https | replay | send-email | \\\n \tsh-i18n--envsubst | shell | show | stage | submodule | svn | \\\n \tupload-archive--writer | upload-pack | web--browse | whatchanged)\n \t\texpect_outcome=expect_failure ;;\n@@ -125,7 +125,6 @@\n \tesac\n \ttest_$expect_outcome \"'git $cmd -h' outside a repository\" '\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n-\t\techo \"Hello\" &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n done\ndiff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh\nindex a1f129db4e..a551e955b5 100755\n--- a/t/t5200-update-server-info.sh\n+++ b/t/t5200-update-server-info.sh\n@@ -48,7 +48,7 @@\n \n test_expect_success 'update-server-info does not crash with -h' '\n \ttest_expect_code 129 git update-server-info -h >usage &&\n-\ttest_grep \"[Uu]sage: git update-server-info \" usage &&\n+\ttest_grep \"[Uu]sage: git update-server-info \" usage\n '\n \n test_done\n-- \n2.48.1\n\n"},{"id":"523300","messageId":"20250803012613.54086-3-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH v2 2/3] parse-options: refactor flags for usage_with_options_internal","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T01:26:03Z","receivedAt":"2025-08-03T01:26:42Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"When reading or editing calls to usage_with_options_internal, it is\ndifficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n(NB there is never a \"1, 1\" case).\n\nGive the flags readable names to improve call-sites without changing any\nbehavior.\n\nSigned-off-by: D. Ben Knoble \n---\n\nI went with a slightly more verbose approach here: Junio argued that the \"0\"\ncase is common so deserves to be short, but it's still hard IMO to tell what \"0\"\nrepresents when reading only the function calls (say, in a patch with minimal\ncontext).\n\nUnfortunately, the deep rightward indent is also a bit challenging on my eyes :/\n\n parse-options.c | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 5224203ffe..169d76fb65 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -953,10 +953,16 @@ static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n+#define USAGE_NORMAL 0\n+#define USAGE_FULL 1\n+#define USAGE_TO_STDOUT 0\n+#define USAGE_TO_STDERR 1\n+\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t\t\t\t const char * const *,\n \t\t\t\t\t\t\t const struct option *,\n-\t\t\t\t\t\t\t int, int);\n+\t\t\t\t\t\t\t int full_usage,\n+\t\t\t\t\t\t\t int usage_to_stderr);\n \n enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *options,\n@@ -1088,7 +1094,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t}\n \n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n-\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n+\t\t\treturn usage_with_options_internal(ctx, usagestr, options,\n+\t\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n \t\tif (internal_help && !strcmp(arg + 2, \"help\"))\n \t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n@@ -1129,7 +1136,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \treturn PARSE_OPT_DONE;\n \n show_usage:\n-\treturn usage_with_options_internal(ctx, usagestr, options, 0, 0);\n+\treturn usage_with_options_internal(ctx, usagestr, options,\n+\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n@@ -1444,7 +1452,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n void NORETURN usage_with_options(const char * const *usagestr,\n \t\t\tconst struct option *opts)\n {\n-\tusage_with_options_internal(NULL, usagestr, opts, 0, 1);\n+\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t USAGE_NORMAL, USAGE_TO_STDERR);\n \texit(129);\n }\n \n@@ -1453,7 +1462,8 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const struct option *opts)\n {\n \tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n \t\texit(129);\n \t}\n }\n-- \n2.48.1\n\n"},{"id":"523301","messageId":"20250803012613.54086-4-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250726165320.4039-1-ben.knoble+github@gmail.com","subject":"[PATCH v2 3/3] builtin: also setup gently for --help-all","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T01:26:04Z","receivedAt":"2025-08-03T01:26:46Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Git experts often check the help summary of a command to make sure they\nspell options right when suggesting advice to colleagues. Further, they\nmight check hidden options when responding to queries about deprecated\noptions like git-rebase(1)'s \"preserve merges\" option. But some commands\ndon't support \"--help-all\" outside of a git directory. Running (for\nexample)\n\n git rebase --help-all\n\noutside a directory fails in \"setup_git_directory\", erroring with the\nlocalized form of\n\n fatal: not a git repository (or any of the parent directories): .git\n\nLike 99caeed05d (Let 'git -h' show usage without a git dir,\n2009-11-09), we want to show the \"--help-all\" output even without a git\ndir. Make \"--help-all\" where we expect \"-h\" to mean\n\"setup_git_directory_gently\", and interpose early in the natural place\n(\"show_usage_with_options_if_asked\").\n\nDo the same for usage callers with show_usage_if_asked.\n\nThe exception is merge-recursive, whose help block doesn't use newer\nAPIs.\n\nBest-viewed-with: --ignore-space-change\nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n Some usage.c callers, like check-ref-format, probably deserve to be\n ported to parse-options at this point.\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 14 ++++++++++----\n t/t1517-outside-repo.sh | 4 ++++\n usage.c | 3 ++-\n 5 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 03b5100cfa..17aa4db37a 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\")) {\n+\tif (argc == 2 && (!strcmp(argv[1], \"-h\") ||\n+\t\t\t !strcmp(argv[1], \"--help-all\"))) {\n \t\tstruct strbuf msg = STRBUF_INIT;\n \t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\n \t\tshow_usage_if_asked(argc, argv, msg.buf);\ndiff --git a/git.c b/git.c\nindex 07a5fe39fb..40d3df1b76 100644\n--- a/git.c\n+++ b/git.c\n@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n \n-\thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n+\thelp = argc == 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n \tif (help && (run_setup & RUN_SETUP))\n \t\t/* demote to GENTLY to allow 'git cmd -h' outside repo */\n \t\trun_setup = RUN_SETUP_GENTLY;\ndiff --git a/parse-options.c b/parse-options.c\nindex 169d76fb65..d9f960b7b5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1461,10 +1461,16 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *opts)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts,\n-\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n-\t\texit(129);\n+\tif (ac == 2) {\n+\t\tif (!strcmp(av[1], \"-h\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n+\t\t\texit(129);\n+\t\t} else if (!strcmp(av[1], \"--help-all\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n+\t\t\texit(129);\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex deb72da66e..a780c54133 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -127,6 +127,10 @@\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n+\ttest_$expect_outcome \"'git $cmd --help-all' outside a repository\" '\n+\t\ttest_expect_code 129 nongit git $cmd --help-all >usage &&\n+\t\ttest_grep \"[Uu]sage: git $cmd \" usage\n+\t'\n done\n \n test_expect_success 'prune does not crash with -h' '\ndiff --git a/usage.c b/usage.c\nindex 81913236a4..4c245ba0cb 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)\n \n void show_usage_if_asked(int ac, const char **av, const char *err)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\"))\n+\tif (ac == 2 && (!strcmp(av[1], \"-h\") ||\n+\t\t\t!strcmp(av[1], \"--help-all\")))\n \t\tshow_usage_if_asked_helper(err);\n }\n \n-- \n2.48.1\n\n"},{"id":"523345","messageId":"20250803161033.77696-1-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250803012613.54086-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 0/3] permit -h/--help-all in more scenarios","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T16:10:24Z","receivedAt":"2025-08-03T16:10:40Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Changes from v2:\n- Rebase on unpublished ua/t1517… v5 (https://lore.kernel.org/git/20250803020744.1037392-1-usmanakinyemi202@gmail.com/)\n which is rebased on master\n- Changed fixup to account for instaweb Perl dependency\n\nChanges from v1:\n- tweak refactor commit message to indicate no behavior is changed;\n- use #define'd constants instead of an enum for internal flags;\n- drop controversial [4/4]\n\nQQ: Is there a better way to add --cc's to format-patch than grabbing\nthe right folks off threads from lore.kernel.org/git? (I could also grab\nthem out of my mail client.) I assume this is where using tools like b4\nstarts to really shine? To be clear: I mean CC's _in addition to_ the\nuse of\n\n sendemail.ccCmd = perl contrib/contacts/git-contacts\n\nto keep folks that participated in prior rounds of discussion abreast of the\nnext version.\n\n--------\nOriginal\n--------\n\nThis series depends on ua/t1517-short-help-tests with some fixes, which\nshow up in the first patch. Start a new topic from the (unpublished)\nv5 of that branch:\n\n git switch -c topic origin/master\n git am ua/t1517-short-help-tests-v5.mbox\n\nThen apply this series.\n\nThis series enables --help-all outside of repository contexts, and\nallows -h with other arguments (without breaking existing ls-remote/grep\nusage).\n\nIt consists of preparatory steps (fixes for a dependency branch;\nrefactoring to make an internal helper's arguments clearer) followed by\nthe main commits.\n\nv1: https://lore.kernel.org/git/20250726165320.4039-1-ben.knoble+github@gmail.com/\nv2: https://lore.kernel.org/git/20250803012613.54086-1-ben.knoble+github@gmail.com/\nPublished-as: https://github.com/benknoble/git/tree/help-all-tweaks\n\nD. Ben Knoble (3):\n t1517: fixup for ua/t1517-short-help-tests\n parse-options: refactor flags for usage_with_options_internal\n builtin: also setup gently for --help-all\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 30 +++++++++++++++++++++++-------\n t/t1517-outside-repo.sh | 12 +++++++++++-\n usage.c | 3 ++-\n 5 files changed, 39 insertions(+), 11 deletions(-)\n\nDiff-intervalle contre v2 :\n1: e552926bc4 < -: ---------- t/t1517: automate `git subcmd -h` tests outside a repository\n2: 344c7067e6 < -: ---------- t5200: move `update-server-info -h` test from t1517\n3: 7a3e0a601d < -: ---------- t1517: fixup for ua/t1517-short-help-tests\n-: ---------- > 1: 855931592f t1517: fixup for ua/t1517-short-help-tests\n4: db74b1eff7 = 2: 2392039374 parse-options: refactor flags for usage_with_options_internal\n5: cb4113f77d ! 3: 4dfc046e6d builtin: also setup gently for --help-all\n @@ t/t1517-outside-repo.sh\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n -+\ttest_$expect_outcome \"'git $cmd --help-all' outside a repository\" '\n ++\ttest_$expect_outcome $prereq \"'git $cmd --help-all' outside a repository\" '\n +\t\ttest_expect_code 129 nongit git $cmd --help-all >usage &&\n +\t\ttest_grep \"[Uu]sage: git $cmd \" usage\n +\t'\n done\n \n - test_expect_success 'prune does not crash with -h' '\n + test_done\n \n ## usage.c ##\n @@ usage.c: static void show_usage_if_asked_helper(const char *err, ...)\n\nbase-commit: 866e6a391f466baeeb98bc585845ea638322c04b\nprerequisite-patch-id: eb2c55de73b2b733aa0c8c94469a45d9bc361d81\nprerequisite-patch-id: 12ddad29e334853cf61c797a2c08302893f4b909\nprerequisite-patch-id: 250bae2541030fcdfc5b35ede44c23e1138c7a3c\n-- \n2.48.1\n\n"},{"id":"523346","messageId":"20250803161033.77696-2-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250803012613.54086-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T16:10:25Z","receivedAt":"2025-08-03T16:10:41Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"- fix instaweb test prereqs\n\nSigned-off-by: D. Ben Knoble \n---\n t/t1517-outside-repo.sh | 8 +++++++-\n 1 file changed, 7 insertions(+), 1 deletion(-)\n\ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 8a417af47a..3dc602872a 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -123,7 +123,13 @@\n \t*)\n \t\texpect_outcome=expect_success ;;\n \tesac\n-\ttest_$expect_outcome \"'git $cmd -h' outside a repository\" '\n+\tcase \"$cmd\" in\n+\tinstaweb)\n+\t\tprereq=PERL ;;\n+\t*)\n+\t\tprereq= ;;\n+\tesac\n+\ttest_$expect_outcome $prereq \"'git $cmd -h' outside a repository\" '\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n-- \n2.48.1\n\n"},{"id":"523347","messageId":"20250803161033.77696-3-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250803012613.54086-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 2/3] parse-options: refactor flags for usage_with_options_internal","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T16:10:26Z","receivedAt":"2025-08-03T16:10:43Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"When reading or editing calls to usage_with_options_internal, it is\ndifficult to tell what trailing \"0, 0\", \"0, 1\", \"1, 0\" arguments mean\n(NB there is never a \"1, 1\" case).\n\nGive the flags readable names to improve call-sites without changing any\nbehavior.\n\nSigned-off-by: D. Ben Knoble \n---\n parse-options.c | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 5224203ffe..169d76fb65 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -953,10 +953,16 @@ static void free_preprocessed_options(struct option *options)\n \tfree(options);\n }\n \n+#define USAGE_NORMAL 0\n+#define USAGE_FULL 1\n+#define USAGE_TO_STDOUT 0\n+#define USAGE_TO_STDERR 1\n+\n static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t *,\n \t\t\t\t\t\t\t const char * const *,\n \t\t\t\t\t\t\t const struct option *,\n-\t\t\t\t\t\t\t int, int);\n+\t\t\t\t\t\t\t int full_usage,\n+\t\t\t\t\t\t\t int usage_to_stderr);\n \n enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t\t\t\t const struct option *options,\n@@ -1088,7 +1094,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \t\t}\n \n \t\tif (internal_help && !strcmp(arg + 2, \"help-all\"))\n-\t\t\treturn usage_with_options_internal(ctx, usagestr, options, 1, 0);\n+\t\t\treturn usage_with_options_internal(ctx, usagestr, options,\n+\t\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n \t\tif (internal_help && !strcmp(arg + 2, \"help\"))\n \t\t\tgoto show_usage;\n \t\tswitch (parse_long_opt(ctx, arg + 2, options)) {\n@@ -1129,7 +1136,8 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,\n \treturn PARSE_OPT_DONE;\n \n show_usage:\n-\treturn usage_with_options_internal(ctx, usagestr, options, 0, 0);\n+\treturn usage_with_options_internal(ctx, usagestr, options,\n+\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n }\n \n int parse_options_end(struct parse_opt_ctx_t *ctx)\n@@ -1444,7 +1452,8 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t\n void NORETURN usage_with_options(const char * const *usagestr,\n \t\t\tconst struct option *opts)\n {\n-\tusage_with_options_internal(NULL, usagestr, opts, 0, 1);\n+\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t USAGE_NORMAL, USAGE_TO_STDERR);\n \texit(129);\n }\n \n@@ -1453,7 +1462,8 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const struct option *opts)\n {\n \tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts, 0, 0);\n+\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n \t\texit(129);\n \t}\n }\n-- \n2.48.1\n\n"},{"id":"523348","messageId":"20250803161033.77696-4-ben.knoble+github@gmail.com","threadId":"63861","inReplyTo":"20250803012613.54086-1-ben.knoble+github@gmail.com","subject":"[PATCH v3 3/3] builtin: also setup gently for --help-all","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T16:10:27Z","receivedAt":"2025-08-03T16:10:47Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"Git experts often check the help summary of a command to make sure they\nspell options right when suggesting advice to colleagues. Further, they\nmight check hidden options when responding to queries about deprecated\noptions like git-rebase(1)'s \"preserve merges\" option. But some commands\ndon't support \"--help-all\" outside of a git directory. Running (for\nexample)\n\n git rebase --help-all\n\noutside a directory fails in \"setup_git_directory\", erroring with the\nlocalized form of\n\n fatal: not a git repository (or any of the parent directories): .git\n\nLike 99caeed05d (Let 'git -h' show usage without a git dir,\n2009-11-09), we want to show the \"--help-all\" output even without a git\ndir. Make \"--help-all\" where we expect \"-h\" to mean\n\"setup_git_directory_gently\", and interpose early in the natural place\n(\"show_usage_with_options_if_asked\").\n\nDo the same for usage callers with show_usage_if_asked.\n\nThe exception is merge-recursive, whose help block doesn't use newer\nAPIs.\n\nBest-viewed-with: --ignore-space-change\nSigned-off-by: D. Ben Knoble \n---\n\nNotes:\n Some usage.c callers, like check-ref-format, probably deserve to be\n ported to parse-options at this point.\n\n builtin/merge-recursive.c | 3 ++-\n git.c | 2 +-\n parse-options.c | 14 ++++++++++----\n t/t1517-outside-repo.sh | 4 ++++\n usage.c | 3 ++-\n 5 files changed, 19 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 03b5100cfa..17aa4db37a 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,7 +38,8 @@ int cmd_merge_recursive(int argc,\n \tif (argv[0] && ends_with(argv[0], \"-subtree\"))\n \t\to.subtree_shift = \"\";\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\")) {\n+\tif (argc == 2 && (!strcmp(argv[1], \"-h\") ||\n+\t\t\t !strcmp(argv[1], \"--help-all\"))) {\n \t\tstruct strbuf msg = STRBUF_INIT;\n \t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\n \t\tshow_usage_if_asked(argc, argv, msg.buf);\ndiff --git a/git.c b/git.c\nindex 07a5fe39fb..40d3df1b76 100644\n--- a/git.c\n+++ b/git.c\n@@ -445,7 +445,7 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv, struct\n \tconst char *prefix;\n \tint run_setup = (p->option & (RUN_SETUP | RUN_SETUP_GENTLY));\n \n-\thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n+\thelp = argc == 2 && (!strcmp(argv[1], \"-h\") || !strcmp(argv[1], \"--help-all\"));\n \tif (help && (run_setup & RUN_SETUP))\n \t\t/* demote to GENTLY to allow 'git cmd -h' outside repo */\n \t\trun_setup = RUN_SETUP_GENTLY;\ndiff --git a/parse-options.c b/parse-options.c\nindex 169d76fb65..d9f960b7b5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1461,10 +1461,16 @@ void show_usage_with_options_if_asked(int ac, const char **av,\n \t\t\t\t const char * const *usagestr,\n \t\t\t\t const struct option *opts)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\")) {\n-\t\tusage_with_options_internal(NULL, usagestr, opts,\n-\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n-\t\texit(129);\n+\tif (ac == 2) {\n+\t\tif (!strcmp(av[1], \"-h\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t\t USAGE_NORMAL, USAGE_TO_STDOUT);\n+\t\t\texit(129);\n+\t\t} else if (!strcmp(av[1], \"--help-all\")) {\n+\t\t\tusage_with_options_internal(NULL, usagestr, opts,\n+\t\t\t\t\t\t USAGE_FULL, USAGE_TO_STDOUT);\n+\t\t\texit(129);\n+\t\t}\n \t}\n }\n \ndiff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\nindex 3dc602872a..e34321dd44 100755\n--- a/t/t1517-outside-repo.sh\n+++ b/t/t1517-outside-repo.sh\n@@ -133,6 +133,10 @@\n \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n \t'\n+\ttest_$expect_outcome $prereq \"'git $cmd --help-all' outside a repository\" '\n+\t\ttest_expect_code 129 nongit git $cmd --help-all >usage &&\n+\t\ttest_grep \"[Uu]sage: git $cmd \" usage\n+\t'\n done\n \n test_done\ndiff --git a/usage.c b/usage.c\nindex 81913236a4..4c245ba0cb 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -192,7 +192,8 @@ static void show_usage_if_asked_helper(const char *err, ...)\n \n void show_usage_if_asked(int ac, const char **av, const char *err)\n {\n-\tif (ac == 2 && !strcmp(av[1], \"-h\"))\n+\tif (ac == 2 && (!strcmp(av[1], \"-h\") ||\n+\t\t\t!strcmp(av[1], \"--help-all\")))\n \t\tshow_usage_if_asked_helper(err);\n }\n \n-- \n2.48.1\n\n"},{"id":"523350","messageId":"xmqq34a8fjdo.fsf@gitster.g","threadId":"63861","inReplyTo":"20250803161033.77696-2-ben.knoble+github@gmail.com","subject":"Re: [PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-03T16:41:07Z","receivedAt":"2025-08-03T16:41:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> - fix instaweb test prereqs\n>\n> Signed-off-by: D. Ben Knoble \n> ---\n> t/t1517-outside-repo.sh | 8 +++++++-\n> 1 file changed, 7 insertions(+), 1 deletion(-)\n>\n> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\n> index 8a417af47a..3dc602872a 100755\n> --- a/t/t1517-outside-repo.sh\n> +++ b/t/t1517-outside-repo.sh\n> @@ -123,7 +123,13 @@\n> \t*)\n> \t\texpect_outcome=expect_success ;;\n> \tesac\n> -\ttest_$expect_outcome \"'git $cmd -h' outside a repository\" '\n> +\tcase \"$cmd\" in\n> +\tinstaweb)\n> +\t\tprereq=PERL ;;\n> +\t*)\n> +\t\tprereq= ;;\n> +\tesac\n> +\ttest_$expect_outcome $prereq \"'git $cmd -h' outside a repository\" '\n> \t\ttest_expect_code 129 nongit git $cmd -h >usage &&\n> \t\ttest_grep \"[Uu]sage: git $cmd \" usage\n> \t'\n\nIdeally this would want to be squashed (or moved) into the base\ntopic. Can you two coordinate among yourselves?\n\nThanks.\n\n"},{"id":"523351","messageId":"CALnO6CDFekoqMjBDQifO7pj-7geXOUqN5yBbQmsPHBtTjj_kZw@mail.gmail.com","threadId":"63861","inReplyTo":"xmqq34a8fjdo.fsf@gitster.g","subject":"Re: [PATCH v3 1/3] t1517: fixup for ua/t1517-short-help-tests","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-03T16:43:28Z","receivedAt":"2025-08-03T16:43:41Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Sun, Aug 3, 2025 at 12:41 PM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > - fix instaweb test prereqs\n> >\n> > Signed-off-by: D. Ben Knoble \n> > ---\n> > t/t1517-outside-repo.sh | 8 +++++++-\n> > 1 file changed, 7 insertions(+), 1 deletion(-)\n> >\n> > diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh\n> > index 8a417af47a..3dc602872a 100755\n> > --- a/t/t1517-outside-repo.sh\n> > +++ b/t/t1517-outside-repo.sh\n> > @@ -123,7 +123,13 @@\n> > *)\n> > expect_outcome=expect_success ;;\n> > esac\n> > - test_$expect_outcome \"'git $cmd -h' outside a repository\" '\n> > + case \"$cmd\" in\n> > + instaweb)\n> > + prereq=PERL ;;\n> > + *)\n> > + prereq= ;;\n> > + esac\n> > + test_$expect_outcome $prereq \"'git $cmd -h' outside a repository\" '\n> > test_expect_code 129 nongit git $cmd -h >usage &&\n> > test_grep \"[Uu]sage: git $cmd \" usage\n> > '\n>\n> Ideally this would want to be squashed (or moved) into the base\n> topic. Can you two coordinate among yourselves?\n\nAgreed, and happy to—just not sure what that looks like here. Advice welcome :)\n"},{"id":"523371","messageId":"xmqqo6svd6wt.fsf@gitster.g","threadId":"63861","inReplyTo":"20250803161033.77696-1-ben.knoble+github@gmail.com","subject":"Re: [PATCH v3 0/3] permit -h/--help-all in more scenarios","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-08-04T04:53:22Z","receivedAt":"2025-08-04T04:53:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"D. Ben Knoble\" writes:\n\n> This series enables --help-all outside of repository contexts,\n\nI've been familiar with \"git commit -h\" (and commands other than\n\"commit\") outside a repository, but did not even know that \"git\ncommit --help-all\" didn't work outside. For commands that use\nparse-options, these come from the same source of informatino, so it\ndoes not make any sense for one to work and the other to refuse to\nwork. Good.\n\n> and\n> allows -h with other arguments (without breaking existing ls-remote/grep\n> usage).\n\nI somehow thought we already talked you out of this.\n\nDo you mean something like \"git add -h foo\" and \"git add -h -N foo\"\nwould say \"'git add foo' would add the current contents in foo to\nthe index\" and \"'git add -N foo' would make the index aware of the\npath foo without actually adding its contents (yet)\"? I do not think\nit makes much sense to behave exactly the same as \"git add -h\" when\nthe user says \"git add -h foo\" or \"git add -h -N foo\", as if we\ndidn't even see the extra things on the command line.\n\nThanks.\n\n\n"},{"id":"523495","messageId":"CALnO6CDemUHgvGLH4gOZSF7o9Yv-=ScgfPmVJsjmfFGBK0Hw1A@mail.gmail.com","threadId":"63861","inReplyTo":"xmqqo6svd6wt.fsf@gitster.g","subject":"Re: [PATCH v3 0/3] permit -h/--help-all in more scenarios","fromName":"D. Ben Knoble","fromEmail":"ben.knoble+github@gmail.com","sentAt":"2025-08-05T01:28:58Z","receivedAt":"2025-08-05T01:29:11Z","isPatch":true,"sender":{"key":"ben.knoble@gmail.com","avatar":"https://avatars.githubusercontent.com/u/22802209?v=4"},"body":"On Mon, Aug 4, 2025 at 12:53 AM Junio C Hamano wrote:\n>\n> \"D. Ben Knoble\" writes:\n>\n> > This series enables --help-all outside of repository contexts,\n>\n> I've been familiar with \"git commit -h\" (and commands other than\n> \"commit\") outside a repository, but did not even know that \"git\n> commit --help-all\" didn't work outside. For commands that use\n> parse-options, these come from the same source of informatino, so it\n> does not make any sense for one to work and the other to refuse to\n> work. Good.\n>\n> > and\n> > allows -h with other arguments (without breaking existing ls-remote/grep\n> > usage).\n>\n> I somehow thought we already talked you out of this.\n\nYou did! Forgot to drop that segment from the cover letter.\n\n> Do you mean something like \"git add -h foo\" and \"git add -h -N foo\"\n> would say \"'git add foo' would add the current contents in foo to\n> the index\" and \"'git add -N foo' would make the index aware of the\n> path foo without actually adding its contents (yet)\"? I do not think\n> it makes much sense to behave exactly the same as \"git add -h\" when\n> the user says \"git add -h foo\" or \"git add -h -N foo\", as if we\n> didn't even see the extra things on the command line.\n\nSo this is moot. Good catch, thanks.\n"}]}