rev-parse: have --parseopt callers exit 0 on --help

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>
Signed-off-by: Junio C Hamano <gitster@pobox.com>

diff --git a/parse-options.c b/parse-options.c
index fd8ceed82bc55705758f1e121e05269de1a794e3..cc3a8b0fe3c658d09f98b96a65ace743cb8ea32e 100644 (file)
--- 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 err ? PARSE_OPT_HELP_ERROR : PARSE_OPT_HELP;
 }

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 3962f1d2882a1f4331e0ca8c83c0ac27fecf2019..455608c42947c768b764167f64a712636bacf6c2 100755 (executable)
--- 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 7a29f8cb03374e076eaf6e435a2abf6a8401b94f..f85be7b8fda4b2082e360f263d6e188cc8cebf17 100644 (file)
--- 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 cbdd54d41b7be0c843471624a021d546fbdb4a7f..ded35ebc822a05119fc2dbd339f76b110ea7e287 100755 (executable)
--- 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 6421bdb3c3a4da306f1c386f179a66ba52f76f66..03fa2f9cdfdb0d9030a418e08b28db690872f461 100755 (executable)
--- a/t/t1517-outside-repo.sh
+++ b/t/t1517-outside-repo.sh
@@ -132,10 +132,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 ;;