Merge branch 'bc/rev-parse-parseopt-fix'

Recent versions of "git rev-parse --parseopt" did not parse the
option specification that does not have the optional flags (*=?!)
correctly, which has been corrected.

* bc/rev-parse-parseopt-fix:
  parse-options: only insert newline in help text if needed
  parse-options: write blank line to correct output stream
  t0040,t1502: Demonstrate parse_options bugs
  git-rebase: don't ignore unexpected command line arguments
  rev-parse parseopt: interpret any whitespace as start of help text
  rev-parse parseopt: do not search help text for flag chars
  t1502: demonstrate rev-parse --parseopt option mis-parsing

builtin/rev-parse.c

@@ -387,6 +387,14 @@ static const char *skipspaces(const char *s)
 	return s;
 }
 
+static char *findspace(const char *s)
+{
+	for (; *s; s++)
+		if (isspace(*s))
+			return (char*)s;
+	return NULL;
+}
+
 static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 {
 	static int keep_dashdash = 0, stop_at_non_option = 0;
@@ -434,7 +442,7 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 	/* parse: (<short>|<short>,<long>|<long>)[*=?!]*<arghint>? SP+ <help> */
 	while (strbuf_getline(&sb, stdin) != EOF) {
 		const char *s;
-		const char *help;
+		char *help;
 		struct option *o;
 
 		if (!sb.len)
@@ -444,15 +452,17 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)
 		memset(opts + onb, 0, sizeof(opts[onb]));
 
 		o = &opts[onb++];
-		help = strchr(sb.buf, ' ');
-		if (!help || *sb.buf == ' ') {
+		help = findspace(sb.buf);
+		if (!help || sb.buf == help) {
 			o->type = OPTION_GROUP;
 			o->help = xstrdup(skipspaces(sb.buf));
 			continue;
 		}
 
+		*help = '\0';
+
 		o->type = OPTION_CALLBACK;
-		o->help = xstrdup(skipspaces(help));
+		o->help = xstrdup(skipspaces(help+1));
 		o->value = &parsed;
 		o->flags = PARSE_OPT_NOARG;
 		o->callback = &parseopt_dump;

git-rebase.sh

@@ -350,6 +350,9 @@ do
 		shift
 		break
 		;;
+	*)
+		usage
+		;;
 	esac
 	shift
 done

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;
@@ -599,12 +600,11 @@ 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++;
 	}
 
-	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/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;

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

t/t1502-rev-parse-parseopt.sh

@@ -28,6 +28,9 @@ test_expect_success 'setup optionspec' '
 |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
@@ -35,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
@@ -62,6 +84,9 @@ test_expect_success 'test --parseopt help output' '
 |    --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
@@ -73,19 +98,100 @@ END_EXPECT
 	test_i18ncmp expect output
 '
 
+test_expect_success '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_success '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_success '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 -- 'arg'
+set -- --foo --bar 'ham' -b --aswitch -- 'arg'
 EOF
 "
 
 test_expect_success 'test --parseopt' '
-	git rev-parse --parseopt -- --foo --bar=ham --baz arg < optionspec > output &&
+	git rev-parse --parseopt -- --foo --bar=ham --baz --aswitch arg < optionspec > output &&
 	test_cmp expect output
 '
 
 test_expect_success 'test --parseopt with mixed options and arguments' '
-	git rev-parse --parseopt -- --foo arg --bar=ham --baz < optionspec > output &&
+	git rev-parse --parseopt -- --foo arg --bar=ham --baz --aswitch < optionspec > output &&
 	test_cmp expect output
 '