[v4,0/4] parse-options: properly align continued usage output

Message

Ævar Arnfjörð Bjarmason Sept. 13, 2021, 12:13 a.m. UTC

This series changes usage_with_options_internal() in parse-options.c
to properly align continued "\n" usage output.

This v4 essentially goes back to the version of this in v1. In v2 I'd
addressed Eric's comments in [1] by trying to remove the last three
in-tree users of this part of the API.

But if "git rev-parse --parseopt" is going to be bug-for-bug
compatible with existing out-of-tree users[2] then changing that
becomes a non-starter.

So let's keep the existing behavior (which also had at least one other
weird & undocumented edge case, which we now test for).

This v4 also has a fix for the "saw_empty_line" bug Eric noted in [2]
(which wasn't applicable to v2..v3), and takes some of his suggestions
for simplifying the loop (but not all, I still kept it as one loop).

1. https://lore.kernel.org/git/[e-mail]/
2. https://lore.kernel.org/git/xmqqpmtdjuqj.fsf@gitster.g/

Ævar Arnfjörð Bjarmason (4):
  parse-options API users: align usage output in C-strings
  send-pack: properly use parse_options() API for usage string
  git rev-parse --parseopt tests: add more usagestr tests
  parse-options: properly align continued usage output

 Documentation/git-send-pack.txt |  4 +-
 builtin/ls-remote.c             |  4 +-
 builtin/send-pack.c             |  8 ++--
 builtin/show-branch.c           |  6 +--
 builtin/stash.c                 |  2 +-
 builtin/tag.c                   |  4 +-
 parse-options.c                 | 76 +++++++++++++++++++++++++++------
 t/t1502-rev-parse-parseopt.sh   | 54 +++++++++++++++++++++++
 8 files changed, 132 insertions(+), 26 deletions(-)

Range-diff against v3:
1:  ad857e80fd8 < -:  ----------- credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
2:  036eb0efb5b < -:  ----------- blame: replace usage end blurb with better option spec
4:  5638d2bc832 ! 1:  64d8647340d built-ins: "properly" align continued usage output
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    built-ins: "properly" align continued usage output
    +    parse-options API users: align usage output in C-strings
     
    -    Let's "fix" various "git <cmd> -h" output by "properly" aligning the
    -    output in cases where we continue usage output after a "\n". The "fix"
    -    and "properly" scare quotes are because this actually makes things
    -    worse in some cases, because e.g. in the case of "git tag -h" the
    -    "\t\t" effectively works around how parse-options.c aligns this
    -    output.
    +    In preparation for having continued usage lines properly aligned in
    +    "git <cmd> -h" output, let's have the "[" on the second such lines
    +    align with the "[" on the first line.
     
    -    But two wrongs don't make a right, let's "fix" this by making it worse
    -    temporarily, in anticipation of improving parse-options.c to handle
    -    this alignment.
    +    In some cases this makes the output worse, because e.g. the "git
    +    ls-remote -h" output had been aligned to account for the extra
    +    whitespace that the usage_with_options_internal() function in
    +    parse-options.c would add.
     
    -    The issue is that we should have whitespace corresponding to the
    -    length of the command name here, e.g. in the case of "git ls-remote"
    -    it should be 14 characters, or the length of ""git ls-remote
    -    ". Instead we had 21 characters in builtin/ls-remote.c, those extra 7
    -    characters are the length of "usage: " (and also " or:"). So in the C
    -    locale the resulting output aligns nicely:
    +    In other cases such as builtin/stash.c (not changed here), we were
    +    aligned in the C strings, but since that didn't account for the extra
    +    padding in usage_with_options_internal() it would come out looking
    +    misaligned, e.g. code like this:
     
    -        $ git ls-remote -h
    -        usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
    -                             [-q | --quiet] [--exit-code] [--get-url]
    -                             [--symref] [<repository> [<refs>...]]
    +            N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
    +               "          [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
     
    -    But that's fragile, we might not be under the C locale. We really
    -    should have parse-options.c itself add this padding. In a subsequent
    -    commit I'll make it do that.
    +    Would emit:
     
    -    In the case of "tag" and "show-branch" and "stash save" the output was
    -    not properly aligned, although in the "git tag" case it was
    -    near-enough (aligned with the "-" in "git tag -l") to look good,
    -    assuming C locale & a tab-width of 8. In any case, let's align this in
    -    a way that looks obviously correct when looking at the source itself,
    -    and then improve parse-options.c itself.
    +       or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
    +              [-u|--include-untracked] [-a|--all] [-m|--message <message>]
    +
    +    Let's change all the usage arrays which use such continued usage
    +    output via "\n"-embedding to be like builtin/stash.c.
    +
    +    This makes the output worse temporarily, but in a subsequent change
    +    I'll improve the usage_with_options_internal() to take this into
    +    account, at which point all of the strings being changed here will
    +    emit prettier output.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
5:  821e5e14132 = 2:  f10ff775c69 send-pack: properly use parse_options() API for usage string
3:  e23a8231f38 ! 3:  05a0c7cac37 parse-options: stop supporting "" in the usagestr array
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## Commit message ##
    -    parse-options: stop supporting "" in the usagestr array
    +    git rev-parse --parseopt tests: add more usagestr tests
     
    -    The strings in the the "usagestr" array have special handling for the
    -    empty string dating back to f389c808b67 (Rework make_usage to print
    -    the usage message immediately, 2007-10-14).
    +    Add tests for the "usagestr" passed to parse-options.c
    +    usage_with_options_internal() through cmd_parseopt().
     
    -    We'll prefix all strings after the first one with "or: ". Then if we
    -    encountered a "" we'll emit all strings after that point verbatim
    -    without any "or: " prefixing.
    -
    -    In the preceding commits we got rid of the two users of this
    -    undocumented part of the API. Let's remove it in preparation for
    -    improving the output emitted by usage_with_options_internal().
    -
    -    I think we might want to use this in the future, but in that case
    -    we'll be much better off with an API that emulates the
    -    non-parse_options() way that git.c does this.
    -
    -    That git.c code uses a separate "git_usage_string" and
    -    "git_more_info_string". See b7d9681974e (Print info about "git help
    -    COMMAND" on git's main usage pages, 2008-06-06). By splitting the two
    -    up we can emit something in the middle, as indeed git.c does.
    -
    -    I'd like our "git <cmd> -h" info to be more helpful, and I'd also like
    -    parse_options() to handle the "git" command itself, because of the
    -    limitations of how this was done in usage_with_options_internal() we
    -    couldn't migrate a caller like git.c to parse_options().
    -
    -    So let's just remove this for now, it has no users, and once we want
    -    to do this again we can simply add another argument to the relevant
    -    functions, or otherwise hook into things so that we can print
    -    something at the end and/or middle.
    -
    -    It's possible that this change introduce breakage somewhere. We'd only
    -    catch these cases at runtime, and the "git rev-parse --parseopt"
    -    command is used by shellscripts, see bac199b7b17 (Update
    -    git-sh-setup(1) to allow transparent use of git-rev-parse --parseopt,
    -    2007-11-04). I've grepped the codebase for "OPTIONS_SPEC",
    -    "char.*\*.*usage\[\]" etc. I'm fairly sure there no outstanding users
    -    of this functionality.
    +    These test for edge cases in the existing behavior related to the
    +    "--parseopt" interface doing its own line-splitting with
    +    strbuf_getline(), and the native C interface expecting and potentially
    +    needing to handle newlines within the strings in the array it
    +    accepts. The results are probably something that wasn't anticipated,
    +    but let's make sure we stay backwards compatible with it.
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
    - ## builtin/rev-parse.c ##
    -@@ builtin/rev-parse.c: static int cmd_parseopt(int argc, const char **argv, const char *prefix)
    - 	for (;;) {
    - 		if (strbuf_getline(&sb, stdin) == EOF)
    - 			die(_("premature end of input"));
    -+		if (!sb.len)
    -+			die(_("empty lines are not permitted before the `--' separator"));
    - 		ALLOC_GROW(usage, unb + 1, usz);
    -+
    - 		if (!strcmp("--", sb.buf)) {
    - 			if (unb < 1)
    - 				die(_("no usage string given before the `--' separator"));
    -
    - ## parse-options.c ##
    -@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
    - 		fprintf(outfile, "cat <<\\EOF\n");
    - 
    - 	fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
    --	while (*usagestr && **usagestr)
    -+	while (*usagestr) {
    - 		/*
    - 		 * TRANSLATORS: the colon here should align with the
    - 		 * one in "usage: %s" translation.
    - 		 */
    - 		fprintf_ln(outfile, _("   or: %s"), _(*usagestr++));
    --	while (*usagestr) {
    --		if (**usagestr)
    --			fprintf_ln(outfile, _("    %s"), _(*usagestr));
    --		else
    --			fputc('\n', outfile);
    --		usagestr++;
    - 	}
    - 
    - 	need_newline = 1;
    -
    - ## t/helper/test-parse-options.c ##
    -@@ t/helper/test-parse-options.c: int cmd__parse_options(int argc, const char **argv)
    - 	const char *prefix = "prefix/";
    - 	const char *usage[] = {
    - 		"test-tool 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 ##
    -@@ t/t0040-parse-options.sh: test_description='our own option parser'
    - cat >expect <<\EOF
    - usage: test-tool 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 ##
    -@@ t/t1502-rev-parse-parseopt.sh: test_description='test git rev-parse --parseopt'
    - test_expect_success 'setup optionspec' '
    - 	sed -e "s/^|//" >optionspec <<\EOF
    - |some-command [options] <args>...
    --|
    --|some-command does foo and bar!
    - |--
    - |h,help    show the help
    - |
    -@@ t/t1502-rev-parse-parseopt.sh: 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
    - '
    -@@ t/t1502-rev-parse-parseopt.sh: 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
    -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output' '
    - |cat <<\EOF
    - |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
    -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output no switches' '
    - |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 &&
    -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output hidden switches' '
    - |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 &&
    -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '
    - |cat <<\EOF
    - |usage: some-command [options] <args>...
    - |
    --|    some-command does foo and bar!
    --|
    - |    --hidden1             A hidden switch
    - |
    - |EOF
    -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt invalid switch help output' '
    - |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
     @@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt --stuck-long and short option with unset op
      	test_cmp expect output
      '
      
    -+test_expect_success 'test --parseopt help output hidden switches' '
    -+	sed -e "s/^|//" >optionspec-trailing-line <<-\EOF &&
    -+	|some-command [options] <args>...
    ++test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" lines' '
    ++	sed -e "s/^|//" >spec <<-\EOF &&
    ++	|cmd [--some-option]
    ++	|    [--another-option]
    ++	|cmd [--yet-another-option]
    ++	|--
    ++	|h,help    show the help
    ++	EOF
    ++
    ++	sed -e "s/^|//" >expect <<-\END_EXPECT &&
    ++	|cat <<\EOF
    ++	|usage: cmd [--some-option]
    ++	|   or:     [--another-option]
    ++	|   or: cmd [--yet-another-option]
    ++	|
    ++	|    -h, --help            show the help
     +	|
    ++	|EOF
    ++	END_EXPECT
    ++
    ++	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
    ++	test_cmp expect actual
    ++'
    ++
    ++test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
    ++	sed -e "s/^|//" >spec <<-\EOF &&
    ++	|cmd [--some-option]
    ++	|    [--another-option]
     +	|
    ++	|multi
    ++	|line
    ++	|blurb
     +	|--
     +	|h,help    show the help
     +	EOF
     +
    -+	cat >expect <<-\EOF &&
    -+	fatal: empty lines are not permitted before the `--'"'"' separator
    -+	EOF
    ++	sed -e "s/^|//" >expect <<-\END_EXPECT &&
    ++	|cat <<\EOF
    ++	|usage: cmd [--some-option]
    ++	|   or:     [--another-option]
    ++	|
    ++	|    multi
    ++	|    line
    ++	|    blurb
    ++	|
    ++	|    -h, --help            show the help
    ++	|
    ++	|EOF
    ++	END_EXPECT
     +
    -+	test_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&
    -+	test_must_be_empty out &&
    ++	test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
     +	test_cmp expect actual
     +'
     +
6:  0df2840ce4e ! 4:  55480dee680 parse-options: properly align continued usage output
    @@ Commit message
         automatically, 2007-10-15) which isn't RTL-safe, but that code would
         be easy to fix. Let's not introduce new RTL translation problems here.
     
    -    I'm also adding a check to catch the mistake of needlessly adding a
    -    trailing "\n", such as:
    -
    -            N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
    -               "          [-u|--include-untracked] [-a|--all] [<message>]\n"),
    -
    -    Or even a mistake like adding just one "\n" in a string with no other
    -    newlines:
    -
    -            N_("git stash list [<options>]\n"),
    -
    -    This catches the cases already tested for in cmd_parseopt(), but this
    -    covers the purely C API. As noted a preceding commit that added the
    -    die() to cmd_parseopt() I'm fairly confident that this will be
    -    triggered by no in-tree user I've missed.
    -
         Signed-off-by: Ævar Arnfjörð Bjarmason <[e-mail]>
     
      ## parse-options.c ##
    @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
     +	 * one in "usage: %s" translation.
     +	 */
     +	const char *or_prefix = _("   or: %s");
    -+
     +	/*
     +	 * TRANSLATORS: You should only need to translate this format
     +	 * string if your language is a RTL language (e.g. Arabic,
    @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
     +	 */
     +	const char *usage_continued = _("%*s%s");
     +	const char *prefix = usage_prefix;
    ++	int saw_empty_line = 0;
     +
      	if (!usagestr)
      		return PARSE_OPT_HELP;
    @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
      		fprintf(outfile, "cat <<\\EOF\n");
      
     -	fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
    - 	while (*usagestr) {
    +-	while (*usagestr && **usagestr)
     -		/*
     -		 * TRANSLATORS: the colon here should align with the
     -		 * one in "usage: %s" translation.
     -		 */
     -		fprintf_ln(outfile, _("   or: %s"), _(*usagestr++));
    + 	while (*usagestr) {
    +-		if (**usagestr)
    +-			fprintf_ln(outfile, _("    %s"), _(*usagestr));
    +-		else
    +-			fputc('\n', outfile);
    +-		usagestr++;
    ++		const char *str = _(*usagestr++);
     +		struct string_list list = STRING_LIST_INIT_DUP;
     +		unsigned int j;
     +
    -+		string_list_split(&list, _(*usagestr++), '\n', -1);
    ++		if (!saw_empty_line && !*str)
    ++			saw_empty_line = 1;
    ++
    ++		string_list_split(&list, str, '\n', -1);
     +		for (j = 0; j < list.nr; j++) {
     +			const char *line = list.items[j].string;
     +
    -+			if (!*line)
    -+				BUG("empty or trailing line in usage string");
    -+
    -+			if (!j)
    ++			if (saw_empty_line && *line)
    ++				fprintf_ln(outfile, _("    %s"), line);
    ++			else if (saw_empty_line)
    ++				fputc('\n', outfile);
    ++			else if (!j)
     +				fprintf_ln(outfile, prefix, line);
     +			else
     +				fprintf_ln(outfile, usage_continued,