[PATCH 1/3] t0040,t1502: Demonstrate parse_options bugs

[PATCH 1/3] t0040,t1502: Demonstrate parse_options bugs

[Brandon Casey]

When the option spec contains no switches or only hidden switches,
parse_options will emit an extra blank line at the end of help output so
that the help text will end in two blank lines instead of one.

When parse_options produces internal help output after an error has
occurred it will emit blank lines within the usage string to stdout
instead of stderr.

Update t/helper/test-parse-options.c to have a description body in the
usage string to exercise this second bug and mark tests as failing in
t0040.

Add tests to t1502 to demonstrate both of these problems.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

Notes:
    FYI: this is built on top of bc/rev-parse-parseopt-fix (697bc88) merged
    into next.

 t/helper/test-parse-options.c |   2 +
 t/t0040-parse-options.sh      |   8 ++--
 t/t1502-rev-parse-parseopt.sh | 100 ++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 107 insertions(+), 3 deletions(-)

diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 75fe883..630c76d 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -99,6 +99,8 @@ int cmd_main(int argc, const char **argv)
 	const char *prefix = "prefix/";
 	const char *usage[] = {
 		"test-parse-options <options>",
+		"",
+		"A helper function for the parse-options API.",
 		NULL
 	};
 	struct string_list expect = STRING_LIST_INIT_NODUP;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index 74d2cd7..a36434b 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -10,6 +10,8 @@ test_description='our own option parser'
 cat >expect <<\EOF
 usage: test-parse-options <options>
 
+    A helper function for the parse-options API.
+
     --yes                 get a boolean
     -D, --no-doubt        begins with 'no-'
     -B, --no-fear         be brave
@@ -90,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
 test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
 test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
 
-test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
-test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'
+test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
+test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
@@ -286,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 
 >expect
 
-test_expect_success 'OPT_CALLBACK() and callback errors work' '
+test_expect_failure 'OPT_CALLBACK() and callback errors work' '
 	test_must_fail test-parse-options --no-length >output 2>output.err &&
 	test_i18ncmp expect output &&
 	test_i18ncmp expect.err output.err
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 6e1b45f..1bfa80f 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -38,6 +38,25 @@ test_expect_success 'setup optionspec' '
 EOF
 '
 
+test_expect_success 'setup optionspec-no-switches' '
+	sed -e "s/^|//" >optionspec_no_switches <<\EOF
+|some-command [options] <args>...
+|
+|some-command does foo and bar!
+|--
+EOF
+'
+
+test_expect_success 'setup optionspec-only-hidden-switches' '
+	sed -e "s/^|//" >optionspec_only_hidden_switches <<\EOF
+|some-command [options] <args>...
+|
+|some-command does foo and bar!
+|--
+|hidden1* A hidden switch
+EOF
+'
+
 test_expect_success 'test --parseopt help output' '
 	sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
@@ -79,6 +98,87 @@ END_EXPECT
 	test_i18ncmp expect output
 '
 
+test_expect_failure 'test --parseopt help output no switches' '
+	sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] <args>...
+|
+|    some-command does foo and bar!
+|
+|EOF
+END_EXPECT
+	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
+	test_i18ncmp expect output
+'
+
+test_expect_failure 'test --parseopt help output hidden switches' '
+	sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] <args>...
+|
+|    some-command does foo and bar!
+|
+|EOF
+END_EXPECT
+	test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
+	test_i18ncmp expect output
+'
+
+test_expect_success 'test --parseopt help-all output hidden switches' '
+	sed -e "s/^|//" >expect <<\END_EXPECT &&
+|cat <<\EOF
+|usage: some-command [options] <args>...
+|
+|    some-command does foo and bar!
+|
+|    --hidden1             A hidden switch
+|
+|EOF
+END_EXPECT
+	test_expect_code 129 git rev-parse --parseopt -- --help-all > output < optionspec_only_hidden_switches &&
+	test_i18ncmp expect output
+'
+
+test_expect_failure 'test --parseopt invalid switch help output' '
+	sed -e "s/^|//" >expect <<\END_EXPECT &&
+|error: unknown option `does-not-exist'\''
+|usage: some-command [options] <args>...
+|
+|    some-command does foo and bar!
+|
+|    -h, --help            show the help
+|    --foo                 some nifty option --foo
+|    --bar ...             some cool option --bar with an argument
+|    -b, --baz             a short and long option
+|
+|An option group Header
+|    -C[...]               option C with an optional argument
+|    -d, --data[=...]      short and long option with an optional argument
+|
+|Argument hints
+|    -B <arg>              short option required argument
+|    --bar2 <arg>          long option required argument
+|    -e, --fuz <with-space>
+|                          short and long option required argument
+|    -s[<some>]            short option optional argument
+|    --long[=<data>]       long option optional argument
+|    -g, --fluf[=<path>]   short and long option optional argument
+|    --longest <very-long-argument-hint>
+|                          a very long argument hint
+|    --pair <key=value>    with an equals sign in the hint
+|    --aswitch             help te=t contains? fl*g characters!`
+|    --bswitch <hint>      hint has trailing tab character
+|    --cswitch             switch has trailing tab character
+|    --short-hint <a>      with a one symbol hint
+|
+|Extras
+|    --extra1              line above used to cause a segfault but no longer does
+|
+END_EXPECT
+	test_expect_code 129 git rev-parse --parseopt -- --does-not-exist 1>/dev/null 2>output < optionspec &&
+	test_i18ncmp expect output
+'
+
 test_expect_success 'setup expect.1' "
 	cat > expect <<EOF
 set -- --foo --bar 'ham' -b --aswitch -- 'arg'
-- 
2.2.0.rc3

[PATCH 2/3] parse-options: write blank line to correct output stream

[Brandon Casey]

When commit 54e6dc7 added translation support to parse-options, an
fprintf was mistakenly replaced by a call to putchar().  Let's use fputc
instead.

Fixes t0040.11, t0040.12, t0040.33, and t1502.8.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 parse-options.c               | 2 +-
 t/t0040-parse-options.sh      | 6 +++---
 t/t1502-rev-parse-parseopt.sh | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 0dd9fc6..6a03a52 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -599,7 +599,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (**usagestr)
 			fprintf_ln(outfile, _("    %s"), _(*usagestr));
 		else
-			putchar('\n');
+			fputc('\n', outfile);
 		usagestr++;
 	}
 
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index a36434b..0c2fc81 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -92,8 +92,8 @@ test_expect_success 'OPT_BOOL() is idempotent #2' 'check boolean: 1 -DB'
 test_expect_success 'OPT_BOOL() negation #1' 'check boolean: 0 -D --no-yes'
 test_expect_success 'OPT_BOOL() negation #2' 'check boolean: 0 -D --no-no-doubt'
 
-test_expect_failure 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
-test_expect_failure 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'
+test_expect_success 'OPT_BOOL() no negation #1' 'check_unknown_i18n --fear'
+test_expect_success 'OPT_BOOL() no negation #2' 'check_unknown_i18n --no-no-fear'
 
 test_expect_success 'OPT_BOOL() positivation' 'check boolean: 0 -D --doubt'
 
@@ -288,7 +288,7 @@ test_expect_success 'OPT_CALLBACK() and OPT_BIT() work' '
 
 >expect
 
-test_expect_failure 'OPT_CALLBACK() and callback errors work' '
+test_expect_success 'OPT_CALLBACK() and callback errors work' '
 	test_must_fail test-parse-options --no-length >output 2>output.err &&
 	test_i18ncmp expect output &&
 	test_i18ncmp expect.err output.err
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 1bfa80f..ce7dda1 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -139,7 +139,7 @@ END_EXPECT
 	test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt invalid switch help output' '
+test_expect_success 'test --parseopt invalid switch help output' '
 	sed -e "s/^|//" >expect <<\END_EXPECT &&
 |error: unknown option `does-not-exist'\''
 |usage: some-command [options] <args>...
-- 
2.2.0.rc3

[PATCH 3/3] parse-options: only insert newline in help text if needed

[Brandon Casey]

Currently, when parse_options() produces a help message it always emits
a blank line after the usage text to separate it from the options text.
If the option spec does not define any switches, or only defines hidden
switches that will not be displayed, then the help text will end up with
two trailing blank lines instead of one.  Let's defer emitting the blank
line between the usage text and the options text until it is clear that
the options section will not be empty.

Fixes t1502.5, t1502.6.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 parse-options.c               | 10 ++++++++--
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 6a03a52..fca7159 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -581,6 +581,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 				       const struct option *opts, int full, int err)
 {
 	FILE *outfile = err ? stderr : stdout;
+	int need_newline;
 
 	if (!usagestr)
 		return PARSE_OPT_HELP;
@@ -603,8 +604,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		usagestr++;
 	}
 
-	if (opts->type != OPTION_GROUP)
-		fputc('\n', outfile);
+	need_newline = 1;
 
 	for (; opts->type != OPTION_END; opts++) {
 		size_t pos;
@@ -612,6 +612,7 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 
 		if (opts->type == OPTION_GROUP) {
 			fputc('\n', outfile);
+			need_newline = 0;
 			if (*opts->help)
 				fprintf(outfile, "%s\n", _(opts->help));
 			continue;
@@ -619,6 +620,11 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 		if (!full && (opts->flags & PARSE_OPT_HIDDEN))
 			continue;
 
+		if (need_newline) {
+			fputc('\n', outfile);
+			need_newline = 0;
+		}
+
 		pos = fprintf(outfile, "    ");
 		if (opts->short_name) {
 			if (opts->flags & PARSE_OPT_NODASH)
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index ce7dda1..a859abe 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -98,7 +98,7 @@ END_EXPECT
 	test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt help output no switches' '
+test_expect_success 'test --parseopt help output no switches' '
 	sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] <args>...
@@ -111,7 +111,7 @@ END_EXPECT
 	test_i18ncmp expect output
 '
 
-test_expect_failure 'test --parseopt help output hidden switches' '
+test_expect_success 'test --parseopt help output hidden switches' '
 	sed -e "s/^|//" >expect <<\END_EXPECT &&
 |cat <<\EOF
 |usage: some-command [options] <args>...
-- 
2.2.0.rc3

Re: [PATCH 3/3] parse-options: only insert newline in help text if needed

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> Currently, when parse_options() produces a help message it always emits
> a blank line after the usage text to separate it from the options text.
> If the option spec does not define any switches, or only defines hidden
> switches that will not be displayed, then the help text will end up with
> two trailing blank lines instead of one.  Let's defer emitting the blank
> line between the usage text and the options text until it is clear that
> the options section will not be empty.

This somehow looks familiar.  I think (together with the fix in 2/3)
this makes it definitely better.  

I also wonder if we want the final blank line, but that is sort-of a
different issue.

Thanks.

Re: [PATCH 3/3] parse-options: only insert newline in help text if needed

[Junio C Hamano]

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

> Brandon Casey <drafnel@gmail.com> writes:
>
>> Currently, when parse_options() produces a help message it always emits
>> a blank line after the usage text to separate it from the options text.
>> If the option spec does not define any switches, or only defines hidden
>> switches that will not be displayed, then the help text will end up with
>> two trailing blank lines instead of one.  Let's defer emitting the blank
>> line between the usage text and the options text until it is clear that
>> the options section will not be empty.
>
> This somehow looks familiar.  I think (together with the fix in 2/3)
> this makes it definitely better.  
>
> I also wonder if we want the final blank line, but that is sort-of a
> different issue.
>
> Thanks.

Oh, no wonder that this looked familiar.  It solves the same issue
as 48b8d3cf ("usage_with_options: omit double new line on empty
option list", 2017-08-25) and of course it conflicts with it.

I find the solution presented with this patch is more direct and
straightforward, leaving less chance to future breakage.  Besides
it comes with tests ;-), so perhaps I should drop the other one.