threads / patch / 62814

v3, 6 partsSend help text from "git cmd -h" to stdout

Subject: [PATCH v3 0/6] Send help text from "git cmd -h" to stdout

## tl;dr

37 messages between Jan 16, 2025 and Jan 18, 2025. Diffs are folded; open one to read it.

replies: 36people: 2as markdown or json

Junio C Hamano· Jan 16, 2025, 01:25 UTC · lore

Jonas Konrad noticed[*] that "git branch -h" started showing the help text to the standard error stream. It turns out that we are fairly inconsistent in our implementation of "git cmd -h". The users of parse-options API will get "If -h is the only option on the command line, give the help text to the standard output" for free, but some commands manually check for the condition and then call the usage_with_options() function, which gives the identical help text to the standard error stream. And "git branch -h" Jonas noticed was one of them.

Older commands written before parse-options API became dominant show the help text by calling the usage() function, which is meant to be used when they fail to parse their command line arguments, which has the same problem. An explicit request for help text "git cmd -h" should be fulfilled by showing the help on the standard output.

This series teachs "git $cmd -h" to send its help text to the standard output stream consistently for built-in commands.

[Reference]

https://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/

Jeff King (1):
  t0012: optionally check that "-h" output goes to stdout

Junio C Hamano (5):
  parse-options: add show_usage_help_and_exit_if_asked()
  builtins: send usage_with_options() help text to standard output
  usage: add show_usage_and_exit_if_asked()
  builtin: send usage() help text to standard output
  builtin: send usage() help text to standard output

 builtin/am.c                |  3 +--
 builtin/branch.c            |  4 ++--
 builtin/check-ref-format.c  |  4 ++--
 builtin/checkout--worker.c  |  6 +++---
 builtin/checkout-index.c    |  6 +++---
 builtin/commit-tree.c       |  4 ++--
 builtin/commit.c            |  8 ++++----
 builtin/credential.c        |  3 ++-
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/fetch-pack.c        |  3 +++
 builtin/fsmonitor--daemon.c |  4 ++--
 builtin/gc.c                |  4 ++--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/ls-files.c          |  4 ++--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/merge.c             |  4 ++--
 builtin/pack-redundant.c    |  3 +--
 builtin/rebase.c            |  6 +++---
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-file.c       |  8 ++++++--
 builtin/unpack-objects.c    |  2 ++
 builtin/update-index.c      |  4 ++--
 builtin/upload-archive.c    |  6 +++---
 builtin/var.c               |  1 +
 git-compat-util.h           |  2 ++
 parse-options.c             | 10 ++++++++++
 parse-options.h             |  4 ++++
 t/helper/test-simple-ipc.c  |  4 ++--
 t/t0012-help.sh             |  3 ++-
 t/t7600-merge.sh            |  2 +-
 usage.c                     | 28 +++++++++++++++++++++++++---
 41 files changed, 123 insertions(+), 64 deletions(-)

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 1/6] parse-options: add show_usage_help_and_exit_if_asked()

Many commands call usage_with_options() when they are asked to give the help message, but it incorrectly sends the help text to the standard error stream. When the user asked for it with "git cmd -h", the help message is the primary output from the command, hence we should send it to the standard output stream.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage_with_options(usage, options);

and replaces it with

	show_usage_help_and_exit_if_asked(argc, argv, usage, options);

to help correct code paths (there are 40 or so of them).

Note that this helper function still exits with status 129, and t0012 insists on it. After converting all the mistaken callers of usage_with_options() to call this new helper, we may want to address it---the end user is asking us to give the help text, and we are doing exactly as asked, so there is no reason to exit with non-zero status.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 10 ++++++++++
 parse-options.h |  4 ++++
 2 files changed, 14 insertions(+)

Show changes to 2 files +14 −0

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..8a8b934e67 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void show_usage_help_and_exit_if_asked(int ac, const char **av,
+				       const char * const *usagestr,
+				       const struct option *opts)
+{
+	if (ac == 2 && !strcmp(av[1], "-h")) {
+		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		exit(129);
+	}
+}
+
 void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
diff --git a/parse-options.h b/parse-options.h
index d01361ca97..6af4ee148a 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
+void show_usage_help_and_exit_if_asked(int ac, const char **av,
+				      const char * const *usage,
+				      const struct option *options);
+
 NORETURN void usage_msg_opt(const char *msg,
 			    const char * const *usagestr,
 			    const struct option *options);

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 2/6] t0012: optionally check that "-h" output goes to stdout

From: Jeff King <peff@peff.net>

For most commands, "git foo -h" will send the help output to stdout, as this is what parse-options.c does. But some commands send it to stderr instead. This is usually because they call usage_with_options(), and should be switched to show_usage_help_and_exit_if_asked().

Currently t0012 is permissive and allows either behavior. We'd like it to eventually enforce that help goes to stdout, and teaching it to do so identifies the commands that need to be changed. But during the transition period, we don't want to enforce that for most test runs.

So let's introduce a flag that will let most test runs use the permissive behavior, and people interested in converting commands can run:

  GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh

to see the failures. Eventually (when all builtins have been converted) we'll remove this flag entirely and always check the strict behavior.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0012-help.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Show changes to t/t0012-help.sh +9 −2

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..9c7ae9fd36 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -255,9 +255,16 @@ do
 		(
 			GIT_CEILING_DIRECTORIES=$(pwd) &&
 			export GIT_CEILING_DIRECTORIES &&
-			test_expect_code 129 git -C sub $builtin -h >output 2>&1
+			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		test_grep usage output
+		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
+		then
+			test_must_be_empty err &&
+			test_grep usage output
+		else
+			test_grep usage output ||
+			test_grep usage err
+		fi
 	'
 done <builtins

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 3/6] builtins: send usage_with_options() help text to standard output

Using the show_usage_help_and_exit_if_asked() helper we introduced earlier, fix callers of usage_with_options() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them.

The test in t7600 for "git merge -h" may want to be retired, as the same is covered by t0012 already. It would be even more true if we later consider changing the exit status from 129 to 0.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c                | 3 +--
 builtin/branch.c            | 4 ++--
 builtin/checkout--worker.c  | 6 +++---
 builtin/checkout-index.c    | 6 +++---
 builtin/commit-tree.c       | 4 ++--
 builtin/commit.c            | 8 ++++----
 builtin/fsmonitor--daemon.c | 4 ++--
 builtin/gc.c                | 4 ++--
 builtin/ls-files.c          | 4 ++--
 builtin/merge.c             | 4 ++--
 builtin/rebase.c            | 6 +++---
 builtin/update-index.c      | 4 ++--
 t/t7600-merge.sh            | 2 +-
 13 files changed, 29 insertions(+), 30 deletions(-)

Show changes to 13 files +29 −30

builtin/am.c, builtin/branch.c, builtin/checkout--worker.c, builtin/checkout-index.c, builtin/commit-tree.c, builtin/commit.c, builtin/fsmonitor--daemon.c, builtin/gc.c, builtin/ls-files.c, builtin/merge.c, builtin/rebase.c, builtin/update-index.c, t/t7600-merge.sh

diff --git a/builtin/am.c b/builtin/am.c
index 1338b606fe..0801b556c2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2427,8 +2427,7 @@ int cmd_am(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv, usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 6e7b0cfddb..366729a78b 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -784,8 +784,8 @@ int cmd_branch(int argc,
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_branch_usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_branch_usage, options);
 
 	/*
 	 * Try to set sort keys from config. If config does not set any,
diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index b81002a1df..7093d1efd5 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(checkout_worker_usage,
-				   checkout_worker_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  checkout_worker_usage,
+					  checkout_worker_options);
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, checkout_worker_options,
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a81501098d..d928d6b5e3 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_checkout_index_usage,
-				   builtin_checkout_index_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_checkout_index_usage,
+					  builtin_checkout_index_options);
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 2ca1a57ebb..2efc224d32 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage_with_options(commit_tree_usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  commit_tree_usage, options);
 
 	argc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index ef5e622c07..4268915120 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_status_usage, builtin_status_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_status_usage, builtin_status_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,
 	struct strbuf err = STRBUF_INIT;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_usage, builtin_commit_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_commit_usage, builtin_commit_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 029dc64d6c..dabf190bbe 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_fsmonitor__daemon_usage, options);
 
 	die(_("fsmonitor--daemon not supported on this platform"));
 }
diff --git a/builtin/gc.c b/builtin/gc.c
index a9b1c36de2..5f831e1f94 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -710,8 +710,8 @@ struct repository *repo UNUSED)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_gc_usage, builtin_gc_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_gc_usage, builtin_gc_options);
 
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 15499cd12b..9efe92b7c0 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,
 	};
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(ls_files_usage, builtin_ls_files_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  ls_files_usage, builtin_ls_files_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/merge.c b/builtin/merge.c
index 5f67007bba..95d798fc89 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,
 	void *branch_to_free;
 	int orig_argc = argc;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_merge_usage, builtin_merge_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_merge_usage, builtin_merge_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0498fff3c9..cb49323c44 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,
 	};
 	int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_rebase_usage,
-				   builtin_rebase_options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  builtin_rebase_usage,
+					  builtin_rebase_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74bbad9f87..b0e2ad4970 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(update_index_usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  update_index_usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index ef54cff4fa..2a8df29219 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git merge -h 2>usage
+		test_expect_code 129 git merge -h >usage
 	) &&
 	test_grep "[Uu]sage: git merge" broken/usage
 '

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

Some commands call usage() when they are asked to give the help message with "git cmd -h", but this has the same problem as we fixed with callers of usage_with_options() for the same purpose.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(usage);

and replaces it with

	show_usage_and_exit_if_asked(argc, argv, usage);

to help correct these code paths.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |  2 ++
 usage.c           | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

Show changes to 2 files +27 −3

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index e283c46c6f..6edf50bc95 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+void show_usage_and_exit_if_asked(int ac, const char **av, const char *err);
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
diff --git a/usage.c b/usage.c
index 47709006c1..26cd54a511 100644
--- a/usage.c
+++ b/usage.c
@@ -8,7 +8,7 @@
 #include "gettext.h"
 #include "trace2.h"
 
-static void vreportf(const char *prefix, const char *err, va_list params)
+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
@@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 	}
 
 	*(p++) = '\n'; /* we no longer need a NUL */
-	fflush(stderr);
-	write_in_full(2, msg, p - msg);
+	if (fd == 2)
+		fflush(stderr);
+	write_in_full(fd, msg, p - msg);
+}
+
+static void vreportf(const char *prefix, const char *err, va_list params)
+{
+	vfdreportf(2, prefix, err, params);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -173,6 +179,22 @@ void NORETURN usage(const char *err)
 	usagef("%s", err);
 }
 
+static void show_usage_and_exit_if_asked_helper(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	vfdreportf(1, _("usage: "), err, params);
+	va_end(params);
+	exit(129);
+}
+
+void show_usage_and_exit_if_asked(int ac, const char **av, const char *err)
+{
+	if (ac == 2 && !strcmp(av[1], "-h"))
+		show_usage_and_exit_if_asked_helper(err);
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;

-- 
2.48.1-191-gafe818080f

Jeff King· Jan 16, 2025, 10:36 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

On Wed, Jan 15, 2025 at 05:25:21PM -0800, Junio C Hamano wrote:

Show 10 quoted lines

> Introduce a helper function that captures the common pattern
> 
> 	if (argc == 2 && !strcmp(argv[1], "-h"))
> 		usage(usage);
> 
> and replaces it with
> 
> 	show_usage_and_exit_if_asked(argc, argv, usage);
> 
> to help correct these code paths.

I found the name hard to distinguish from the earlier helper, show_usage_help_and_exit_if_asked(). The difference is that one takes only a usage string, and the other takes the usage string along with options. Maybe the other should be:

  show_usage_and_exit_if_asked_with_options();

? It just keeps getting longer and longer...

I think the "and_exit" could probably be implied, since showing the usage is always a final thing (just like in usage() and usage_with_options()). So:

  show_usage_if_asked();
  show_usage_with_options_if_asked();

? I dunno. We are in deep bikeshed territory. I otherwise like what the patch is doing. ;)

Show 15 quoted lines

> -static void vreportf(const char *prefix, const char *err, va_list params)
> +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
>  {
>  	char msg[4096];
>  	char *p, *pend = msg + sizeof(msg);
> @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)
>  	}
>  
>  	*(p++) = '\n'; /* we no longer need a NUL */
> -	fflush(stderr);
> -	write_in_full(2, msg, p - msg);
> +	if (fd == 2)
> +		fflush(stderr);
> +	write_in_full(fd, msg, p - msg);
> +}

Gross. :) I think the existing code is conceptually:

  write_in_full(fileno(stderr), msg, p - msg);

In which case vfreportf() could just take a FILE*, flush it and then write.

My main motivation is being less ugly, but I think it would also potentially prevent a bug. If there was unflushed data in stdout as we go into vdreportf(), then we'd get an out of order write. That's why the fflush() is there in the first place (it's of course weird that we are using write() in the first place, but IIRC it's for avoiding sheared writes of error messages).

-Peff

Jeff King· Jan 16, 2025, 10:44 UTC · re: Jeff King · lore

Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

On Thu, Jan 16, 2025 at 05:36:20AM -0500, Jeff King wrote:

Show 15 quoted lines

> > -static void vreportf(const char *prefix, const char *err, va_list params)
> > +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
> >  {
> >  	char msg[4096];
> >  	char *p, *pend = msg + sizeof(msg);
> > @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)
> >  	}
> >  
> >  	*(p++) = '\n'; /* we no longer need a NUL */
> > -	fflush(stderr);
> > -	write_in_full(2, msg, p - msg);
> > +	if (fd == 2)
> > +		fflush(stderr);
> > +	write_in_full(fd, msg, p - msg);
> > +}

Oh, I meant to mention also: I wondered about the other references to stderr in this function. But we hit those only when we fail to format the error message that was asked for, so continuing to send them to stderr is the right thing (i.e., your patch is right but I wanted to help out other reviewers).

-Peff

Junio C Hamano· Jan 16, 2025, 17:22 UTC · re: Jeff King · lore

Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

Jeff King <peff@peff.net> writes:

Show 5 quoted lines

>> 	show_usage_and_exit_if_asked(argc, argv, usage);
>> 
>> to help correct these code paths.
>
> I found the name hard to distinguish from the earlier helper,

As we all know that usage_with_options() and usage() exits,

    show_usage_with_options_if_asked()
    show_usage_if_asked()

may be good enough, I wonder? "show" -> "help" may give us better names.

Show 6 quoted lines

> I think the "and_exit" could probably be implied, since showing the
> usage is always a final thing (just like in usage() and
> usage_with_options()). So:
>
>   show_usage_if_asked();
>   show_usage_with_options_if_asked();

Ah, We came to the same conclusion. The only thing we haven't made it obvious is that "show" implies the output goes to the standard output stream, while without "show", the output goes to the standard error stream. I wonder if these help better:

    show_help_if_asked();
    show_help_with_options_if_asked()

"show help" explains the output goes to where the "help" message would go ;-)

But probably "if-asked" is clear enough indication that the output is made in response to an explicit end-user request, so let's take the show_(usage|usage_with_options)_if_asked as the final names.

Show 22 quoted lines

>> -static void vreportf(const char *prefix, const char *err, va_list params)
>> +static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
>>  {
>>  	char msg[4096];
>>  	char *p, *pend = msg + sizeof(msg);
>> @@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)
>>  	}
>>  
>>  	*(p++) = '\n'; /* we no longer need a NUL */
>> -	fflush(stderr);
>> -	write_in_full(2, msg, p - msg);
>> +	if (fd == 2)
>> +		fflush(stderr);
>> +	write_in_full(fd, msg, p - msg);
>> +}
>
> Gross. :) I think the existing code is conceptually:
>
>   write_in_full(fileno(stderr), msg, p - msg);
>
> In which case vfreportf() could just take a FILE*, flush it and then
> write.

Sure, but these "stderr" are real error reporting that need to stay to be stderr, and flush needs to be done only when our true payload goes to fd#2 and I do not think these fflush() are about stdio calls made by the caller _before_ it called this function. It may become a bit tricky to read the resulting code if we pass "FILE *".

Jeff King· Jan 16, 2025, 21:54 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

On Thu, Jan 16, 2025 at 09:22:54AM -0800, Junio C Hamano wrote:

Show 19 quoted lines

> >> -	fflush(stderr);
> >> -	write_in_full(2, msg, p - msg);
> >> +	if (fd == 2)
> >> +		fflush(stderr);
> >> +	write_in_full(fd, msg, p - msg);
> >> +}
> >
> > Gross. :) I think the existing code is conceptually:
> >
> >   write_in_full(fileno(stderr), msg, p - msg);
> >
> > In which case vfreportf() could just take a FILE*, flush it and then
> > write.
> 
> Sure, but these "stderr" are real error reporting that need to stay
> to be stderr, and flush needs to be done only when our true payload
> goes to fd#2 and I do not think these fflush() are about stdio calls
> made by the caller _before_ it called this function.  It may become
> a bit tricky to read the resulting code if we pass "FILE *".

I think the flush _is_ about earlier stdio calls in the process; that's what 116d1fa6c6 (vreportf(): avoid relying on stdio buffering, 2019-10-30) says. I do think it's unlikely for the process to write to stdout() before processing "-h", but it seems like we should do the safer thing as a general principle, unless it ends up hard to do so.

You said "may become a bit tricky" above, but unless I'm missing something, it's just:

---
 usage.c | 11 +++++------
 1 file changed, 5 insertions(+), 6 deletions(-)

Show changes to usage.c +5 −6

diff --git a/usage.c b/usage.c
index d9f323a0bd..533d296e41 100644
--- a/usage.c
+++ b/usage.c
@@ -8,7 +8,7 @@
 #include "gettext.h"
 #include "trace2.h"
 
-static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
+static void vfreportf(FILE *fh, const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
@@ -32,14 +32,13 @@ static void vfdreportf(int fd, const char *prefix, const char *err, va_list para
 	}
 
 	*(p++) = '\n'; /* we no longer need a NUL */
-	if (fd == 2)
-		fflush(stderr);
-	write_in_full(fd, msg, p - msg);
+	fflush(fh);
+	write_in_full(fileno(fh), msg, p - msg);
 }
 
 static void vreportf(const char *prefix, const char *err, va_list params)
 {
-	vfdreportf(2, prefix, err, params);
+	vfreportf(stderr, prefix, err, params);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -184,7 +183,7 @@ static void show_usage_if_asked_helper(const char *err, ...)
 	va_list params;
 
 	va_start(params, err);
-	vfdreportf(1, _("usage: "), err, params);
+	vfreportf(stdout, _("usage: "), err, params);
 	va_end(params);
 	exit(129);
 }

Junio C Hamano· Jan 16, 2025, 22:26 UTC · re: Jeff King · lore

Re: [PATCH v3 4/6] usage: add show_usage_and_exit_if_asked()

Jeff King <peff@peff.net> writes:

> You said "may become a bit tricky" above, but unless I'm missing
> something, it's just:

I didn't mean "tricky to write". I meant it would become tricky to reason about while reading the resulting code why we flush only when fh is stderr.

> I think the flush _is_ about earlier stdio calls in the process; that's
> what 116d1fa6c6 (vreportf(): avoid relying on stdio buffering,
> 2019-10-30) says.

But as vreportf() has always been about stderr, when that old commit talks about "stdio", it is talking about stderr, isn't it?

> I do think it's unlikely for the process to write to
> stdout() before processing "-h", but it seems like we should do the
> safer thing as a general principle, unless it ends up hard to do so.

We are going to exit immediately at the end of this function, so there is no point in avoiding an fflush() call that may not be needed. So I am not fundamentally opposed to unconditionally flushing before doing the write().

Thanks.

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 5/6] oddballs: send usage() help text to standard output

Using the show_usage_and_exit_if_asked() helper we introduced earlier, fix callers of usage() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them.

The callers in this step are oddballs in that their invocations of usage() are *not* guarded by

	if (argc == 2 && !strcmp(argv[1], "-h")
		usage(...);

They are (unnecessarily) being clever and do things like

	if (argc != 2 || !strcmp(argv[1], "-h")
		usage(...);

to say "I know I take only one argument, so argc != 2 is always an error regardless of what is in argv[]. Ah, by the way, even if argc is 2, "-h" is a request for usage text, so we do the same". Some just do not treat "-h" any specially, and let it take the same error code paths as a parameter error.

Now we cannot do the same, so these callers are rewrittin to do the show_usage_and_exit_if_asked() first and then handle the usage error the way they used to.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/credential.c     | 3 ++-
 builtin/unpack-file.c    | 8 ++++++--
 builtin/upload-archive.c | 3 ++-
 builtin/var.c            | 1 +
 4 files changed, 11 insertions(+), 4 deletions(-)

Show changes to 4 files +11 −4

builtin/credential.c, builtin/unpack-file.c, builtin/upload-archive.c, builtin/var.c

diff --git a/builtin/credential.c b/builtin/credential.c
index 14c8c6608b..8a8cffecc8 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -18,7 +18,8 @@ int cmd_credential(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_and_exit_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
 		usage(usage_msg);
 	op = argv[1];
 
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 6da2825753..307351af55 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)
 	return path;
 }
 
+static const char usage_msg[] =
+"git unpack-file <blob>";
+
 int cmd_unpack_file(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,
 {
 	struct object_id oid;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
-		usage("git unpack-file <blob>");
+	show_usage_and_exit_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
+		usage(usage_msg);
 	if (repo_get_oid(the_repository, argv[1], &oid))
 		die("Not a valid object name %s", argv[1]);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 9e9343f121..3b282d41e6 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,
 	const char *arg_cmd = "argument ";
 	int ret;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_and_exit_if_asked(argc, argv, upload_archive_usage);
+	if (argc != 2)
 		usage(upload_archive_usage);
 
 	if (!enter_repo(argv[1], 0))
diff --git a/builtin/var.c b/builtin/var.c
index 1449656cc9..6a09c1c39a 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -221,6 +221,7 @@ int cmd_var(int argc,
 	const struct git_var *git_var;
 	char *val;
 
+	show_usage_and_exit_if_asked(argc, argv, var_usage);
 	if (argc != 2)
 		usage(var_usage);

-- 
2.48.1-191-gafe818080f

Jeff King· Jan 16, 2025, 10:42 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 5/6] oddballs: send usage() help text to standard output

On Wed, Jan 15, 2025 at 05:25:22PM -0800, Junio C Hamano wrote:

Show 21 quoted lines

> Using the show_usage_and_exit_if_asked() helper we introduced
> earlier, fix callers of usage() that want to show the help text when
> explicitly asked by the end-user.  The help text now goes to the
> standard output stream for them.
> 
> The callers in this step are oddballs in that their invocations of
> usage() are *not* guarded by
> 
> 	if (argc == 2 && !strcmp(argv[1], "-h")
> 		usage(...);
> 
> They are (unnecessarily) being clever and do things like
> 
> 	if (argc != 2 || !strcmp(argv[1], "-h")
> 		usage(...);
> 
> to say "I know I take only one argument, so argc != 2 is always an
> error regardless of what is in argv[].  Ah, by the way, even if argc
> is 2, "-h" is a request for usage text, so we do the same".  Some
> just do not treat "-h" any specially, and let it take the same error
> code paths as a parameter error.

As the author of at least one of these, I feel judged. :)

But yes, untangling this is obviously good (especially if we change the exit code later!). And pulling these into their own commit made reviewing much easier.

Show 11 quoted lines

> diff --git a/builtin/var.c b/builtin/var.c
> index 1449656cc9..6a09c1c39a 100644
> --- a/builtin/var.c
> +++ b/builtin/var.c
> @@ -221,6 +221,7 @@ int cmd_var(int argc,
>  	const struct git_var *git_var;
>  	char *val;
>  
> +	show_usage_and_exit_if_asked(argc, argv, var_usage);
>  	if (argc != 2)
>  		usage(var_usage);

Hmm, what's going on in this one? It does not check "-h" at all.

Ah, I see. It simply hits the get_git_var() call, sees there is no var called "-h", and exits with an error. So checking for "-h" up front is correct. It is an oddball, though not quite the same as the others (the oddest ball?).

-Peff

Junio C Hamano· Jan 16, 2025, 17:24 UTC · re: Jeff King · lore

Re: [PATCH v3 5/6] oddballs: send usage() help text to standard output

Jeff King <peff@peff.net> writes:

Show 9 quoted lines

>> ....  Some
>> just do not treat "-h" any specially, and let it take the same error
>> code paths as a parameter error.
> ...
>> +	show_usage_and_exit_if_asked(argc, argv, var_usage);
>>  	if (argc != 2)
>>  		usage(var_usage);
>
> Hmm, what's going on in this one? It does not check "-h" at all.

Yes, this is the one that let the usual error code paths to treat it as a mere parameter error.

Junio C Hamano· Jan 16, 2025, 01:25 UTC · re: Junio C Hamano · lore

[PATCH v3 6/6] builtin: send usage() help text to standard output

These are the bog standard "if we got only '-h', then that is a request for help" callers. Their

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(message);

are simply replaced with

	show_usage_and_exit_if_asked(argc, argv, message);

With this, the built-ins tested by t0012 all send their help text to their standard output stream, so the check in t0012 that was half tightened earlier is now fully tightened to insist on standard error stream being empty.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/check-ref-format.c  |  4 ++--
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/fetch-pack.c        |  3 +++
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/pack-redundant.c    |  3 +--
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-objects.c    |  2 ++
 builtin/upload-archive.c    |  3 +--
 t/helper/test-simple-ipc.c  |  4 ++--
 t/t0012-help.sh             | 10 ++--------
 21 files changed, 42 insertions(+), 34 deletions(-)

Show changes to 21 files +42 −34

builtin/check-ref-format.c, builtin/diff-files.c, builtin/diff-index.c, builtin/diff-tree.c, builtin/fast-import.c, builtin/fetch-pack.c, builtin/get-tar-commit-id.c, builtin/index-pack.c, builtin/mailsplit.c, builtin/merge-index.c, builtin/merge-ours.c, builtin/merge-recursive.c, builtin/pack-redundant.c, builtin/remote-ext.c, builtin/remote-fd.c, builtin/rev-list.c, builtin/rev-parse.c, builtin/unpack-objects.c, builtin/upload-archive.c, t/helper/test-simple-ipc.c, t/t0012-help.sh

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index cef1ffe3ce..acc4366d85 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_check_ref_format_usage);
+	show_usage_and_exit_if_asked(argc, argv,
+				     builtin_check_ref_format_usage);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
 		return check_ref_format_branch(argv[2]);
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 604b04bb2c..f8816e0d9e 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,
 	int result;
 	unsigned options = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_files_usage);
+	show_usage_and_exit_if_asked(argc, argv, diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index ebc824602e..a1759b8291 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,
 	int i;
 	int result;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_cache_usage);
+	show_usage_and_exit_if_asked(argc, argv, diff_cache_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 40804e7b48..fc7c026555 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,
 	int read_stdin = 0;
 	int merge_base = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_tree_usage);
+	show_usage_and_exit_if_asked(argc, argv, diff_tree_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761..e1f6bfdc73 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,
 {
 	unsigned int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(fast_import_usage);
+	show_usage_and_exit_if_asked(argc, argv, fast_import_usage);
 
 	reset_pack_idx_option(&pack_idx_opts);
 	git_pack_config();
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bed2816c2d..9bd4b29c5b 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,
 			list_objects_filter_set_no_filter(&args.filter_options);
 			continue;
 		}
+
+		if (!strcmp(arg, "-h"))
+			show_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);
 		usage(fetch_pack_usage);
 	}
 	if (deepen_not.nr)
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index 6bec0d1854..033a205ba4 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =
 #define HEADERSIZE (2 * RECORDSIZE)
 
 int cmd_get_tar_commit_id(int argc,
-			  const char **argv UNUSED,
+			  const char **argv,
 			  const char *prefix,
 			  struct repository *repo UNUSED)
 {
@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_and_exit_if_asked(argc, argv, builtin_get_tar_commit_id_usage);
+
 	if (argc != 1)
 		usage(builtin_get_tar_commit_id_usage);
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0b62b2589f..92ef59fad5 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,
 	 */
 	fetch_if_missing = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(index_pack_usage);
+	show_usage_and_exit_if_asked(argc, argv, index_pack_usage);
 
 	disable_replace_refs();
 	fsck_options.walk = mark_link;
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 41dd304731..9e11bffdfb 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_and_exit_if_asked(argc, argv, git_mailsplit_usage);
+
 	for (argp = argv+1; *argp; argp++) {
 		const char *arg = *argp;
 
@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,
 			continue;
 		} else if ( arg[1] == 'f' ) {
 			nr = strtol(arg+2, NULL, 10);
-		} else if ( arg[1] == 'h' ) {
-			usage(git_mailsplit_usage);
 		} else if ( arg[1] == 'b' && !arg[2] ) {
 			allow_bare = 1;
 		} else if (!strcmp(arg, "--keep-cr")) {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 342699edb7..f243a55d32 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -75,6 +75,9 @@ static void merge_all(void)
 	}
 }
 
+static const char usage_string[] =
+"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])";
+
 int cmd_merge_index(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
+	show_usage_and_exit_if_asked(argc, argv, usage_string);
+
 	if (argc < 3)
-		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
+		usage(usage_string);
 
 	repo_read_index(the_repository);
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 3ecd9172f1..ab78fffb52 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,
 		   const char *prefix UNUSED,
 		   struct repository *repo UNUSED)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_merge_ours_usage);
+	show_usage_and_exit_if_asked(argc, argv, builtin_merge_ours_usage);
 
 	/*
 	 * The contents of the current index becomes the tree we
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 1dd295558b..b71c7e1c2f 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,6 +38,12 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
+	if (argc == 2 && !strcmp(argv[1], "-h")) {
+		struct strbuf msg = STRBUF_INIT;
+		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
+		show_usage_and_exit_if_asked(argc, argv, msg.buf);
+	}
+
 	if (argc < 4)
 		usagef(builtin_merge_recursive_usage, argv[0]);
 
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index e046575871..ce2e1fde8d 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
 	struct strbuf idx_name = STRBUF_INIT;
 	char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(pack_redundant_usage);
+	show_usage_and_exit_if_asked(argc, argv, pack_redundant_usage);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 33c8ae0fc7..f7591e7701 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,
 {
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_and_exit_if_asked(argc, argv, usage_msg);
+
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index ae896eda57..1a1aa65273 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_and_exit_if_asked(argc, argv, usage_msg);
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 3196da7b2d..5a21d57c43 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,
 	const char *show_progress = NULL;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(rev_list_usage);
+	show_usage_and_exit_if_asked(argc, argv, rev_list_usage);
 
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 949747a6b6..f4c2b1b000 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;
 
+	show_usage_and_exit_if_asked(argc, argv, builtin_rev_parse_usage);
+
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
 
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 2197d6d933..1f1fb8e682 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,
 
 	quiet = !isatty(2);
 
+	show_usage_and_exit_if_asked(argc, argv, unpack_usage);
+
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 3b282d41e6..42d8f67174 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -93,8 +93,7 @@ struct repository *repo UNUSED)
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(upload_archive_usage);
+	show_usage_and_exit_if_asked(argc, argv, upload_archive_usage);
 
 	/*
 	 * Set up sideband subprocess.
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index fb5927775d..d7a41cd053 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)
 	if (argc < 2)
 		usage_with_options(simple_ipc_usage, options);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(simple_ipc_usage, options);
+	show_usage_help_and_exit_if_asked(argc, argv,
+					  simple_ipc_usage, options);
 
 	if (argc == 2 && !strcmp(argv[1], "SUPPORTS_SIMPLE_IPC"))
 		return 0;
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 9c7ae9fd36..d3a0967e9d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -257,14 +257,8 @@ do
 			export GIT_CEILING_DIRECTORIES &&
 			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
-		then
-			test_must_be_empty err &&
-			test_grep usage output
-		else
-			test_grep usage output ||
-			test_grep usage err
-		fi
+		test_must_be_empty err &&
+		test_grep usage output
 	'
 done <builtins

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 17:30 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 6/6] builtin: send usage() help text to standard output

Junio C Hamano <gitster@pobox.com> writes:

Show 15 quoted lines

> Using the show_usage_and_exit_if_asked() helper we introduced
> earlier, fix callers of usage() that want to show the help text when
> explicitly asked by the end-user.  The help text now goes to the
> standard output stream for them.
>
> These are the bog standard "if we got only '-h', then that is a
> request for help" callers.  Their
>
> 	if (argc == 2 && !strcmp(argv[1], "-h"))
> 		usage(message);
>
> are simply replaced with
>
> 	show_usage_and_exit_if_asked(argc, argv, message);
> ...

The above is a bit of a lie. There is one strange thing I did, which needs to be redone.

Show 14 quoted lines

> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
> index bed2816c2d..9bd4b29c5b 100644
> --- a/builtin/fetch-pack.c
> +++ b/builtin/fetch-pack.c
> @@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,
>  			list_objects_filter_set_no_filter(&args.filter_options);
>  			continue;
>  		}
> +
> +		if (!strcmp(arg, "-h"))
> +			show_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);
>  		usage(fetch_pack_usage);
>  	}
>  	if (deepen_not.nr)

I think we should just call show_usage_and_exit_if_asked() before entering the loop without changing anything else.

Junio C Hamano· Jan 16, 2025, 20:37 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 6/6] builtin: send usage() help text to standard output

Junio C Hamano <gitster@pobox.com> writes:

Show 20 quoted lines

> The above is a bit of a lie.  There is one strange thing I did,
> which needs to be redone.
>
>> diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
>> index bed2816c2d..9bd4b29c5b 100644
>> --- a/builtin/fetch-pack.c
>> +++ b/builtin/fetch-pack.c
>> @@ -176,6 +176,9 @@ int cmd_fetch_pack(int argc,
>>  			list_objects_filter_set_no_filter(&args.filter_options);
>>  			continue;
>>  		}
>> +
>> +		if (!strcmp(arg, "-h"))
>> +			show_usage_and_exit_if_asked(2, &arg - 1, fetch_pack_usage);
>>  		usage(fetch_pack_usage);
>>  	}
>>  	if (deepen_not.nr)
>
> I think we should just call show_usage_and_exit_if_asked() before
> entering the loop without changing anything else.

I have worked on a reroll and moved this piece to the "oddball" pile.

Jeff King· Jan 16, 2025, 10:46 UTC · re: Junio C Hamano · lore

Re: [PATCH v3 0/6] Send help text from "git cmd -h" to stdout

On Wed, Jan 15, 2025 at 05:25:17PM -0800, Junio C Hamano wrote:

Show 18 quoted lines

> Jonas Konrad noticed[*] that "git branch -h" started showing the help
> text to the standard error stream.  It turns out that we are fairly
> inconsistent in our implementation of "git cmd -h".  The users of
> parse-options API will get "If -h is the only option on the command
> line, give the help text to the standard output" for free, but some
> commands manually check for the condition and then call the
> usage_with_options() function, which gives the identical help text
> to the standard error stream.  And "git branch -h" Jonas noticed was
> one of them.
> 
> Older commands written before parse-options API became dominant show
> the help text by calling the usage() function, which is meant to be
> used when they fail to parse their command line arguments, which has
> the same problem.  An explicit request for help text "git cmd -h"
> should be fulfilled by showing the help on the standard output.
> 
> This series teachs "git $cmd -h" to send its help text to the
> standard output stream consistently for built-in commands.

I had a small complaint in patch 4, but otherwise this looks good to me.

If we want to switch the exit code for this case from 129 to 0, I think we could easily do so on top (it would need modifications in three places, but now that you've untangled all of the individual builtins, that would get all of them).

I guess there may be non-builtins that would need to be handled individually, though. We don't have too many of them these days, but they are not covered by t0012.

-Peff

Junio C Hamano· Jan 16, 2025, 17:28 UTC · re: Jeff King · lore

Re: [PATCH v3 0/6] Send help text from "git cmd -h" to stdout

Jeff King <peff@peff.net> writes:

> If we want to switch the exit code for this case from 129 to 0, I think
> we could easily do so on top (it would need modifications in three
> places, but now that you've untangled all of the individual builtins,
> that would get all of them).

Yes, I consider it pretty much an orthogonal issue to update the exit status.

> I guess there may be non-builtins that would need to be handled
> individually, though. We don't have too many of them these days, but
> they are not covered by t0012.

Yes. We can probably leave them as they are, as we have established our expectation with this series (i.e. an explicit end user request for help text should emit to the standard output stream), so any bug report can be handled without needing any policy discussion. "Hey, I noticed 'git cmd -h' writes to stderr, not stdout"---"Thanks, cmd need to be fixed".

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 0/6] Send help text from "git cmd -h" to stdout

This series teachs "git $cmd -h" to send its help text to the standard output stream consistently for built-in commands.

There is a related "what status should these command exit with?" question, and an argument can be made to switch them to exit with 0, instead of historical 129, but it is left outside of the series for now.

Changes since v3:

 - The helper functions for usage_with_options() and usage() are
   renamed to show_usage_with_options_if_asked() and
   show_usage_if_asked(), respectively.

 - The call to usage() in builtin/fetch-pack.c turns out to be the
   same class as builtin/var.c that simply treats "-h" as an
   unrecognised command line option.  The fix was rewritten to
   follow the pattern taken to fix "git var -h" and moved to the
   "oddball" step.

 - The patches are reordered; the first three patches prepare the
   new helper functions and ways to test their effect, and the
   remaining patches convert the commands to use the new helper
   functions.

[Reference]

https://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/

Jeff King (1):
  t0012: optionally check that "-h" output goes to stdout

Junio C Hamano (5):
  parse-options: add show_usage_with_options_if_asked()
  usage: add show_usage_if_asked()
  builtins: send usage_with_options() help text to standard output
  oddballs: send usage() help text to standard output
  builtin: send usage() help text to standard output

 builtin/am.c                |  3 +--
 builtin/branch.c            |  4 ++--
 builtin/check-ref-format.c  |  4 ++--
 builtin/checkout--worker.c  |  6 +++---
 builtin/checkout-index.c    |  6 +++---
 builtin/commit-tree.c       |  4 ++--
 builtin/commit.c            |  8 ++++----
 builtin/credential.c        |  3 ++-
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/fetch-pack.c        |  2 ++
 builtin/fsmonitor--daemon.c |  4 ++--
 builtin/gc.c                |  4 ++--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/ls-files.c          |  4 ++--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/merge.c             |  4 ++--
 builtin/pack-redundant.c    |  3 +--
 builtin/rebase.c            |  6 +++---
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-file.c       |  8 ++++++--
 builtin/unpack-objects.c    |  2 ++
 builtin/update-index.c      |  4 ++--
 builtin/upload-archive.c    |  6 +++---
 builtin/var.c               |  1 +
 git-compat-util.h           |  2 ++
 parse-options.c             | 10 ++++++++++
 parse-options.h             |  4 ++++
 t/helper/test-simple-ipc.c  |  4 ++--
 t/t0012-help.sh             |  3 ++-
 t/t7600-merge.sh            |  2 +-
 usage.c                     | 28 +++++++++++++++++++++++++---
 41 files changed, 122 insertions(+), 64 deletions(-)

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 1/6] t0012: optionally check that "-h" output goes to stdout

From: Jeff King <peff@peff.net>

So let's introduce a flag that will let most test runs use the permissive behavior, and people interested in converting commands can run:

  GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh

to see the failures. Eventually (when all builtins have been converted) we'll remove this flag entirely and always check the strict behavior.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Same as before.

 t/t0012-help.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Show changes to t/t0012-help.sh +9 −2

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..9c7ae9fd36 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -255,9 +255,16 @@ do
 		(
 			GIT_CEILING_DIRECTORIES=$(pwd) &&
 			export GIT_CEILING_DIRECTORIES &&
-			test_expect_code 129 git -C sub $builtin -h >output 2>&1
+			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		test_grep usage output
+		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
+		then
+			test_must_be_empty err &&
+			test_grep usage output
+		else
+			test_grep usage output ||
+			test_grep usage err
+		fi
 	'
 done <builtins

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 2/6] parse-options: add show_usage_with_options_if_asked()

Many commands call usage_with_options() when they are asked to give the help message, but it sends the help text to the standard error stream. When the user asked for it with "git cmd -h", the help message is the primary output from the command, hence we should send it to the standard output stream, instead.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage_with_options(usage, options);

and replaces it with

	show_usage_with_options_if_asked(argc, argv, usage, options);

to help correct code paths.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Helper function renamed.

 parse-options.c | 10 ++++++++++
 parse-options.h |  4 ++++
 2 files changed, 14 insertions(+)

Show changes to 2 files +14 −0

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..4051a8e1d1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void show_usage_with_options_if_asked(int ac, const char **av,
+				      const char * const *usagestr,
+				      const struct option *opts)
+{
+	if (ac == 2 && !strcmp(av[1], "-h")) {
+		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		exit(129);
+	}
+}
+
 void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
diff --git a/parse-options.h b/parse-options.h
index d01361ca97..39f0886254 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
+void show_usage_with_options_if_asked(int ac, const char **av,
+				      const char * const *usage,
+				      const struct option *options);
+
 NORETURN void usage_msg_opt(const char *msg,
 			    const char * const *usagestr,
 			    const struct option *options);

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 3/6] usage: add show_usage_if_asked()

Some commands call usage() when they are asked to give the help message with "git cmd -h", but this has the same problem as we fixed with callers of usage_with_options() for the same purpose.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(usage);

and replaces it with

	show_usage_if_asked(argc, argv, usage);

to help correct these code paths.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Helper function renamed.

 git-compat-util.h |  2 ++
 usage.c           | 28 +++++++++++++++++++++++++---
 2 files changed, 27 insertions(+), 3 deletions(-)

Show changes to 2 files +27 −3

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index e283c46c6f..d43dd248c4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+void show_usage_if_asked(int ac, const char **av, const char *err);
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
diff --git a/usage.c b/usage.c
index 47709006c1..d9f323a0bd 100644
--- a/usage.c
+++ b/usage.c
@@ -8,7 +8,7 @@
 #include "gettext.h"
 #include "trace2.h"
 
-static void vreportf(const char *prefix, const char *err, va_list params)
+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
@@ -32,8 +32,14 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 	}
 
 	*(p++) = '\n'; /* we no longer need a NUL */
-	fflush(stderr);
-	write_in_full(2, msg, p - msg);
+	if (fd == 2)
+		fflush(stderr);
+	write_in_full(fd, msg, p - msg);
+}
+
+static void vreportf(const char *prefix, const char *err, va_list params)
+{
+	vfdreportf(2, prefix, err, params);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -173,6 +179,22 @@ void NORETURN usage(const char *err)
 	usagef("%s", err);
 }
 
+static void show_usage_if_asked_helper(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	vfdreportf(1, _("usage: "), err, params);
+	va_end(params);
+	exit(129);
+}
+
+void show_usage_if_asked(int ac, const char **av, const char *err)
+{
+	if (ac == 2 && !strcmp(av[1], "-h"))
+		show_usage_if_asked_helper(err);
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 23:00 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/6] usage: add show_usage_if_asked()

So here is a replacement for this step. Everything else is unaffected, so I'll wait for other comments until sending a full reroll.

Thanks.

--- >8 ---
Subject: [PATCH v5 3/6] usage: add show_usage_if_asked()

Some commands call usage() when they are asked to give the help message with "git cmd -h", but this has the same problem as we fixed with callers of usage_with_options() for the same purpose.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(usage);

and replaces it with

	show_usage_if_asked(argc, argv, usage);

to help correct these code paths.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

1:  2b57538eab ! 1:  94a6fdf72f usage: add show_usage_if_asked()
    @@ Commit message
         doing exactly as asked, so there is no reason to exit with non-zero
         status.
     
    +    Helped-by: Jeff King <peff@peff.net>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## git-compat-util.h ##
    @@ usage.c
      #include "trace2.h"
      
     -static void vreportf(const char *prefix, const char *err, va_list params)
    -+static void vfdreportf(int fd, const char *prefix, const char *err, va_list params)
    ++static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)
      {
      	char msg[4096];
      	char *p, *pend = msg + sizeof(msg);
    @@ usage.c: static void vreportf(const char *prefix, const char *err, va_list param
      	*(p++) = '\n'; /* we no longer need a NUL */
     -	fflush(stderr);
     -	write_in_full(2, msg, p - msg);
    -+	if (fd == 2)
    -+		fflush(stderr);
    -+	write_in_full(fd, msg, p - msg);
    ++	fflush(f);
    ++	write_in_full(fileno(f), msg, p - msg);
     +}
     +
     +static void vreportf(const char *prefix, const char *err, va_list params)
     +{
    -+	vfdreportf(2, prefix, err, params);
    ++	vfreportf(stderr, prefix, err, params);
      }
      
      static NORETURN void usage_builtin(const char *err, va_list params)
    @@ usage.c: void NORETURN usage(const char *err)
     +	va_list params;
     +
     +	va_start(params, err);
    -+	vfdreportf(1, _("usage: "), err, params);
    ++	vfreportf(stdout, _("usage: "), err, params);
     +	va_end(params);
     +	exit(129);
     +}

 git-compat-util.h |  2 ++
 usage.c           | 27 ++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

Show changes to 2 files +26 −3

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index e283c46c6f..d43dd248c4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+void show_usage_if_asked(int ac, const char **av, const char *err);
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
diff --git a/usage.c b/usage.c
index 47709006c1..38b46bbbfe 100644
--- a/usage.c
+++ b/usage.c
@@ -8,7 +8,7 @@
 #include "gettext.h"
 #include "trace2.h"
 
-static void vreportf(const char *prefix, const char *err, va_list params)
+static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
@@ -32,8 +32,13 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 	}
 
 	*(p++) = '\n'; /* we no longer need a NUL */
-	fflush(stderr);
-	write_in_full(2, msg, p - msg);
+	fflush(f);
+	write_in_full(fileno(f), msg, p - msg);
+}
+
+static void vreportf(const char *prefix, const char *err, va_list params)
+{
+	vfreportf(stderr, prefix, err, params);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -173,6 +178,22 @@ void NORETURN usage(const char *err)
 	usagef("%s", err);
 }
 
+static void show_usage_if_asked_helper(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	vfreportf(stdout, _("usage: "), err, params);
+	va_end(params);
+	exit(129);
+}
+
+void show_usage_if_asked(int ac, const char **av, const char *err)
+{
+	if (ac == 2 && !strcmp(av[1], "-h"))
+		show_usage_if_asked_helper(err);
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;

-- 
2.48.1-210-gaa1682cadd

Jeff King· Jan 17, 2025, 11:41 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 3/6] usage: add show_usage_if_asked()

On Thu, Jan 16, 2025 at 03:00:45PM -0800, Junio C Hamano wrote:

> So here is a replacement for this step.  Everything else is
> unaffected, so I'll wait for other comments until sending a full
> reroll.

Thanks, with this replacement patch the whole series looks good to me.

-Peff

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 4/6] builtins: send usage_with_options() help text to standard output

Using the show_usage_with_options_if_asked() helper we introduced earlier, fix callers of usage_with_options() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them.

The test in t7600 for "git merge -h" may want to be retired, as the same is covered by t0012 already, but it is specifically testing that the "-h" option gets a response even with a corrupt index file, so for now let's leave it there.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Helper function renamed.

 builtin/am.c                | 3 +--
 builtin/branch.c            | 4 ++--
 builtin/checkout--worker.c  | 6 +++---
 builtin/checkout-index.c    | 6 +++---
 builtin/commit-tree.c       | 4 ++--
 builtin/commit.c            | 8 ++++----
 builtin/fsmonitor--daemon.c | 4 ++--
 builtin/gc.c                | 4 ++--
 builtin/ls-files.c          | 4 ++--
 builtin/merge.c             | 4 ++--
 builtin/rebase.c            | 6 +++---
 builtin/update-index.c      | 4 ++--
 t/helper/test-simple-ipc.c  | 4 ++--
 t/t7600-merge.sh            | 2 +-
 14 files changed, 31 insertions(+), 32 deletions(-)

Show changes to 14 files +31 −32

diff --git a/builtin/am.c b/builtin/am.c
index 1338b606fe..8d8f38fa1d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2427,8 +2427,7 @@ int cmd_am(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(usage, options);
+	show_usage_with_options_if_asked(argc, argv, usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 6e7b0cfddb..7c8b4b65b6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -784,8 +784,8 @@ int cmd_branch(int argc,
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_branch_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_branch_usage, options);
 
 	/*
 	 * Try to set sort keys from config. If config does not set any,
diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index b81002a1df..da9345a44b 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(checkout_worker_usage,
-				   checkout_worker_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 checkout_worker_usage,
+					 checkout_worker_options);
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, checkout_worker_options,
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a81501098d..e30086c7d4 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_checkout_index_usage,
-				   builtin_checkout_index_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_checkout_index_usage,
+					 builtin_checkout_index_options);
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 2ca1a57ebb..38457600a4 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage_with_options(commit_tree_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 commit_tree_usage, options);
 
 	argc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index ef5e622c07..c84062bfc1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_status_usage, builtin_status_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_status_usage, builtin_status_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,
 	struct strbuf err = STRBUF_INIT;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_usage, builtin_commit_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_commit_usage, builtin_commit_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 029dc64d6c..0820e524f1 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_fsmonitor__daemon_usage, options);
 
 	die(_("fsmonitor--daemon not supported on this platform"));
 }
diff --git a/builtin/gc.c b/builtin/gc.c
index a9b1c36de2..ea007c8e7e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -710,8 +710,8 @@ struct repository *repo UNUSED)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_gc_usage, builtin_gc_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_gc_usage, builtin_gc_options);
 
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 15499cd12b..a4431429b7 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,
 	};
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(ls_files_usage, builtin_ls_files_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 ls_files_usage, builtin_ls_files_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/merge.c b/builtin/merge.c
index 5f67007bba..ba9faf126a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,
 	void *branch_to_free;
 	int orig_argc = argc;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_merge_usage, builtin_merge_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_merge_usage, builtin_merge_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0498fff3c9..6c9eaf3788 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,
 	};
 	int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_rebase_usage,
-				   builtin_rebase_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_rebase_usage,
+					 builtin_rebase_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74bbad9f87..b2f6b1a3fb 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(update_index_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 update_index_usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index fb5927775d..03cc5eea2c 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)
 	if (argc < 2)
 		usage_with_options(simple_ipc_usage, options);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(simple_ipc_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 simple_ipc_usage, options);
 
 	if (argc == 2 && !strcmp(argv[1], "SUPPORTS_SIMPLE_IPC"))
 		return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index ef54cff4fa..2a8df29219 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git merge -h 2>usage
+		test_expect_code 129 git merge -h >usage
 	) &&
 	test_grep "[Uu]sage: git merge" broken/usage
 '

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 5/6] oddballs: send usage() help text to standard output

Using the show_usage_if_asked() helper we introduced earlier, fix callers of usage() that want to show the help text when explicitly asked by the end-user. The help text now goes to the standard output stream for them.

The callers in this step are oddballs in that their invocations of usage() are *not* guarded by

	if (argc == 2 && !strcmp(argv[1], "-h")
		usage(...);

There are (unnecessarily) being clever ones that do things like

	if (argc != 2 || !strcmp(argv[1], "-h")
		usage(...);

to say "I know I take only one argument, so argc != 2 is always an error regardless of what is in argv[]. Ah, by the way, even if argc is 2, "-h" is a request for usage text, so we do the same".

Some like "git var -h" just do not treat "-h" any specially, and let it take the same error code paths as a parameter error.

Now we cannot do the same, so these callers are rewrittin to do the show_usage_and_exit_if_asked() first and then handle the usage error the way they used to.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Helper function renamed.
   Fix fo fetch-pack.c moved from the next step and redone.

 builtin/credential.c     | 3 ++-
 builtin/fetch-pack.c     | 2 ++
 builtin/unpack-file.c    | 8 ++++++--
 builtin/upload-archive.c | 3 ++-
 builtin/var.c            | 1 +
 5 files changed, 13 insertions(+), 4 deletions(-)

Show changes to 5 files +13 −4

builtin/credential.c, builtin/fetch-pack.c, builtin/unpack-file.c, builtin/upload-archive.c, builtin/var.c

diff --git a/builtin/credential.c b/builtin/credential.c
index 14c8c6608b..f6fc948123 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -18,7 +18,8 @@ int cmd_credential(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
 		usage(usage_msg);
 	op = argv[1];
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bed2816c2d..d07eec9e55 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -75,6 +75,8 @@ int cmd_fetch_pack(int argc,
 	list_objects_filter_init(&args.filter_options);
 	args.uploadpack = "git-upload-pack";
 
+	show_usage_if_asked(argc, argv, fetch_pack_usage);
+
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 6da2825753..fb5fcbc40a 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)
 	return path;
 }
 
+static const char usage_msg[] =
+"git unpack-file <blob>";
+
 int cmd_unpack_file(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,
 {
 	struct object_id oid;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
-		usage("git unpack-file <blob>");
+	show_usage_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
+		usage(usage_msg);
 	if (repo_get_oid(the_repository, argv[1], &oid))
 		die("Not a valid object name %s", argv[1]);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 9e9343f121..9d76a31c8f 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,
 	const char *arg_cmd = "argument ";
 	int ret;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_if_asked(argc, argv, upload_archive_usage);
+	if (argc != 2)
 		usage(upload_archive_usage);
 
 	if (!enter_repo(argv[1], 0))
diff --git a/builtin/var.c b/builtin/var.c
index 1449656cc9..46d40d6fba 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -221,6 +221,7 @@ int cmd_var(int argc,
 	const struct git_var *git_var;
 	char *val;
 
+	show_usage_if_asked(argc, argv, var_usage);
 	if (argc != 2)
 		usage(var_usage);

-- 
2.48.1-191-gafe818080f

Junio C Hamano· Jan 16, 2025, 21:35 UTC · re: Junio C Hamano · lore

[PATCH v4 6/6] builtin: send usage() help text to standard output

These are the bog standard "if we got only '-h', then that is a request for help" callers. Their

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(message);

are simply replaced with

	show_usage_and_exit_if_asked(argc, argv, message);

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * Helper function renamed.

 builtin/check-ref-format.c  |  4 ++--
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/pack-redundant.c    |  3 +--
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-objects.c    |  2 ++
 builtin/upload-archive.c    |  3 +--
 t/t0012-help.sh             | 10 ++--------
 19 files changed, 37 insertions(+), 32 deletions(-)

Show changes to 19 files +37 −32

builtin/check-ref-format.c, builtin/diff-files.c, builtin/diff-index.c, builtin/diff-tree.c, builtin/fast-import.c, builtin/get-tar-commit-id.c, builtin/index-pack.c, builtin/mailsplit.c, builtin/merge-index.c, builtin/merge-ours.c, builtin/merge-recursive.c, builtin/pack-redundant.c, builtin/remote-ext.c, builtin/remote-fd.c, builtin/rev-list.c, builtin/rev-parse.c, builtin/unpack-objects.c, builtin/upload-archive.c, t/t0012-help.sh

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index cef1ffe3ce..5d80afeec0 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_check_ref_format_usage);
+	show_usage_if_asked(argc, argv,
+			    builtin_check_ref_format_usage);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
 		return check_ref_format_branch(argv[2]);
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 604b04bb2c..99b1749723 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,
 	int result;
 	unsigned options = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_files_usage);
+	show_usage_if_asked(argc, argv, diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index ebc824602e..81c0bc8ed7 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,
 	int i;
 	int result;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_cache_usage);
+	show_usage_if_asked(argc, argv, diff_cache_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 40804e7b48..e31cc797fe 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,
 	int read_stdin = 0;
 	int merge_base = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_tree_usage);
+	show_usage_if_asked(argc, argv, diff_tree_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761..2da46fecdc 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,
 {
 	unsigned int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(fast_import_usage);
+	show_usage_if_asked(argc, argv, fast_import_usage);
 
 	reset_pack_idx_option(&pack_idx_opts);
 	git_pack_config();
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index 6bec0d1854..e4cd1627b4 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =
 #define HEADERSIZE (2 * RECORDSIZE)
 
 int cmd_get_tar_commit_id(int argc,
-			  const char **argv UNUSED,
+			  const char **argv,
 			  const char *prefix,
 			  struct repository *repo UNUSED)
 {
@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, builtin_get_tar_commit_id_usage);
+
 	if (argc != 1)
 		usage(builtin_get_tar_commit_id_usage);
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0b62b2589f..d41b126ec0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,
 	 */
 	fetch_if_missing = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(index_pack_usage);
+	show_usage_if_asked(argc, argv, index_pack_usage);
 
 	disable_replace_refs();
 	fsck_options.walk = mark_link;
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 41dd304731..264df6259a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, git_mailsplit_usage);
+
 	for (argp = argv+1; *argp; argp++) {
 		const char *arg = *argp;
 
@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,
 			continue;
 		} else if ( arg[1] == 'f' ) {
 			nr = strtol(arg+2, NULL, 10);
-		} else if ( arg[1] == 'h' ) {
-			usage(git_mailsplit_usage);
 		} else if ( arg[1] == 'b' && !arg[2] ) {
 			allow_bare = 1;
 		} else if (!strcmp(arg, "--keep-cr")) {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 342699edb7..3314fb1336 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -75,6 +75,9 @@ static void merge_all(void)
 	}
 }
 
+static const char usage_string[] =
+"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])";
+
 int cmd_merge_index(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
+	show_usage_if_asked(argc, argv, usage_string);
+
 	if (argc < 3)
-		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
+		usage(usage_string);
 
 	repo_read_index(the_repository);
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 3ecd9172f1..97b8a792c7 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,
 		   const char *prefix UNUSED,
 		   struct repository *repo UNUSED)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_merge_ours_usage);
+	show_usage_if_asked(argc, argv, builtin_merge_ours_usage);
 
 	/*
 	 * The contents of the current index becomes the tree we
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 1dd295558b..abfc060e28 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,6 +38,12 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
+	if (argc == 2 && !strcmp(argv[1], "-h")) {
+		struct strbuf msg = STRBUF_INIT;
+		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
+		show_usage_if_asked(argc, argv, msg.buf);
+	}
+
 	if (argc < 4)
 		usagef(builtin_merge_recursive_usage, argv[0]);
 
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index e046575871..3febe732f8 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
 	struct strbuf idx_name = STRBUF_INIT;
 	char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(pack_redundant_usage);
+	show_usage_if_asked(argc, argv, pack_redundant_usage);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 33c8ae0fc7..bd2037f27d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,
 {
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, usage_msg);
+
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index ae896eda57..39908546ba 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, usage_msg);
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 3196da7b2d..28f148049f 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,
 	const char *show_progress = NULL;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(rev_list_usage);
+	show_usage_if_asked(argc, argv, rev_list_usage);
 
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 949747a6b6..428c866c05 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;
 
+	show_usage_if_asked(argc, argv, builtin_rev_parse_usage);
+
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
 
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 2197d6d933..8faa6024b2 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,
 
 	quiet = !isatty(2);
 
+	show_usage_if_asked(argc, argv, unpack_usage);
+
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 9d76a31c8f..97d7c9522f 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -93,8 +93,7 @@ struct repository *repo UNUSED)
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(upload_archive_usage);
+	show_usage_if_asked(argc, argv, upload_archive_usage);
 
 	/*
 	 * Set up sideband subprocess.
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 9c7ae9fd36..d3a0967e9d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -257,14 +257,8 @@ do
 			export GIT_CEILING_DIRECTORIES &&
 			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
-		then
-			test_must_be_empty err &&
-			test_grep usage output
-		else
-			test_grep usage output ||
-			test_grep usage err
-		fi
+		test_must_be_empty err &&
+		test_grep usage output
 	'
 done <builtins

-- 
2.48.1-191-gafe818080f

Jeff King· Jan 17, 2025, 11:42 UTC · re: Junio C Hamano · lore

Re: [PATCH v4 6/6] builtin: send usage() help text to standard output

On Thu, Jan 16, 2025 at 01:35:53PM -0800, Junio C Hamano wrote:

Show 17 quoted lines

> --- a/t/t0012-help.sh
> +++ b/t/t0012-help.sh
> @@ -257,14 +257,8 @@ do
>  			export GIT_CEILING_DIRECTORIES &&
>  			test_expect_code 129 git -C sub $builtin -h >output 2>err
>  		) &&
> -		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
> -		then
> -			test_must_be_empty err &&
> -			test_grep usage output
> -		else
> -			test_grep usage output ||
> -			test_grep usage err
> -		fi
> +		test_must_be_empty err &&
> +		test_grep usage output
>  	'

Just a side note for the future: you wondered if we might ever get bitten by insisting on an empty stderr here (since it might catch warnings, etc).

If we do, I think it would be OK to just drop the test_must_be_empty line. Now that we are capturing only stdout in "output", checking that the usage message is there is probably sufficient.

In the meantime, I think I prefer keeping the slightly more strict form above.

-Peff

Junio C Hamano· Jan 17, 2025, 19:46 UTC · re: Jeff King · lore

Re: [PATCH v4 6/6] builtin: send usage() help text to standard output

Jeff King <peff@peff.net> writes:

> If we do, I think it would be OK to just drop the test_must_be_empty
> line. Now that we are capturing only stdout in "output", checking that
> the usage message is there is probably sufficient.

Yes.

> In the meantime, I think I prefer keeping the slightly more strict form
> above.

I am also in favor of keeping it, if only to learn why we do not want the overly strict check when the time comes. That would mean we would be loosening with a concrete reason why we would want to loose it at that time, instead of guessing that it might be overly strict without seeing the fallout before seeing any.

Thanks.

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 0/6] Send help text from "git cmd -h" to stdout

This series teachs "git $cmd -h" to send its help text to the standard output stream consistently for built-in commands.

Changes since v4:

 - vfdreportf() helper that took a file descriptor has been replaced
   with vfreportf() that takes a FILE * instead, with Peff's help.

[Reference]

https://lore.kernel.org/git/04cfaa3b-847f-4850-9dd6-c1cf9f72807f@uni-muenster.de/

Jeff King (1):
  t0012: optionally check that "-h" output goes to stdout

Junio C Hamano (5):
  parse-options: add show_usage_with_options_if_asked()
  usage: add show_usage_if_asked()
  builtins: send usage_with_options() help text to standard output
  oddballs: send usage() help text to standard output
  builtin: send usage() help text to standard output

 builtin/am.c                |  3 +--
 builtin/branch.c            |  4 ++--
 builtin/check-ref-format.c  |  4 ++--
 builtin/checkout--worker.c  |  6 +++---
 builtin/checkout-index.c    |  6 +++---
 builtin/commit-tree.c       |  4 ++--
 builtin/commit.c            |  8 ++++----
 builtin/credential.c        |  3 ++-
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/fetch-pack.c        |  2 ++
 builtin/fsmonitor--daemon.c |  4 ++--
 builtin/gc.c                |  4 ++--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/ls-files.c          |  4 ++--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/merge.c             |  4 ++--
 builtin/pack-redundant.c    |  3 +--
 builtin/rebase.c            |  6 +++---
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-file.c       |  8 ++++++--
 builtin/unpack-objects.c    |  2 ++
 builtin/update-index.c      |  4 ++--
 builtin/upload-archive.c    |  6 +++---
 builtin/var.c               |  1 +
 git-compat-util.h           |  2 ++
 parse-options.c             | 10 ++++++++++
 parse-options.h             |  4 ++++
 t/helper/test-simple-ipc.c  |  4 ++--
 t/t0012-help.sh             |  3 ++-
 t/t7600-merge.sh            |  2 +-
 usage.c                     | 27 ++++++++++++++++++++++++---
 41 files changed, 121 insertions(+), 64 deletions(-)

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 1/6] t0012: optionally check that "-h" output goes to stdout

From: Jeff King <peff@peff.net>

So let's introduce a flag that will let most test runs use the permissive behavior, and people interested in converting commands can run:

  GIT_TEST_HELP_MUST_BE_STDOUT=1 ./t0012-help.sh

to see the failures. Eventually (when all builtins have been converted) we'll remove this flag entirely and always check the strict behavior.

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t0012-help.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

Show changes to t/t0012-help.sh +9 −2

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..9c7ae9fd36 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -255,9 +255,16 @@ do
 		(
 			GIT_CEILING_DIRECTORIES=$(pwd) &&
 			export GIT_CEILING_DIRECTORIES &&
-			test_expect_code 129 git -C sub $builtin -h >output 2>&1
+			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		test_grep usage output
+		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
+		then
+			test_must_be_empty err &&
+			test_grep usage output
+		else
+			test_grep usage output ||
+			test_grep usage err
+		fi
 	'
 done <builtins

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 2/6] parse-options: add show_usage_with_options_if_asked()

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage_with_options(usage, options);

and replaces it with

	show_usage_with_options_if_asked(argc, argv, usage, options);

to help correct code paths.

Suggested-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 parse-options.c | 10 ++++++++++
 parse-options.h |  4 ++++
 2 files changed, 14 insertions(+)

Show changes to 2 files +14 −0

parse-options.c, parse-options.h

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..4051a8e1d1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1282,6 +1282,16 @@ void NORETURN usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void show_usage_with_options_if_asked(int ac, const char **av,
+				      const char * const *usagestr,
+				      const struct option *opts)
+{
+	if (ac == 2 && !strcmp(av[1], "-h")) {
+		usage_with_options_internal(NULL, usagestr, opts, 0, 0);
+		exit(129);
+	}
+}
+
 void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
diff --git a/parse-options.h b/parse-options.h
index d01361ca97..39f0886254 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -402,6 +402,10 @@ int parse_options(int argc, const char **argv, const char *prefix,
 NORETURN void usage_with_options(const char * const *usagestr,
 				 const struct option *options);
 
+void show_usage_with_options_if_asked(int ac, const char **av,
+				      const char * const *usage,
+				      const struct option *options);
+
 NORETURN void usage_msg_opt(const char *msg,
 			    const char * const *usagestr,
 			    const struct option *options);

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 3/6] usage: add show_usage_if_asked()

Some commands call usage() when they are asked to give the help message with "git cmd -h", but this has the same problem as we fixed with callers of usage_with_options() for the same purpose.

Introduce a helper function that captures the common pattern

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(usage);

and replaces it with

	show_usage_if_asked(argc, argv, usage);

to help correct these code paths.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-compat-util.h |  2 ++
 usage.c           | 27 ++++++++++++++++++++++++---
 2 files changed, 26 insertions(+), 3 deletions(-)

Show changes to 2 files +26 −3

git-compat-util.h, usage.c

diff --git a/git-compat-util.h b/git-compat-util.h
index e283c46c6f..d43dd248c4 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -701,6 +701,8 @@ int error_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning(const char *err, ...) __attribute__((format (printf, 1, 2)));
 void warning_errno(const char *err, ...) __attribute__((format (printf, 1, 2)));
 
+void show_usage_if_asked(int ac, const char **av, const char *err);
+
 #ifndef NO_OPENSSL
 #ifdef APPLE_COMMON_CRYPTO
 #include "compat/apple-common-crypto.h"
diff --git a/usage.c b/usage.c
index 47709006c1..38b46bbbfe 100644
--- a/usage.c
+++ b/usage.c
@@ -8,7 +8,7 @@
 #include "gettext.h"
 #include "trace2.h"
 
-static void vreportf(const char *prefix, const char *err, va_list params)
+static void vfreportf(FILE *f, const char *prefix, const char *err, va_list params)
 {
 	char msg[4096];
 	char *p, *pend = msg + sizeof(msg);
@@ -32,8 +32,13 @@ static void vreportf(const char *prefix, const char *err, va_list params)
 	}
 
 	*(p++) = '\n'; /* we no longer need a NUL */
-	fflush(stderr);
-	write_in_full(2, msg, p - msg);
+	fflush(f);
+	write_in_full(fileno(f), msg, p - msg);
+}
+
+static void vreportf(const char *prefix, const char *err, va_list params)
+{
+	vfreportf(stderr, prefix, err, params);
 }
 
 static NORETURN void usage_builtin(const char *err, va_list params)
@@ -173,6 +178,22 @@ void NORETURN usage(const char *err)
 	usagef("%s", err);
 }
 
+static void show_usage_if_asked_helper(const char *err, ...)
+{
+	va_list params;
+
+	va_start(params, err);
+	vfreportf(stdout, _("usage: "), err, params);
+	va_end(params);
+	exit(129);
+}
+
+void show_usage_if_asked(int ac, const char **av, const char *err)
+{
+	if (ac == 2 && !strcmp(av[1], "-h"))
+		show_usage_if_asked_helper(err);
+}
+
 void NORETURN die(const char *err, ...)
 {
 	va_list params;

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 4/6] builtins: send usage_with_options() help text to standard output

Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/am.c                | 3 +--
 builtin/branch.c            | 4 ++--
 builtin/checkout--worker.c  | 6 +++---
 builtin/checkout-index.c    | 6 +++---
 builtin/commit-tree.c       | 4 ++--
 builtin/commit.c            | 8 ++++----
 builtin/fsmonitor--daemon.c | 4 ++--
 builtin/gc.c                | 4 ++--
 builtin/ls-files.c          | 4 ++--
 builtin/merge.c             | 4 ++--
 builtin/rebase.c            | 6 +++---
 builtin/update-index.c      | 4 ++--
 t/helper/test-simple-ipc.c  | 4 ++--
 t/t7600-merge.sh            | 2 +-
 14 files changed, 31 insertions(+), 32 deletions(-)

Show changes to 14 files +31 −32

diff --git a/builtin/am.c b/builtin/am.c
index 1338b606fe..8d8f38fa1d 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2427,8 +2427,7 @@ int cmd_am(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(usage, options);
+	show_usage_with_options_if_asked(argc, argv, usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/builtin/branch.c b/builtin/branch.c
index 6e7b0cfddb..7c8b4b65b6 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -784,8 +784,8 @@ int cmd_branch(int argc,
 	filter.kind = FILTER_REFS_BRANCHES;
 	filter.abbrev = -1;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_branch_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_branch_usage, options);
 
 	/*
 	 * Try to set sort keys from config. If config does not set any,
diff --git a/builtin/checkout--worker.c b/builtin/checkout--worker.c
index b81002a1df..da9345a44b 100644
--- a/builtin/checkout--worker.c
+++ b/builtin/checkout--worker.c
@@ -128,9 +128,9 @@ int cmd_checkout__worker(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(checkout_worker_usage,
-				   checkout_worker_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 checkout_worker_usage,
+					 checkout_worker_options);
 
 	git_config(git_default_config, NULL);
 	argc = parse_options(argc, argv, prefix, checkout_worker_options,
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index a81501098d..e30086c7d4 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -250,9 +250,9 @@ int cmd_checkout_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_checkout_index_usage,
-				   builtin_checkout_index_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_checkout_index_usage,
+					 builtin_checkout_index_options);
 	git_config(git_default_config, NULL);
 	prefix_length = prefix ? strlen(prefix) : 0;
 
diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 2ca1a57ebb..38457600a4 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -119,8 +119,8 @@ int cmd_commit_tree(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc < 2 || !strcmp(argv[1], "-h"))
-		usage_with_options(commit_tree_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 commit_tree_usage, options);
 
 	argc = parse_options(argc, argv, prefix, options, commit_tree_usage, 0);
 
diff --git a/builtin/commit.c b/builtin/commit.c
index ef5e622c07..c84062bfc1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1559,8 +1559,8 @@ struct repository *repo UNUSED)
 		OPT_END(),
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_status_usage, builtin_status_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_status_usage, builtin_status_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
@@ -1736,8 +1736,8 @@ int cmd_commit(int argc,
 	struct strbuf err = STRBUF_INIT;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_commit_usage, builtin_commit_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_commit_usage, builtin_commit_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/fsmonitor--daemon.c b/builtin/fsmonitor--daemon.c
index 029dc64d6c..0820e524f1 100644
--- a/builtin/fsmonitor--daemon.c
+++ b/builtin/fsmonitor--daemon.c
@@ -1598,8 +1598,8 @@ int cmd_fsmonitor__daemon(int argc, const char **argv, const char *prefix UNUSED
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_fsmonitor__daemon_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_fsmonitor__daemon_usage, options);
 
 	die(_("fsmonitor--daemon not supported on this platform"));
 }
diff --git a/builtin/gc.c b/builtin/gc.c
index a9b1c36de2..ea007c8e7e 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -710,8 +710,8 @@ struct repository *repo UNUSED)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_gc_usage, builtin_gc_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_gc_usage, builtin_gc_options);
 
 	strvec_pushl(&reflog, "reflog", "expire", "--all", NULL);
 	strvec_pushl(&repack, "repack", "-d", "-l", NULL);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 15499cd12b..a4431429b7 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -644,8 +644,8 @@ int cmd_ls_files(int argc,
 	};
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(ls_files_usage, builtin_ls_files_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 ls_files_usage, builtin_ls_files_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/merge.c b/builtin/merge.c
index 5f67007bba..ba9faf126a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1300,8 +1300,8 @@ int cmd_merge(int argc,
 	void *branch_to_free;
 	int orig_argc = argc;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_merge_usage, builtin_merge_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_merge_usage, builtin_merge_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/rebase.c b/builtin/rebase.c
index 0498fff3c9..6c9eaf3788 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1223,9 +1223,9 @@ int cmd_rebase(int argc,
 	};
 	int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(builtin_rebase_usage,
-				   builtin_rebase_options);
+	show_usage_with_options_if_asked(argc, argv,
+					 builtin_rebase_usage,
+					 builtin_rebase_options);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.command_requires_full_index = 0;
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 74bbad9f87..b2f6b1a3fb 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1045,8 +1045,8 @@ int cmd_update_index(int argc,
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(update_index_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 update_index_usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git a/t/helper/test-simple-ipc.c b/t/helper/test-simple-ipc.c
index fb5927775d..03cc5eea2c 100644
--- a/t/helper/test-simple-ipc.c
+++ b/t/helper/test-simple-ipc.c
@@ -612,8 +612,8 @@ int cmd__simple_ipc(int argc, const char **argv)
 	if (argc < 2)
 		usage_with_options(simple_ipc_usage, options);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(simple_ipc_usage, options);
+	show_usage_with_options_if_asked(argc, argv,
+					 simple_ipc_usage, options);
 
 	if (argc == 2 && !strcmp(argv[1], "SUPPORTS_SIMPLE_IPC"))
 		return 0;
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index ef54cff4fa..2a8df29219 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -173,7 +173,7 @@ test_expect_success 'merge -h with invalid index' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git merge -h 2>usage
+		test_expect_code 129 git merge -h >usage
 	) &&
 	test_grep "[Uu]sage: git merge" broken/usage
 '

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 5/6] oddballs: send usage() help text to standard output

The callers in this step are oddballs in that their invocations of usage() are *not* guarded by

	if (argc == 2 && !strcmp(argv[1], "-h")
		usage(...);

There are (unnecessarily) being clever ones that do things like

	if (argc != 2 || !strcmp(argv[1], "-h")
		usage(...);

to say "I know I take only one argument, so argc != 2 is always an error regardless of what is in argv[]. Ah, by the way, even if argc is 2, "-h" is a request for usage text, so we do the same".

Some like "git var -h" just do not treat "-h" any specially, and let it take the same error code paths as a parameter error.

Now we cannot do the same, so these callers are rewrittin to do the show_usage_and_exit_if_asked() first and then handle the usage error the way they used to.

Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/credential.c     | 3 ++-
 builtin/fetch-pack.c     | 2 ++
 builtin/unpack-file.c    | 8 ++++++--
 builtin/upload-archive.c | 3 ++-
 builtin/var.c            | 1 +
 5 files changed, 13 insertions(+), 4 deletions(-)

Show changes to 5 files +13 −4

builtin/credential.c, builtin/fetch-pack.c, builtin/unpack-file.c, builtin/upload-archive.c, builtin/var.c

diff --git a/builtin/credential.c b/builtin/credential.c
index 14c8c6608b..f6fc948123 100644
--- a/builtin/credential.c
+++ b/builtin/credential.c
@@ -18,7 +18,8 @@ int cmd_credential(int argc,
 
 	git_config(git_default_config, NULL);
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
 		usage(usage_msg);
 	op = argv[1];
 
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index bed2816c2d..d07eec9e55 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -75,6 +75,8 @@ int cmd_fetch_pack(int argc,
 	list_objects_filter_init(&args.filter_options);
 	args.uploadpack = "git-upload-pack";
 
+	show_usage_if_asked(argc, argv, fetch_pack_usage);
+
 	for (i = 1; i < argc && *argv[i] == '-'; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/unpack-file.c b/builtin/unpack-file.c
index 6da2825753..fb5fcbc40a 100644
--- a/builtin/unpack-file.c
+++ b/builtin/unpack-file.c
@@ -26,6 +26,9 @@ static char *create_temp_file(struct object_id *oid)
 	return path;
 }
 
+static const char usage_msg[] =
+"git unpack-file <blob>";
+
 int cmd_unpack_file(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -33,8 +36,9 @@ int cmd_unpack_file(int argc,
 {
 	struct object_id oid;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
-		usage("git unpack-file <blob>");
+	show_usage_if_asked(argc, argv, usage_msg);
+	if (argc != 2)
+		usage(usage_msg);
 	if (repo_get_oid(the_repository, argv[1], &oid))
 		die("Not a valid object name %s", argv[1]);
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 9e9343f121..9d76a31c8f 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -27,7 +27,8 @@ int cmd_upload_archive_writer(int argc,
 	const char *arg_cmd = "argument ";
 	int ret;
 
-	if (argc != 2 || !strcmp(argv[1], "-h"))
+	show_usage_if_asked(argc, argv, upload_archive_usage);
+	if (argc != 2)
 		usage(upload_archive_usage);
 
 	if (!enter_repo(argv[1], 0))
diff --git a/builtin/var.c b/builtin/var.c
index 1449656cc9..46d40d6fba 100644
--- a/builtin/var.c
+++ b/builtin/var.c
@@ -221,6 +221,7 @@ int cmd_var(int argc,
 	const struct git_var *git_var;
 	char *val;
 
+	show_usage_if_asked(argc, argv, var_usage);
 	if (argc != 2)
 		usage(var_usage);

-- 
2.48.1-218-gc7e8be6a8f

Junio C Hamano· Jan 17, 2025, 21:31 UTC · re: Junio C Hamano · lore

[PATCH v5 6/6] builtin: send usage() help text to standard output

These are the bog standard "if we got only '-h', then that is a request for help" callers. Their

	if (argc == 2 && !strcmp(argv[1], "-h"))
		usage(message);

are simply replaced with

	show_usage_and_exit_if_asked(argc, argv, message);

Acked-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/check-ref-format.c  |  4 ++--
 builtin/diff-files.c        |  3 +--
 builtin/diff-index.c        |  3 +--
 builtin/diff-tree.c         |  3 +--
 builtin/fast-import.c       |  3 +--
 builtin/get-tar-commit-id.c |  4 +++-
 builtin/index-pack.c        |  3 +--
 builtin/mailsplit.c         |  4 ++--
 builtin/merge-index.c       |  7 ++++++-
 builtin/merge-ours.c        |  3 +--
 builtin/merge-recursive.c   |  6 ++++++
 builtin/pack-redundant.c    |  3 +--
 builtin/remote-ext.c        |  2 ++
 builtin/remote-fd.c         |  1 +
 builtin/rev-list.c          |  3 +--
 builtin/rev-parse.c         |  2 ++
 builtin/unpack-objects.c    |  2 ++
 builtin/upload-archive.c    |  3 +--
 t/t0012-help.sh             | 10 ++--------
 19 files changed, 37 insertions(+), 32 deletions(-)

Show changes to 19 files +37 −32

diff --git a/builtin/check-ref-format.c b/builtin/check-ref-format.c
index cef1ffe3ce..5d80afeec0 100644
--- a/builtin/check-ref-format.c
+++ b/builtin/check-ref-format.c
@@ -64,8 +64,8 @@ int cmd_check_ref_format(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_check_ref_format_usage);
+	show_usage_if_asked(argc, argv,
+			    builtin_check_ref_format_usage);
 
 	if (argc == 3 && !strcmp(argv[1], "--branch"))
 		return check_ref_format_branch(argv[2]);
diff --git a/builtin/diff-files.c b/builtin/diff-files.c
index 604b04bb2c..99b1749723 100644
--- a/builtin/diff-files.c
+++ b/builtin/diff-files.c
@@ -29,8 +29,7 @@ int cmd_diff_files(int argc,
 	int result;
 	unsigned options = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_files_usage);
+	show_usage_if_asked(argc, argv, diff_files_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-index.c b/builtin/diff-index.c
index ebc824602e..81c0bc8ed7 100644
--- a/builtin/diff-index.c
+++ b/builtin/diff-index.c
@@ -26,8 +26,7 @@ int cmd_diff_index(int argc,
 	int i;
 	int result;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_cache_usage);
+	show_usage_if_asked(argc, argv, diff_cache_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/diff-tree.c b/builtin/diff-tree.c
index 40804e7b48..e31cc797fe 100644
--- a/builtin/diff-tree.c
+++ b/builtin/diff-tree.c
@@ -122,8 +122,7 @@ int cmd_diff_tree(int argc,
 	int read_stdin = 0;
 	int merge_base = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(diff_tree_usage);
+	show_usage_if_asked(argc, argv, diff_tree_usage);
 
 	git_config(git_diff_basic_config, NULL); /* no "diff" UI options */
 
diff --git a/builtin/fast-import.c b/builtin/fast-import.c
index 0f86392761..2da46fecdc 100644
--- a/builtin/fast-import.c
+++ b/builtin/fast-import.c
@@ -3565,8 +3565,7 @@ int cmd_fast_import(int argc,
 {
 	unsigned int i;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(fast_import_usage);
+	show_usage_if_asked(argc, argv, fast_import_usage);
 
 	reset_pack_idx_option(&pack_idx_opts);
 	git_pack_config();
diff --git a/builtin/get-tar-commit-id.c b/builtin/get-tar-commit-id.c
index 6bec0d1854..e4cd1627b4 100644
--- a/builtin/get-tar-commit-id.c
+++ b/builtin/get-tar-commit-id.c
@@ -13,7 +13,7 @@ static const char builtin_get_tar_commit_id_usage[] =
 #define HEADERSIZE (2 * RECORDSIZE)
 
 int cmd_get_tar_commit_id(int argc,
-			  const char **argv UNUSED,
+			  const char **argv,
 			  const char *prefix,
 			  struct repository *repo UNUSED)
 {
@@ -27,6 +27,8 @@ int cmd_get_tar_commit_id(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, builtin_get_tar_commit_id_usage);
+
 	if (argc != 1)
 		usage(builtin_get_tar_commit_id_usage);
 
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 0b62b2589f..d41b126ec0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1897,8 +1897,7 @@ int cmd_index_pack(int argc,
 	 */
 	fetch_if_missing = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(index_pack_usage);
+	show_usage_if_asked(argc, argv, index_pack_usage);
 
 	disable_replace_refs();
 	fsck_options.walk = mark_link;
diff --git a/builtin/mailsplit.c b/builtin/mailsplit.c
index 41dd304731..264df6259a 100644
--- a/builtin/mailsplit.c
+++ b/builtin/mailsplit.c
@@ -284,6 +284,8 @@ int cmd_mailsplit(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, git_mailsplit_usage);
+
 	for (argp = argv+1; *argp; argp++) {
 		const char *arg = *argp;
 
@@ -297,8 +299,6 @@ int cmd_mailsplit(int argc,
 			continue;
 		} else if ( arg[1] == 'f' ) {
 			nr = strtol(arg+2, NULL, 10);
-		} else if ( arg[1] == 'h' ) {
-			usage(git_mailsplit_usage);
 		} else if ( arg[1] == 'b' && !arg[2] ) {
 			allow_bare = 1;
 		} else if (!strcmp(arg, "--keep-cr")) {
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 342699edb7..3314fb1336 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -75,6 +75,9 @@ static void merge_all(void)
 	}
 }
 
+static const char usage_string[] =
+"git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])";
+
 int cmd_merge_index(int argc,
 		    const char **argv,
 		    const char *prefix UNUSED,
@@ -87,8 +90,10 @@ int cmd_merge_index(int argc,
 	 */
 	signal(SIGCHLD, SIG_DFL);
 
+	show_usage_if_asked(argc, argv, usage_string);
+
 	if (argc < 3)
-		usage("git merge-index [-o] [-q] <merge-program> (-a | [--] [<filename>...])");
+		usage(usage_string);
 
 	repo_read_index(the_repository);
 
diff --git a/builtin/merge-ours.c b/builtin/merge-ours.c
index 3ecd9172f1..97b8a792c7 100644
--- a/builtin/merge-ours.c
+++ b/builtin/merge-ours.c
@@ -23,8 +23,7 @@ int cmd_merge_ours(int argc,
 		   const char *prefix UNUSED,
 		   struct repository *repo UNUSED)
 {
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(builtin_merge_ours_usage);
+	show_usage_if_asked(argc, argv, builtin_merge_ours_usage);
 
 	/*
 	 * The contents of the current index becomes the tree we
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 1dd295558b..abfc060e28 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -38,6 +38,12 @@ int cmd_merge_recursive(int argc,
 	if (argv[0] && ends_with(argv[0], "-subtree"))
 		o.subtree_shift = "";
 
+	if (argc == 2 && !strcmp(argv[1], "-h")) {
+		struct strbuf msg = STRBUF_INIT;
+		strbuf_addf(&msg, builtin_merge_recursive_usage, argv[0]);
+		show_usage_if_asked(argc, argv, msg.buf);
+	}
+
 	if (argc < 4)
 		usagef(builtin_merge_recursive_usage, argv[0]);
 
diff --git a/builtin/pack-redundant.c b/builtin/pack-redundant.c
index e046575871..3febe732f8 100644
--- a/builtin/pack-redundant.c
+++ b/builtin/pack-redundant.c
@@ -595,8 +595,7 @@ int cmd_pack_redundant(int argc, const char **argv, const char *prefix UNUSED, s
 	struct strbuf idx_name = STRBUF_INIT;
 	char buf[GIT_MAX_HEXSZ + 2]; /* hex hash + \n + \0 */
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(pack_redundant_usage);
+	show_usage_if_asked(argc, argv, pack_redundant_usage);
 
 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
diff --git a/builtin/remote-ext.c b/builtin/remote-ext.c
index 33c8ae0fc7..bd2037f27d 100644
--- a/builtin/remote-ext.c
+++ b/builtin/remote-ext.c
@@ -202,6 +202,8 @@ int cmd_remote_ext(int argc,
 {
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, usage_msg);
+
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/remote-fd.c b/builtin/remote-fd.c
index ae896eda57..39908546ba 100644
--- a/builtin/remote-fd.c
+++ b/builtin/remote-fd.c
@@ -64,6 +64,7 @@ int cmd_remote_fd(int argc,
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
+	show_usage_if_asked(argc, argv, usage_msg);
 	if (argc != 3)
 		usage(usage_msg);
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 3196da7b2d..28f148049f 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -542,8 +542,7 @@ int cmd_rev_list(int argc,
 	const char *show_progress = NULL;
 	int ret = 0;
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(rev_list_usage);
+	show_usage_if_asked(argc, argv, rev_list_usage);
 
 	git_config(git_default_config, NULL);
 	repo_init_revisions(the_repository, &revs, prefix);
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 949747a6b6..428c866c05 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -713,6 +713,8 @@ int cmd_rev_parse(int argc,
 	int seen_end_of_options = 0;
 	enum format_type format = FORMAT_DEFAULT;
 
+	show_usage_if_asked(argc, argv, builtin_rev_parse_usage);
+
 	if (argc > 1 && !strcmp("--parseopt", argv[1]))
 		return cmd_parseopt(argc - 1, argv + 1, prefix);
 
diff --git a/builtin/unpack-objects.c b/builtin/unpack-objects.c
index 2197d6d933..8faa6024b2 100644
--- a/builtin/unpack-objects.c
+++ b/builtin/unpack-objects.c
@@ -619,6 +619,8 @@ int cmd_unpack_objects(int argc,
 
 	quiet = !isatty(2);
 
+	show_usage_if_asked(argc, argv, unpack_usage);
+
 	for (i = 1 ; i < argc; i++) {
 		const char *arg = argv[i];
 
diff --git a/builtin/upload-archive.c b/builtin/upload-archive.c
index 9d76a31c8f..97d7c9522f 100644
--- a/builtin/upload-archive.c
+++ b/builtin/upload-archive.c
@@ -93,8 +93,7 @@ struct repository *repo UNUSED)
 
 	BUG_ON_NON_EMPTY_PREFIX(prefix);
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage(upload_archive_usage);
+	show_usage_if_asked(argc, argv, upload_archive_usage);
 
 	/*
 	 * Set up sideband subprocess.
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 9c7ae9fd36..d3a0967e9d 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -257,14 +257,8 @@ do
 			export GIT_CEILING_DIRECTORIES &&
 			test_expect_code 129 git -C sub $builtin -h >output 2>err
 		) &&
-		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
-		then
-			test_must_be_empty err &&
-			test_grep usage output
-		else
-			test_grep usage output ||
-			test_grep usage err
-		fi
+		test_must_be_empty err &&
+		test_grep usage output
 	'
 done <builtins

-- 
2.48.1-218-gc7e8be6a8f

Jeff King· Jan 18, 2025, 11:42 UTC · re: Junio C Hamano · lore

Re: [PATCH v5 0/6] Send help text from "git cmd -h" to stdout

On Fri, Jan 17, 2025 at 01:31:42PM -0800, Junio C Hamano wrote:

> Changes since v4:
> 
>  - vfdreportf() helper that took a file descriptor has been replaced
>    with vfreportf() that takes a FILE * instead, with Peff's help.

Thanks, this all looks good to me.

-Peff