t0040,t1502: Demonstrate parse_options bugs

PATCH In review Brandon Casey 2 participants 5 messages Sep 25, 2017

Share & tools

Brandon Casey

author

2017-09-25 04:08 UTC #0 lore raw

PATCH 1/3 t0040,t1502: Demonstrate parse_options bugs

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 <[email protected]>

---

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

t/helper/test-parse-options.c +2 -0

index 75fe883..630c76d 100644
@@ -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;

t/t0040-parse-options.sh +5 -3

index 74d2cd7..a36434b 100755
@@ -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

t/t1502-rev-parse-parseopt.sh +100 -0

112 lines not on this page · raw message

Signature · 1 line

2.2.0.rc3

Brandon Casey

author

2017-09-25 04:08 UTC #1 lore raw

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

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 <[email protected]>

---
 parse-options.c               | 2 +-
 t/t0040-parse-options.sh      | 6 +++---
 t/t1502-rev-parse-parseopt.sh | 2 +-
 3 files changed, 5 insertions(+), 5 deletions(-)

parse-options.c +1 -1

index 0dd9fc6..6a03a52 100644
@@ -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++;
 	}

t/t0040-parse-options.sh +3 -3

index a36434b..0c2fc81 100755
@@ -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

t/t1502-rev-parse-parseopt.sh +1 -1

index 1bfa80f..ce7dda1 100755
@@ -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>...

Signature · 1 line

2.2.0.rc3

Brandon Casey

author

2017-09-25 04:08 UTC #2 lore raw

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

In reply to 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 <[email protected]>

---
 parse-options.c               | 10 ++++++++--
 t/t1502-rev-parse-parseopt.sh |  4 ++--
 2 files changed, 10 insertions(+), 4 deletions(-)

parse-options.c +8 -2

index 6a03a52..fca7159 100644
@@ -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)

t/t1502-rev-parse-parseopt.sh +2 -2

index ce7dda1..a859abe 100755
@@ -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>...

Signature · 1 line

2.2.0.rc3

Junio C Hamano

2017-09-25 05:39 UTC #3 lore raw

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

Quoted 7 lines · 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.

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.

Junio C Hamano

2017-09-25 05:53 UTC #4 lore raw

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

Quoted 10 lines · Junio C Hamano

Quoted 7 lines · 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.

Quoted 7 lines

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.