Git branch outputs usage message on stderr

Git branch outputs usage message on stderr

[Jonas Konrad]

[-- Attachment #1: Type: text/plain, Size: 2137 bytes --]

Hey, please find the report below.

What did you do before the bug happened? (Steps to reproduce your issue)
I opened a terminal on Arch Linux with a bash shell and called `git 
branch -h` to get a usage overview of git's `branch` command. I then 
tried processing the output with `grep` by `git branch -h | grep list` 
which gave the whole (unfiltered) output, i.e., the displayed message 
was not processed by `grep`.

What did you expect to happen? (Expected behavior)
Pipe redirects stdout to grep (here: usage info of git branch) and grep 
can filter it.

What happened instead? (Actual behavior)
`git branch -h` output was not redirected to `grep`. Instead, it went to 
stderr, bypassing the pipe. Using `|&` as pipe gives the expected result 
and confirms that the usage output is indeed redirected to stderr.

What's different between what you expected and what actually happened?
To me, `git branch -h` (invoked without any non-valid option) shall 
output its help/usage message to stdout, and exit without error status 
(exit status 0).

Anything else you want to add:
In fact, `git branch -h` exits with a non-zero status. This is coherent 
with outputting the usage message on stderr and made me think the 
behavior could be intended. I can hardly imagine, as outputting usage 
info to stderr to me only made sense if `git branch` is invoked with, 
e.g., a non-valid option (if at all). I would prefer the usage output on 
stderr even in this case, but allow for a non-zero exit status of git 
branch. Also, `git -h` gives its output on stdout.


[System Info]
git version:
git version 2.48.1
cpu: x86_64
no commit associated with this build
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
libcurl: 8.11.1
OpenSSL: OpenSSL 3.4.0 22 Oct 2024
zlib: 1.3.1
uname: Linux 6.12.8-arch1-1 #1 SMP PREEMPT_DYNAMIC Thu, 02 Jan 2025 
22:52:26 +0000 x86_64
compiler info: gnuc: 14.2
libc info: glibc: 2.40
$SHELL (typically, interactive shell): /bin/bash

Best,

-- 
Jonas Konrad, PhD Student
Institute for Geoinformatics, University Münster
Heisenbergstr. 2, 48149 Münster


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 6990 bytes --]

Re: Git branch outputs usage message on stderr

[Matěj Cepl]

[-- Attachment #1.1.1: Type: text/plain, Size: 990 bytes --]

On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:
> What did you do before the bug happened? (Steps to reproduce your issue)
> I opened a terminal on Arch Linux with a bash shell and called `git 
> branch -h` to get a usage overview of git's `branch` command. I then 
> tried processing the output with `grep` by `git branch -h | grep list` 
> which gave the whole (unfiltered) output, i.e., the displayed message 
> was not processed by `grep`.

And that is exactly the correct behaviour. In the world of UNIX,
where pipes are normal, utilities should send to the stdout
only substantial material, which could be processed down the
pipeline. Error messages, help, and similar diagnostics, should
go to stderr. Also, you know about `|&`, right?

Best,

Matěj

-- 
http://matej.ceplovi.cz/blog/, @mcepl@en.osm.town
GPG Finger: 3C76 A027 CA45 AD70 98B5  BC1D 7920 5802 880B C9D8
 
Thou shalt not lie. Thou shalt not deceive one another.
  -- Leviticus 19:11


[-- Attachment #1.2: E09FEF25D96484AC.asc --]
[-- Type: application/pgp-keys, Size: 3102 bytes --]

-----BEGIN PGP PUBLIC KEY BLOCK-----

mQGiBD2g5T0RBACZdnG/9T4JS2mlxsHeFbex1KWweKPuYTpnbu8Fe7rNYMWZ/AKc
9Vm+RuoVErm4HGsb0pL5ZPnncA+m80W8EzQm2rs8PD2mHNsUhDOGnk+0fm+25WSU
6YLzd8lttxPia75A5OqBEAmJlyJUSmoWKjAK/q1Tj5HW3+/7XqWYYCJzAwCgjR2D
irw8QP8GCoUUXxeNpIOTqzMD/j66VTln+rxYT12U4jxLlsOs5Y0LVQfUbpDFEYy9
mkWX8iNTUZsx+m6uhylamm3EkN/dW0b2sQ4D3ocZekriLPDR/X0P1XPUdcy28a6o
WZoVAKN26X+PwxSq3JCiQEJgPJeKxiLiExh3lDitNyAS0WUD/xQOqryEFb9ksGxL
R9UCA/9WUQMwgQvEUhuVB7qSnREo3+ks34Kltp71uUjuMjLk3ykSptyn8oV+XZgx
rxPAD+WOJn51yFxbo+OPNdH6wG2ZaXFj47rX6GQ9W6wI7K0QhdyQTps8KNlsJuDQ
pz7XME98ob8SszsvkPPm/gX0oWdOIqHipHnMlL684jRHCWHVjrQdTWF0ZWogQ2Vw
bCA8bWF0ZWpAY2VwbG92aS5jej6IYAQTEQIAIAIeAQIXgAIZAQUCRSoWAgYLCQgH
AwIEFQIIAwQWAgMBAAoJEOCf7yXZZISsr5sAoIAqsNcs1Sl9jrmqv7vJzL4QG68V
AJ9+30NmBClQwpmqnA26nCa4+WS5abQbTWF0ZWogQ2VwbCA8Y2VwbC5tQG5ldS5l
ZHU+iGAEExECACACGwMCHgECF4AFAkUqFgkGCwkIBwMCBBUCCAMEFgIDAQAKCRDg
n+8l2WSErAULAJoC8yrptOgooJOzLzmLxDc1mzeGDACdFBwZlvFcj1T2dmCRNdn5
cErRyBe0G01hdMSbaiBDZXBsIDxtY2VwbEBjZXBsLmV1PohiBBMRAgAiBQJQixpw
AhsDBgsJCAcDAgYVCAIJCgsEFgIDAQIeAQIXgAAKCRDgn+8l2WSErBMYAJ9eQEpi
bL6Vm7sUOhupxD/UsHiWlQCdHYi+UNpzC1mKYtDSWa1ocfO1Q760HE1hdGVqIENl
cGwgPGNlcGxtQHNlem5hbS5jej6IYAQTEQIAIAIbAwIeAQIXgAUCRSoWCQYLCQgH
AwIEFQIIAwQWAgMBAAoJEOCf7yXZZISsP14Ani6U87hSUXDU+3ZTaDRXIwasTttl
AJ0QWhjSmaJTdkkpfqmRB9bRi9pAQbQfTWF0xJtqIENlcGwgPGNlcGxAc3VyZmJl
c3QubmV0PohgBBMRAgAgAhsDAh4BAheABQJFKhYJBgsJCAcDAgQVAggDBBYCAwEA
CgkQ4J/vJdlkhKwBBwCbBOoTY52hYeKnKuU/uRjOTsUMg3IAnjTTrXYHD49xyLs8
T/Vpsuk6ZP/htCFNYXRlaiBDZXBsIDxtYXRlai5jZXBsQGdtYWlsLmNvbT6IYAQT
EQIAIAIbAwIeAQIXgAUCRSoWCQYLCQgHAwIEFQIIAwQWAgMBAAoJEOCf7yXZZISs
ki0An0Gw1MjZJATtVq11Su0mjd3rDQChAJ0eePE0amSwYVGSpSNb264+XjUotrQs
TWF0ZWogQ2VwbCAoUmVkSGF0IEN6ZWNoKSA8bWNlcGxAcmVkaGF0LmNvbT6IYAQT
EQIAIAUCRSyciwIbAwYLCQgHAwIEFQIIAwQWAgMBAh4BAheAAAoJEOCf7yXZZISs
byQAniqw1PX24BlbBD22zNqYwzfIPDhwAJ4m/3ytuJzsfxrEac1tSoEb2+H9vrQ5
TWF0ZWogQ2VwbCA8Y2VwbC1aTzRGMEtubUNESGsxdU1KU0JrUW1RQHB1YmxpYy5n
bWFuZS5vcmc+iGAEExECACACGwMCHgECF4AFAkUqFgkGCwkIBwMCBBUCCAMEFgID
AQAKCRDgn+8l2WSErAn9AJ9bO0NUqLnMDTCcchtVzK6yEOLkCgCfXwkty1uEAzQI
5kt9Gec8yQpxDli0Gk1hdGVqIENlcGwgPG1jZXBsQHN1c2UuZGU+iGMEExECACMF
Alr65CsCGwMHCwkIBwMCAQYVCAIJCgsEFgIDAQIeAQIXgAAKCRDgn+8l2WSErHjO
AJ47yF9STX/Es4qsJPjW961He9H3bgCdEsjOgt7czE87Gy0D1KXWWNTdTtW0G01h
dGVqIENlcGwgPG1jZXBsQHN1c2UuY29tPohjBBMRAgAjBQJa+uQ/AhsDBwsJCAcD
AgEGFQgCCQoLBBYCAwECHgECF4AACgkQ4J/vJdlkhKwsQQCdGmGXW73O6Q3TB0V0
xP9yLwMjDtEAnjKWDW8PKO90nx8IkPodxr1nCvJbtBpNYXRlaiBDZXBsIDxtY2Vw
bEBzdXNlLmN6PohjBBMRAgAjBQJa+uRPAhsDBwsJCAcDAgEGFQgCCQoLBBYCAwEC
HgECF4AACgkQ4J/vJdlkhKyKtQCdHDpolHg/1qDaw/4CQyUzAfNvHk0AniEYL6BF
rdyonhgQf/ZXzXjnKzSeuQENBD2g5UEQBACfxoz2nmzGJz6ueKHkTeXcQZvK4WzK
TN/uJJhEmSuQmOKymbIkGL6vBQb+W4KxvLl2lAbNlfIgLGDLCs1YAwfSpJ4vS4mt
liPgA2OtZ5j1WSOqpxedQPGVba5gVo7HNSOMUtZKTz7VsCvR94v05comhO1Gok75
ZxHtYyVHuk5V8wADBQP/ft+W4F0tccwslzz8O/c9/Mj8KZDYmfMyNb7ielT2WeQ3
iFF9AxMT6OvOxAQbDJvurfKeYlydcXLs6cy4lKce1hFaJ4i+MOFLVV1ZnZDDChRP
pQ6KrRCHLb+mLY+SYD37O7p0spQA+9gsEE/tmn+5sW7LE8hqSOoPVdf7Y5yUDj6I
RgQYEQIABgUCPaDlQQAKCRDgn+8l2WSErEUSAJ42T1l/2TFykbULBqqAtnbC6kR0
wwCdEnRlCGlvnO78R0FgKXlt3RyzGuE=
=sxoW
-----END PGP PUBLIC KEY BLOCK-----

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 216 bytes --]

Re: Git branch outputs usage message on stderr

[Jonas Konrad]

[-- Attachment #1: Type: text/plain, Size: 1844 bytes --]

Well, let me add that one detail: I could not find any other git sub 
command which behaves the same (admittedly haven't tried all). Instead, 
the ones I have tried give their help output usually on stdout. Try 
yourself with `git -h`. It perfectly acts according to what I'd expect 
(exit status 0). Where as `git reflog -h`outputs to stdout but still 
exit with non-zero status (which seems okay). `git branch -h` adds a 
third way, as described. I argue: That diverging behavior is confusing 
to the user.

If your claim was right, this bug report got even bigger as a lot of 
behavior from other subcommands would have to be changed. In fact, what 
"substantial material" is, has to be defined for every piece of software 
regarding each output. However, I cannot see how one subcommand's usage 
message is considered "substantial" whereas another subcommand's usage 
message is not (besides, I guess, `git branch` is more frequently used 
than `git reflog`).


Best,
Jonas

On 15.01.25 12:36, Matěj Cepl wrote:
> On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> I opened a terminal on Arch Linux with a bash shell and called `git
>> branch -h` to get a usage overview of git's `branch` command. I then
>> tried processing the output with `grep` by `git branch -h | grep list`
>> which gave the whole (unfiltered) output, i.e., the displayed message
>> was not processed by `grep`.
> And that is exactly the correct behaviour. In the world of UNIX,
> where pipes are normal, utilities should send to the stdout
> only substantial material, which could be processed down the
> pipeline. Error messages, help, and similar diagnostics, should
> go to stderr. Also, you know about `|&`, right?
>
> Best,
>
> Matěj
>

[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 6990 bytes --]

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Matěj Cepl <mcepl@cepl.eu> writes:

> On Wed Jan 15, 2025 at 12:22 PM CET, Jonas Konrad wrote:
>> What did you do before the bug happened? (Steps to reproduce your issue)
>> I opened a terminal on Arch Linux with a bash shell and called `git 
>> branch -h` to get a usage overview of git's `branch` command. I then 
>> tried processing the output with `grep` by `git branch -h | grep list` 
>> which gave the whole (unfiltered) output, i.e., the displayed message 
>> was not processed by `grep`.
>
> And that is exactly the correct behaviour. In the world of UNIX,
> where pipes are normal, utilities should send to the stdout
> only substantial material, ...

If I understand the case Jonas reports correctly, he is talking
about "git branch -h<RETURN>", and the "substantial material" (I'd
rather phrase it as the primary output in response to the end user
request) in that case is the help text.

Somebody may want to go over "git help --all" and for each of them
try "git $cmd -h >/dev/null" to find those that give the help output
to their standard error stream.

Thanks, both.

Re: Git branch outputs usage message on stderr

[Kristoffer Haugsbakk]

On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:
> Somebody may want to go over "git help --all" and for each of them
> try "git $cmd -h >/dev/null" to find those that give the help output
> to their standard error stream.

    #!/bin/sh

    for cmd in $(git --list-cmds=builtins); do
        git $cmd -h >/dev/null
    done 2>&1 | grep '^usage: ' \
        | perl -pe 's/^usage:\s*(\(EXPERIMENTAL!\)\s*)?//; s/^(git\s+[a-zA-Z0-9-]+).*/\1/'

Gives

    git am
    git branch
    git check-ref-format
    git checkout--worker
    git checkout-index
    git commit
    git commit-tree
    git credential
    git diff
    git diff-files
    git diff-index
    git diff-tree
    git fast-import
    git fetch-pack
    git fsmonitor--daemon
    git gc
    git get-tar-commit-id
    git index-pack
    git ls-files
    git mailsplit
    git merge
    git merge-index
    git merge-ours
    git merge-recursive
    git merge-recursive-ours
    git merge-recursive-theirs
    git merge-subtree
    git pack-redundant
    git rebase
    git remote-ext
    git remote-fd
    git rev-list
    git rev-parse
    git status
    git unpack-file
    git unpack-objects
    git update-index
    git upload-archive
    git upload-archive
    git var

For

    $ git version
    git version 2.48.0

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 05:55:19PM +0100, Kristoffer Haugsbakk wrote:

> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:
> > Somebody may want to go over "git help --all" and for each of them
> > try "git $cmd -h >/dev/null" to find those that give the help output
> > to their standard error stream.
> 
>     #!/bin/sh
> 
>     for cmd in $(git --list-cmds=builtins); do
>         git $cmd -h >/dev/null
>     done 2>&1 | grep '^usage: ' \
>         | perl -pe 's/^usage:\s*(\(EXPERIMENTAL!\)\s*)?//; s/^(git\s+[a-zA-Z0-9-]+).*/\1/'
> [...]

We may want:

diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 1d273d91c2..469cb12eb2 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -255,7 +255,8 @@ 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_must_be_empty err
 		) &&
 		test_grep usage output
 	'

which produces a similar list. In the case of git-branch, it is due to
1dacfbcf13 (branch -h: show usage even in an invalid repository,
2010-10-22). Instead of letting parse-options handle it (which then goes
to stdout), we call usage_with_options(), which is usually for
complaining about a broken option, not showing "-h".

The reason there is that some of the pre-parse_options() setup accesses
the ref store (causing a BUG() if you run "branch -h" outside of a
repository). In this case, I think it can simply be reordered like:

diff --git a/builtin/branch.c b/builtin/branch.c
index 6e7b0cfddb..4617e32fff 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -784,31 +784,28 @@ 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);
-
 	/*
 	 * Try to set sort keys from config. If config does not set any,
 	 * fall back on default (refname) sorting.
 	 */
 	git_config(git_branch_config, &sorting_options);
 	if (!sorting_options.nr)
 		string_list_append(&sorting_options, "refname");
 
 	track = git_branch_track;
 
+	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
+			     0);
+
 	head = refs_resolve_refdup(get_main_ref_store(the_repository), "HEAD",
 				   0, &head_oid, NULL);
 	if (!head)
 		die(_("failed to resolve HEAD as a valid ref"));
 	if (!strcmp(head, "HEAD"))
 		filter.detached = 1;
 	else if (!skip_prefix(head, "refs/heads/", &head))
 		die(_("HEAD not found below refs/heads!"));
 
-	argc = parse_options(argc, argv, prefix, options, builtin_branch_usage,
-			     0);
-
 	if (!delete && !rename && !copy && !edit_description && !new_upstream &&
 	    !show_current && !unset_upstream && argc == 0)
 		list = 1;

Knowing that is safe means confirming manually that setup code is not
needed by parse_options(). E.g., if it were setting defaults the user
could overwrite with an option. In this case neither "head" nor
"filter.detached" is touched by any of the options.

But there are a ton of commands, as you saw, and handling each one would
be a pain. So it would probably be easier to just introduce a variant of
usage_with_options() that writes to stdout (the underlying _internal
function already does so, we'd just need a one-liner wrapper). And then
use that everywhere. Possibly it could even do the argc/argv check, too,
since every call site is going to be doing that itself.

-Peff

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But there are a ton of commands, as you saw, and handling each one would
> be a pain. So it would probably be easier to just introduce a variant of
> usage_with_options() that writes to stdout (the underlying _internal
> function already does so, we'd just need a one-liner wrapper).

Yup, I was looking at the code involved and came to the same conclusion.

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> use that everywhere. Possibly it could even do the argc/argv check, too,
> since every call site is going to be doing that itself.

It would look something like this; I am not sure if I like the "this
may show help and exit if the user requested, but otherwise it is a
no-op" semantics, though.

 builtin/am.c    |  3 +--
 parse-options.c | 10 ++++++++++
 parse-options.h |  4 ++++
 3 files changed, 15 insertions(+), 2 deletions(-)

diff --git c/builtin/am.c w/builtin/am.c
index 370f5593f2..c9571f605a 100644
--- c/builtin/am.c
+++ w/builtin/am.c
@@ -2417,8 +2417,7 @@ int cmd_am(int argc, const char **argv, const char *prefix)
 		OPT_END()
 	};
 
-	if (argc == 2 && !strcmp(argv[1], "-h"))
-		usage_with_options(usage, options);
+	show_usage_help(argc, argv, usage, options);
 
 	git_config(git_default_config, NULL);
 
diff --git c/parse-options.c w/parse-options.c
index 30b9e68f8a..9419b174de 100644
--- c/parse-options.c
+++ w/parse-options.c
@@ -1276,6 +1276,16 @@ void NORETURN usage_with_options(const char * const *usagestr,
 	exit(129);
 }
 
+void show_usage_help(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(0);
+	}
+}
+
 void NORETURN usage_msg_opt(const char *msg,
 		   const char * const *usagestr,
 		   const struct option *options)
diff --git c/parse-options.h w/parse-options.h
index ae15342390..75a7493350 100644
--- c/parse-options.h
+++ w/parse-options.h
@@ -388,6 +388,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(int, const char **,
+		     const char * const *usagestr,
+		     const struct option *options);
+
 NORETURN void usage_msg_opt(const char *msg,
 			    const char * const *usagestr,
 			    const struct option *options);

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 09:56:23AM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > use that everywhere. Possibly it could even do the argc/argv check, too,
> > since every call site is going to be doing that itself.
> 
> It would look something like this; I am not sure if I like the "this
> may show help and exit if the user requested, but otherwise it is a
> no-op" semantics, though.

Yeah, I agree it is funny to have a "maybe noop, maybe exit" function.
Perhaps a different name would help? I'd expect show_usage_help() to
always do what the name says. Maybe check_help_option() or something?

> +void show_usage_help(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(0);
> +	}
> +}

I think parse-options will exit(129) in this case, and that's what t0012
insists upon.

-Peff

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Yeah, I agree it is funny to have a "maybe noop, maybe exit" function.
> Perhaps a different name would help? I'd expect show_usage_help() to
> always do what the name says. Maybe check_help_option() or something?

maybe_show_usage_help()?

>> +void show_usage_help(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(0);
>> +	}
>> +}
>
> I think parse-options will exit(129) in this case, and that's what t0012
> insists upon.

Yeah, but the test can be adjusted to updated reality if needed.  

In this case, the command is doing what the end-user asked it to do,
and if we were writing the system from scratch, 0 would certainly be
the right exit status in this case.  If hit usage_with_options()
because the command line option supplied by the user was nonsense,
we should exit with non-zero, but I am not sure if exit(129) is a
good idea here.

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 01:16:50PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Yeah, I agree it is funny to have a "maybe noop, maybe exit" function.
> > Perhaps a different name would help? I'd expect show_usage_help() to
> > always do what the name says. Maybe check_help_option() or something?
> 
> maybe_show_usage_help()?

Heh, I almost suggested that one, too, but worried it was too clunky.
But maybe since we both thought of it...

> > I think parse-options will exit(129) in this case, and that's what t0012
> > insists upon.
> 
> Yeah, but the test can be adjusted to updated reality if needed.  
> 
> In this case, the command is doing what the end-user asked it to do,
> and if we were writing the system from scratch, 0 would certainly be
> the right exit status in this case.  If hit usage_with_options()
> because the command line option supplied by the user was nonsense,
> we should exit with non-zero, but I am not sure if exit(129) is a
> good idea here.

I certainly see an argument for exit(0), but whatever we do should be
consistent with how parse-options handles it (since whether we use this
or leave it to parse-options is purely an implementation detail that the
user should not need to be aware of).

And it uses code 129, even for "-h". I don't see any explicit rationale
for that in the history; I think it goes back to the beginning of
parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we
also trigger that for ambiguous options (which should exit with error).
That might be a bug-in-waiting if we start handling PARSE_OPT_HELP
differently.

-Peff

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I certainly see an argument for exit(0), but whatever we do should be
> consistent with how parse-options handles it (since whether we use this
> or leave it to parse-options is purely an implementation detail that the
> user should not need to be aware of).
>
> And it uses code 129, even for "-h". I don't see any explicit rationale
> for that in the history; I think it goes back to the beginning of
> parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we
> also trigger that for ambiguous options (which should exit with error).
> That might be a bug-in-waiting if we start handling PARSE_OPT_HELP
> differently.

Here is what I have as v2; there will be patches that touch
builtin/*.c in between and I expect that the last patch to conclude
the series will end with an update to parse-options.c (to exit with
0 when asked to give a help) and t0012 (to stop expecting 129).

--- >8 ---
Subject: [PATCH v2] 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(+)

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-187-gd93ffc6ef3

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 01:56:31PM -0800, Junio C Hamano wrote:

> Here is what I have as v2; there will be patches that touch
> builtin/*.c in between and I expect that the last patch to conclude
> the series will end with an update to parse-options.c (to exit with
> 0 when asked to give a help) and t0012 (to stop expecting 129).
> 
> --- >8 ---
> Subject: [PATCH v2] parse-options: add show_usage_help_and_exit_if_asked()

Thanks, this looks fine. The name is clunky but probably OK. ;)

I don't know if we'd want something like this on top. If somebody is
interested in just doing all the conversions in the near-term, we could
do without the optional flag.

-- >8 --
Subject: [PATCH] t0012: optionally check that "-h" output goes to stdout

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>
---
 t/t0012-help.sh | 11 +++++++++--
 1 file changed, 9 insertions(+), 2 deletions(-)

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.434.g4084d8f956

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't know if we'd want something like this on top. If somebody is
> interested in just doing all the conversions in the near-term, we could
> do without the optional flag.

Ah, you are much more practical than I am ;-)  I was wondering if we
want a list of "these commands have already been updated" and behave
differently.

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

Yeah, that is why the "git branch -h" thing has been left broken for
such a long time.

> 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>
> ---
>  t/t0012-help.sh | 11 +++++++++--
>  1 file changed, 9 insertions(+), 2 deletions(-)

Nice.

It is a tangent, but I wonder how many among the 40 really needed to
use usage_with_options() to react to "-h" in the first place.  In
other words, these manual checks for "-h" are done only because the
code _wants_ to react to "-h" before it calls parse_options(), but
does everybody who _wants_ to do so really _needs_ to do so?  You
already have shown that "gir branch" did not have to, and to me, 40
among 100+ felt way too many.

> 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 &&

This may be a bit stricter than needed (things other than usage may
need to be spitted out), but it is sufficent to declare that we will
deal with any potential fallout only after it becomes necessary ;-).

> +			test_grep usage output
> +		else
> +			test_grep usage output ||
> +			test_grep usage err
> +		fi
>  	'
>  done <builtins

Will queue.  Thanks.

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

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

> It is a tangent, but I wonder how many among the 40 really needed to
> use usage_with_options() to react to "-h" in the first place.  In
> other words, these manual checks for "-h" are done only because the
> code _wants_ to react to "-h" before it calls parse_options(), but
> does everybody who _wants_ to do so really _needs_ to do so?  You
> already have shown that "gir branch" did not have to, and to me, 40
> among 100+ felt way too many.

It turns out that some of the actually cannot be helped, as they do
not even use parse-options and calling usage() themselves is the
only way to show the usage text for them.

I'll send a 6-patch series out shortly.

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 03:32:29PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I don't know if we'd want something like this on top. If somebody is
> > interested in just doing all the conversions in the near-term, we could
> > do without the optional flag.
> 
> Ah, you are much more practical than I am ;-)  I was wondering if we
> want a list of "these commands have already been updated" and behave
> differently.

Heh, I started writing it that way but then got turned off by how
annoying it is to look up in a list of strings in bash. ;)

> It is a tangent, but I wonder how many among the 40 really needed to
> use usage_with_options() to react to "-h" in the first place.  In
> other words, these manual checks for "-h" are done only because the
> code _wants_ to react to "-h" before it calls parse_options(), but
> does everybody who _wants_ to do so really _needs_ to do so?  You
> already have shown that "gir branch" did not have to, and to me, 40
> among 100+ felt way too many.

Yes, I had the same thought. Unfortunately it is a lot of brain-power to
examine each one, with relatively little gain.

> > -		test_grep usage output
> > +		if test -n "$GIT_TEST_HELP_MUST_BE_STDOUT"
> > +		then
> > +			test_must_be_empty err &&
> 
> This may be a bit stricter than needed (things other than usage may
> need to be spitted out), but it is sufficent to declare that we will
> deal with any potential fallout only after it becomes necessary ;-).

Yep. I'd hope for the most part that "-h" would spew help and nothing
else, but I guess we could get hit with a deprecation notice or
something. I agree on crossing that bridge when we come to it.

-Peff

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> And it uses code 129, even for "-h". I don't see any explicit rationale
> for that in the history; I think it goes back to the beginning of
> parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we
> also trigger that for ambiguous options (which should exit with error).
> That might be a bug-in-waiting if we start handling PARSE_OPT_HELP
> differently.

There is another class of callers that are protected by the same
"argc == 2 && !strcmp(argv[1], "-h")" condition, and they call
usage.c:usage(), instead of calling usage_with_options().  These
calls (but not all calls to usage()) need to be updated to use a
similar helper, say, show_usage_and_exit_if_asked().  Sigh...

Re: Git branch outputs usage message on stderr

[Jeff King]

On Wed, Jan 15, 2025 at 02:11:56PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > And it uses code 129, even for "-h". I don't see any explicit rationale
> > for that in the history; I think it goes back to the beginning of
> > parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we
> > also trigger that for ambiguous options (which should exit with error).
> > That might be a bug-in-waiting if we start handling PARSE_OPT_HELP
> > differently.
> 
> There is another class of callers that are protected by the same
> "argc == 2 && !strcmp(argv[1], "-h")" condition, and they call
> usage.c:usage(), instead of calling usage_with_options().  These
> calls (but not all calls to usage()) need to be updated to use a
> similar helper, say, show_usage_and_exit_if_asked().  Sigh...

Oof. And that uses vreportf(), which always writes to stderr. So more
refactoring.

-Peff

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jan 15, 2025 at 02:11:56PM -0800, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>> 
>> > And it uses code 129, even for "-h". I don't see any explicit rationale
>> > for that in the history; I think it goes back to the beginning of
>> > parse-options. It happens via the PARSE_OPT_HELP flag, but curiously we
>> > also trigger that for ambiguous options (which should exit with error).
>> > That might be a bug-in-waiting if we start handling PARSE_OPT_HELP
>> > differently.
>> 
>> There is another class of callers that are protected by the same
>> "argc == 2 && !strcmp(argv[1], "-h")" condition, and they call
>> usage.c:usage(), instead of calling usage_with_options().  These
>> calls (but not all calls to usage()) need to be updated to use a
>> similar helper, say, show_usage_and_exit_if_asked().  Sigh...
>
> Oof. And that uses vreportf(), which always writes to stderr. So more
> refactoring.

Yes, it's ugly ;-)

In any case, this is a result of my sweek in builtin/ for
usage_with_options().  The remaining

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

are all protecting usage() call.

---- >8 ----
Subject: [PATCH] builtins: send 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.

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(-)

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-187-gd93ffc6ef3

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

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

> Jeff King <peff@peff.net> writes:
>
>> use that everywhere. Possibly it could even do the argc/argv check, too,
>> since every call site is going to be doing that itself.
>
> It would look something like this; I am not sure if I like the "this
> may show help and exit if the user requested, but otherwise it is a
> no-op" semantics, though.

After thinking about it a bit more, I am starting to like it, not
because it reduces a few more lines from the calling site, but
because it makes it almost impossible to use for a careless and
incorrect conversion from usage_with_options().  Any existing
callsite of usage_with_options() MUST be inspected to make sure it
is responding to an end-user request to give "-h"(elp) text before
being replaced with the call to a new helper, and if I made
show_usage_help() without argc/argv, a careless developer can easily
and blindly do string replacement to send a ton of patch that
reviewers need to waste time to do the inspection for them.

But with your "argc and argv checking included" approach, it is
harder to do the blind replacement.

--- >8 ---
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 15 Jan 2025 09:56:23 -0800
Subject: [PATCH] parse-options: add show_usage_help()

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(argc, argv, usage, options);

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

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(+)

diff --git a/parse-options.c b/parse-options.c
index 33bfba0ed4..4b00065692 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(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(0);
+	}
+}
+
 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..f93f13434c 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(int, const char **,
+		     const char * const *usagestr,
+		     const struct option *options);
+
 NORETURN void usage_msg_opt(const char *msg,
 			    const char * const *usagestr,
 			    const struct option *options);
-- 
2.48.1-187-gd93ffc6ef3

Re: Git branch outputs usage message on stderr

[Kristoffer Haugsbakk]

On Wed, Jan 15, 2025, at 19:29, Junio C Hamano wrote:
> From: Junio C Hamano <gitster@pobox.com>
> Date: Wed, 15 Jan 2025 09:56:23 -0800
> Subject: [PATCH] parse-options: add show_usage_help()
>
> 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(argc, argv, usage, options);
>
> to help correct code paths (there are 40 or so of them).
>
> Suggested-by: Jeff King <peff@peff.net>

+Reported-by: Jonas Konrad <jonas.konrad@uni-muenster.de>

> Signed-off-by: Junio C Hamano <gitster@pobox.com>

-- 
Kristoffer Haugsbakk

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

>> and replaces it with
>>
>> 	show_usage_help(argc, argv, usage, options);
>>
>> to help correct code paths (there are 40 or so of them).
>>
>> Suggested-by: Jeff King <peff@peff.net>
>
> +Reported-by: Jonas Konrad <jonas.konrad@uni-muenster.de>

Not on this patch, but it would be a good addition to a follow-up
patch that uses this new API function to update builtin/branch.c
(which I do not plan to do myself).

Thanks.

Re: Git branch outputs usage message on stderr

[Jonas Konrad]

[-- Attachment #1: Type: text/plain, Size: 1193 bytes --]

On 15.01.25 17:55, Kristoffer Haugsbakk wrote:
> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:
>> Somebody may want to go over "git help --all" and for each of them
>> try "git $cmd -h >/dev/null" to find those that give the help output
>> to their standard error stream.
>      #!/bin/sh
>
>      for cmd in $(git --list-cmds=builtins); do
>          git $cmd -h >/dev/null
>      done 2>&1 | grep '^usage: ' \
>          | perl -pe 's/^usage:\s*(\(EXPERIMENTAL!\)\s*)?//; s/^(git\s+[a-zA-Z0-9-]+).*/\1/'
>
> Gives
>
>      git am
>      [...]
> For
>
>      $ git version
>      git version 2.48.0

Was just about to share the very same results, leading to 40 commands 
out of 142 built-ins outputting their usage info to stderr. Some 
additions on non-builtins: git-scalar also outputs its usage info to 
stderr. git-lfs does not have "usage text for -h". I have not tested 
git-svn and git-cvsserver properly (do not have installed the respective 
modules). On another note, git-p4 does not know "-h", but then gives 
usage info - to stdout(!)). Lastly, if you still read, test 
git-http-backend and git-filter-branch, as they show special behavior.


[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 6990 bytes --]

Re: Git branch outputs usage message on stderr

[Kristoffer Haugsbakk]

On Wed, Jan 15, 2025, at 18:14, Jonas Konrad wrote:
> On 15.01.25 17:55, Kristoffer Haugsbakk wrote:
>> [snip]
>
> Was just about to share the very same results, leading to 40 commands
> out of 142 built-ins outputting their usage info to stderr. Some
> additions on non-builtins: git-scalar also outputs its usage info to
> stderr. git-lfs does not have "usage text for -h". I have not tested
> git-svn and git-cvsserver properly (do not have installed the respective
> modules). On another note, git-p4 does not know "-h", but then gives
> usage info - to stdout(!)). Lastly, if you still read, test
> git-http-backend and git-filter-branch, as they show special behavior.

Okay, `git-http-backend` is listed in `git --list-cmds=main`.

That one doesn’t support `-h`.

    $ git http-backend -h >/dev/null
    fatal: No REQUEST_METHOD from server

and

    $ git http-backend -h 2>/dev/null
    Status: 500 Internal Server Error
    Expires: Fri, 01 Jan 1980 00:00:00 GMT
    Pragma: no-cache
    Cache-Control: no-cache, max-age=0, must-revalidate

git-filter-branch(1) prints a warning about “glut of gotchas”, waits 10
seconds, then prints usage to stdout.

    $ time git filter-branch -h >/dev/null

    real	0m10,058s
    user	0m0,023s
    sys	0m0,041s

-- 
Kristoffer Haugsbakk

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> On Wed, Jan 15, 2025, at 16:28, Junio C Hamano wrote:
>> Somebody may want to go over "git help --all" and for each of them
>> try "git $cmd -h >/dev/null" to find those that give the help output
>> to their standard error stream.
>
>     #!/bin/sh
>
>     for cmd in $(git --list-cmds=builtins); do
>         git $cmd -h >/dev/null
>     done 2>&1 | grep '^usage: ' \
>         | perl -pe 's/^usage:\s*(\(EXPERIMENTAL!\)\s*)?//; s/^(git\s+[a-zA-Z0-9-]+).*/\1/'
>
> Gives ...

Being consistent is a good idea, and I wanted to first gauge which
way we should unify.  It seems that those who spit their help text
into their standard error stream are indeed in minority?

Thanks.

Re: Git branch outputs usage message on stderr

[Kristoffer Haugsbakk]

On Wed, Jan 15, 2025, at 18:19, Junio C Hamano wrote:
> "Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:
>> [snip]
>
> Being consistent is a good idea, and I wanted to first gauge which
> way we should unify.  It seems that those who spit their help text
> into their standard error stream are indeed in minority?

Yes: 40 of those stderr `-h` outputs.

Versus 102 that use stdout.[1]

Trying a random command with usage-on-error:

    $ git-upload-pack >/dev/null
    usage: [snip]

Does give usage on stderr.

† 1:
    git add
    git annotate
    git apply
    git archive
    git bisect
    git blame
    git bugreport
    git bundle
    git cat-file
    git check-attr
    git check-ignore
    git check-mailmap
    git checkout
    git cherry
    git cherry-pick
    git clean
    git clone
    git column
    git commit-graph
    git config
    git count-objects
    git credential-cache
    git credential-cache--daemon
    git credential-store
    git describe
    git diagnose
    git difftool
    git fast-export
    git fetch
    git fmt-merge-msg
    git for-each-ref
    git for-each-repo
    git format-patch
    git fsck
    git fsck
    git grep
    git hash-object
    git help
    git hook
    git init
    git init
    git interpret-trailers
    git log
    git ls-remote
    git ls-tree
    git mailinfo
    git maintenance
    git merge-base
    git merge-file
    git merge-tree
    git mktag
    git mktree
    git multi-pack-index
    git mv
    git name-rev
    git notes
    git pack-objects
    git pack-refs
    git patch-id
    git blame
    git prune
    git prune-packed
    git pull
    git push
    git range-diff
    git read-tree
    git receive-pack
    git reflog
    git refs
    git remote
    git repack
    git replace
    git replay
    git rerere
    git reset
    git restore
    git revert
    git rm
    git send-pack
    git shortlog
    git log
    git show-branch
    git show-index
    git show-ref
    git sparse-checkout
    git add
    git stash
    git stripspace
    git submodule--helper
    git switch
    git symbolic-ref
    git tag
    git update-ref
    git update-server-info
    git-upload-pack
    git verify-commit
    git verify-pack
    git verify-tag
    git version
    git log
    git worktree
    git write-tree

Re: Git branch outputs usage message on stderr

[Junio C Hamano]

"Kristoffer Haugsbakk" <kristofferhaugsbakk@fastmail.com> writes:

> Trying a random command with usage-on-error:
>
>     $ git-upload-pack >/dev/null
>     usage: [snip]
>
> Does give usage on stderr.

Good; that is what we want to see.  Thanks.