{"thread":{"id":"62814","subject":"[PATCH v3 0/6] Send help text from \"git cmd -h\" to stdout","startedAt":"2025-01-16T01:25:26Z","lastAt":"2025-01-18T11:42:55Z","messageCount":37,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":6},"messages":[{"id":"510624","messageId":"20250116012524.1557441-1-gitster@pobox.com","threadId":"62814","inReplyTo":null,"subject":"[PATCH v3 0/6] Send help text from \"git cmd -h\" to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:17Z","receivedAt":"2025-01-16T01:25:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonas Konrad noticed[*] that \"git branch -h\" started showing the help\ntext to the standard error stream. It turns out that we are fairly\ninconsistent in our implementation of \"git cmd -h\". The users of\nparse-options API will get \"If -h is the only option on the command\nline, give the help text to the standard output\" for free, but some\ncommands manually check for the condition and then call the\nusage_with_options() function, which gives the identical help text\nto the standard error stream. And \"git branch -h\" Jonas noticed was\none of them.\n\nOlder commands written before parse-options API became dominant show\nthe help text by calling the usage() function, which is meant to be\nused when they fail to parse their command line arguments, which has\nthe same problem. An explicit request for help text \"git cmd -h\"\nshould be fulfilled by showing the help on the standard output.\n\nThis series teachs \"git $cmd -h\" to send its help text to the\nstandard output stream consistently for built-in commands.\n\n\n[Reference]\n\nhttps://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/\n\nJeff King (1):\n t0012: optionally check that \"-h\" output goes to stdout\n\nJunio C Hamano (5):\n parse-options: add show_usage_help_and_exit_if_asked()\n builtins: send usage_with_options() help text to standard output\n usage: add show_usage_and_exit_if_asked()\n builtin: send usage() help text to standard output\n builtin: send usage() help text to standard output\n\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/check-ref-format.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/credential.c | 3 ++-\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/fetch-pack.c | 3 +++\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/ls-files.c | 4 ++--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/merge.c | 4 ++--\n builtin/pack-redundant.c | 3 +--\n builtin/rebase.c | 6 +++---\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-file.c | 8 ++++++--\n builtin/unpack-objects.c | 2 ++\n builtin/update-index.c | 4 ++--\n builtin/upload-archive.c | 6 +++---\n builtin/var.c | 1 +\n git-compat-util.h | 2 ++\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n t/helper/test-simple-ipc.c | 4 ++--\n t/t0012-help.sh | 3 ++-\n t/t7600-merge.sh | 2 +-\n usage.c | 28 +++++++++++++++++++++++++---\n 41 files changed, 123 insertions(+), 64 deletions(-)\n\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510625","messageId":"20250116012524.1557441-2-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v3 1/6] parse-options: add show_usage_help_and_exit_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:18Z","receivedAt":"2025-01-16T01:25:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Many commands call usage_with_options() when they are asked to give\nthe help message, but it incorrectly sends the help text to the\nstandard error stream. When the user asked for it with \"git cmd -h\",\nthe help message is the primary output from the command, hence we\nshould send it to the standard output stream.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage_with_options(usage, options);\n\nand replaces it with\n\n\tshow_usage_help_and_exit_if_asked(argc, argv, usage, options);\n\nto help correct code paths (there are 40 or so of them).\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it. After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nSuggested-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..8a8b934e67 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+void show_usage_help_and_exit_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, 0, 0);\n+\t\texit(129);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t const char * const *usagestr,\n \t\t const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d01361ca97..6af4ee148a 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_help_and_exit_if_asked(int ac, const char **av,\n+\t\t\t\t const char * const *usage,\n+\t\t\t\t const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510626","messageId":"20250116012524.1557441-3-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v3 2/6] t0012: optionally check that \"-h\" output goes to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:19Z","receivedAt":"2025-01-16T01:25:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King \n\nFor most commands, \"git foo -h\" will send the help output to stdout, as\nthis is what parse-options.c does. But some commands send it to stderr\ninstead. This is usually because they call usage_with_options(), and\nshould be switched to show_usage_help_and_exit_if_asked().\n\nCurrently t0012 is permissive and allows either behavior. We'd like it\nto eventually enforce that help goes to stdout, and teaching it to do so\nidentifies the commands that need to be changed. But during the\ntransition period, we don't want to enforce that for most test runs.\n\nSo let's introduce a flag that will let most test runs use the\npermissive behavior, and people interested in converting commands can\nrun:\n\n GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh\n\nto see the failures. Eventually (when all builtins have been converted)\nwe'll remove this flag entirely and always check the strict behavior.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/t0012-help.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..9c7ae9fd36 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -255,9 +255,16 @@ do\n \t\t(\n \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\ttest_grep usage output\n+\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n+\t\tthen\n+\t\t\ttest_must_be_empty err &&\n+\t\t\ttest_grep usage output\n+\t\telse\n+\t\t\ttest_grep usage output ||\n+\t\t\ttest_grep usage err\n+\t\tfi\n \t'\n done \n---\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/ls-files.c | 4 ++--\n builtin/merge.c | 4 ++--\n builtin/rebase.c | 6 +++---\n builtin/update-index.c | 4 ++--\n t/t7600-merge.sh | 2 +-\n 13 files changed, 29 insertions(+), 30 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1338b606fe..0801b556c2 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2427,8 +2427,7 @@ int cmd_am(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv, usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..366729a78b 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -784,8 +784,8 @@ int cmd_branch(int argc,\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_branch_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_branch_usage, options);\n \n \t/*\n \t * Try to set sort keys from config. If config does not set any,\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex b81002a1df..7093d1efd5 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(checkout_worker_usage,\n-\t\t\t\t checkout_worker_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t checkout_worker_usage,\n+\t\t\t\t\t checkout_worker_options);\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, checkout_worker_options,\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a81501098d..d928d6b5e3 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_checkout_index_usage,\n-\t\t\t\t builtin_checkout_index_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_checkout_index_usage,\n+\t\t\t\t\t builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 2ca1a57ebb..2efc224d32 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(commit_tree_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t commit_tree_usage, options);\n \n \targc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex ef5e622c07..4268915120 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_status_usage, builtin_status_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_commit_usage, builtin_commit_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 029dc64d6c..dabf190bbe 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_fsmonitor__daemon_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_fsmonitor__daemon_usage, options);\n \n \tdie(_(\"fsmonitor--daemon not supported on this platform\"));\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de2..5f831e1f94 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -710,8 +710,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_gc_usage, builtin_gc_options);\n \n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 15499cd12b..9efe92b7c0 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,\n \t};\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t ls_files_usage, builtin_ls_files_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5f67007bba..95d798fc89 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,\n \tvoid *branch_to_free;\n \tint orig_argc = argc;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_merge_usage, builtin_merge_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0498fff3c9..cb49323c44 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,\n \t};\n \tint i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_rebase_usage,\n-\t\t\t\t builtin_rebase_options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t builtin_rebase_usage,\n+\t\t\t\t\t builtin_rebase_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 74bbad9f87..b0e2ad4970 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(update_index_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t update_index_usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex ef54cff4fa..2a8df29219 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '\n \t\tcd broken &&\n \t\tgit init &&\n \t\t>.git/index &&\n-\t\ttest_expect_code 129 git merge -h 2>usage\n+\t\ttest_expect_code 129 git merge -h >usage\n \t) &&\n \ttest_grep \"[Uu]sage: git merge\" broken/usage\n '\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510628","messageId":"20250116012524.1557441-5-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:21Z","receivedAt":"2025-01-16T01:25:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some commands call usage() when they are asked to give the help\nmessage with \"git cmd -h\", but this has the same problem as we\nfixed with callers of usage_with_options() for the same purpose.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(usage);\n\nand replaces it with\n\n\tshow_usage_and_exit_if_asked(argc, argv, usage);\n\nto help correct these code paths.\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it. After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nSigned-off-by: Junio C Hamano \n---\n git-compat-util.h | 2 ++\n usage.c | 28 +++++++++++++++++++++++++---\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e283c46c6f..6edf50bc95 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+void show_usage_and_exit_if_asked(int ac, const char **av, const char *err);\n+\n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\n #include \"compat/apple-common-crypto.h\"\ndiff --git a/usage.c b/usage.c\nindex 47709006c1..26cd54a511 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,7 +8,7 @@\n #include \"gettext.h\"\n #include \"trace2.h\"\n \n-static void vreportf(const char *prefix, const char *err, va_list params)\n+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n@@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n \t}\n \n \t*(p++) = '\\n'; /* we no longer need a NUL */\n-\tfflush(stderr);\n-\twrite_in_full(2, msg, p - msg);\n+\tif (fd == 2)\n+\t\tfflush(stderr);\n+\twrite_in_full(fd, msg, p - msg);\n+}\n+\n+static void vreportf(const char *prefix, const char *err, va_list params)\n+{\n+\tvfdreportf(2, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -173,6 +179,22 @@ void NORETURN usage(const char *err)\n \tusagef(\"%s\", err);\n }\n \n+static void show_usage_and_exit_if_asked_helper(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tvfdreportf(1, _(\"usage: \"), err, params);\n+\tva_end(params);\n+\texit(129);\n+}\n+\n+void show_usage_and_exit_if_asked(int ac, const char **av, const char *err)\n+{\n+\tif (ac == 2 && !strcmp(av[1], \"-h\"))\n+\t\tshow_usage_and_exit_if_asked_helper(err);\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510629","messageId":"20250116012524.1557441-6-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v3 5/6] oddballs: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:22Z","receivedAt":"2025-01-16T01:25:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_and_exit_if_asked() helper we introduced\nearlier, fix callers of usage() that want to show the help text when\nexplicitly asked by the end-user. The help text now goes to the\nstandard output stream for them.\n\nThe callers in this step are oddballs in that their invocations of\nusage() are *not* guarded by\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nThey are (unnecessarily) being clever and do things like\n\n\tif (argc != 2 || !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nto say \"I know I take only one argument, so argc != 2 is always an\nerror regardless of what is in argv[]. Ah, by the way, even if argc\nis 2, \"-h\" is a request for usage text, so we do the same\". Some\njust do not treat \"-h\" any specially, and let it take the same error\ncode paths as a parameter error.\n\nNow we cannot do the same, so these callers are rewrittin to do the\nshow_usage_and_exit_if_asked() first and then handle the usage error\nthe way they used to.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/credential.c | 3 ++-\n builtin/unpack-file.c | 8 ++++++--\n builtin/upload-archive.c | 3 ++-\n builtin/var.c | 1 +\n 4 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/credential.c b/builtin/credential.c\nindex 14c8c6608b..8a8cffecc8 100644\n--- a/builtin/credential.c\n+++ b/builtin/credential.c\n@@ -18,7 +18,8 @@ int cmd_credential(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_and_exit_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n \t\tusage(usage_msg);\n \top = argv[1];\n \ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 6da2825753..307351af55 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)\n \treturn path;\n }\n \n+static const char usage_msg[] =\n+\"git unpack-file \";\n+\n int cmd_unpack_file(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,\n {\n \tstruct object_id oid;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git unpack-file \");\n+\tshow_usage_and_exit_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n+\t\tusage(usage_msg);\n \tif (repo_get_oid(the_repository, argv[1], &oid))\n \t\tdie(\"Not a valid object name %s\", argv[1]);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 9e9343f121..3b282d41e6 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,\n \tconst char *arg_cmd = \"argument \";\n \tint ret;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_and_exit_if_asked(argc, argv, upload_archive_usage);\n+\tif (argc != 2)\n \t\tusage(upload_archive_usage);\n \n \tif (!enter_repo(argv[1], 0))\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 1449656cc9..6a09c1c39a 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -221,6 +221,7 @@ int cmd_var(int argc,\n \tconst struct git_var *git_var;\n \tchar *val;\n \n+\tshow_usage_and_exit_if_asked(argc, argv, var_usage);\n \tif (argc != 2)\n \t\tusage(var_usage);\n \n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510630","messageId":"20250116012524.1557441-7-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v3 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T01:25:23Z","receivedAt":"2025-01-16T01:25:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_and_exit_if_asked() helper we introduced\nearlier, fix callers of usage() that want to show the help text when\nexplicitly asked by the end-user. The help text now goes to the\nstandard output stream for them.\n\nThese are the bog standard \"if we got only '-h', then that is a\nrequest for help\" callers. Their\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(message);\n\nare simply replaced with\n\n\tshow_usage_and_exit_if_asked(argc, argv, message);\n\nWith this, the built-ins tested by t0012 all send their help text to\ntheir standard output stream, so the check in t0012 that was half\ntightened earlier is now fully tightened to insist on standard error\nstream being empty.\n\nSigned-off-by: Junio C Hamano \n---\n builtin/check-ref-format.c | 4 ++--\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/fetch-pack.c | 3 +++\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/pack-redundant.c | 3 +--\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-objects.c | 2 ++\n builtin/upload-archive.c | 3 +--\n t/helper/test-simple-ipc.c | 4 ++--\n t/t0012-help.sh | 10 ++--------\n 21 files changed, 42 insertions(+), 34 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex cef1ffe3ce..acc4366d85 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_check_ref_format_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv,\n+\t\t\t\t builtin_check_ref_format_usage);\n \n \tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n \t\treturn check_ref_format_branch(argv[2]);\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 604b04bb2c..f8816e0d9e 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,\n \tint result;\n \tunsigned options = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_files_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex ebc824602e..a1759b8291 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,\n \tint i;\n \tint result;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_cache_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, diff_cache_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 40804e7b48..fc7c026555 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,\n \tint read_stdin = 0;\n \tint merge_base = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_tree_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, diff_tree_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 0f86392761..e1f6bfdc73 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,\n {\n \tunsigned int i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(fast_import_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, fast_import_usage);\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_pack_config();\ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bed2816c2d..9bd4b29c5b 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,\n \t\t\tlist_objects_filter_set_no_filter(&args.filter_options);\n \t\t\tcontinue;\n \t\t}\n+\n+\t\tif (!strcmp(arg, \"-h\"))\n+\t\t\tshow_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);\n \t\tusage(fetch_pack_usage);\n \t}\n \tif (deepen_not.nr)\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 6bec0d1854..033a205ba4 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =\n #define HEADERSIZE (2 * RECORDSIZE)\n \n int cmd_get_tar_commit_id(int argc,\n-\t\t\t const char **argv UNUSED,\n+\t\t\t const char **argv,\n \t\t\t const char *prefix,\n \t\t\t struct repository *repo UNUSED)\n {\n@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, builtin_get_tar_commit_id_usage);\n+\n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0b62b2589f..92ef59fad5 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,\n \t */\n \tfetch_if_missing = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(index_pack_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, index_pack_usage);\n \n \tdisable_replace_refs();\n \tfsck_options.walk = mark_link;\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 41dd304731..9e11bffdfb 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, git_mailsplit_usage);\n+\n \tfor (argp = argv+1; *argp; argp++) {\n \t\tconst char *arg = *argp;\n \n@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,\n \t\t\tcontinue;\n \t\t} else if ( arg[1] == 'f' ) {\n \t\t\tnr = strtol(arg+2, NULL, 10);\n-\t\t} else if ( arg[1] == 'h' ) {\n-\t\t\tusage(git_mailsplit_usage);\n \t\t} else if ( arg[1] == 'b' && !arg[2] ) {\n \t\t\tallow_bare = 1;\n \t\t} else if (!strcmp(arg, \"--keep-cr\")) {\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex 342699edb7..f243a55d32 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -75,6 +75,9 @@ static void merge_all(void)\n \t}\n }\n \n+static const char usage_string[] =\n+\"git merge-index [-o] [-q] (-a | [--] [...])\";\n+\n int cmd_merge_index(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, usage_string);\n+\n \tif (argc < 3)\n-\t\tusage(\"git merge-index [-o] [-q] (-a | [--] [...])\");\n+\t\tusage(usage_string);\n \n \trepo_read_index(the_repository);\n \ndiff --git a/builtin/merge-ours.c b/builtin/merge-ours.c\nindex 3ecd9172f1..ab78fffb52 100644\n--- a/builtin/merge-ours.c\n+++ b/builtin/merge-ours.c\n@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,\n \t\t const char *prefix UNUSED,\n \t\t struct repository *repo UNUSED)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_merge_ours_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, builtin_merge_ours_usage);\n \n \t/*\n \t * The contents of the current index becomes the tree we\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 1dd295558b..b71c7e1c2f 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,6 +38,12 @@ 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+\t\tstruct strbuf msg = STRBUF_INIT;\n+\t\tstrbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);\n+\t\tshow_usage_and_exit_if_asked(argc, argv, msg.buf);\n+\t}\n+\n \tif (argc < 4)\n \t\tusagef(builtin_merge_recursive_usage, argv[0]);\n \ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex e046575871..ce2e1fde8d 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \tstruct strbuf idx_name = STRBUF_INIT;\n \tchar buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \\n + \\0 */\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(pack_redundant_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, pack_redundant_usage);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 33c8ae0fc7..f7591e7701 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,\n {\n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, usage_msg);\n+\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/remote-fd.c b/builtin/remote-fd.c\nindex ae896eda57..1a1aa65273 100644\n--- a/builtin/remote-fd.c\n+++ b/builtin/remote-fd.c\n@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, usage_msg);\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3196da7b2d..5a21d57c43 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,\n \tconst char *show_progress = NULL;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(rev_list_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, rev_list_usage);\n \n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 949747a6b6..f4c2b1b000 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n+\tshow_usage_and_exit_if_asked(argc, argv, builtin_rev_parse_usage);\n+\n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n \ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 2197d6d933..1f1fb8e682 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,\n \n \tquiet = !isatty(2);\n \n+\tshow_usage_and_exit_if_asked(argc, argv, unpack_usage);\n+\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 3b282d41e6..42d8f67174 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -93,8 +93,7 @@ struct repository *repo UNUSED)\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(upload_archive_usage);\n+\tshow_usage_and_exit_if_asked(argc, argv, upload_archive_usage);\n \n \t/*\n \t * Set up sideband subprocess.\ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex fb5927775d..d7a41cd053 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)\n \tif (argc < 2)\n \t\tusage_with_options(simple_ipc_usage, options);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(simple_ipc_usage, options);\n+\tshow_usage_help_and_exit_if_asked(argc, argv,\n+\t\t\t\t\t simple_ipc_usage, options);\n \n \tif (argc == 2 && !strcmp(argv[1], \"SUPPORTS_SIMPLE_IPC\"))\n \t\treturn 0;\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 9c7ae9fd36..d3a0967e9d 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -257,14 +257,8 @@ do\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n \t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n-\t\tthen\n-\t\t\ttest_must_be_empty err &&\n-\t\t\ttest_grep usage output\n-\t\telse\n-\t\t\ttest_grep usage output ||\n-\t\t\ttest_grep usage err\n-\t\tfi\n+\t\ttest_must_be_empty err &&\n+\t\ttest_grep usage output\n \t'\n done Introduce a helper function that captures the common pattern\n> \n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage(usage);\n> \n> and replaces it with\n> \n> \tshow_usage_and_exit_if_asked(argc, argv, usage);\n> \n> to help correct these code paths.\n\nI found the name hard to distinguish from the earlier helper,\nshow_usage_help_and_exit_if_asked(). The difference is that one takes\nonly a usage string, and the other takes the usage string along with\noptions. Maybe the other should be:\n\n show_usage_and_exit_if_asked_with_options();\n\n? It just keeps getting longer and longer...\n\nI think the \"and_exit\" could probably be implied, since showing the\nusage is always a final thing (just like in usage() and\nusage_with_options()). So:\n\n show_usage_if_asked();\n show_usage_with_options_if_asked();\n\n? I dunno. We are in deep bikeshed territory. I otherwise like what the\npatch is doing. ;)\n\n> -static void vreportf(const char *prefix, const char *err, va_list params)\n> +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n> {\n> \tchar msg[4096];\n> \tchar *p, *pend = msg + sizeof(msg);\n> @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n> \t}\n> \n> \t*(p++) = '\\n'; /* we no longer need a NUL */\n> -\tfflush(stderr);\n> -\twrite_in_full(2, msg, p - msg);\n> +\tif (fd == 2)\n> +\t\tfflush(stderr);\n> +\twrite_in_full(fd, msg, p - msg);\n> +}\n\nGross. :) I think the existing code is conceptually:\n\n write_in_full(fileno(stderr), msg, p - msg);\n\nIn which case vfreportf() could just take a FILE*, flush it and then\nwrite.\n\nMy main motivation is being less ugly, but I think it would also\npotentially prevent a bug. If there was unflushed data in stdout as we\ngo into vdreportf(), then we'd get an out of order write. That's why the\nfflush() is there in the first place (it's of course weird that we are\nusing write() in the first place, but IIRC it's for avoiding sheared\nwrites of error messages).\n\n-Peff\n"},{"id":"510676","messageId":"20250116104219.GC773990@coredump.intra.peff.net","threadId":"62814","inReplyTo":"20250116012524.1557441-6-gitster@pobox.com","subject":"Re: [PATCH v3 5/6] oddballs: send usage() help text to standard output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:42:19Z","receivedAt":"2025-01-16T10:42:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 05:25:22PM -0800, Junio C Hamano wrote:\n\n> Using the show_usage_and_exit_if_asked() helper we introduced\n> earlier, fix callers of usage() that want to show the help text when\n> explicitly asked by the end-user. The help text now goes to the\n> standard output stream for them.\n> \n> The callers in this step are oddballs in that their invocations of\n> usage() are *not* guarded by\n> \n> \tif (argc == 2 && !strcmp(argv[1], \"-h\")\n> \t\tusage(...);\n> \n> They are (unnecessarily) being clever and do things like\n> \n> \tif (argc != 2 || !strcmp(argv[1], \"-h\")\n> \t\tusage(...);\n> \n> to say \"I know I take only one argument, so argc != 2 is always an\n> error regardless of what is in argv[]. Ah, by the way, even if argc\n> is 2, \"-h\" is a request for usage text, so we do the same\". Some\n> just do not treat \"-h\" any specially, and let it take the same error\n> code paths as a parameter error.\n\nAs the author of at least one of these, I feel judged. :)\n\nBut yes, untangling this is obviously good (especially if we change the\nexit code later!). And pulling these into their own commit made\nreviewing much easier.\n\n> diff --git a/builtin/var.c b/builtin/var.c\n> index 1449656cc9..6a09c1c39a 100644\n> --- a/builtin/var.c\n> +++ b/builtin/var.c\n> @@ -221,6 +221,7 @@ int cmd_var(int argc,\n> \tconst struct git_var *git_var;\n> \tchar *val;\n> \n> +\tshow_usage_and_exit_if_asked(argc, argv, var_usage);\n> \tif (argc != 2)\n> \t\tusage(var_usage);\n\nHmm, what's going on in this one? It does not check \"-h\" at all.\n\nAh, I see. It simply hits the get_git_var() call, sees there is no var\ncalled \"-h\", and exits with an error. So checking for \"-h\" up front is\ncorrect. It is an oddball, though not quite the same as the others (the\noddest ball?).\n\n-Peff\n"},{"id":"510677","messageId":"20250116104414.GD773990@coredump.intra.peff.net","threadId":"62814","inReplyTo":"20250116103620.GB773990@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:44:14Z","receivedAt":"2025-01-16T10:44:16Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2025 at 05:36:20AM -0500, Jeff King wrote:\n\n> > -static void vreportf(const char *prefix, const char *err, va_list params)\n> > +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n> > {\n> > \tchar msg[4096];\n> > \tchar *p, *pend = msg + sizeof(msg);\n> > @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n> > \t}\n> > \n> > \t*(p++) = '\\n'; /* we no longer need a NUL */\n> > -\tfflush(stderr);\n> > -\twrite_in_full(2, msg, p - msg);\n> > +\tif (fd == 2)\n> > +\t\tfflush(stderr);\n> > +\twrite_in_full(fd, msg, p - msg);\n> > +}\n\nOh, I meant to mention also: I wondered about the other references to\nstderr in this function. But we hit those only when we fail to format\nthe error message that was asked for, so continuing to send them to\nstderr is the right thing (i.e., your patch is right but I wanted to\nhelp out other reviewers).\n\n-Peff\n"},{"id":"510678","messageId":"20250116104612.GE773990@coredump.intra.peff.net","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"Re: [PATCH v3 0/6] Send help text from \"git cmd -h\" to stdout","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-16T10:46:12Z","receivedAt":"2025-01-16T10:46:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jan 15, 2025 at 05:25:17PM -0800, Junio C Hamano wrote:\n\n> Jonas Konrad noticed[*] that \"git branch -h\" started showing the help\n> text to the standard error stream. It turns out that we are fairly\n> inconsistent in our implementation of \"git cmd -h\". The users of\n> parse-options API will get \"If -h is the only option on the command\n> line, give the help text to the standard output\" for free, but some\n> commands manually check for the condition and then call the\n> usage_with_options() function, which gives the identical help text\n> to the standard error stream. And \"git branch -h\" Jonas noticed was\n> one of them.\n> \n> Older commands written before parse-options API became dominant show\n> the help text by calling the usage() function, which is meant to be\n> used when they fail to parse their command line arguments, which has\n> the same problem. An explicit request for help text \"git cmd -h\"\n> should be fulfilled by showing the help on the standard output.\n> \n> This series teachs \"git $cmd -h\" to send its help text to the\n> standard output stream consistently for built-in commands.\n\nI had a small complaint in patch 4, but otherwise this looks good to me.\n\nIf we want to switch the exit code for this case from 129 to 0, I think\nwe could easily do so on top (it would need modifications in three\nplaces, but now that you've untangled all of the individual builtins,\nthat would get all of them).\n\nI guess there may be non-builtins that would need to be handled\nindividually, though. We don't have too many of them these days, but\nthey are not covered by t0012.\n\n-Peff\n"},{"id":"510717","messageId":"xmqqbjw6u101.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116103620.GB773990@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T17:22:54Z","receivedAt":"2025-01-16T17:22:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> \tshow_usage_and_exit_if_asked(argc, argv, usage);\n>> \n>> to help correct these code paths.\n>\n> I found the name hard to distinguish from the earlier helper,\n\nAs we all know that usage_with_options() and usage() exits,\n\n show_usage_with_options_if_asked()\n show_usage_if_asked()\n\nmay be good enough, I wonder? \"show\" -> \"help\" may give us better\nnames.\n\n> I think the \"and_exit\" could probably be implied, since showing the\n> usage is always a final thing (just like in usage() and\n> usage_with_options()). So:\n>\n> show_usage_if_asked();\n> show_usage_with_options_if_asked();\n\nAh, We came to the same conclusion. The only thing we haven't made\nit obvious is that \"show\" implies the output goes to the standard\noutput stream, while without \"show\", the output goes to the standard\nerror stream. I wonder if these help better:\n\n show_help_if_asked();\n show_help_with_options_if_asked()\n\n\"show help\" explains the output goes to where the \"help\" message\nwould go ;-)\n\nBut probably \"if-asked\" is clear enough indication that the output\nis made in response to an explicit end-user request, so let's take\nthe show_(usage|usage_with_options)_if_asked as the final names.\n\n>> -static void vreportf(const char *prefix, const char *err, va_list params)\n>> +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n>> {\n>> \tchar msg[4096];\n>> \tchar *p, *pend = msg + sizeof(msg);\n>> @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n>> \t}\n>> \n>> \t*(p++) = '\\n'; /* we no longer need a NUL */\n>> -\tfflush(stderr);\n>> -\twrite_in_full(2, msg, p - msg);\n>> +\tif (fd == 2)\n>> +\t\tfflush(stderr);\n>> +\twrite_in_full(fd, msg, p - msg);\n>> +}\n>\n> Gross. :) I think the existing code is conceptually:\n>\n> write_in_full(fileno(stderr), msg, p - msg);\n>\n> In which case vfreportf() could just take a FILE*, flush it and then\n> write.\n\nSure, but these \"stderr\" are real error reporting that need to stay\nto be stderr, and flush needs to be done only when our true payload\ngoes to fd#2 and I do not think these fflush() are about stdio calls\nmade by the caller _before_ it called this function. It may become\na bit tricky to read the resulting code if we pass \"FILE *\".\n"},{"id":"510718","messageId":"xmqq7c6uu0y6.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116104219.GC773990@coredump.intra.peff.net","subject":"Re: [PATCH v3 5/6] oddballs: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T17:24:01Z","receivedAt":"2025-01-16T17:24:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> .... Some\n>> just do not treat \"-h\" any specially, and let it take the same error\n>> code paths as a parameter error.\n> ...\n>> +\tshow_usage_and_exit_if_asked(argc, argv, var_usage);\n>> \tif (argc != 2)\n>> \t\tusage(var_usage);\n>\n> Hmm, what's going on in this one? It does not check \"-h\" at all.\n\nYes, this is the one that let the usual error code paths to treat it\nas a mere parameter error.\n"},{"id":"510719","messageId":"xmqq34hiu0rh.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116104612.GE773990@coredump.intra.peff.net","subject":"Re: [PATCH v3 0/6] Send help text from \"git cmd -h\" to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T17:28:02Z","receivedAt":"2025-01-16T17:28:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we want to switch the exit code for this case from 129 to 0, I think\n> we could easily do so on top (it would need modifications in three\n> places, but now that you've untangled all of the individual builtins,\n> that would get all of them).\n\nYes, I consider it pretty much an orthogonal issue to update the\nexit status.\n\n> I guess there may be non-builtins that would need to be handled\n> individually, though. We don't have too many of them these days, but\n> they are not covered by t0012.\n\nYes. We can probably leave them as they are, as we have established\nour expectation with this series (i.e. an explicit end user request\nfor help text should emit to the standard output stream), so any bug\nreport can be handled without needing any policy discussion. \"Hey,\nI noticed 'git cmd -h' writes to stderr, not stdout\"---\"Thanks, cmd\nneed to be fixed\".\n"},{"id":"510720","messageId":"xmqqv7uesm26.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116012524.1557441-7-gitster@pobox.com","subject":"Re: [PATCH v3 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T17:30:57Z","receivedAt":"2025-01-16T17:30:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Using the show_usage_and_exit_if_asked() helper we introduced\n> earlier, fix callers of usage() that want to show the help text when\n> explicitly asked by the end-user. The help text now goes to the\n> standard output stream for them.\n>\n> These are the bog standard \"if we got only '-h', then that is a\n> request for help\" callers. Their\n>\n> \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n> \t\tusage(message);\n>\n> are simply replaced with\n>\n> \tshow_usage_and_exit_if_asked(argc, argv, message);\n> ...\n\nThe above is a bit of a lie. There is one strange thing I did,\nwhich needs to be redone.\n\n> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n> index bed2816c2d..9bd4b29c5b 100644\n> --- a/builtin/fetch-pack.c\n> +++ b/builtin/fetch-pack.c\n> @@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,\n> \t\t\tlist_objects_filter_set_no_filter(&args.filter_options);\n> \t\t\tcontinue;\n> \t\t}\n> +\n> +\t\tif (!strcmp(arg, \"-h\"))\n> +\t\t\tshow_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);\n> \t\tusage(fetch_pack_usage);\n> \t}\n> \tif (deepen_not.nr)\n\nI think we should just call show_usage_and_exit_if_asked() before\nentering the loop without changing anything else.\n"},{"id":"510730","messageId":"xmqqy0zapka5.fsf@gitster.g","threadId":"62814","inReplyTo":"xmqqv7uesm26.fsf@gitster.g","subject":"Re: [PATCH v3 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T20:37:38Z","receivedAt":"2025-01-16T20:37:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> The above is a bit of a lie. There is one strange thing I did,\n> which needs to be redone.\n>\n>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\n>> index bed2816c2d..9bd4b29c5b 100644\n>> --- a/builtin/fetch-pack.c\n>> +++ b/builtin/fetch-pack.c\n>> @@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,\n>> \t\t\tlist_objects_filter_set_no_filter(&args.filter_options);\n>> \t\t\tcontinue;\n>> \t\t}\n>> +\n>> +\t\tif (!strcmp(arg, \"-h\"))\n>> +\t\t\tshow_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);\n>> \t\tusage(fetch_pack_usage);\n>> \t}\n>> \tif (deepen_not.nr)\n>\n> I think we should just call show_usage_and_exit_if_asked() before\n> entering the loop without changing anything else.\n\nI have worked on a reroll and moved this piece to the \"oddball\"\npile.\n"},{"id":"510734","messageId":"20250116213553.2563751-1-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116012524.1557441-1-gitster@pobox.com","subject":"[PATCH v4 0/6] Send help text from \"git cmd -h\" to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:47Z","receivedAt":"2025-01-16T21:35:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonas Konrad noticed[*] that \"git branch -h\" started showing the help\ntext to the standard error stream. It turns out that we are fairly\ninconsistent in our implementation of \"git cmd -h\". The users of\nparse-options API will get \"If -h is the only option on the command\nline, give the help text to the standard output\" for free, but some\ncommands manually check for the condition and then call the\nusage_with_options() function, which gives the identical help text\nto the standard error stream. And \"git branch -h\" Jonas noticed was\none of them.\n\nOlder commands written before parse-options API became dominant show\nthe help text by calling the usage() function, which is meant to be\nused when they fail to parse their command line arguments, which has\nthe same problem. An explicit request for help text \"git cmd -h\"\nshould be fulfilled by showing the help on the standard output.\n\nThis series teachs \"git $cmd -h\" to send its help text to the\nstandard output stream consistently for built-in commands.\n\nThere is a related \"what status should these command exit with?\"\nquestion, and an argument can be made to switch them to exit with 0,\ninstead of historical 129, but it is left outside of the series for\nnow.\n\n\nChanges since v3:\n\n - The helper functions for usage_with_options() and usage() are\n renamed to show_usage_with_options_if_asked() and\n show_usage_if_asked(), respectively.\n\n - The call to usage() in builtin/fetch-pack.c turns out to be the\n same class as builtin/var.c that simply treats \"-h\" as an\n unrecognised command line option. The fix was rewritten to\n follow the pattern taken to fix \"git var -h\" and moved to the\n \"oddball\" step.\n\n - The patches are reordered; the first three patches prepare the\n new helper functions and ways to test their effect, and the\n remaining patches convert the commands to use the new helper\n functions.\n\n[Reference]\n\nhttps://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/\n\n\nJeff King (1):\n t0012: optionally check that \"-h\" output goes to stdout\n\nJunio C Hamano (5):\n parse-options: add show_usage_with_options_if_asked()\n usage: add show_usage_if_asked()\n builtins: send usage_with_options() help text to standard output\n oddballs: send usage() help text to standard output\n builtin: send usage() help text to standard output\n\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/check-ref-format.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/credential.c | 3 ++-\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/fetch-pack.c | 2 ++\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/ls-files.c | 4 ++--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/merge.c | 4 ++--\n builtin/pack-redundant.c | 3 +--\n builtin/rebase.c | 6 +++---\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-file.c | 8 ++++++--\n builtin/unpack-objects.c | 2 ++\n builtin/update-index.c | 4 ++--\n builtin/upload-archive.c | 6 +++---\n builtin/var.c | 1 +\n git-compat-util.h | 2 ++\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n t/helper/test-simple-ipc.c | 4 ++--\n t/t0012-help.sh | 3 ++-\n t/t7600-merge.sh | 2 +-\n usage.c | 28 +++++++++++++++++++++++++---\n 41 files changed, 122 insertions(+), 64 deletions(-)\n\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510735","messageId":"20250116213553.2563751-2-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v4 1/6] t0012: optionally check that \"-h\" output goes to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:48Z","receivedAt":"2025-01-16T21:35:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King \n\nFor most commands, \"git foo -h\" will send the help output to stdout, as\nthis is what parse-options.c does. But some commands send it to stderr\ninstead. This is usually because they call usage_with_options(), and\nshould be switched to show_usage_help_and_exit_if_asked().\n\nCurrently t0012 is permissive and allows either behavior. We'd like it\nto eventually enforce that help goes to stdout, and teaching it to do so\nidentifies the commands that need to be changed. But during the\ntransition period, we don't want to enforce that for most test runs.\n\nSo let's introduce a flag that will let most test runs use the\npermissive behavior, and people interested in converting commands can\nrun:\n\n GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh\n\nto see the failures. Eventually (when all builtins have been converted)\nwe'll remove this flag entirely and always check the strict behavior.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n * Same as before.\n\n t/t0012-help.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..9c7ae9fd36 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -255,9 +255,16 @@ do\n \t\t(\n \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\ttest_grep usage output\n+\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n+\t\tthen\n+\t\t\ttest_must_be_empty err &&\n+\t\t\ttest_grep usage output\n+\t\telse\n+\t\t\ttest_grep usage output ||\n+\t\t\ttest_grep usage err\n+\t\tfi\n \t'\n done \nSigned-off-by: Junio C Hamano \n---\n * Helper function renamed.\n\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..4051a8e1d1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+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, 0, 0);\n+\t\texit(129);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t const char * const *usagestr,\n \t\t const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d01361ca97..39f0886254 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_with_options_if_asked(int ac, const char **av,\n+\t\t\t\t const char * const *usage,\n+\t\t\t\t const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510737","messageId":"20250116213553.2563751-4-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v4 3/6] usage: add show_usage_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:50Z","receivedAt":"2025-01-16T21:36:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some commands call usage() when they are asked to give the help\nmessage with \"git cmd -h\", but this has the same problem as we\nfixed with callers of usage_with_options() for the same purpose.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(usage);\n\nand replaces it with\n\n\tshow_usage_if_asked(argc, argv, usage);\n\nto help correct these code paths.\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it. After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nSigned-off-by: Junio C Hamano \n---\n * Helper function renamed.\n\n git-compat-util.h | 2 ++\n usage.c | 28 +++++++++++++++++++++++++---\n 2 files changed, 27 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e283c46c6f..d43dd248c4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+void show_usage_if_asked(int ac, const char **av, const char *err);\n+\n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\n #include \"compat/apple-common-crypto.h\"\ndiff --git a/usage.c b/usage.c\nindex 47709006c1..d9f323a0bd 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,7 +8,7 @@\n #include \"gettext.h\"\n #include \"trace2.h\"\n \n-static void vreportf(const char *prefix, const char *err, va_list params)\n+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n@@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n \t}\n \n \t*(p++) = '\\n'; /* we no longer need a NUL */\n-\tfflush(stderr);\n-\twrite_in_full(2, msg, p - msg);\n+\tif (fd == 2)\n+\t\tfflush(stderr);\n+\twrite_in_full(fd, msg, p - msg);\n+}\n+\n+static void vreportf(const char *prefix, const char *err, va_list params)\n+{\n+\tvfdreportf(2, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -173,6 +179,22 @@ void NORETURN usage(const char *err)\n \tusagef(\"%s\", err);\n }\n \n+static void show_usage_if_asked_helper(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tvfdreportf(1, _(\"usage: \"), err, params);\n+\tva_end(params);\n+\texit(129);\n+}\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+\t\tshow_usage_if_asked_helper(err);\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510738","messageId":"20250116213553.2563751-5-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v4 4/6] builtins: send usage_with_options() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:51Z","receivedAt":"2025-01-16T21:36:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_with_options_if_asked() helper we introduced\nearlier, fix callers of usage_with_options() that want to show the\nhelp text when explicitly asked by the end-user. The help text now\ngoes to the standard output stream for them.\n\nThe test in t7600 for \"git merge -h\" may want to be retired, as the\nsame is covered by t0012 already, but it is specifically testing that\nthe \"-h\" option gets a response even with a corrupt index file, so\nfor now let's leave it there.\n\nSigned-off-by: Junio C Hamano \n---\n * Helper function renamed.\n\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/ls-files.c | 4 ++--\n builtin/merge.c | 4 ++--\n builtin/rebase.c | 6 +++---\n builtin/update-index.c | 4 ++--\n t/helper/test-simple-ipc.c | 4 ++--\n t/t7600-merge.sh | 2 +-\n 14 files changed, 31 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1338b606fe..8d8f38fa1d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2427,8 +2427,7 @@ int cmd_am(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv, usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..7c8b4b65b6 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -784,8 +784,8 @@ int cmd_branch(int argc,\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_branch_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_branch_usage, options);\n \n \t/*\n \t * Try to set sort keys from config. If config does not set any,\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex b81002a1df..da9345a44b 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(checkout_worker_usage,\n-\t\t\t\t checkout_worker_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t checkout_worker_usage,\n+\t\t\t\t\t checkout_worker_options);\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, checkout_worker_options,\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a81501098d..e30086c7d4 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_checkout_index_usage,\n-\t\t\t\t builtin_checkout_index_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_checkout_index_usage,\n+\t\t\t\t\t builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 2ca1a57ebb..38457600a4 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(commit_tree_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t commit_tree_usage, options);\n \n \targc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex ef5e622c07..c84062bfc1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_status_usage, builtin_status_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_commit_usage, builtin_commit_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 029dc64d6c..0820e524f1 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_fsmonitor__daemon_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_fsmonitor__daemon_usage, options);\n \n \tdie(_(\"fsmonitor--daemon not supported on this platform\"));\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de2..ea007c8e7e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -710,8 +710,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_gc_usage, builtin_gc_options);\n \n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 15499cd12b..a4431429b7 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,\n \t};\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t ls_files_usage, builtin_ls_files_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5f67007bba..ba9faf126a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,\n \tvoid *branch_to_free;\n \tint orig_argc = argc;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_merge_usage, builtin_merge_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0498fff3c9..6c9eaf3788 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,\n \t};\n \tint i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_rebase_usage,\n-\t\t\t\t builtin_rebase_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_rebase_usage,\n+\t\t\t\t\t builtin_rebase_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 74bbad9f87..b2f6b1a3fb 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(update_index_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t update_index_usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex fb5927775d..03cc5eea2c 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)\n \tif (argc < 2)\n \t\tusage_with_options(simple_ipc_usage, options);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(simple_ipc_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t simple_ipc_usage, options);\n \n \tif (argc == 2 && !strcmp(argv[1], \"SUPPORTS_SIMPLE_IPC\"))\n \t\treturn 0;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex ef54cff4fa..2a8df29219 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '\n \t\tcd broken &&\n \t\tgit init &&\n \t\t>.git/index &&\n-\t\ttest_expect_code 129 git merge -h 2>usage\n+\t\ttest_expect_code 129 git merge -h >usage\n \t) &&\n \ttest_grep \"[Uu]sage: git merge\" broken/usage\n '\n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510739","messageId":"20250116213553.2563751-6-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v4 5/6] oddballs: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:52Z","receivedAt":"2025-01-16T21:36:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_if_asked() helper we introduced earlier, fix\ncallers of usage() that want to show the help text when explicitly\nasked by the end-user. The help text now goes to the standard\noutput stream for them.\n\nThe callers in this step are oddballs in that their invocations of\nusage() are *not* guarded by\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nThere are (unnecessarily) being clever ones that do things like\n\n\tif (argc != 2 || !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nto say \"I know I take only one argument, so argc != 2 is always an\nerror regardless of what is in argv[]. Ah, by the way, even if argc\nis 2, \"-h\" is a request for usage text, so we do the same\".\n\nSome like \"git var -h\" just do not treat \"-h\" any specially, and let\nit take the same error code paths as a parameter error.\n\nNow we cannot do the same, so these callers are rewrittin to do the\nshow_usage_and_exit_if_asked() first and then handle the usage error\nthe way they used to.\n\nSigned-off-by: Junio C Hamano \n---\n * Helper function renamed.\n Fix fo fetch-pack.c moved from the next step and redone.\n\n builtin/credential.c | 3 ++-\n builtin/fetch-pack.c | 2 ++\n builtin/unpack-file.c | 8 ++++++--\n builtin/upload-archive.c | 3 ++-\n builtin/var.c | 1 +\n 5 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/credential.c b/builtin/credential.c\nindex 14c8c6608b..f6fc948123 100644\n--- a/builtin/credential.c\n+++ b/builtin/credential.c\n@@ -18,7 +18,8 @@ int cmd_credential(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n \t\tusage(usage_msg);\n \top = argv[1];\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bed2816c2d..d07eec9e55 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -75,6 +75,8 @@ int cmd_fetch_pack(int argc,\n \tlist_objects_filter_init(&args.filter_options);\n \targs.uploadpack = \"git-upload-pack\";\n \n+\tshow_usage_if_asked(argc, argv, fetch_pack_usage);\n+\n \tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 6da2825753..fb5fcbc40a 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)\n \treturn path;\n }\n \n+static const char usage_msg[] =\n+\"git unpack-file \";\n+\n int cmd_unpack_file(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,\n {\n \tstruct object_id oid;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git unpack-file \");\n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n+\t\tusage(usage_msg);\n \tif (repo_get_oid(the_repository, argv[1], &oid))\n \t\tdie(\"Not a valid object name %s\", argv[1]);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 9e9343f121..9d76a31c8f 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,\n \tconst char *arg_cmd = \"argument \";\n \tint ret;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_if_asked(argc, argv, upload_archive_usage);\n+\tif (argc != 2)\n \t\tusage(upload_archive_usage);\n \n \tif (!enter_repo(argv[1], 0))\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 1449656cc9..46d40d6fba 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -221,6 +221,7 @@ int cmd_var(int argc,\n \tconst struct git_var *git_var;\n \tchar *val;\n \n+\tshow_usage_if_asked(argc, argv, var_usage);\n \tif (argc != 2)\n \t\tusage(var_usage);\n \n-- \n2.48.1-191-gafe818080f\n\n"},{"id":"510740","messageId":"20250116213553.2563751-7-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v4 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T21:35:53Z","receivedAt":"2025-01-16T21:36:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_and_exit_if_asked() helper we introduced\nearlier, fix callers of usage() that want to show the help text when\nexplicitly asked by the end-user. The help text now goes to the\nstandard output stream for them.\n\nThese are the bog standard \"if we got only '-h', then that is a\nrequest for help\" callers. Their\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(message);\n\nare simply replaced with\n\n\tshow_usage_and_exit_if_asked(argc, argv, message);\n\nWith this, the built-ins tested by t0012 all send their help text to\ntheir standard output stream, so the check in t0012 that was half\ntightened earlier is now fully tightened to insist on standard error\nstream being empty.\n\nSigned-off-by: Junio C Hamano \n---\n * Helper function renamed.\n\n builtin/check-ref-format.c | 4 ++--\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/pack-redundant.c | 3 +--\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-objects.c | 2 ++\n builtin/upload-archive.c | 3 +--\n t/t0012-help.sh | 10 ++--------\n 19 files changed, 37 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex cef1ffe3ce..5d80afeec0 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_check_ref_format_usage);\n+\tshow_usage_if_asked(argc, argv,\n+\t\t\t builtin_check_ref_format_usage);\n \n \tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n \t\treturn check_ref_format_branch(argv[2]);\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 604b04bb2c..99b1749723 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,\n \tint result;\n \tunsigned options = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_files_usage);\n+\tshow_usage_if_asked(argc, argv, diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex ebc824602e..81c0bc8ed7 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,\n \tint i;\n \tint result;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_cache_usage);\n+\tshow_usage_if_asked(argc, argv, diff_cache_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 40804e7b48..e31cc797fe 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,\n \tint read_stdin = 0;\n \tint merge_base = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_tree_usage);\n+\tshow_usage_if_asked(argc, argv, diff_tree_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 0f86392761..2da46fecdc 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,\n {\n \tunsigned int i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(fast_import_usage);\n+\tshow_usage_if_asked(argc, argv, fast_import_usage);\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_pack_config();\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 6bec0d1854..e4cd1627b4 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =\n #define HEADERSIZE (2 * RECORDSIZE)\n \n int cmd_get_tar_commit_id(int argc,\n-\t\t\t const char **argv UNUSED,\n+\t\t\t const char **argv,\n \t\t\t const char *prefix,\n \t\t\t struct repository *repo UNUSED)\n {\n@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, builtin_get_tar_commit_id_usage);\n+\n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0b62b2589f..d41b126ec0 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,\n \t */\n \tfetch_if_missing = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(index_pack_usage);\n+\tshow_usage_if_asked(argc, argv, index_pack_usage);\n \n \tdisable_replace_refs();\n \tfsck_options.walk = mark_link;\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 41dd304731..264df6259a 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, git_mailsplit_usage);\n+\n \tfor (argp = argv+1; *argp; argp++) {\n \t\tconst char *arg = *argp;\n \n@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,\n \t\t\tcontinue;\n \t\t} else if ( arg[1] == 'f' ) {\n \t\t\tnr = strtol(arg+2, NULL, 10);\n-\t\t} else if ( arg[1] == 'h' ) {\n-\t\t\tusage(git_mailsplit_usage);\n \t\t} else if ( arg[1] == 'b' && !arg[2] ) {\n \t\t\tallow_bare = 1;\n \t\t} else if (!strcmp(arg, \"--keep-cr\")) {\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex 342699edb7..3314fb1336 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -75,6 +75,9 @@ static void merge_all(void)\n \t}\n }\n \n+static const char usage_string[] =\n+\"git merge-index [-o] [-q] (-a | [--] [...])\";\n+\n int cmd_merge_index(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n+\tshow_usage_if_asked(argc, argv, usage_string);\n+\n \tif (argc < 3)\n-\t\tusage(\"git merge-index [-o] [-q] (-a | [--] [...])\");\n+\t\tusage(usage_string);\n \n \trepo_read_index(the_repository);\n \ndiff --git a/builtin/merge-ours.c b/builtin/merge-ours.c\nindex 3ecd9172f1..97b8a792c7 100644\n--- a/builtin/merge-ours.c\n+++ b/builtin/merge-ours.c\n@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,\n \t\t const char *prefix UNUSED,\n \t\t struct repository *repo UNUSED)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_merge_ours_usage);\n+\tshow_usage_if_asked(argc, argv, builtin_merge_ours_usage);\n \n \t/*\n \t * The contents of the current index becomes the tree we\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 1dd295558b..abfc060e28 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,6 +38,12 @@ 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+\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+\t}\n+\n \tif (argc < 4)\n \t\tusagef(builtin_merge_recursive_usage, argv[0]);\n \ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex e046575871..3febe732f8 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \tstruct strbuf idx_name = STRBUF_INIT;\n \tchar buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \\n + \\0 */\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(pack_redundant_usage);\n+\tshow_usage_if_asked(argc, argv, pack_redundant_usage);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 33c8ae0fc7..bd2037f27d 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,\n {\n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/remote-fd.c b/builtin/remote-fd.c\nindex ae896eda57..39908546ba 100644\n--- a/builtin/remote-fd.c\n+++ b/builtin/remote-fd.c\n@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, usage_msg);\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3196da7b2d..28f148049f 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,\n \tconst char *show_progress = NULL;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(rev_list_usage);\n+\tshow_usage_if_asked(argc, argv, rev_list_usage);\n \n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 949747a6b6..428c866c05 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n+\tshow_usage_if_asked(argc, argv, builtin_rev_parse_usage);\n+\n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n \ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 2197d6d933..8faa6024b2 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,\n \n \tquiet = !isatty(2);\n \n+\tshow_usage_if_asked(argc, argv, unpack_usage);\n+\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 9d76a31c8f..97d7c9522f 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -93,8 +93,7 @@ struct repository *repo UNUSED)\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(upload_archive_usage);\n+\tshow_usage_if_asked(argc, argv, upload_archive_usage);\n \n \t/*\n \t * Set up sideband subprocess.\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 9c7ae9fd36..d3a0967e9d 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -257,14 +257,8 @@ do\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n \t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n-\t\tthen\n-\t\t\ttest_must_be_empty err &&\n-\t\t\ttest_grep usage output\n-\t\telse\n-\t\t\ttest_grep usage output ||\n-\t\t\ttest_grep usage err\n-\t\tfi\n+\t\ttest_must_be_empty err &&\n+\t\ttest_grep usage output\n \t'\n done >> -\tfflush(stderr);\n> >> -\twrite_in_full(2, msg, p - msg);\n> >> +\tif (fd == 2)\n> >> +\t\tfflush(stderr);\n> >> +\twrite_in_full(fd, msg, p - msg);\n> >> +}\n> >\n> > Gross. :) I think the existing code is conceptually:\n> >\n> > write_in_full(fileno(stderr), msg, p - msg);\n> >\n> > In which case vfreportf() could just take a FILE*, flush it and then\n> > write.\n> \n> Sure, but these \"stderr\" are real error reporting that need to stay\n> to be stderr, and flush needs to be done only when our true payload\n> goes to fd#2 and I do not think these fflush() are about stdio calls\n> made by the caller _before_ it called this function. It may become\n> a bit tricky to read the resulting code if we pass \"FILE *\".\n\nI think the flush _is_ about earlier stdio calls in the process; that's\nwhat 116d1fa6c6 (vreportf(): avoid relying on stdio buffering,\n2019-10-30) says. I do think it's unlikely for the process to write to\nstdout() before processing \"-h\", but it seems like we should do the\nsafer thing as a general principle, unless it ends up hard to do so.\n\nYou said \"may become a bit tricky\" above, but unless I'm missing\nsomething, it's just:\n\n---\n usage.c | 11 +++++------\n 1 file changed, 5 insertions(+), 6 deletions(-)\n\ndiff --git a/usage.c b/usage.c\nindex d9f323a0bd..533d296e41 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,7 +8,7 @@\n #include \"gettext.h\"\n #include \"trace2.h\"\n \n-static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n+static void vfreportf(FILE *fh, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n@@ -32,14 +32,13 @@ static void vfdreportf(int fd, const char *prefix, const char *err, va_list para\n \t}\n \n \t*(p++) = '\\n'; /* we no longer need a NUL */\n-\tif (fd == 2)\n-\t\tfflush(stderr);\n-\twrite_in_full(fd, msg, p - msg);\n+\tfflush(fh);\n+\twrite_in_full(fileno(fh), msg, p - msg);\n }\n \n static void vreportf(const char *prefix, const char *err, va_list params)\n {\n-\tvfdreportf(2, prefix, err, params);\n+\tvfreportf(stderr, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -184,7 +183,7 @@ static void show_usage_if_asked_helper(const char *err, ...)\n \tva_list params;\n \n \tva_start(params, err);\n-\tvfdreportf(1, _(\"usage: \"), err, params);\n+\tvfreportf(stdout, _(\"usage: \"), err, params);\n \tva_end(params);\n \texit(129);\n }\n"},{"id":"510744","messageId":"xmqqfrlipf8b.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116215431.GA2338133@coredump.intra.peff.net","subject":"Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T22:26:44Z","receivedAt":"2025-01-16T22:26:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> You said \"may become a bit tricky\" above, but unless I'm missing\n> something, it's just:\n\nI didn't mean \"tricky to write\". I meant it would become tricky to\nreason about while reading the resulting code why we flush only when\nfh is stderr.\n\n> I think the flush _is_ about earlier stdio calls in the process; that's\n> what 116d1fa6c6 (vreportf(): avoid relying on stdio buffering,\n> 2019-10-30) says.\n\nBut as vreportf() has always been about stderr, when that old commit\ntalks about \"stdio\", it is talking about stderr, isn't it?\n\n> I do think it's unlikely for the process to write to\n> stdout() before processing \"-h\", but it seems like we should do the\n> safer thing as a general principle, unless it ends up hard to do so.\n\nWe are going to exit immediately at the end of this function, so\nthere is no point in avoiding an fflush() call that may not be\nneeded. So I am not fundamentally opposed to unconditionally\nflushing before doing the write().\n\nThanks.\n\n"},{"id":"510746","messageId":"xmqqr052nz36.fsf@gitster.g","threadId":"62814","inReplyTo":"20250116213553.2563751-4-gitster@pobox.com","subject":"Re: [PATCH v4 3/6] usage: add show_usage_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-16T23:00:45Z","receivedAt":"2025-01-16T23:00:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"So here is a replacement for this step. Everything else is\nunaffected, so I'll wait for other comments until sending a full\nreroll.\n\nThanks.\n\n--- >8 ---\nSubject: [PATCH v5 3/6] usage: add show_usage_if_asked()\n\nSome commands call usage() when they are asked to give the help\nmessage with \"git cmd -h\", but this has the same problem as we\nfixed with callers of usage_with_options() for the same purpose.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(usage);\n\nand replaces it with\n\n\tshow_usage_if_asked(argc, argv, usage);\n\nto help correct these code paths.\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it. After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n\n1: 2b57538eab ! 1: 94a6fdf72f usage: add show_usage_if_asked()\n @@ Commit message\n doing exactly as asked, so there is no reason to exit with non-zero\n status.\n \n + Helped-by: Jeff King \n Signed-off-by: Junio C Hamano \n \n ## git-compat-util.h ##\n @@ usage.c\n #include \"trace2.h\"\n \n -static void vreportf(const char *prefix, const char *err, va_list params)\n -+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)\n ++static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n @@ usage.c: static void vreportf(const char *prefix, const char *err, va_list param\n \t*(p++) = '\\n'; /* we no longer need a NUL */\n -\tfflush(stderr);\n -\twrite_in_full(2, msg, p - msg);\n -+\tif (fd == 2)\n -+\t\tfflush(stderr);\n -+\twrite_in_full(fd, msg, p - msg);\n ++\tfflush(f);\n ++\twrite_in_full(fileno(f), msg, p - msg);\n +}\n +\n +static void vreportf(const char *prefix, const char *err, va_list params)\n +{\n -+\tvfdreportf(2, prefix, err, params);\n ++\tvfreportf(stderr, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n @@ usage.c: void NORETURN usage(const char *err)\n +\tva_list params;\n +\n +\tva_start(params, err);\n -+\tvfdreportf(1, _(\"usage: \"), err, params);\n ++\tvfreportf(stdout, _(\"usage: \"), err, params);\n +\tva_end(params);\n +\texit(129);\n +}\n\n\n git-compat-util.h | 2 ++\n usage.c | 27 ++++++++++++++++++++++++---\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e283c46c6f..d43dd248c4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+void show_usage_if_asked(int ac, const char **av, const char *err);\n+\n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\n #include \"compat/apple-common-crypto.h\"\ndiff --git a/usage.c b/usage.c\nindex 47709006c1..38b46bbbfe 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,7 +8,7 @@\n #include \"gettext.h\"\n #include \"trace2.h\"\n \n-static void vreportf(const char *prefix, const char *err, va_list params)\n+static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n@@ -32,8 +32,13 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n \t}\n \n \t*(p++) = '\\n'; /* we no longer need a NUL */\n-\tfflush(stderr);\n-\twrite_in_full(2, msg, p - msg);\n+\tfflush(f);\n+\twrite_in_full(fileno(f), msg, p - msg);\n+}\n+\n+static void vreportf(const char *prefix, const char *err, va_list params)\n+{\n+\tvfreportf(stderr, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -173,6 +178,22 @@ void NORETURN usage(const char *err)\n \tusagef(\"%s\", err);\n }\n \n+static void show_usage_if_asked_helper(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tvfreportf(stdout, _(\"usage: \"), err, params);\n+\tva_end(params);\n+\texit(129);\n+}\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+\t\tshow_usage_if_asked_helper(err);\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n-- \n2.48.1-210-gaa1682cadd\n\n"},{"id":"510794","messageId":"20250117114123.GA2356746@coredump.intra.peff.net","threadId":"62814","inReplyTo":"xmqqr052nz36.fsf@gitster.g","subject":"Re: [PATCH v4 3/6] usage: add show_usage_if_asked()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-17T11:41:23Z","receivedAt":"2025-01-17T11:41:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2025 at 03:00:45PM -0800, Junio C Hamano wrote:\n\n> So here is a replacement for this step. Everything else is\n> unaffected, so I'll wait for other comments until sending a full\n> reroll.\n\nThanks, with this replacement patch the whole series looks good to me.\n\n-Peff\n"},{"id":"510795","messageId":"20250117114250.GB2356746@coredump.intra.peff.net","threadId":"62814","inReplyTo":"20250116213553.2563751-7-gitster@pobox.com","subject":"Re: [PATCH v4 6/6] builtin: send usage() help text to standard output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-01-17T11:42:50Z","receivedAt":"2025-01-17T11:42:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 16, 2025 at 01:35:53PM -0800, Junio C Hamano wrote:\n\n> --- a/t/t0012-help.sh\n> +++ b/t/t0012-help.sh\n> @@ -257,14 +257,8 @@ do\n> \t\t\texport GIT_CEILING_DIRECTORIES &&\n> \t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n> \t\t) &&\n> -\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n> -\t\tthen\n> -\t\t\ttest_must_be_empty err &&\n> -\t\t\ttest_grep usage output\n> -\t\telse\n> -\t\t\ttest_grep usage output ||\n> -\t\t\ttest_grep usage err\n> -\t\tfi\n> +\t\ttest_must_be_empty err &&\n> +\t\ttest_grep usage output\n> \t'\n\nJust a side note for the future: you wondered if we might ever get\nbitten by insisting on an empty stderr here (since it might catch\nwarnings, etc).\n\nIf we do, I think it would be OK to just drop the test_must_be_empty\nline. Now that we are capturing only stdout in \"output\", checking that\nthe usage message is there is probably sufficient.\n\nIn the meantime, I think I prefer keeping the slightly more strict form\nabove.\n\n-Peff\n"},{"id":"510824","messageId":"xmqqtt9xjka9.fsf@gitster.g","threadId":"62814","inReplyTo":"20250117114250.GB2356746@coredump.intra.peff.net","subject":"Re: [PATCH v4 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T19:46:22Z","receivedAt":"2025-01-17T19:46:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> If we do, I think it would be OK to just drop the test_must_be_empty\n> line. Now that we are capturing only stdout in \"output\", checking that\n> the usage message is there is probably sufficient.\n\nYes.\n\n> In the meantime, I think I prefer keeping the slightly more strict form\n> above.\n\nI am also in favor of keeping it, if only to learn why we do not\nwant the overly strict check when the time comes. That would mean\nwe would be loosening with a concrete reason why we would want to\nloose it at that time, instead of guessing that it might be overly\nstrict without seeing the fallout before seeing any.\n\nThanks.\n\n"},{"id":"510831","messageId":"20250117213148.3974552-1-gitster@pobox.com","threadId":"62814","inReplyTo":"20250116213553.2563751-1-gitster@pobox.com","subject":"[PATCH v5 0/6] Send help text from \"git cmd -h\" to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:42Z","receivedAt":"2025-01-17T21:31:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonas Konrad noticed[*] that \"git branch -h\" started showing the help\ntext to the standard error stream. It turns out that we are fairly\ninconsistent in our implementation of \"git cmd -h\". The users of\nparse-options API will get \"If -h is the only option on the command\nline, give the help text to the standard output\" for free, but some\ncommands manually check for the condition and then call the\nusage_with_options() function, which gives the identical help text\nto the standard error stream. And \"git branch -h\" Jonas noticed was\none of them.\n\nOlder commands written before parse-options API became dominant show\nthe help text by calling the usage() function, which is meant to be\nused when they fail to parse their command line arguments, which has\nthe same problem. An explicit request for help text \"git cmd -h\"\nshould be fulfilled by showing the help on the standard output.\n\nThis series teachs \"git $cmd -h\" to send its help text to the\nstandard output stream consistently for built-in commands.\n\nThere is a related \"what status should these command exit with?\"\nquestion, and an argument can be made to switch them to exit with 0,\ninstead of historical 129, but it is left outside of the series for\nnow.\n\n\nChanges since v4:\n\n - vfdreportf() helper that took a file descriptor has been replaced\n with vfreportf() that takes a FILE * instead, with Peff's help.\n\n[Reference]\n\nhttps://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/\n\nJeff King (1):\n t0012: optionally check that \"-h\" output goes to stdout\n\nJunio C Hamano (5):\n parse-options: add show_usage_with_options_if_asked()\n usage: add show_usage_if_asked()\n builtins: send usage_with_options() help text to standard output\n oddballs: send usage() help text to standard output\n builtin: send usage() help text to standard output\n\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/check-ref-format.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/credential.c | 3 ++-\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/fetch-pack.c | 2 ++\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/ls-files.c | 4 ++--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/merge.c | 4 ++--\n builtin/pack-redundant.c | 3 +--\n builtin/rebase.c | 6 +++---\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-file.c | 8 ++++++--\n builtin/unpack-objects.c | 2 ++\n builtin/update-index.c | 4 ++--\n builtin/upload-archive.c | 6 +++---\n builtin/var.c | 1 +\n git-compat-util.h | 2 ++\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n t/helper/test-simple-ipc.c | 4 ++--\n t/t0012-help.sh | 3 ++-\n t/t7600-merge.sh | 2 +-\n usage.c | 27 ++++++++++++++++++++++++---\n 41 files changed, 121 insertions(+), 64 deletions(-)\n\n-- \n2.48.1-218-gc7e8be6a8f\n\n"},{"id":"510832","messageId":"20250117213148.3974552-2-gitster@pobox.com","threadId":"62814","inReplyTo":"20250117213148.3974552-1-gitster@pobox.com","subject":"[PATCH v5 1/6] t0012: optionally check that \"-h\" output goes to stdout","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:43Z","receivedAt":"2025-01-17T21:31:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"From: Jeff King \n\nFor most commands, \"git foo -h\" will send the help output to stdout, as\nthis is what parse-options.c does. But some commands send it to stderr\ninstead. This is usually because they call usage_with_options(), and\nshould be switched to show_usage_help_and_exit_if_asked().\n\nCurrently t0012 is permissive and allows either behavior. We'd like it\nto eventually enforce that help goes to stdout, and teaching it to do so\nidentifies the commands that need to be changed. But during the\ntransition period, we don't want to enforce that for most test runs.\n\nSo let's introduce a flag that will let most test runs use the\npermissive behavior, and people interested in converting commands can\nrun:\n\n GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh\n\nto see the failures. Eventually (when all builtins have been converted)\nwe'll remove this flag entirely and always check the strict behavior.\n\nSigned-off-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n t/t0012-help.sh | 11 +++++++++--\n 1 file changed, 9 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 1d273d91c2..9c7ae9fd36 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -255,9 +255,16 @@ do\n \t\t(\n \t\t\tGIT_CEILING_DIRECTORIES=$(pwd) &&\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n-\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>&1\n+\t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\ttest_grep usage output\n+\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n+\t\tthen\n+\t\t\ttest_must_be_empty err &&\n+\t\t\ttest_grep usage output\n+\t\telse\n+\t\t\ttest_grep usage output ||\n+\t\t\ttest_grep usage err\n+\t\tfi\n \t'\n done \nSigned-off-by: Junio C Hamano \n---\n parse-options.c | 10 ++++++++++\n parse-options.h | 4 ++++\n 2 files changed, 14 insertions(+)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 33bfba0ed4..4051a8e1d1 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,\n \texit(129);\n }\n \n+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, 0, 0);\n+\t\texit(129);\n+\t}\n+}\n+\n void NORETURN usage_msg_opt(const char *msg,\n \t\t const char * const *usagestr,\n \t\t const struct option *options)\ndiff --git a/parse-options.h b/parse-options.h\nindex d01361ca97..39f0886254 100644\n--- a/parse-options.h\n+++ b/parse-options.h\n@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,\n NORETURN void usage_with_options(const char * const *usagestr,\n \t\t\t\t const struct option *options);\n \n+void show_usage_with_options_if_asked(int ac, const char **av,\n+\t\t\t\t const char * const *usage,\n+\t\t\t\t const struct option *options);\n+\n NORETURN void usage_msg_opt(const char *msg,\n \t\t\t const char * const *usagestr,\n \t\t\t const struct option *options);\n-- \n2.48.1-218-gc7e8be6a8f\n\n"},{"id":"510834","messageId":"20250117213148.3974552-4-gitster@pobox.com","threadId":"62814","inReplyTo":"20250117213148.3974552-1-gitster@pobox.com","subject":"[PATCH v5 3/6] usage: add show_usage_if_asked()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:45Z","receivedAt":"2025-01-17T21:31:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Some commands call usage() when they are asked to give the help\nmessage with \"git cmd -h\", but this has the same problem as we\nfixed with callers of usage_with_options() for the same purpose.\n\nIntroduce a helper function that captures the common pattern\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(usage);\n\nand replaces it with\n\n\tshow_usage_if_asked(argc, argv, usage);\n\nto help correct these code paths.\n\nNote that this helper function still exits with status 129, and\nt0012 insists on it. After converting all the mistaken callers of\nusage_with_options() to call this new helper, we may want to address\nit---the end user is asking us to give the help text, and we are\ndoing exactly as asked, so there is no reason to exit with non-zero\nstatus.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n git-compat-util.h | 2 ++\n usage.c | 27 ++++++++++++++++++++++++---\n 2 files changed, 26 insertions(+), 3 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex e283c46c6f..d43dd248c4 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));\n void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));\n \n+void show_usage_if_asked(int ac, const char **av, const char *err);\n+\n #ifndef NO_OPENSSL\n #ifdef APPLE_COMMON_CRYPTO\n #include \"compat/apple-common-crypto.h\"\ndiff --git a/usage.c b/usage.c\nindex 47709006c1..38b46bbbfe 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -8,7 +8,7 @@\n #include \"gettext.h\"\n #include \"trace2.h\"\n \n-static void vreportf(const char *prefix, const char *err, va_list params)\n+static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)\n {\n \tchar msg[4096];\n \tchar *p, *pend = msg + sizeof(msg);\n@@ -32,8 +32,13 @@ static void vreportf(const char *prefix, const char *err, va_list params)\n \t}\n \n \t*(p++) = '\\n'; /* we no longer need a NUL */\n-\tfflush(stderr);\n-\twrite_in_full(2, msg, p - msg);\n+\tfflush(f);\n+\twrite_in_full(fileno(f), msg, p - msg);\n+}\n+\n+static void vreportf(const char *prefix, const char *err, va_list params)\n+{\n+\tvfreportf(stderr, prefix, err, params);\n }\n \n static NORETURN void usage_builtin(const char *err, va_list params)\n@@ -173,6 +178,22 @@ void NORETURN usage(const char *err)\n \tusagef(\"%s\", err);\n }\n \n+static void show_usage_if_asked_helper(const char *err, ...)\n+{\n+\tva_list params;\n+\n+\tva_start(params, err);\n+\tvfreportf(stdout, _(\"usage: \"), err, params);\n+\tva_end(params);\n+\texit(129);\n+}\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+\t\tshow_usage_if_asked_helper(err);\n+}\n+\n void NORETURN die(const char *err, ...)\n {\n \tva_list params;\n-- \n2.48.1-218-gc7e8be6a8f\n\n"},{"id":"510835","messageId":"20250117213148.3974552-5-gitster@pobox.com","threadId":"62814","inReplyTo":"20250117213148.3974552-1-gitster@pobox.com","subject":"[PATCH v5 4/6] builtins: send usage_with_options() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:46Z","receivedAt":"2025-01-17T21:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_with_options_if_asked() helper we introduced\nearlier, fix callers of usage_with_options() that want to show the\nhelp text when explicitly asked by the end-user. The help text now\ngoes to the standard output stream for them.\n\nThe test in t7600 for \"git merge -h\" may want to be retired, as the\nsame is covered by t0012 already, but it is specifically testing that\nthe \"-h\" option gets a response even with a corrupt index file, so\nfor now let's leave it there.\n\nAcked-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/am.c | 3 +--\n builtin/branch.c | 4 ++--\n builtin/checkout--worker.c | 6 +++---\n builtin/checkout-index.c | 6 +++---\n builtin/commit-tree.c | 4 ++--\n builtin/commit.c | 8 ++++----\n builtin/fsmonitor--daemon.c | 4 ++--\n builtin/gc.c | 4 ++--\n builtin/ls-files.c | 4 ++--\n builtin/merge.c | 4 ++--\n builtin/rebase.c | 6 +++---\n builtin/update-index.c | 4 ++--\n t/helper/test-simple-ipc.c | 4 ++--\n t/t7600-merge.sh | 2 +-\n 14 files changed, 31 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 1338b606fe..8d8f38fa1d 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -2427,8 +2427,7 @@ int cmd_am(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv, usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6e7b0cfddb..7c8b4b65b6 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -784,8 +784,8 @@ int cmd_branch(int argc,\n \tfilter.kind = FILTER_REFS_BRANCHES;\n \tfilter.abbrev = -1;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_branch_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_branch_usage, options);\n \n \t/*\n \t * Try to set sort keys from config. If config does not set any,\ndiff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c\nindex b81002a1df..da9345a44b 100644\n--- a/builtin/checkout--worker.c\n+++ b/builtin/checkout--worker.c\n@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(checkout_worker_usage,\n-\t\t\t\t checkout_worker_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t checkout_worker_usage,\n+\t\t\t\t\t checkout_worker_options);\n \n \tgit_config(git_default_config, NULL);\n \targc = parse_options(argc, argv, prefix, checkout_worker_options,\ndiff --git a/builtin/checkout-index.c b/builtin/checkout-index.c\nindex a81501098d..e30086c7d4 100644\n--- a/builtin/checkout-index.c\n+++ b/builtin/checkout-index.c\n@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_checkout_index_usage,\n-\t\t\t\t builtin_checkout_index_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_checkout_index_usage,\n+\t\t\t\t\t builtin_checkout_index_options);\n \tgit_config(git_default_config, NULL);\n \tprefix_length = prefix ? strlen(prefix) : 0;\n \ndiff --git a/builtin/commit-tree.c b/builtin/commit-tree.c\nindex 2ca1a57ebb..38457600a4 100644\n--- a/builtin/commit-tree.c\n+++ b/builtin/commit-tree.c\n@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc < 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(commit_tree_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t commit_tree_usage, options);\n \n \targc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex ef5e622c07..c84062bfc1 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END(),\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_status_usage, builtin_status_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_status_usage, builtin_status_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\n@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,\n \tstruct strbuf err = STRBUF_INIT;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_commit_usage, builtin_commit_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_commit_usage, builtin_commit_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c\nindex 029dc64d6c..0820e524f1 100644\n--- a/builtin/fsmonitor--daemon.c\n+++ b/builtin/fsmonitor--daemon.c\n@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_fsmonitor__daemon_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_fsmonitor__daemon_usage, options);\n \n \tdie(_(\"fsmonitor--daemon not supported on this platform\"));\n }\ndiff --git a/builtin/gc.c b/builtin/gc.c\nindex a9b1c36de2..ea007c8e7e 100644\n--- a/builtin/gc.c\n+++ b/builtin/gc.c\n@@ -710,8 +710,8 @@ struct repository *repo UNUSED)\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_gc_usage, builtin_gc_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_gc_usage, builtin_gc_options);\n \n \tstrvec_pushl(&reflog, \"reflog\", \"expire\", \"--all\", NULL);\n \tstrvec_pushl(&repack, \"repack\", \"-d\", \"-l\", NULL);\ndiff --git a/builtin/ls-files.c b/builtin/ls-files.c\nindex 15499cd12b..a4431429b7 100644\n--- a/builtin/ls-files.c\n+++ b/builtin/ls-files.c\n@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,\n \t};\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(ls_files_usage, builtin_ls_files_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t ls_files_usage, builtin_ls_files_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 5f67007bba..ba9faf126a 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,\n \tvoid *branch_to_free;\n \tint orig_argc = argc;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_merge_usage, builtin_merge_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex 0498fff3c9..6c9eaf3788 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,\n \t};\n \tint i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(builtin_rebase_usage,\n-\t\t\t\t builtin_rebase_options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t builtin_rebase_usage,\n+\t\t\t\t\t builtin_rebase_options);\n \n \tprepare_repo_settings(the_repository);\n \tthe_repository->settings.command_requires_full_index = 0;\ndiff --git a/builtin/update-index.c b/builtin/update-index.c\nindex 74bbad9f87..b2f6b1a3fb 100644\n--- a/builtin/update-index.c\n+++ b/builtin/update-index.c\n@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,\n \t\tOPT_END()\n \t};\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(update_index_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t update_index_usage, options);\n \n \tgit_config(git_default_config, NULL);\n \ndiff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c\nindex fb5927775d..03cc5eea2c 100644\n--- a/t/helper/test-simple-ipc.c\n+++ b/t/helper/test-simple-ipc.c\n@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)\n \tif (argc < 2)\n \t\tusage_with_options(simple_ipc_usage, options);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage_with_options(simple_ipc_usage, options);\n+\tshow_usage_with_options_if_asked(argc, argv,\n+\t\t\t\t\t simple_ipc_usage, options);\n \n \tif (argc == 2 && !strcmp(argv[1], \"SUPPORTS_SIMPLE_IPC\"))\n \t\treturn 0;\ndiff --git a/t/t7600-merge.sh b/t/t7600-merge.sh\nindex ef54cff4fa..2a8df29219 100755\n--- a/t/t7600-merge.sh\n+++ b/t/t7600-merge.sh\n@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '\n \t\tcd broken &&\n \t\tgit init &&\n \t\t>.git/index &&\n-\t\ttest_expect_code 129 git merge -h 2>usage\n+\t\ttest_expect_code 129 git merge -h >usage\n \t) &&\n \ttest_grep \"[Uu]sage: git merge\" broken/usage\n '\n-- \n2.48.1-218-gc7e8be6a8f\n\n"},{"id":"510836","messageId":"20250117213148.3974552-6-gitster@pobox.com","threadId":"62814","inReplyTo":"20250117213148.3974552-1-gitster@pobox.com","subject":"[PATCH v5 5/6] oddballs: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:47Z","receivedAt":"2025-01-17T21:31:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_if_asked() helper we introduced earlier, fix\ncallers of usage() that want to show the help text when explicitly\nasked by the end-user. The help text now goes to the standard\noutput stream for them.\n\nThe callers in this step are oddballs in that their invocations of\nusage() are *not* guarded by\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nThere are (unnecessarily) being clever ones that do things like\n\n\tif (argc != 2 || !strcmp(argv[1], \"-h\")\n\t\tusage(...);\n\nto say \"I know I take only one argument, so argc != 2 is always an\nerror regardless of what is in argv[]. Ah, by the way, even if argc\nis 2, \"-h\" is a request for usage text, so we do the same\".\n\nSome like \"git var -h\" just do not treat \"-h\" any specially, and let\nit take the same error code paths as a parameter error.\n\nNow we cannot do the same, so these callers are rewrittin to do the\nshow_usage_and_exit_if_asked() first and then handle the usage error\nthe way they used to.\n\nAcked-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/credential.c | 3 ++-\n builtin/fetch-pack.c | 2 ++\n builtin/unpack-file.c | 8 ++++++--\n builtin/upload-archive.c | 3 ++-\n builtin/var.c | 1 +\n 5 files changed, 13 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/credential.c b/builtin/credential.c\nindex 14c8c6608b..f6fc948123 100644\n--- a/builtin/credential.c\n+++ b/builtin/credential.c\n@@ -18,7 +18,8 @@ int cmd_credential(int argc,\n \n \tgit_config(git_default_config, NULL);\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n \t\tusage(usage_msg);\n \top = argv[1];\n \ndiff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c\nindex bed2816c2d..d07eec9e55 100644\n--- a/builtin/fetch-pack.c\n+++ b/builtin/fetch-pack.c\n@@ -75,6 +75,8 @@ int cmd_fetch_pack(int argc,\n \tlist_objects_filter_init(&args.filter_options);\n \targs.uploadpack = \"git-upload-pack\";\n \n+\tshow_usage_if_asked(argc, argv, fetch_pack_usage);\n+\n \tfor (i = 1; i < argc && *argv[i] == '-'; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/unpack-file.c b/builtin/unpack-file.c\nindex 6da2825753..fb5fcbc40a 100644\n--- a/builtin/unpack-file.c\n+++ b/builtin/unpack-file.c\n@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)\n \treturn path;\n }\n \n+static const char usage_msg[] =\n+\"git unpack-file \";\n+\n int cmd_unpack_file(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,\n {\n \tstruct object_id oid;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n-\t\tusage(\"git unpack-file \");\n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\tif (argc != 2)\n+\t\tusage(usage_msg);\n \tif (repo_get_oid(the_repository, argv[1], &oid))\n \t\tdie(\"Not a valid object name %s\", argv[1]);\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 9e9343f121..9d76a31c8f 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,\n \tconst char *arg_cmd = \"argument \";\n \tint ret;\n \n-\tif (argc != 2 || !strcmp(argv[1], \"-h\"))\n+\tshow_usage_if_asked(argc, argv, upload_archive_usage);\n+\tif (argc != 2)\n \t\tusage(upload_archive_usage);\n \n \tif (!enter_repo(argv[1], 0))\ndiff --git a/builtin/var.c b/builtin/var.c\nindex 1449656cc9..46d40d6fba 100644\n--- a/builtin/var.c\n+++ b/builtin/var.c\n@@ -221,6 +221,7 @@ int cmd_var(int argc,\n \tconst struct git_var *git_var;\n \tchar *val;\n \n+\tshow_usage_if_asked(argc, argv, var_usage);\n \tif (argc != 2)\n \t\tusage(var_usage);\n \n-- \n2.48.1-218-gc7e8be6a8f\n\n"},{"id":"510837","messageId":"20250117213148.3974552-7-gitster@pobox.com","threadId":"62814","inReplyTo":"20250117213148.3974552-1-gitster@pobox.com","subject":"[PATCH v5 6/6] builtin: send usage() help text to standard output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-01-17T21:31:48Z","receivedAt":"2025-01-17T21:31:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Using the show_usage_and_exit_if_asked() helper we introduced\nearlier, fix callers of usage() that want to show the help text when\nexplicitly asked by the end-user. The help text now goes to the\nstandard output stream for them.\n\nThese are the bog standard \"if we got only '-h', then that is a\nrequest for help\" callers. Their\n\n\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n\t\tusage(message);\n\nare simply replaced with\n\n\tshow_usage_and_exit_if_asked(argc, argv, message);\n\nWith this, the built-ins tested by t0012 all send their help text to\ntheir standard output stream, so the check in t0012 that was half\ntightened earlier is now fully tightened to insist on standard error\nstream being empty.\n\nAcked-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/check-ref-format.c | 4 ++--\n builtin/diff-files.c | 3 +--\n builtin/diff-index.c | 3 +--\n builtin/diff-tree.c | 3 +--\n builtin/fast-import.c | 3 +--\n builtin/get-tar-commit-id.c | 4 +++-\n builtin/index-pack.c | 3 +--\n builtin/mailsplit.c | 4 ++--\n builtin/merge-index.c | 7 ++++++-\n builtin/merge-ours.c | 3 +--\n builtin/merge-recursive.c | 6 ++++++\n builtin/pack-redundant.c | 3 +--\n builtin/remote-ext.c | 2 ++\n builtin/remote-fd.c | 1 +\n builtin/rev-list.c | 3 +--\n builtin/rev-parse.c | 2 ++\n builtin/unpack-objects.c | 2 ++\n builtin/upload-archive.c | 3 +--\n t/t0012-help.sh | 10 ++--------\n 19 files changed, 37 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c\nindex cef1ffe3ce..5d80afeec0 100644\n--- a/builtin/check-ref-format.c\n+++ b/builtin/check-ref-format.c\n@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_check_ref_format_usage);\n+\tshow_usage_if_asked(argc, argv,\n+\t\t\t builtin_check_ref_format_usage);\n \n \tif (argc == 3 && !strcmp(argv[1], \"--branch\"))\n \t\treturn check_ref_format_branch(argv[2]);\ndiff --git a/builtin/diff-files.c b/builtin/diff-files.c\nindex 604b04bb2c..99b1749723 100644\n--- a/builtin/diff-files.c\n+++ b/builtin/diff-files.c\n@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,\n \tint result;\n \tunsigned options = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_files_usage);\n+\tshow_usage_if_asked(argc, argv, diff_files_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-index.c b/builtin/diff-index.c\nindex ebc824602e..81c0bc8ed7 100644\n--- a/builtin/diff-index.c\n+++ b/builtin/diff-index.c\n@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,\n \tint i;\n \tint result;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_cache_usage);\n+\tshow_usage_if_asked(argc, argv, diff_cache_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/diff-tree.c b/builtin/diff-tree.c\nindex 40804e7b48..e31cc797fe 100644\n--- a/builtin/diff-tree.c\n+++ b/builtin/diff-tree.c\n@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,\n \tint read_stdin = 0;\n \tint merge_base = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(diff_tree_usage);\n+\tshow_usage_if_asked(argc, argv, diff_tree_usage);\n \n \tgit_config(git_diff_basic_config, NULL); /* no \"diff\" UI options */\n \ndiff --git a/builtin/fast-import.c b/builtin/fast-import.c\nindex 0f86392761..2da46fecdc 100644\n--- a/builtin/fast-import.c\n+++ b/builtin/fast-import.c\n@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,\n {\n \tunsigned int i;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(fast_import_usage);\n+\tshow_usage_if_asked(argc, argv, fast_import_usage);\n \n \treset_pack_idx_option(&pack_idx_opts);\n \tgit_pack_config();\ndiff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c\nindex 6bec0d1854..e4cd1627b4 100644\n--- a/builtin/get-tar-commit-id.c\n+++ b/builtin/get-tar-commit-id.c\n@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =\n #define HEADERSIZE (2 * RECORDSIZE)\n \n int cmd_get_tar_commit_id(int argc,\n-\t\t\t const char **argv UNUSED,\n+\t\t\t const char **argv,\n \t\t\t const char *prefix,\n \t\t\t struct repository *repo UNUSED)\n {\n@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, builtin_get_tar_commit_id_usage);\n+\n \tif (argc != 1)\n \t\tusage(builtin_get_tar_commit_id_usage);\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 0b62b2589f..d41b126ec0 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,\n \t */\n \tfetch_if_missing = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(index_pack_usage);\n+\tshow_usage_if_asked(argc, argv, index_pack_usage);\n \n \tdisable_replace_refs();\n \tfsck_options.walk = mark_link;\ndiff --git a/builtin/mailsplit.c b/builtin/mailsplit.c\nindex 41dd304731..264df6259a 100644\n--- a/builtin/mailsplit.c\n+++ b/builtin/mailsplit.c\n@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, git_mailsplit_usage);\n+\n \tfor (argp = argv+1; *argp; argp++) {\n \t\tconst char *arg = *argp;\n \n@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,\n \t\t\tcontinue;\n \t\t} else if ( arg[1] == 'f' ) {\n \t\t\tnr = strtol(arg+2, NULL, 10);\n-\t\t} else if ( arg[1] == 'h' ) {\n-\t\t\tusage(git_mailsplit_usage);\n \t\t} else if ( arg[1] == 'b' && !arg[2] ) {\n \t\t\tallow_bare = 1;\n \t\t} else if (!strcmp(arg, \"--keep-cr\")) {\ndiff --git a/builtin/merge-index.c b/builtin/merge-index.c\nindex 342699edb7..3314fb1336 100644\n--- a/builtin/merge-index.c\n+++ b/builtin/merge-index.c\n@@ -75,6 +75,9 @@ static void merge_all(void)\n \t}\n }\n \n+static const char usage_string[] =\n+\"git merge-index [-o] [-q] (-a | [--] [...])\";\n+\n int cmd_merge_index(int argc,\n \t\t const char **argv,\n \t\t const char *prefix UNUSED,\n@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,\n \t */\n \tsignal(SIGCHLD, SIG_DFL);\n \n+\tshow_usage_if_asked(argc, argv, usage_string);\n+\n \tif (argc < 3)\n-\t\tusage(\"git merge-index [-o] [-q] (-a | [--] [...])\");\n+\t\tusage(usage_string);\n \n \trepo_read_index(the_repository);\n \ndiff --git a/builtin/merge-ours.c b/builtin/merge-ours.c\nindex 3ecd9172f1..97b8a792c7 100644\n--- a/builtin/merge-ours.c\n+++ b/builtin/merge-ours.c\n@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,\n \t\t const char *prefix UNUSED,\n \t\t struct repository *repo UNUSED)\n {\n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(builtin_merge_ours_usage);\n+\tshow_usage_if_asked(argc, argv, builtin_merge_ours_usage);\n \n \t/*\n \t * The contents of the current index becomes the tree we\ndiff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c\nindex 1dd295558b..abfc060e28 100644\n--- a/builtin/merge-recursive.c\n+++ b/builtin/merge-recursive.c\n@@ -38,6 +38,12 @@ 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+\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+\t}\n+\n \tif (argc < 4)\n \t\tusagef(builtin_merge_recursive_usage, argv[0]);\n \ndiff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c\nindex e046575871..3febe732f8 100644\n--- a/builtin/pack-redundant.c\n+++ b/builtin/pack-redundant.c\n@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s\n \tstruct strbuf idx_name = STRBUF_INIT;\n \tchar buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \\n + \\0 */\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(pack_redundant_usage);\n+\tshow_usage_if_asked(argc, argv, pack_redundant_usage);\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\ndiff --git a/builtin/remote-ext.c b/builtin/remote-ext.c\nindex 33c8ae0fc7..bd2037f27d 100644\n--- a/builtin/remote-ext.c\n+++ b/builtin/remote-ext.c\n@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,\n {\n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, usage_msg);\n+\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/remote-fd.c b/builtin/remote-fd.c\nindex ae896eda57..39908546ba 100644\n--- a/builtin/remote-fd.c\n+++ b/builtin/remote-fd.c\n@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n+\tshow_usage_if_asked(argc, argv, usage_msg);\n \tif (argc != 3)\n \t\tusage(usage_msg);\n \ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 3196da7b2d..28f148049f 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,\n \tconst char *show_progress = NULL;\n \tint ret = 0;\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(rev_list_usage);\n+\tshow_usage_if_asked(argc, argv, rev_list_usage);\n \n \tgit_config(git_default_config, NULL);\n \trepo_init_revisions(the_repository, &revs, prefix);\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 949747a6b6..428c866c05 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,\n \tint seen_end_of_options = 0;\n \tenum format_type format = FORMAT_DEFAULT;\n \n+\tshow_usage_if_asked(argc, argv, builtin_rev_parse_usage);\n+\n \tif (argc > 1 && !strcmp(\"--parseopt\", argv[1]))\n \t\treturn cmd_parseopt(argc - 1, argv + 1, prefix);\n \ndiff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 2197d6d933..8faa6024b2 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,\n \n \tquiet = !isatty(2);\n \n+\tshow_usage_if_asked(argc, argv, unpack_usage);\n+\n \tfor (i = 1 ; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n \ndiff --git a/builtin/upload-archive.c b/builtin/upload-archive.c\nindex 9d76a31c8f..97d7c9522f 100644\n--- a/builtin/upload-archive.c\n+++ b/builtin/upload-archive.c\n@@ -93,8 +93,7 @@ struct repository *repo UNUSED)\n \n \tBUG_ON_NON_EMPTY_PREFIX(prefix);\n \n-\tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n-\t\tusage(upload_archive_usage);\n+\tshow_usage_if_asked(argc, argv, upload_archive_usage);\n \n \t/*\n \t * Set up sideband subprocess.\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 9c7ae9fd36..d3a0967e9d 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -257,14 +257,8 @@ do\n \t\t\texport GIT_CEILING_DIRECTORIES &&\n \t\t\ttest_expect_code 129 git -C sub $builtin -h >output 2>err\n \t\t) &&\n-\t\tif test -n \"$GIT_TEST_HELP_MUST_BE_STDOUT\"\n-\t\tthen\n-\t\t\ttest_must_be_empty err &&\n-\t\t\ttest_grep usage output\n-\t\telse\n-\t\t\ttest_grep usage output ||\n-\t\t\ttest_grep usage err\n-\t\tfi\n+\t\ttest_must_be_empty err &&\n+\t\ttest_grep usage output\n \t'\n done Changes since v4:\n> \n> - vfdreportf() helper that took a file descriptor has been replaced\n> with vfreportf() that takes a FILE * instead, with Peff's help.\n\nThanks, this all looks good to me.\n\n-Peff\n"}]}