Unexpected exit code for --help with rev-parse --parseopt

Unexpected exit code for --help with rev-parse --parseopt

[brian m. carlson]

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

I use git rev-parse --parseopt to parse command-line options in various
script.  I've noticed that it exits 129 if the options are invalid,
which is fine, but that it also exits 129 if --help or -h are given,
which is not.

The standard philosophy is that if the user explicitly asked for help,
then help output should be printed to standard output (since that's what
the user asked for) and it should exit 0, since the program fulfilled
the user's request successfully.  If the help output is provided because
the user provided an invalid option or argument, then the output should
Go to standard error (since it's an error message) and the program
should exit unsuccessfully (since it did not fulfill the user's request
successfully).

Git gets the location of the output just fine, but currently there's no
way for a program to detect the help output and exit successfully.  Note
that Git subcommands don't have this problem because Git itself
intercepts the help output and sends it to man (or whatever the user has
configured), which then exits successfully.

I'd like to fix this in git rev-parse --parseopt (that is, I am willing
to send a patch), but I'm unsure about the best way to go about this.
Does anyone have ideas about how they'd like this to be fixed?  I could
simply change the exit code in this case or I could add an option to
control this behaviour for backwards compatibility.

Example:

----
#!/bin/sh

OPTS_SPEC="\
foo-cmd [<options>] <args>...

Do stuff.
--
h,help!         show this help

o,output=       output to this file
d,dir=          specify dependencies relative to this directory
"

main () {
    eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"

    # Do something with the options here.
}

main "$@"
----

Output:

----
% ./foo-cmd --help >/dev/null; echo $?
129
% ./foo-cmd -h >/dev/null; echo $?
129
% ./foo-cmd --bassoon >/dev/null; echo $?
error: unknown option `bassoon'
usage: foo-cmd [<options>] <args>...

    Do stuff.

    -h, --help            show this help
    -o, --[no-]output ... output to this file
    -d, --[no-]dir ...    specify dependencies relative to this directory

129
----
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: Unexpected exit code for --help with rev-parse --parseopt

[Jeff King]

On Sun, Mar 15, 2026 at 12:52:22AM +0000, brian m. carlson wrote:

> I use git rev-parse --parseopt to parse command-line options in various
> script.  I've noticed that it exits 129 if the options are invalid,
> which is fine, but that it also exits 129 if --help or -h are given,
> which is not.
> 
> The standard philosophy is that if the user explicitly asked for help,
> then help output should be printed to standard output (since that's what
> the user asked for) and it should exit 0, since the program fulfilled
> the user's request successfully.  If the help output is provided because
> the user provided an invalid option or argument, then the output should
> Go to standard error (since it's an error message) and the program
> should exit unsuccessfully (since it did not fulfill the user's request
> successfully).

I agree with this philosophy in general, but there's catch here with
"rev-parse --parseopt", because it's running as a separate program. It
needs to signal to the calling program that "-h" was seen, the program
should not proceed normally (because rev-parse already dumped help
output).

So if your proposal is to just exit with code 0 from rev-parse, I don't
think that works.

My first instinct is that it would have to exit with some other
well-known code, the callers would recognize that, and then exit
themselves (with code 0).

But there's a little more to the story. The output of --parseopt is meant
to be eval'd by the shell. And so when it works, we get a "set" command:

  $ git rev-parse --parseopt <input -- --output=foo
  set -- -o 'foo' --

And when there's an error, it dumps the usage text straight to stderr
and exits 129:

  $ git rev-parse --parseopt <input -- --foo
  error: unknown option `foo'
  usage: foo-cmd [<options>] <args>...
  
      Do stuff.
  
      -h, --help            show this help
      -o, --[no-]output ... output to this file
      -d, --[no-]dir ...    specify dependencies relative to this directory

But when the user asks for "-h", we get a here-doc! Like this:

  $ git rev-parse --parseopt <input -- -h
  cat <<\EOF
  usage: foo-cmd [<options>] <args>...
  
      Do stuff.
  
      -h, --help            show this help
      -o, --[no-]output ... output to this file
      -d, --[no-]dir ...    specify dependencies relative to this directory
  
  EOF

So the calling program runs that cat command. And I think what you
really want is to tack "exit 0" onto the end of that output, which would
tell the callers to exit. Something like this:

diff --git a/parse-options.c b/parse-options.c
index a676da86f5..b990f38419 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1473,8 +1473,10 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	}
 	fputc('\n', outfile);
 
-	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
+	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) {
 		fputs("EOF\n", outfile);
+		fputs("exit 0\n", outfile);
+	}
 
 	return PARSE_OPT_HELP;
 }

And then you don't even need to change the exit code of rev-parse itself
(since we'd never hit the "exit $?" that the caller tacks on in case of
failure). Though I think it might be reasonable to switch it to 0
anyway.

-Peff

Re: Unexpected exit code for --help with rev-parse --parseopt

[brian m. carlson]

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

On 2026-03-15 at 03:14:47, Jeff King wrote:
> So the calling program runs that cat command. And I think what you
> really want is to tack "exit 0" onto the end of that output, which would
> tell the callers to exit. Something like this:
> 
> diff --git a/parse-options.c b/parse-options.c
> index a676da86f5..b990f38419 100644
> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -1473,8 +1473,10 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
>  	}
>  	fputc('\n', outfile);
>  
> -	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> +	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL) {
>  		fputs("EOF\n", outfile);
> +		fputs("exit 0\n", outfile);
> +	}
>  
>  	return PARSE_OPT_HELP;
>  }

Yeah, I did some poking around, saw the heredoc, and came to the same
conclusion that this was a viable solution.  Since we both seem to agree
that this is a good solution and it also has the advantage of being
backward compatible (in that nobody has to change their code to get the
new behaviour[0]), this seems like the best option.

> And then you don't even need to change the exit code of rev-parse itself
> (since we'd never hit the "exit $?" that the caller tacks on in case of
> failure). Though I think it might be reasonable to switch it to 0
> anyway.

I can do that as well, even though I think that is actually
substantially more complicated than the first part.  I'll write up a
bunch of new tests for these cases in addition.

Thanks for a sober second opinion.

[0] As we all know, sometimes one has to use older systems and having
scripts break on older systems needlessly is quite inconvenient.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: Unexpected exit code for --help with rev-parse --parseopt

[Jeff King]

On Sun, Mar 15, 2026 at 04:59:48PM +0000, brian m. carlson wrote:

> > And then you don't even need to change the exit code of rev-parse itself
> > (since we'd never hit the "exit $?" that the caller tacks on in case of
> > failure). Though I think it might be reasonable to switch it to 0
> > anyway.
> 
> I can do that as well, even though I think that is actually
> substantially more complicated than the first part.  I'll write up a
> bunch of new tests for these cases in addition.

I would be perfectly happy to leave it as-is. The exit code _shouldn't_
matter anymore if the stdout result (with "exit 0") is being eval'd. So
in that case, maybe leaving it as-is might be the more conservative
choice, if the caller has somehow screwed up the eval and we want to
make sure they still exit.

-Peff

[PATCH] rev-parse: have --parseopt callers exit 0 on --help

[brian m. carlson]

The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully.  If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.

git rev-parse --parseopt properly directs the output in both of these
cases, but it currently exits 129 when it receives a --help or -h option
on the command line, which causes its invoking script to do the same.
This is not in line with the usual behavior and it causes scripts using
this command to exit unsuccessfully on --help as well.

Note that Git subcommands implemented using scripts, such as git
submodule, don't have this problem because Git itself intercepts the
--help option and runs man (or a similar tool), which then exits 0.
However, this still affects the myriad scripts that use this
functionality because Git is widespread and the --parseopt functionality
is a good way to get sensible option parsing across shells in a portable
way.

Because git rev-parse --parseopt is intended to be eval'd by the shell,
when help output is to be printed to standard output, Git actually
prints a cat command with a heredoc since the standard output is being
evaluated by the shell.  Thus, to do the right thing, simply add an
"exit 0" right after the end of the heredoc, which will cause the
invoking program to exit successfully.

The usual invocation recommended by the manual page is this:

    eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"

Thus, the fact that git rev-parse --parseopt still exits 129 in this
case is irrelevant, since the "echo exit $?" will print "exit 129", but
that will be after the "exit 0" printed by Git—and thus ignored, since
the shell will have already exited successfully.

Update the tests for this case.  Note that we no longer need to delete
only the first and last lines in some tests, so add a command to delete
the end of the heredoc as well.  We could do something clever with sed
to delete all but the last two lines or switch to head and tail, but
those would be more complicated and less readable, so just stick with
the simple approach.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 parse-options.c               | 2 +-
 t/t1502-rev-parse-parseopt.sh | 9 +++++++--
 t/t1502/optionspec-neg.help   | 1 +
 t/t1502/optionspec.help       | 1 +
 4 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index a676da86f5..85e2f0ea7c 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1474,7 +1474,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	fputc('\n', outfile);
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
-		fputs("EOF\n", outfile);
+		fputs("EOF\nexit 0\n", outfile);
 
 	return PARSE_OPT_HELP;
 }
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 3962f1d288..455608c429 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -12,7 +12,7 @@ check_invalid_long_option () {
 			cat <<-\EOF &&
 			error: unknown option `'${opt#--}\''
 			EOF
-			sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
+			sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
 		} >expect &&
 		test_expect_code 129 git rev-parse --parseopt -- $opt \
 			2>output <"$TEST_DIRECTORY/t1502/$spec" &&
@@ -87,6 +87,7 @@ test_expect_success 'test --parseopt help output no switches' '
 |    some-command does foo and bar!
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
 	test_cmp expect output
@@ -100,6 +101,7 @@ test_expect_success 'test --parseopt help output hidden switches' '
 |    some-command does foo and bar!
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
@@ -115,6 +117,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |    --[no-]hidden1        A hidden switch
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
@@ -125,7 +128,7 @@ test_expect_success 'test --parseopt invalid switch help output' '
 		cat <<-\EOF &&
 		error: unknown option `does-not-exist'\''
 		EOF
-		sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
+		sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
 	} >expect &&
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
 	test_cmp expect output
@@ -252,6 +255,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|    -h, --help            show the help
 	|
 	|EOF
+	|exit 0
 	END_EXPECT
 
 	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
@@ -289,6 +293,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|    -h, --help            show the help
 	|
 	|EOF
+	|exit 0
 	END_EXPECT
 
 	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 7a29f8cb03..f85be7b8fd 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -10,3 +10,4 @@ usage: some-command [options] <args>...
     --no-negative         cannot be positivated
 
 EOF
+exit 0
diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
index cbdd54d41b..ded35ebc82 100755
--- a/t/t1502/optionspec.help
+++ b/t/t1502/optionspec.help
@@ -34,3 +34,4 @@ Extras
     --[no-]extra1         line above used to cause a segfault but no longer does
 
 EOF
+exit 0

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  parse-options.c               | 2 +-
>  t/t1502-rev-parse-parseopt.sh | 9 +++++++--
>  t/t1502/optionspec-neg.help   | 1 +
>  t/t1502/optionspec.help       | 1 +
>  4 files changed, 10 insertions(+), 3 deletions(-)

Has t1517 passed for you?

Queued directly on top of v2.53.0, I am seeing:

>>>>>
expecting success of 1517.169 ''git instaweb -h' outside a repository':
                test_expect_code 129 nongit git $cmd -h >usage &&
                test_grep "[Uu]sage: git $cmd " usage

test_expect_code: command exited with 0, we wanted 129 nongit git instaweb -h
not ok 169 - 'git instaweb -h' outside a repository
<<<<<

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[brian m. carlson]

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

On 2026-03-17 at 00:47:19, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >  parse-options.c               | 2 +-
> >  t/t1502-rev-parse-parseopt.sh | 9 +++++++--
> >  t/t1502/optionspec-neg.help   | 1 +
> >  t/t1502/optionspec.help       | 1 +
> >  4 files changed, 10 insertions(+), 3 deletions(-)
> 
> Has t1517 passed for you?
> 
> Queued directly on top of v2.53.0, I am seeing:
> 
> >>>>>
> expecting success of 1517.169 ''git instaweb -h' outside a repository':
>                 test_expect_code 129 nongit git $cmd -h >usage &&
>                 test_grep "[Uu]sage: git $cmd " usage
> 
> test_expect_code: command exited with 0, we wanted 129 nongit git instaweb -h
> not ok 169 - 'git instaweb -h' outside a repository
> <<<<<

I thought the tests passed, but I may have neglected to run them on the
latest revision.  Go ahead and drop this for now and I'll send out a v2
either tonight or later this week.  My apologies.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Jeff King]

On Tue, Mar 17, 2026 at 11:59:02AM +0000, brian m. carlson wrote:

> On 2026-03-17 at 00:47:19, Junio C Hamano wrote:
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> > 
> > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > > ---
> > >  parse-options.c               | 2 +-
> > >  t/t1502-rev-parse-parseopt.sh | 9 +++++++--
> > >  t/t1502/optionspec-neg.help   | 1 +
> > >  t/t1502/optionspec.help       | 1 +
> > >  4 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > Has t1517 passed for you?
> > 
> > Queued directly on top of v2.53.0, I am seeing:
> > 
> > >>>>>
> > expecting success of 1517.169 ''git instaweb -h' outside a repository':
> >                 test_expect_code 129 nongit git $cmd -h >usage &&
> >                 test_grep "[Uu]sage: git $cmd " usage
> > 
> > test_expect_code: command exited with 0, we wanted 129 nongit git instaweb -h
> > not ok 169 - 'git instaweb -h' outside a repository
> > <<<<<
> 
> I thought the tests passed, but I may have neglected to run them on the
> latest revision.  Go ahead and drop this for now and I'll send out a v2
> either tonight or later this week.  My apologies.

Hmm. Is this just a matter of tweaking the tests, or is there something
bigger going on?

These tests are _expecting_ the 129 exit code from most commands. And
indeed, "git log -h" produces 129 for example. So it is not just shell
scripts using "rev-parse --parseopt" that do this, and those scripts are
(currently) consistent with the rest of Git.

You'd need something like this (untested) to hit the non-shell commands:

diff --git a/parse-options.c b/parse-options.c
index 85e2f0ea7c..0ad43ca278 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1197,6 +1197,7 @@ int parse_options(int argc, const char **argv,
 	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
+		exit(0);
 	case PARSE_OPT_ERROR:
 		exit(129);
 	case PARSE_OPT_COMPLETE:
@@ -1495,11 +1496,11 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 		if (!strcmp(av[1], "-h")) {
 			usage_with_options_internal(NULL, usagestr, opts,
 						    USAGE_NORMAL, USAGE_TO_STDOUT);
-			exit(129);
+			exit(0);
 		} else if (!strcmp(av[1], "--help-all")) {
 			usage_with_options_internal(NULL, usagestr, opts,
 						    USAGE_FULL, USAGE_TO_STDOUT);
-			exit(129);
+			exit(0);
 		}
 	}
 }

I agree with the general idea that "-h" usually should exit 0. But this
is not just a bug fix for --parseopt, but a change in overall intent. It
might be worth digging in the commit history or list archive to see if
there's any discussion on why we are using 129 in the first place.

-Peff

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Jeff King]

On Tue, Mar 17, 2026 at 10:55:43AM -0400, Jeff King wrote:

> I agree with the general idea that "-h" usually should exit 0. But this
> is not just a bug fix for --parseopt, but a change in overall intent. It
> might be worth digging in the commit history or list archive to see if
> there's any discussion on why we are using 129 in the first place.

I dug around a little but couldn't find anything conclusive. The first
"129" goes back to 2007:

  https://lore.kernel.org/git/20071013221450.GC2875@steel.home/

But there (and most spots where it is added) it is being used to show
options when we have an unknown option. So I _think_ what mostly
happened is that we show options in two different scenarios: on an
error, and when somebody asks for "-h". And we never differentiated the
two, and the latter just inherited the exit code for the former.

For example, when you look at ff43ec3e2d (parse-opt: create
parse_options_step., 2008-06-23), it introduces:

+       switch (parse_options_step(&ctx, options, usagestr)) {
+       case PARSE_OPT_HELP:
+               exit(129);

and only later did we add PARSE_OPT_ERROR to that switch statement. And
back then, PARSE_OPT_HELP came only from running usage_with_options().

But I couldn't find any discussion or intentional use of 129 for "-h"
output. I do suspect that there may still be some untangling to do. In
the earlier (again, untested) patch I showed, I put an exit(0) for that
PARSE_OPT_HELP case. But it may be that there's more surgery needed to
differentiate "we showed help because of -h" versus "we showed help
because you gave a bogus option".

-Peff

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I agree with the general idea that "-h" usually should exit 0.

Yes.

> But this
> is not just a bug fix for --parseopt, but a change in overall intent. It
> might be worth digging in the commit history or list archive to see if
> there's any discussion on why we are using 129 in the first place.

And if it turns out that old discussion was convincing enough, I'd
prefer to see us moving to --help/-h exiting with 0 eventually.  I
do not mind doing so at the Git 3.0 boundary, if an excuse to make
a big change is needed ;-)

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Jeff King]

On Tue, Mar 17, 2026 at 10:06:17AM -0700, Junio C Hamano wrote:

> > But this
> > is not just a bug fix for --parseopt, but a change in overall intent. It
> > might be worth digging in the commit history or list archive to see if
> > there's any discussion on why we are using 129 in the first place.
> 
> And if it turns out that old discussion was convincing enough, I'd
> prefer to see us moving to --help/-h exiting with 0 eventually.  I
> do not mind doing so at the Git 3.0 boundary, if an excuse to make
> a big change is needed ;-)

I could not find anything convincing. :)

We should also apply our little grey cells and see if we can think of
any reason somebody would be unhappy with a change of exit code now. The
most I could come up with is a script like:

  #!/bin/sh
  git rev-list "$@" >tmp &&
  do_something <tmp

where you might imagine it is run as "foo.sh --objects HEAD" or
something. Right now, running:

  foo.sh -h

will bail when rev-list returns 129, but in the proposed world it would
keep going and run do_something. And you can further imagine a world
where the script then quietly produces the wrong answer, because
do_something thinks the rev-list output was empty (or actually it is
worse; it gets the help output here).

I think this is mostly a case of "if it hurts, don't do it". The "-h" is
not doing anything useful (the user does not even see the help text!),
and if the script wants to support a usage message, it should parse the
"-h" out separately.

Could somebody maliciously convince you to pass "-h" to such a script?
Maybe, but not only are we getting into a series of increasingly
unlikely events, but I think that means you probably have worse
option-injection risks.

So unless somebody can come up with a more compelling example, I don't
really see much backwards-compatibility risk. But maybe I just lack
imagination. ;)

-Peff

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[brian m. carlson]

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

On 2026-03-17 at 18:44:41, Jeff King wrote:
> So unless somebody can come up with a more compelling example, I don't
> really see much backwards-compatibility risk. But maybe I just lack
> imagination. ;)

The only reason I can imagine intentionally using `-h` in a script is to
find out whether an option is supported.  For instance, I have this
alias:

    co = "!f() { if git checkout -h | grep -qs recurse-submodules; \
        then git checkout --recurse-submodules \"$@\"; \
        else git checkout \"$@\" && git sui; \
        fi; };f"

(`sui` is `submodule update --init`.)

In most cases, that's going to be upstream of a pipe, so unless you're
using `-o pipefail` (which is only in POSIX in POSIX 1003.1-2024), it's
going to succeed anyway.

I suppose one can also use it to generate a manual page, which manual
page generators do, but that seems silly when Git provides much better
ones unless you desperately need one in a different language for which
Git has translations but no manual page.

None of these seem like they're likely to care about the exit status and
I suspect that if they do, they are probably using `|| true` to ignore
the unexpected 129 exit code.

So I agree that there's unlikely to be any sort of backward
compatibility issues.  If the consensus is that this is shipped only in
3.0, then we can do that, but I think many people are not going to care
and those that do will welcome the change, so I'd just rather treat it
as a bug that we fix.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Jeff King]

On Wed, Mar 18, 2026 at 12:24:38AM +0000, brian m. carlson wrote:

> None of these seem like they're likely to care about the exit status and
> I suspect that if they do, they are probably using `|| true` to ignore
> the unexpected 129 exit code.

Yeah, I'd venture to say that moving from 129 to 0 would be a strict
improvement for the cases you outlined.

> So I agree that there's unlikely to be any sort of backward
> compatibility issues.  If the consensus is that this is shipped only in
> 3.0, then we can do that, but I think many people are not going to care
> and those that do will welcome the change, so I'd just rather treat it
> as a bug that we fix.

Nah, I think everything I have seen points to treating this as a simple
improvement / fix that we can do in the regular way.

-Peff

Re: [PATCH] rev-parse: have --parseopt callers exit 0 on --help

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Mar 18, 2026 at 12:24:38AM +0000, brian m. carlson wrote:
>
>> None of these seem like they're likely to care about the exit status and
>> I suspect that if they do, they are probably using `|| true` to ignore
>> the unexpected 129 exit code.
>
> Yeah, I'd venture to say that moving from 129 to 0 would be a strict
> improvement for the cases you outlined.
>
>> So I agree that there's unlikely to be any sort of backward
>> compatibility issues.  If the consensus is that this is shipped only in
>> 3.0, then we can do that, but I think many people are not going to care
>> and those that do will welcome the change, so I'd just rather treat it
>> as a bug that we fix.
>
> Nah, I think everything I have seen points to treating this as a simple
> improvement / fix that we can do in the regular way.

I 100% agree.  The message I earlier wrote (which contained the
mention of Git 3.0) lacked "even" before an "if".

[PATCH v2 0/4] rev-parse: exit 0 on --help

[brian m. carlson]

The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully.  If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.

git rev-parse --parseopt properly directs the output in both of these
cases, but it currently exits 129 when it receives a --help or -h option
on the command line, which causes its invoking script to do the same.
This is not in line with the usual behavior and it causes scripts using
this command to exit unsuccessfully on --help as well.

This series introduces some changes to distinguish the --help and -h
options from other cases in which we print help output and adjusts the
exit code to 0 from those two options.  We continue to exit 129 when the
options are invalid, which is useful information to have for callers.
We also make the relevant changes such that `git rev-parse --parseopt`
does the same thing as long as it is invoked in the way specified in the
manual page (which a quick GitHub search shows almost everyone does).

One of the patches is rather long because we have many cases in which
we've hard-coded exit code 129 into our tests.  However, the changes
there should not be complex, only somewhat tedious to review.

brian m. carlson (4):
  t1517: skip svn tests if svn is not installed
  parse-options: add a separate case for help output on error
  rev-parse: have --parseopt callers exit 0 on --help
  parse-options: exit 0 on -h

 builtin/blame.c                    |  2 ++
 builtin/shortlog.c                 |  2 ++
 builtin/update-index.c             |  2 ++
 contrib/subtree/t/t7900-subtree.sh |  2 +-
 parse-options.c                    | 20 ++++++++++----
 parse-options.h                    |  3 ++-
 t/for-each-ref-tests.sh            |  2 +-
 t/t0012-help.sh                    |  2 +-
 t/t0040-parse-options.sh           |  2 +-
 t/t0450-txt-doc-vs-help.sh         |  2 +-
 t/t0610-reftable-basics.sh         |  4 +--
 t/t1403-show-ref.sh                |  2 +-
 t/t1410-reflog.sh                  |  4 +--
 t/t1418-reflog-exists.sh           |  2 +-
 t/t1502-rev-parse-parseopt.sh      | 23 +++++++++-------
 t/t1502/optionspec-neg.help        |  1 +
 t/t1502/optionspec.help            |  1 +
 t/t1517-outside-repo.sh            | 43 +++++++++++++++++++++---------
 t/t1800-hook.sh                    |  4 +--
 t/t1900-repo-info.sh               |  2 +-
 t/t1901-repo-structure.sh          |  2 +-
 t/t2006-checkout-index-basic.sh    |  6 ++---
 t/t2107-update-index-basic.sh      |  2 +-
 t/t3004-ls-files-basic.sh          |  6 ++---
 t/t3200-branch.sh                  |  2 +-
 t/t3903-stash.sh                   |  4 +--
 t/t4200-rerere.sh                  |  2 +-
 t/t5200-update-server-info.sh      |  2 +-
 t/t5304-prune.sh                   |  2 +-
 t/t5400-send-pack.sh               |  4 +--
 t/t5512-ls-remote.sh               |  2 +-
 t/t6300-for-each-ref.sh            |  4 +--
 t/t6500-gc.sh                      |  2 +-
 t/t7030-verify-tag.sh              |  4 +--
 t/t7508-status.sh                  |  4 +--
 t/t7510-signed-commit.sh           |  4 +--
 t/t7600-merge.sh                   |  2 +-
 t/t7800-difftool.sh                |  3 +--
 t/t7900-maintenance.sh             |  2 +-
 usage.c                            |  2 +-
 40 files changed, 113 insertions(+), 73 deletions(-)

[PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[brian m. carlson]

The svn tests currently assume that git-svn's option parsing will always
fail the tests because it exits 0 on --help, not 129.  However, in a
future commit, we'll expect it to exit 0 and the tests will then need to
be updated to succeed in some cases and fail in others.

We therefore need to have t1517 determine whether the Subversion Perl
modules are present, since if they are not, git-svn will die on start
and then it needs to continue to expect failure.  Add a stripped down
version of the tests in t/lib-git-svn.sh as a prerequisite we can use
here for our svn tests.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/t1517-outside-repo.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index c557f2f55c..583784f21b 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -4,6 +4,14 @@ test_description='check random commands outside repo'
 
 . ./test-lib.sh
 
+test_lazy_prereq SVN '
+	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
+		use SVN::Core;
+		use SVN::Repos;
+		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
+	"
+'
+
 test_expect_success 'set up a non-repo directory and test file' '
 	GIT_CEILING_DIRECTORIES=$(pwd) &&
 	export GIT_CEILING_DIRECTORIES &&
@@ -138,6 +146,8 @@ do
 	case "$cmd" in
 	instaweb)
 		prereq=PERL ;;
+	svn)
+		prereq=SVN ;;
 	*)
 		prereq= ;;
 	esac

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> The svn tests currently assume that git-svn's option parsing will always
> fail the tests because it exits 0 on --help, not 129.  However, in a
> future commit, we'll expect it to exit 0 and the tests will then need to
> be updated to succeed in some cases and fail in others.
>
> We therefore need to have t1517 determine whether the Subversion Perl
> modules are present, since if they are not, git-svn will die on start
> and then it needs to continue to expect failure.  Add a stripped down
> version of the tests in t/lib-git-svn.sh as a prerequisite we can use
> here for our svn tests.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  t/t1517-outside-repo.sh | 10 ++++++++++
>  1 file changed, 10 insertions(+)
>
> diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
> index c557f2f55c..583784f21b 100755
> --- a/t/t1517-outside-repo.sh
> +++ b/t/t1517-outside-repo.sh
> @@ -4,6 +4,14 @@ test_description='check random commands outside repo'
>  
>  . ./test-lib.sh
>  
> +test_lazy_prereq SVN '
> +	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
> +		use SVN::Core;
> +		use SVN::Repos;
> +		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
> +	"
> +'

This corresponds to 42 in t/lib-git-svn.sh?  We can use any non-zero
value here, but just being curious.

>  test_expect_success 'set up a non-repo directory and test file' '
>  	GIT_CEILING_DIRECTORIES=$(pwd) &&
>  	export GIT_CEILING_DIRECTORIES &&
> @@ -138,6 +146,8 @@ do
>  	case "$cmd" in
>  	instaweb)
>  		prereq=PERL ;;
> +	svn)
> +		prereq=SVN ;;
>  	*)
>  		prereq= ;;
>  	esac

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> +test_lazy_prereq SVN '
> +	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
> +		use SVN::Core;
> +		use SVN::Repos;
> +		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
> +	"
> +'

If "have_prereq PERL" is not satisfied, SVN is not satisfied.

If NO_SVN_TESTS is an empty string (or unset), "test -n" fails, and
SVN is not satisfied.  Questionable---am I misreading this part of
the logic???

The perl script would not barf only if use SVN::* succeed and then
SVN::Core::VERSION is strictly better than '1.1.0'.  If not, i.e.,
libsvn-perl is not available, or its version is older, then we fail
with exit(42), and SVN is not satisfied.



>  test_expect_success 'set up a non-repo directory and test file' '
>  	GIT_CEILING_DIRECTORIES=$(pwd) &&
>  	export GIT_CEILING_DIRECTORIES &&
> @@ -138,6 +146,8 @@ do
>  	case "$cmd" in
>  	instaweb)
>  		prereq=PERL ;;
> +	svn)
> +		prereq=SVN ;;
>  	*)
>  		prereq= ;;
>  	esac

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed2sy

[brian m. carlson]

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

On 2026-07-01 at 22:27:04, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > +test_lazy_prereq SVN '
> > +	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
> > +		use SVN::Core;
> > +		use SVN::Repos;
> > +		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
> > +	"
> > +'
> 
> If "have_prereq PERL" is not satisfied, SVN is not satisfied.

Correct.

> If NO_SVN_TESTS is an empty string (or unset), "test -n" fails, and
> SVN is not satisfied.  Questionable---am I misreading this part of
> the logic???

I think that's reversed, yes.

> The perl script would not barf only if use SVN::* succeed and then
> SVN::Core::VERSION is strictly better than '1.1.0'.  If not, i.e.,
> libsvn-perl is not available, or its version is older, then we fail
> with exit(42), and SVN is not satisfied.

Correct.  And yes, this came in from `t/lib-git-svn.sh`.  I'll probably
just simplify this to omit the version check since it's very unlikely
that anybody is using SVN 1.0 any more and, as Peff pointed out, this
doesn't actually work using a string comparison.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[Jeff King]

On Wed, Jul 01, 2026 at 09:24:39PM +0000, brian m. carlson wrote:

> +test_lazy_prereq SVN '
> +	test_have_prereq PERL && test -n "$NO_SVN_TESTS" && perl -w -e "
> +		use SVN::Core;
> +		use SVN::Repos;
> +		\$SVN::Core::VERSION gt '1.1.0' or exit(42);
> +	"
> +'

The single-quotes in your inline perl will be interpreted as ending (and
restarting) the lazy-prereq snippet. So you actually get a bare:

  $SVN::Core::VERSION gt 1.1.0 or exit(42);

fed to perl (no quotes around 1.1.0). We sometimes catch these cases
automatically it results in an extra argument to test_expect_success,
etc. But here you are unlucky enough that it does not (and anyway, we do
not seem to have the same safety check for test_lazy_prereq; we'd just
ignore the extra arguments).

And of course being perl, it doesn't complain. I'm not sure how it is
interpreted, but I doubt the use of "gt" is right. My version of
SVN::Core is 1.14.5, which is (correctly) more than "1.1.0", but is
(incorrect) not more than "1.2.0".

I think the "gt" bug is inherited from lib-git-svn.sh (unless I'm just
holding it wrong), but the single-quote one is new (it happens at the
top-level in the original).

-Peff

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> fed to perl (no quotes around 1.1.0). We sometimes catch these cases
> automatically it results in an extra argument to test_expect_success,
> etc. But here you are unlucky enough that it does not (and anyway, we do
> not seem to have the same safety check for test_lazy_prereq; we'd just
> ignore the extra arguments).
>
> And of course being perl, it doesn't complain. I'm not sure how it is
> interpreted,

I happen to know ;-).

When you have more than two sequences of digits separated by dot,
like IP address 192.168.1.1, you are telling Perl to interpret the
sequence as a string, each byte of it is the number denoted by these
digits.  I believe this was invented primarily for IP addresses, but
it does not have to be just four digits.  To wit:

    $ perl -e 'print 65.66.67;'
    ABC
    $ perl -e 'print 65.66.67.68.69;'
    ABCDE

Of course, 65.66 is not AB, but a floating-point number that is
between integers 65 and 66:

    $ perl -e 'print 65.66;'
    65.66

> but I doubt the use of "gt" is right.

True.

Re: [PATCH v2 1/4] t1517: skip svn tests if svn is not installed

[Jeff King]

On Fri, Jul 03, 2026 at 01:36:37PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > fed to perl (no quotes around 1.1.0). We sometimes catch these cases
> > automatically it results in an extra argument to test_expect_success,
> > etc. But here you are unlucky enough that it does not (and anyway, we do
> > not seem to have the same safety check for test_lazy_prereq; we'd just
> > ignore the extra arguments).
> >
> > And of course being perl, it doesn't complain. I'm not sure how it is
> > interpreted,
> 
> I happen to know ;-).
> 
> When you have more than two sequences of digits separated by dot,
> like IP address 192.168.1.1, you are telling Perl to interpret the
> sequence as a string, each byte of it is the number denoted by these
> digits.  I believe this was invented primarily for IP addresses, but
> it does not have to be just four digits.  To wit:
> 
>     $ perl -e 'print 65.66.67;'
>     ABC
>     $ perl -e 'print 65.66.67.68.69;'
>     ABCDE
> 
> Of course, 65.66 is not AB, but a floating-point number that is
> between integers 65 and 66:
> 
>     $ perl -e 'print 65.66;'
>     65.66

Ah, thanks. It is both exciting and horrifying that in 2026 I can still
learn new perl esoterica. :)

-Peff

[PATCH v2 2/4] parse-options: add a separate case for help output on error

[brian m. carlson]

When we parse a command line option such as -h or --help, we currently
exit 129, since that is the exit code when help output is printed. In a
future commit, we'll change this to exit 0 instead, since we're doing
what the user wanted successfully.

However, there are some cases where we print help output because the
user has provided ambiguous or invalid input, such as an ambiguous
option, and we'll want to exit unsuccessfully there.  Make this easier
by defining a new return code, PARSE_OPT_HELP_ERROR, that can be used in
this case, while reserving PARSE_OPT_HELP for those cases where the user
has requested help directly.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/blame.c        | 1 +
 builtin/shortlog.c     | 1 +
 builtin/update-index.c | 1 +
 parse-options.c        | 7 ++++++-
 parse-options.h        | 3 ++-
 5 files changed, 11 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index ffbd3ce5c5..65d43c7d48 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1013,6 +1013,7 @@ int cmd_blame(int argc,
 		case PARSE_OPT_UNKNOWN:
 			break;
 		case PARSE_OPT_HELP:
+		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 		case PARSE_OPT_SUBCOMMAND:
 			exit(129);
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index 6b2a0b93b5..cd262bd376 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -433,6 +433,7 @@ int cmd_shortlog(int argc,
 		case PARSE_OPT_UNKNOWN:
 			break;
 		case PARSE_OPT_HELP:
+		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 		case PARSE_OPT_SUBCOMMAND:
 			exit(129);
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 3d6646c318..ac4610ec94 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1133,6 +1133,7 @@ int cmd_update_index(int argc,
 			break;
 		switch (parseopt_state) {
 		case PARSE_OPT_HELP:
+		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 			exit(129);
 		case PARSE_OPT_COMPLETE:
diff --git a/parse-options.c b/parse-options.c
index f4647e0099..a623961800 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -583,7 +583,7 @@ static enum parse_opt_result parse_long_opt(
 			ambiguous.option->long_name,
 			(abbrev.flags & OPT_UNSET) ?  "no-" : "",
 			abbrev.option->long_name);
-		return PARSE_OPT_HELP;
+		return PARSE_OPT_HELP_ERROR;
 	}
 	if (abbrev.option) {
 		if (*arg_end)
@@ -1037,6 +1037,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 				usage_with_options(usagestr, options);
 			case PARSE_OPT_COMPLETE:
 			case PARSE_OPT_HELP:
+			case PARSE_OPT_HELP_ERROR:
 			case PARSE_OPT_ERROR:
 			case PARSE_OPT_DONE:
 			case PARSE_OPT_NON_OPTION:
@@ -1072,6 +1073,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 			case PARSE_OPT_NON_OPTION:
 			case PARSE_OPT_SUBCOMMAND:
 			case PARSE_OPT_HELP:
+			case PARSE_OPT_HELP_ERROR:
 			case PARSE_OPT_COMPLETE:
 				BUG("parse_short_opt() cannot return these");
 			case PARSE_OPT_DONE:
@@ -1099,6 +1101,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 				case PARSE_OPT_SUBCOMMAND:
 				case PARSE_OPT_COMPLETE:
 				case PARSE_OPT_HELP:
+				case PARSE_OPT_HELP_ERROR:
 					BUG("parse_short_opt() cannot return these");
 				case PARSE_OPT_DONE:
 					break;
@@ -1132,6 +1135,7 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 		case PARSE_OPT_UNKNOWN:
 			goto unknown;
 		case PARSE_OPT_HELP:
+		case PARSE_OPT_HELP_ERROR:
 			goto show_usage;
 		case PARSE_OPT_NON_OPTION:
 		case PARSE_OPT_SUBCOMMAND:
@@ -1197,6 +1201,7 @@ int parse_options(int argc, const char **argv,
 	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
+	case PARSE_OPT_HELP_ERROR:
 	case PARSE_OPT_ERROR:
 		exit(129);
 	case PARSE_OPT_COMPLETE:
diff --git a/parse-options.h b/parse-options.h
index 0d1f738f8d..3ec8ba5cc8 100644
--- a/parse-options.h
+++ b/parse-options.h
@@ -57,7 +57,8 @@ enum parse_opt_option_flags {
 };
 
 enum parse_opt_result {
-	PARSE_OPT_COMPLETE = -3,
+	PARSE_OPT_COMPLETE = -4,
+	PARSE_OPT_HELP_ERROR = -3,
 	PARSE_OPT_HELP = -2,
 	PARSE_OPT_ERROR = -1,	/* must be the same as error() */
 	PARSE_OPT_DONE = 0,	/* fixed so that "return 0" works */

Re: [PATCH v2 2/4] parse-options: add a separate case for help output on error

[Jeff King]

On Wed, Jul 01, 2026 at 09:24:40PM +0000, brian m. carlson wrote:

> However, there are some cases where we print help output because the
> user has provided ambiguous or invalid input, such as an ambiguous
> option, and we'll want to exit unsuccessfully there.  Make this easier
> by defining a new return code, PARSE_OPT_HELP_ERROR, that can be used in
> this case, while reserving PARSE_OPT_HELP for those cases where the user
> has requested help directly.

Makes sense. We'd want to audit every spot that generates PARSE_OPT_HELP
and see if it should be PARSE_OPT_HELP_ERROR. I only see one spot
touched here:

> --- a/parse-options.c
> +++ b/parse-options.c
> @@ -583,7 +583,7 @@ static enum parse_opt_result parse_long_opt(
>  			ambiguous.option->long_name,
>  			(abbrev.flags & OPT_UNSET) ?  "no-" : "",
>  			abbrev.option->long_name);
> -		return PARSE_OPT_HELP;
> +		return PARSE_OPT_HELP_ERROR;
>  	}

That one makes sense. The other site that generates it is within
usage_with_options_internal(), which handles both asked-for "-h" and
unexpected errors, but still always returns PARSE_OPT_HELP.

Ah...it looks like you _do_ switch it in patch 4 (when the distinction
between the two starts to make a difference). I think it should be done
in this patch, though, since the point is generating the correct
HELP/HELP_ERROR here (even though it does not yet matter).

I wonder if we'd also want:

diff --git a/parse-options.c b/parse-options.c
index 742444eead..08c21d9fc0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1373,7 +1373,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	parse_options_check_harder(opts);
 
 	if (!usagestr)
-		return PARSE_OPT_HELP;
+		return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");

I can't figure out when we wouldn't have a usagestr, though. Perhaps not
ever from parse-options itself, but only when called via
usage_with_options() or something? That function does not look at our
return value so it would not matter, but it feels like we should keep
things consistent.

-Peff

[PATCH v2 3/4] rev-parse: have --parseopt callers exit 0 on --help

[brian m. carlson]

The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully.  If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.

git rev-parse --parseopt properly directs the output in both of these
cases, but it currently exits 129 when it receives a --help or -h option
on the command line, which causes its invoking script to do the same.
This is not in line with the usual behavior and it causes scripts using
this command to exit unsuccessfully on --help as well.

Note that Git subcommands implemented using scripts, such as git
submodule, don't have this problem because Git itself intercepts the
--help option and runs man (or a similar tool), which then exits 0.
However, this still affects the myriad scripts that use this
functionality because Git is widespread and the --parseopt functionality
is a good way to get sensible option parsing across shells in a portable
way.

Because git rev-parse --parseopt is intended to be eval'd by the shell,
when help output is to be printed to standard output, Git actually
prints a cat command with a heredoc since the standard output is being
evaluated by the shell.  Thus, to do the right thing, simply add an
"exit 0" right after the end of the heredoc, which will cause the
invoking program to exit successfully.

The usual invocation recommended by the manual page is this:

    eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"

Thus, the fact that git rev-parse --parseopt still exits 129 in this
case is irrelevant, since the "echo exit $?" will print "exit 129", but
that will be after the "exit 0" printed by Git—and thus ignored, since
the shell will have already exited successfully.

Update the tests for this case.  Note that we no longer need to delete
only the first and last lines in some tests, so add a command to delete
the end of the heredoc as well.  We could do something clever with sed
to delete all but the last two lines or switch to head and tail, but
those would be more complicated and less readable, so just stick with
the simple approach.

In t1517, add three shell scripts to the failure case because they no
longer return 129 as expected.  In a future commit, we'll change the
expected result to exit 0 and these will become successful again.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 parse-options.c               | 2 +-
 t/t1502-rev-parse-parseopt.sh | 9 +++++++--
 t/t1502/optionspec-neg.help   | 1 +
 t/t1502/optionspec.help       | 1 +
 t/t1517-outside-repo.sh       | 6 +++---
 5 files changed, 13 insertions(+), 6 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index a623961800..67a2d372d0 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1479,7 +1479,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	fputc('\n', outfile);
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
-		fputs("EOF\n", outfile);
+		fputs("EOF\nexit 0\n", outfile);
 
 	return PARSE_OPT_HELP;
 }
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 3962f1d288..455608c429 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -12,7 +12,7 @@ check_invalid_long_option () {
 			cat <<-\EOF &&
 			error: unknown option `'${opt#--}\''
 			EOF
-			sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
+			sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/$spec.help"
 		} >expect &&
 		test_expect_code 129 git rev-parse --parseopt -- $opt \
 			2>output <"$TEST_DIRECTORY/t1502/$spec" &&
@@ -87,6 +87,7 @@ test_expect_success 'test --parseopt help output no switches' '
 |    some-command does foo and bar!
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
 	test_cmp expect output
@@ -100,6 +101,7 @@ test_expect_success 'test --parseopt help output hidden switches' '
 |    some-command does foo and bar!
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
@@ -115,6 +117,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |    --[no-]hidden1        A hidden switch
 |
 |EOF
+|exit 0
 END_EXPECT
 	test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
@@ -125,7 +128,7 @@ test_expect_success 'test --parseopt invalid switch help output' '
 		cat <<-\EOF &&
 		error: unknown option `does-not-exist'\''
 		EOF
-		sed -e 1d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
+		sed -e 1d -e /EOF/d -e \$d <"$TEST_DIRECTORY/t1502/optionspec.help"
 	} >expect &&
 	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
 	test_cmp expect output
@@ -252,6 +255,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|    -h, --help            show the help
 	|
 	|EOF
+	|exit 0
 	END_EXPECT
 
 	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
@@ -289,6 +293,7 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|    -h, --help            show the help
 	|
 	|EOF
+	|exit 0
 	END_EXPECT
 
 	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
diff --git a/t/t1502/optionspec-neg.help b/t/t1502/optionspec-neg.help
index 7a29f8cb03..f85be7b8fd 100644
--- a/t/t1502/optionspec-neg.help
+++ b/t/t1502/optionspec-neg.help
@@ -10,3 +10,4 @@ usage: some-command [options] <args>...
     --no-negative         cannot be positivated
 
 EOF
+exit 0
diff --git a/t/t1502/optionspec.help b/t/t1502/optionspec.help
index cbdd54d41b..ded35ebc82 100755
--- a/t/t1502/optionspec.help
+++ b/t/t1502/optionspec.help
@@ -34,3 +34,4 @@ Extras
     --[no-]extra1         line above used to cause a segfault but no longer does
 
 EOF
+exit 0
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 583784f21b..99bda36d17 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -133,10 +133,10 @@ do
 	difftool--helper | filter-branch | format-rev | fsck-objects | \
 	get-tar-commit-id | \
 	gui | gui--askpass | \
-	http-backend | http-fetch | http-push | init-db | \
+	http-backend | http-fetch | http-push | init-db | instaweb | \
 	merge-octopus | merge-one-file | merge-resolve | mergetool | \
-	mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
-	remote-http | remote-https | replay | send-email | \
+	mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
+	remote-http | remote-https | replay | request-pull | send-email | \
 	sh-i18n--envsubst | shell | show | stage | submodule | svn | \
 	upload-archive--writer | upload-pack | web--browse | whatchanged)
 		expect_outcome=expect_failure ;;

Re: [PATCH v2 3/4] rev-parse: have --parseopt callers exit 0 on --help

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> The usual invocation recommended by the manual page is this:
>
>     eval "$(echo "$OPTS_SPEC" | git rev-parse --parseopt -- "$@" || echo exit $?)"
>
> Thus, the fact that git rev-parse --parseopt still exits 129 in this
> case is irrelevant, since the "echo exit $?" will print "exit 129", but
> that will be after the "exit 0" printed by Git—and thus ignored, since
> the shell will have already exited successfully.

Yuck, but ...

>  	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
> -		fputs("EOF\n", outfile);
> +		fputs("EOF\nexit 0\n", outfile);

... it does its job ;-).

[PATCH v2 4/4] parse-options: exit 0 on -h

[brian m. carlson]

The standard philosophy for Unix software when a help option (such as
--help) is specified is that the software should exit 0, printing the
help output to standard output, since the standard output is for
user-requested output and the program performed the requested task
successfully.  If the user specifies an incorrect option, then the help
output should be printed to standard error (since the user has made a
mistake) and it should exit unsuccessfully.

Most of our commands currently exit 129 on receiving the -h option to
print the short help, which does not line up with the standard
philosophy above.  Let's change that to exit 0 instead.

This requires changes to a variety of tests which previously wanted the
129 exit code, so update them.  Note that because git diff does its own
option parsing, it still exits with 129, so update some of the tests to
expect either exit status.

Some commands also now pass with -h but not --help-all, so handle those
cases differently for those commands.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/blame.c                    |  1 +
 builtin/shortlog.c                 |  1 +
 builtin/update-index.c             |  1 +
 contrib/subtree/t/t7900-subtree.sh |  2 +-
 parse-options.c                    | 13 +++++++---
 t/for-each-ref-tests.sh            |  2 +-
 t/t0012-help.sh                    |  2 +-
 t/t0040-parse-options.sh           |  2 +-
 t/t0450-txt-doc-vs-help.sh         |  2 +-
 t/t0610-reftable-basics.sh         |  4 +--
 t/t1403-show-ref.sh                |  2 +-
 t/t1410-reflog.sh                  |  4 +--
 t/t1418-reflog-exists.sh           |  2 +-
 t/t1502-rev-parse-parseopt.sh      | 14 +++++------
 t/t1517-outside-repo.sh            | 39 ++++++++++++++++++------------
 t/t1800-hook.sh                    |  4 +--
 t/t1900-repo-info.sh               |  2 +-
 t/t1901-repo-structure.sh          |  2 +-
 t/t2006-checkout-index-basic.sh    |  6 ++---
 t/t2107-update-index-basic.sh      |  2 +-
 t/t3004-ls-files-basic.sh          |  6 ++---
 t/t3200-branch.sh                  |  2 +-
 t/t3903-stash.sh                   |  4 +--
 t/t4200-rerere.sh                  |  2 +-
 t/t5200-update-server-info.sh      |  2 +-
 t/t5304-prune.sh                   |  2 +-
 t/t5400-send-pack.sh               |  4 +--
 t/t5512-ls-remote.sh               |  2 +-
 t/t6300-for-each-ref.sh            |  4 +--
 t/t6500-gc.sh                      |  2 +-
 t/t7030-verify-tag.sh              |  4 +--
 t/t7508-status.sh                  |  4 +--
 t/t7510-signed-commit.sh           |  4 +--
 t/t7600-merge.sh                   |  2 +-
 t/t7800-difftool.sh                |  3 +--
 t/t7900-maintenance.sh             |  2 +-
 usage.c                            |  2 +-
 37 files changed, 86 insertions(+), 72 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 65d43c7d48..38749f79c2 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1013,6 +1013,7 @@ int cmd_blame(int argc,
 		case PARSE_OPT_UNKNOWN:
 			break;
 		case PARSE_OPT_HELP:
+			exit(0);
 		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 		case PARSE_OPT_SUBCOMMAND:
diff --git a/builtin/shortlog.c b/builtin/shortlog.c
index cd262bd376..4c78d2e5ba 100644
--- a/builtin/shortlog.c
+++ b/builtin/shortlog.c
@@ -433,6 +433,7 @@ int cmd_shortlog(int argc,
 		case PARSE_OPT_UNKNOWN:
 			break;
 		case PARSE_OPT_HELP:
+			exit(0);
 		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 		case PARSE_OPT_SUBCOMMAND:
diff --git a/builtin/update-index.c b/builtin/update-index.c
index ac4610ec94..6810327209 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1133,6 +1133,7 @@ int cmd_update_index(int argc,
 			break;
 		switch (parseopt_state) {
 		case PARSE_OPT_HELP:
+			exit(0);
 		case PARSE_OPT_HELP_ERROR:
 		case PARSE_OPT_ERROR:
 			exit(129);
diff --git a/contrib/subtree/t/t7900-subtree.sh b/contrib/subtree/t/t7900-subtree.sh
index 4194687cfb..c10f283b38 100755
--- a/contrib/subtree/t/t7900-subtree.sh
+++ b/contrib/subtree/t/t7900-subtree.sh
@@ -99,7 +99,7 @@ test_create_subtree_add () {
 }
 
 test_expect_success 'shows short help text for -h' '
-	test_expect_code 129 git subtree -h >out 2>err &&
+	git subtree -h >out 2>err &&
 	test_must_be_empty err &&
 	grep -e "^ *or: git subtree pull" out &&
 	grep -F -e "--[no-]annotate" out
diff --git a/parse-options.c b/parse-options.c
index 67a2d372d0..742444eead 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -1135,8 +1135,9 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
 		case PARSE_OPT_UNKNOWN:
 			goto unknown;
 		case PARSE_OPT_HELP:
-		case PARSE_OPT_HELP_ERROR:
 			goto show_usage;
+		case PARSE_OPT_HELP_ERROR:
+			goto show_usage_stderr;
 		case PARSE_OPT_NON_OPTION:
 		case PARSE_OPT_SUBCOMMAND:
 		case PARSE_OPT_COMPLETE:
@@ -1170,6 +1171,9 @@ enum parse_opt_result parse_options_step(struct parse_opt_ctx_t *ctx,
  show_usage:
 	return usage_with_options_internal(ctx, usagestr, options,
 					   USAGE_NORMAL, USAGE_TO_STDOUT);
+ show_usage_stderr:
+	return usage_with_options_internal(ctx, usagestr, options,
+					   USAGE_NORMAL, USAGE_TO_STDERR);
 }
 
 int parse_options_end(struct parse_opt_ctx_t *ctx)
@@ -1201,6 +1205,7 @@ int parse_options(int argc, const char **argv,
 	parse_options_start_1(&ctx, argc, argv, prefix, options, flags);
 	switch (parse_options_step(&ctx, options, usagestr)) {
 	case PARSE_OPT_HELP:
+		exit(0);
 	case PARSE_OPT_HELP_ERROR:
 	case PARSE_OPT_ERROR:
 		exit(129);
@@ -1481,7 +1486,7 @@ static enum parse_opt_result usage_with_options_internal(struct parse_opt_ctx_t
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fputs("EOF\nexit 0\n", outfile);
 
-	return PARSE_OPT_HELP;
+	return err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
 }
 
 void NORETURN usage_with_options(const char * const *usagestr,
@@ -1500,11 +1505,11 @@ void show_usage_with_options_if_asked(int ac, const char **av,
 		if (!strcmp(av[1], "-h")) {
 			usage_with_options_internal(NULL, usagestr, opts,
 						    USAGE_NORMAL, USAGE_TO_STDOUT);
-			exit(129);
+			exit(0);
 		} else if (!strcmp(av[1], "--help-all")) {
 			usage_with_options_internal(NULL, usagestr, opts,
 						    USAGE_FULL, USAGE_TO_STDOUT);
-			exit(129);
+			exit(0);
 		}
 	}
 }
diff --git a/t/for-each-ref-tests.sh b/t/for-each-ref-tests.sh
index bd2d45c971..b95e5b6ca0 100644
--- a/t/for-each-ref-tests.sh
+++ b/t/for-each-ref-tests.sh
@@ -522,7 +522,7 @@ test_expect_success 'Verify descending sort' '
 '
 
 test_expect_success 'Give help even with invalid sort atoms' '
-	test_expect_code 129 ${git_for_each_ref} --sort=bogus -h >actual 2>&1 &&
+	${git_for_each_ref} --sort=bogus -h >actual 2>&1 &&
 	grep "^usage: ${git_for_each_ref}" actual
 '
 
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index c33501bdcd..7815ff14f2 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -260,7 +260,7 @@ do
 		(
 			GIT_CEILING_DIRECTORIES=$(pwd) &&
 			export GIT_CEILING_DIRECTORIES &&
-			test_expect_code 129 git -C sub $builtin -h >output 2>err
+			git -C sub $builtin -h >output 2>err
 		) &&
 		test_must_be_empty err &&
 		test_grep usage output
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ca55ea8228..30895ad6d2 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -68,7 +68,7 @@ Alias
 EOF
 
 test_expect_success 'test help' '
-	test_must_fail test-tool parse-options -h >output 2>output.err &&
+	test-tool parse-options -h >output 2>output.err &&
 	test_must_be_empty output.err &&
 	test_cmp expect output
 '
diff --git a/t/t0450-txt-doc-vs-help.sh b/t/t0450-txt-doc-vs-help.sh
index 822b0d55a5..d2844368e4 100755
--- a/t/t0450-txt-doc-vs-help.sh
+++ b/t/t0450-txt-doc-vs-help.sh
@@ -29,7 +29,7 @@ help_to_synopsis () {
 		return 0
 	fi &&
 	mkdir -p "$out_dir" &&
-	test_expect_code 129 git $builtin -h >"$out.raw" 2>&1 &&
+	test_might_fail git $builtin -h >"$out.raw" 2>&1 &&
 	sed -n \
 		-e '1,/^$/ {
 			/^$/d;
diff --git a/t/t0610-reftable-basics.sh b/t/t0610-reftable-basics.sh
index e19e036898..4135db95ed 100755
--- a/t/t0610-reftable-basics.sh
+++ b/t/t0610-reftable-basics.sh
@@ -15,9 +15,9 @@ export GIT_TEST_DEFAULT_REF_FORMAT
 INVALID_OID=$(test_oid 001)
 
 test_expect_success 'pack-refs does not crash with -h' '
-	test_expect_code 129 git pack-refs -h >usage &&
+	git pack-refs -h >usage &&
 	test_grep "[Uu]sage: git pack-refs " usage &&
-	test_expect_code 129 nongit git pack-refs -h >usage &&
+	nongit git pack-refs -h >usage &&
 	test_grep "[Uu]sage: git pack-refs " usage
 '
 
diff --git a/t/t1403-show-ref.sh b/t/t1403-show-ref.sh
index 36c903ca19..db4300da44 100755
--- a/t/t1403-show-ref.sh
+++ b/t/t1403-show-ref.sh
@@ -165,7 +165,7 @@ test_expect_success 'show-ref --branches, --tags, --head, pattern' '
 '
 
 test_expect_success 'show-ref --heads is deprecated and hidden' '
-	test_expect_code 129 git show-ref -h >short-help &&
+	git show-ref -h >short-help &&
 	test_grep ! -e --heads short-help &&
 	git show-ref --heads >actual 2>warning &&
 	test_grep ! deprecated warning &&
diff --git a/t/t1410-reflog.sh b/t/t1410-reflog.sh
index ce71f9a30a..6e921bc167 100755
--- a/t/t1410-reflog.sh
+++ b/t/t1410-reflog.sh
@@ -107,12 +107,12 @@ test_expect_success setup '
 '
 
 test_expect_success 'correct usage on sub-command -h' '
-	test_expect_code 129 git reflog expire -h >err &&
+	git reflog expire -h >err &&
 	grep "git reflog expire" err
 '
 
 test_expect_success 'correct usage on "git reflog show -h"' '
-	test_expect_code 129 git reflog show -h >err &&
+	git reflog show -h >err &&
 	grep -F "git reflog [show]" err
 '
 
diff --git a/t/t1418-reflog-exists.sh b/t/t1418-reflog-exists.sh
index d51ecd5e92..10387792e3 100755
--- a/t/t1418-reflog-exists.sh
+++ b/t/t1418-reflog-exists.sh
@@ -12,7 +12,7 @@ test_expect_success 'setup' '
 
 test_expect_success 'usage' '
 	test_expect_code 129 git reflog exists &&
-	test_expect_code 129 git reflog exists -h
+	git reflog exists -h
 '
 
 test_expect_success 'usage: unknown option' '
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 455608c429..fa97591b9f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -75,7 +75,7 @@ EOF
 '
 
 test_expect_success 'test --parseopt help output' '
-	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec &&
+	git rev-parse --parseopt -- -h > output < optionspec &&
 	test_cmp "$TEST_DIRECTORY/t1502/optionspec.help" output
 '
 
@@ -89,7 +89,7 @@ test_expect_success 'test --parseopt help output no switches' '
 |EOF
 |exit 0
 END_EXPECT
-	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
+	git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
 	test_cmp expect output
 '
 
@@ -103,7 +103,7 @@ test_expect_success 'test --parseopt help output hidden switches' '
 |EOF
 |exit 0
 END_EXPECT
-	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
+	git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
 '
 
@@ -119,7 +119,7 @@ test_expect_success 'test --parseopt help-all output hidden switches' '
 |EOF
 |exit 0
 END_EXPECT
-	test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
+	git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
 	test_cmp expect output
 '
 
@@ -258,7 +258,7 @@ test_expect_success 'test --parseopt help output: "wrapped" options normal "or:"
 	|exit 0
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
+	git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
@@ -296,12 +296,12 @@ test_expect_success 'test --parseopt help output: multi-line blurb after empty l
 	|exit 0
 	END_EXPECT
 
-	test_must_fail git rev-parse --parseopt -- -h <spec >actual &&
+	git rev-parse --parseopt -- -h <spec >actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'test --parseopt help output for optionspec-neg' '
-	test_expect_code 129 git rev-parse --parseopt -- \
+	git rev-parse --parseopt -- \
 		-h >output <"$TEST_DIRECTORY/t1502/optionspec-neg" &&
 	test_cmp "$TEST_DIRECTORY/t1502/optionspec-neg.help" output
 '
diff --git a/t/t1517-outside-repo.sh b/t/t1517-outside-repo.sh
index 99bda36d17..b45c52bd02 100755
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -130,18 +130,25 @@ do
 	archimport | citool | credential-netrc | credential-libsecret | \
 	credential-osxkeychain | cvsexportcommit | cvsimport | cvsserver | \
 	daemon | \
-	difftool--helper | filter-branch | format-rev | fsck-objects | \
-	get-tar-commit-id | \
+	difftool--helper | format-rev | fsck-objects |  get-tar-commit-id | \
 	gui | gui--askpass | \
-	http-backend | http-fetch | http-push | init-db | instaweb | \
-	merge-octopus | merge-one-file | merge-resolve | mergetool | \
-	mktag | p4 | p4.py | pickaxe | quiltimport | remote-ftp | remote-ftps | \
-	remote-http | remote-https | replay | request-pull | send-email | \
-	sh-i18n--envsubst | shell | show | stage | submodule | svn | \
-	upload-archive--writer | upload-pack | web--browse | whatchanged)
-		expect_outcome=expect_failure ;;
+	http-backend | http-fetch | http-push | init-db | \
+	mktag | p4 | p4.py | pickaxe | remote-ftp | remote-ftps | \
+	remote-http | remote-https | replay | send-email | \
+	sh-i18n--envsubst | shell | show | stage | \
+	upload-archive--writer | upload-pack | whatchanged)
+		h_expect_outcome=expect_failure
+		all_expect_outcome=expect_failure
+		;;
+	filter-branch | merge-octopus | merge-one-file | merge-resolve | \
+	mergetool | submodule | svn | web--browse)
+		h_expect_outcome=expect_success
+		all_expect_outcome=expect_failure
+		;;
 	*)
-		expect_outcome=expect_success ;;
+		h_expect_outcome=expect_success
+		all_expect_outcome=expect_success
+		;;
 	esac
 	case "$cmd" in
 	instaweb)
@@ -151,20 +158,20 @@ do
 	*)
 		prereq= ;;
 	esac
-	test_$expect_outcome $prereq "'git $cmd -h' outside a repository" '
-		test_expect_code 129 nongit git $cmd -h >usage &&
+	test_$h_expect_outcome $prereq "'git $cmd -h' outside a repository" '
+		nongit git $cmd -h >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
-	test_$expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
-		test_expect_code 129 nongit git $cmd --help-all >usage &&
+	test_$all_expect_outcome $prereq "'git $cmd --help-all' outside a repository" '
+		nongit git $cmd --help-all >usage &&
 		test_grep "[Uu]sage: git $cmd " usage
 	'
 done
 
 test_expect_success 'fmt-merge-msg does not crash with -h' '
-	test_expect_code 129 git fmt-merge-msg -h >usage &&
+	git fmt-merge-msg -h >usage &&
 	test_grep "[Uu]sage: git fmt-merge-msg " usage &&
-	test_expect_code 129 nongit git fmt-merge-msg -h >usage &&
+	nongit git fmt-merge-msg -h >usage &&
 	test_grep "[Uu]sage: git fmt-merge-msg " usage
 '
 
diff --git a/t/t1800-hook.sh b/t/t1800-hook.sh
index 0132e772e4..2ea9fa13c5 100755
--- a/t/t1800-hook.sh
+++ b/t/t1800-hook.sh
@@ -75,10 +75,10 @@ sentinel_detector () {
 test_expect_success 'git hook usage' '
 	test_expect_code 129 git hook &&
 	test_expect_code 129 git hook run &&
-	test_expect_code 129 git hook run -h &&
+	git hook run -h &&
 	test_expect_code 129 git hook run --unknown 2>err &&
 	test_expect_code 129 git hook list &&
-	test_expect_code 129 git hook list -h &&
+	git hook list -h &&
 	grep "unknown option" err
 '
 
diff --git a/t/t1900-repo-info.sh b/t/t1900-repo-info.sh
index 39bb77dda0..826686955d 100755
--- a/t/t1900-repo-info.sh
+++ b/t/t1900-repo-info.sh
@@ -150,7 +150,7 @@ test_expect_success 'git repo info --keys uses lines as its default output forma
 '
 
 test_expect_success 'git repo info -h shows only repo info usage' '
-	test_must_fail git repo info -h >actual &&
+	git repo info -h >actual &&
 	test_grep "git repo info" actual &&
 	test_grep ! "git repo structure" actual
 '
diff --git a/t/t1901-repo-structure.sh b/t/t1901-repo-structure.sh
index 10050abd70..02cc2b594a 100755
--- a/t/t1901-repo-structure.sh
+++ b/t/t1901-repo-structure.sh
@@ -225,7 +225,7 @@ test_expect_success 'progress meter option' '
 '
 
 test_expect_success 'git repo structure -h shows only repo structure usage' '
-	test_must_fail git repo structure -h >actual &&
+	git repo structure -h >actual &&
 	test_grep "git repo structure" actual &&
 	test_grep ! "git repo info" actual
 '
diff --git a/t/t2006-checkout-index-basic.sh b/t/t2006-checkout-index-basic.sh
index fedd2cc097..6538a24c95 100755
--- a/t/t2006-checkout-index-basic.sh
+++ b/t/t2006-checkout-index-basic.sh
@@ -16,15 +16,15 @@ test_expect_success 'checkout-index -h in broken repository' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git checkout-index -h >usage 2>&1
+		git checkout-index -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage" broken/usage
 '
 
 test_expect_success 'checkout-index does not crash with -h' '
-	test_expect_code 129 git checkout-index -h >usage &&
+	git checkout-index -h >usage &&
 	test_grep "[Uu]sage: git checkout-index " usage &&
-	test_expect_code 129 nongit git checkout-index -h >usage &&
+	nongit git checkout-index -h >usage &&
 	test_grep "[Uu]sage: git checkout-index " usage
 '
 
diff --git a/t/t2107-update-index-basic.sh b/t/t2107-update-index-basic.sh
index 3bffe5da8a..004878322e 100755
--- a/t/t2107-update-index-basic.sh
+++ b/t/t2107-update-index-basic.sh
@@ -23,7 +23,7 @@ test_expect_success 'update-index -h with corrupt index' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git update-index -h >usage 2>&1
+		git update-index -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage: git update-index" broken/usage
 '
diff --git a/t/t3004-ls-files-basic.sh b/t/t3004-ls-files-basic.sh
index 4034a5a59f..c57afcb841 100755
--- a/t/t3004-ls-files-basic.sh
+++ b/t/t3004-ls-files-basic.sh
@@ -29,15 +29,15 @@ test_expect_success 'ls-files -h in corrupt repository' '
 		cd broken &&
 		git init &&
 		>.git/index &&
-		test_expect_code 129 git ls-files -h >usage 2>&1
+		git ls-files -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage: git ls-files " broken/usage
 '
 
 test_expect_success 'ls-files does not crash with -h' '
-	test_expect_code 129 git ls-files -h >usage &&
+	git ls-files -h >usage &&
 	test_grep "[Uu]sage: git ls-files " usage &&
-	test_expect_code 129 nongit git ls-files -h >usage &&
+	nongit git ls-files -h >usage &&
 	test_grep "[Uu]sage: git ls-files " usage
 '
 
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index e7829c2c4b..dec0e77e3c 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -33,7 +33,7 @@ test_expect_success REFFILES 'branch -h in broken repository' '
 		cd broken &&
 		git init -b main &&
 		>.git/refs/heads/main &&
-		test_expect_code 129 git branch -h >usage 2>&1
+		git branch -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage" broken/usage
 '
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index ecc35aae82..bc07e2a6ec 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -27,13 +27,13 @@ test_expect_success 'usage on cmd and subcommand invalid option' '
 '
 
 test_expect_success 'usage on main command -h emits a summary of subcommands' '
-	test_expect_code 129 git stash -h >usage &&
+	git stash -h >usage &&
 	grep -F "usage: git stash list" usage &&
 	grep -F "or: git stash show" usage
 '
 
 test_expect_success 'usage for subcommands should emit subcommand usage' '
-	test_expect_code 129 git stash push -h >usage &&
+	git stash push -h >usage &&
 	grep -F "usage: git stash [push" usage
 '
 
diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 1717f407c8..e1b474cc0f 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -438,7 +438,7 @@ test_expect_success 'rerere --no-no-rerere-autoupdate' '
 '
 
 test_expect_success 'rerere -h' '
-	test_must_fail git rerere -h >help &&
+	git rerere -h >help &&
 	test_grep [Uu]sage help
 '
 
diff --git a/t/t5200-update-server-info.sh b/t/t5200-update-server-info.sh
index a551e955b5..a0630cc1fc 100755
--- a/t/t5200-update-server-info.sh
+++ b/t/t5200-update-server-info.sh
@@ -47,7 +47,7 @@ test_expect_success 'midx does not create duplicate pack entries' '
 '
 
 test_expect_success 'update-server-info does not crash with -h' '
-	test_expect_code 129 git update-server-info -h >usage &&
+	git update-server-info -h >usage &&
 	test_grep "[Uu]sage: git update-server-info " usage
 '
 
diff --git a/t/t5304-prune.sh b/t/t5304-prune.sh
index 2be7cd30de..e26e833d89 100755
--- a/t/t5304-prune.sh
+++ b/t/t5304-prune.sh
@@ -365,7 +365,7 @@ test_expect_success 'gc.recentObjectsHook' '
 '
 
 test_expect_success 'prune does not crash with -h' '
-	test_expect_code 129 git prune -h >usage &&
+	git prune -h >usage &&
 	test_grep "[Uu]sage: git prune " usage
 '
 
diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index b32a0a6aa7..6aa5838e2b 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -56,9 +56,9 @@ test_expect_success setup '
 	git log'
 
 test_expect_success 'send-pack does not crash with -h' '
-	test_expect_code 129 git send-pack -h >usage &&
+	git send-pack -h >usage &&
 	test_grep "[Uu]sage: git send-pack " usage &&
-	test_expect_code 129 nongit git send-pack -h >usage &&
+	nongit git send-pack -h >usage &&
 	test_grep "[Uu]sage: git send-pack " usage
 '
 
diff --git a/t/t5512-ls-remote.sh b/t/t5512-ls-remote.sh
index 5930f55186..8345bc0b14 100755
--- a/t/t5512-ls-remote.sh
+++ b/t/t5512-ls-remote.sh
@@ -86,7 +86,7 @@ test_expect_success 'ls-remote -h is deprecated w/o warning' '
 '
 
 test_expect_success 'ls-remote --heads is deprecated and hidden w/o warning' '
-	test_expect_code 129 git ls-remote -h >short-help &&
+	git ls-remote -h >short-help &&
 	test_grep ! -e --head short-help &&
 	git ls-remote --heads self >actual 2>warning &&
 	test_cmp expected.branches actual &&
diff --git a/t/t6300-for-each-ref.sh b/t/t6300-for-each-ref.sh
index 1d9809114d..6d27b42ff1 100755
--- a/t/t6300-for-each-ref.sh
+++ b/t/t6300-for-each-ref.sh
@@ -8,9 +8,9 @@ test_description='for-each-ref test'
 . ./test-lib.sh
 
 test_expect_success "for-each-ref does not crash with -h" '
-	test_expect_code 129 git for-each-ref -h >usage &&
+	git for-each-ref -h >usage &&
 	test_grep "[Uu]sage: git for-each-ref " usage &&
-	test_expect_code 129 nongit git for-each-ref -h >usage &&
+	nongit git for-each-ref -h >usage &&
 	test_grep "[Uu]sage: git for-each-ref " usage
 '
 
diff --git a/t/t6500-gc.sh b/t/t6500-gc.sh
index ea9aaad470..b40d13d7ff 100755
--- a/t/t6500-gc.sh
+++ b/t/t6500-gc.sh
@@ -35,7 +35,7 @@ test_expect_success 'gc -h with invalid configuration' '
 		cd broken &&
 		git init &&
 		echo "[gc] pruneexpire = CORRUPT" >>.git/config &&
-		test_expect_code 129 git gc -h >usage 2>&1
+		git gc -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage" broken/usage
 '
diff --git a/t/t7030-verify-tag.sh b/t/t7030-verify-tag.sh
index 2c147072c1..3bc5d1e9a2 100755
--- a/t/t7030-verify-tag.sh
+++ b/t/t7030-verify-tag.sh
@@ -8,9 +8,9 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success GPG 'verify-tag does not crash with -h' '
-	test_expect_code 129 git verify-tag -h >usage &&
+	git verify-tag -h >usage &&
 	test_grep "[Uu]sage: git verify-tag " usage &&
-	test_expect_code 129 nongit git verify-tag -h >usage &&
+	nongit git verify-tag -h >usage &&
 	test_grep "[Uu]sage: git verify-tag " usage
 '
 
diff --git a/t/t7508-status.sh b/t/t7508-status.sh
index c2057bc94c..de7d7beec3 100755
--- a/t/t7508-status.sh
+++ b/t/t7508-status.sh
@@ -16,7 +16,7 @@ test_expect_success 'status -h in broken repository' '
 		cd broken &&
 		git init &&
 		echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
-		test_expect_code 129 git status -h >usage 2>&1
+		git status -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage" broken/usage
 '
@@ -28,7 +28,7 @@ test_expect_success 'commit -h in broken repository' '
 		cd broken &&
 		git init &&
 		echo "[status] showuntrackedfiles = CORRUPT" >>.git/config &&
-		test_expect_code 129 git commit -h >usage 2>&1
+		git commit -h >usage 2>&1
 	) &&
 	test_grep "[Uu]sage" broken/usage
 '
diff --git a/t/t7510-signed-commit.sh b/t/t7510-signed-commit.sh
index aa9108da54..e641f9e334 100755
--- a/t/t7510-signed-commit.sh
+++ b/t/t7510-signed-commit.sh
@@ -9,9 +9,9 @@ GNUPGHOME_NOT_USED=$GNUPGHOME
 . "$TEST_DIRECTORY/lib-gpg.sh"
 
 test_expect_success GPG 'verify-commit does not crash with -h' '
-	test_expect_code 129 git verify-commit -h >usage &&
+	git verify-commit -h >usage &&
 	test_grep "[Uu]sage: git verify-commit " usage &&
-	test_expect_code 129 nongit git verify-commit -h >usage &&
+	nongit git verify-commit -h >usage &&
 	test_grep "[Uu]sage: git verify-commit " usage
 '
 
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index f877d9a433..fd3d1d67f9 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 >usage
+		git merge -h >usage
 	) &&
 	test_grep "[Uu]sage: git merge" broken/usage
 '
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 8a91ff3603..961de3efab 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -27,12 +27,11 @@ prompt_given ()
 }
 
 test_expect_success 'basic usage requires no repo' '
-	test_expect_code 129 git difftool -h >output &&
+	git difftool -h >output &&
 	test_grep ^usage: output &&
 	# create a ceiling directory to prevent Git from finding a repo
 	mkdir -p not/repo &&
 	test_when_finished rm -r not &&
-	test_expect_code 129 \
 	env GIT_CEILING_DIRECTORIES="$(pwd)/not" \
 	git -C not/repo difftool -h >output &&
 	test_grep ^usage: output
diff --git a/t/t7900-maintenance.sh b/t/t7900-maintenance.sh
index d7f82e1bec..9886f641fc 100755
--- a/t/t7900-maintenance.sh
+++ b/t/t7900-maintenance.sh
@@ -35,7 +35,7 @@ test_systemd_analyze_verify () {
 }
 
 test_expect_success 'help text' '
-	test_expect_code 129 git maintenance -h >actual &&
+	git maintenance -h >actual &&
 	test_grep "usage: git maintenance <subcommand>" actual &&
 	test_expect_code 129 git maintenance barf 2>err &&
 	test_grep "unknown subcommand: \`barf'\''" err &&
diff --git a/usage.c b/usage.c
index 527edb1e79..3f0118ab2a 100644
--- a/usage.c
+++ b/usage.c
@@ -188,7 +188,7 @@ static void show_usage_if_asked_helper(const char *err, ...)
 	va_start(params, err);
 	vfreportf(stdout, _("usage: "), err, params);
 	va_end(params);
-	exit(129);
+	exit(0);
 }
 
 void show_usage_if_asked(int ac, const char **av, const char *err)

Re: [PATCH v2 0/4] rev-parse: exit 0 on --help

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> The standard philosophy for Unix software when a help option (such as
> --help) is specified is that the software should exit 0, printing the
> help output to standard output, since the standard output is for
> user-requested output and the program performed the requested task
> successfully.  If the user specifies an incorrect option, then the help
> output should be printed to standard error (since the user has made a
> mistake) and it should exit unsuccessfully.

Hmph.

> git rev-parse --parseopt properly directs the output in both of these
> cases, but it currently exits 129 when it receives a --help or -h option
> on the command line, which causes its invoking script to do the same.
> This is not in line with the usual behavior and it causes scripts using
> this command to exit unsuccessfully on --help as well.
>
> This series introduces some changes to distinguish the --help and -h
> options from other cases in which we print help output and adjusts the
> exit code to 0 from those two options.  We continue to exit 129 when the
> options are invalid, which is useful information to have for callers.
> We also make the relevant changes such that `git rev-parse --parseopt`
> does the same thing as long as it is invoked in the way specified in the
> manual page (which a quick GitHub search shows almost everyone does).
>
> One of the patches is rather long because we have many cases in which
> we've hard-coded exit code 129 into our tests.  However, the changes
> there should not be complex, only somewhat tedious to review.

It is borderline for "yes, we all know it is obvious that things
should have worked this way from day one, we regret that it is not
the case, but it has been working differently and users' scripts all
have been working with the current behaviour, and it is likely that
they will all break".

Two big things that make it much less likely, saving grace, are that
this is only about "--help" (which is unlikely to be a part of
end-user script), and this makes the invocation succeed (if we were
changing from exit 0 to exit 129, we would be breaking tons more).

;-)

Re: [PATCH v2 0/4] rev-parse: exit 0 on --help

[Jeff King]

On Wed, Jul 01, 2026 at 03:06:52PM -0700, Junio C Hamano wrote:

> > One of the patches is rather long because we have many cases in which
> > we've hard-coded exit code 129 into our tests.  However, the changes
> > there should not be complex, only somewhat tedious to review.
> 
> It is borderline for "yes, we all know it is obvious that things
> should have worked this way from day one, we regret that it is not
> the case, but it has been working differently and users' scripts all
> have been working with the current behaviour, and it is likely that
> they will all break".
> 
> Two big things that make it much less likely, saving grace, are that
> this is only about "--help" (which is unlikely to be a part of
> end-user script), and this makes the invocation succeed (if we were
> changing from exit 0 to exit 129, we would be breaking tons more).

My big concern is a script accidentally continuing when fed "--help" and
generating nonsense. But I think the eval magic explained in patch 3
makes that unlikely (any such caller was already kind-of broken).

The other issue I raised in the earlier round is that a script like:

  cat >git-foo <<\EOF
  #!/bin/sh
  git log --my-options "$@" >output || exit 1
  do_something <output
  EOF

when invoked as "git foo --help" will now call do_something with
nonsense input, rather than exiting from the "error" returned by
git-log. This only affects hacky little scripts like this that are not
otherwise parsing their own options, but sometimes those are the most
common. ;)

I'm not convinced there will be much fallout, but it is possible.

-Peff

Re: [PATCH v2 0/4] rev-parse: exit 0 on --help

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The other issue I raised in the earlier round is that a script like:
>
>   cat >git-foo <<\EOF
>   #!/bin/sh
>   git log --my-options "$@" >output || exit 1
>   do_something <output
>   EOF
>
> when invoked as "git foo --help" will now call do_something with
> nonsense input, rather than exiting from the "error" returned by
> git-log. This only affects hacky little scripts like this that are not
> otherwise parsing their own options, but sometimes those are the most
> common. ;)

Yeah, I agree that the above is a much more likely breakage scenario
than I imagined.

> I'm not convinced there will be much fallout, but it is possible.

True.