[PATCH v2 0/6] parse-options: properly align continued usage output & related

From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
	"Carlo Arenas" <carenas@gmail.com>,
	"Eric Sunshine" <sunshine@sunshineco.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 0/6] parse-options: properly align continued usage output & related
Date: Fri, 10 Sep 2021 17:38:30 +0200	[thread overview]
Message-ID: <cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com>

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

This v2 now also gets rid of the support for "" in the usage string
array. Eric Sunshine had ideas[1] for how to simplify the code in v1,
along with a suggestion that we could just get rid of the "" from
"builtin/blame.c".

I'd done some experiments with that before submitting v1, but decided
to try to submit something simpler to begin with. But let's just grow
this in scope a bit and do that, as shown here we also need to do the
same in builtin/credential-cache*.c.

This gives us a much nicer end-state, and as an added bonus
effectively brings back the check removed with the removal of
USE_PARENS_AROUND_GETTEXT_N in in-flight ]2], which is what brought
this alignment issue & edge cases in parse_options() usage to my
attention in the first place.

1. https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/
2. https://lore.kernel.org/git/878s0gwmvq.fsf@evledraar.gmail.com/

Ævar Arnfjörð Bjarmason (6):
  test-lib.sh: add a UNIX_SOCKETS prerequisite
  git.c: add a NEED_UNIX_SOCKETS option for built-ins
  parse-options: stop supporting "" in the usagestr array
  built-ins: "properly" align continued usage output
  send-pack: properly use parse_options() API for usage string
  parse-options: properly align continued usage output

 Documentation/git-send-pack.txt    |  4 +-
 builtin.h                          |  6 +++
 builtin/blame.c                    |  9 ++--
 builtin/credential-cache--daemon.c | 11 +----
 builtin/credential-cache.c         | 11 +----
 builtin/ls-remote.c                |  4 +-
 builtin/rev-parse.c                |  3 ++
 builtin/send-pack.c                |  8 ++--
 builtin/show-branch.c              |  6 +--
 builtin/stash.c                    |  2 +-
 builtin/tag.c                      |  4 +-
 git.c                              | 15 +++++--
 parse-options.c                    | 71 +++++++++++++++++++++++++-----
 t/helper/test-parse-options.c      |  2 -
 t/t0012-help.sh                    | 10 +++++
 t/t0040-parse-options.sh           |  2 -
 t/t0301-credential-cache.sh        |  5 ++-
 t/t1502-rev-parse-parseopt.sh      | 34 +++++++-------
 t/test-lib.sh                      |  1 +
 19 files changed, 131 insertions(+), 77 deletions(-)

Range-diff against v1:
-:  ----------- > 1:  9e8facb6f8c test-lib.sh: add a UNIX_SOCKETS prerequisite
-:  ----------- > 2:  d6c44402715 git.c: add a NEED_UNIX_SOCKETS option for built-ins
-:  ----------- > 3:  11f4a119563 parse-options: stop supporting "" in the usagestr array
1:  ccc024c414f ! 4:  4547cc968b1 built-ins: "properly" align continued usage output
    @@ Commit message
         output.

         But two wrongs don't make a right, let's "fix" this by making it worse
    -    temporarily, in anticipating of improving parse-options.c to handle
    +    temporarily, in anticipation of improving parse-options.c to handle
         this alignment.

         The issue is that we should have whitespace corresponding to the
    @@ builtin/stash.c: static const char * const git_stash_push_usage[] = {

      ## builtin/tag.c ##
    -@@ builtin/tag.c: static const char * const git_tag_usage[] = {
    - 		"\t\t<tagname> [<head>]"),
    +@@
    + 
    + static const char * const git_tag_usage[] = {
    + 	N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
    +-		"\t\t<tagname> [<head>]"),
    ++	   "        <tagname> [<head>]"),
      	N_("git tag -d <tagname>..."),
      	N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
     -		"\t\t[--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
-:  ----------- > 5:  b884b361ace send-pack: properly use parse_options() API for usage string
2:  ab4bb70902b ! 6:  e83d66da6d4 parse-options: properly align continued usage output
    @@ Commit message

                 N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
                    "          [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
    +               [...]

         We'd like to have that output aligned with the length of the initial
         "git stash " output, but since usage_with_options_internal() adds its
    @@ Commit message
                              [-u|--include-untracked] [-a|--all] [-m|--message <message>]
                              [...]

    -    In making this change we can can fold the two for-loops over *usagestr
    -    into one. We had two of them purely to account for the case where an
    -    empty string in the array delimits the usage output from free-form
    -    text output.
    +    We could also go for an approach where we have the caller support no
    +    padding of their own, i.e. (same as the first example, except for the
    +    padding on the second line):
    +
    +            N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
    +               "[-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
    +               [...]
    +
    +    But to do that we'll need to find the length of "git stash". We can
    +    discover that from the "cmd" in the "struct cmd_struct", but there
    +    might cases with sub-commands or "git" itself taking arguments that
    +    would make that non-trivial.
    +
    +    Even if it was I still think this approach is better, because this way
    +    we'll get the same legible alignment in the C code. The fact that
    +    usage_with_options_internal() is adding its own prefix padding is an
    +    implementation detail that callers shouldn't need to worry about.
    +
    +    Implementation notes:

         We could skip the string_list_split() with a strchr(str, '\n') check,
         but we'd then need to duplicate our state machine for strings that do
    @@ 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 <avarab@gmail.com>

      ## parse-options.c ##
    @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *

     +	const char *usage_prefix = _("usage: %s");
     +	/*
    ++	 * The translation could be anything, but we can count on
    ++	 * msgfmt(1)'s --check option to have asserted that "%s" is in
    ++	 * the translation. So compute the length of the "usage: "
    ++	 * part. We are assuming that the translator wasn't overly
    ++	 * clever and used e.g. "%1$s" instead of "%s", there's only
    ++	 * one "%s" in "usage_prefix" above, so there's no reason to
    ++	 * do so even with a RTL language.
    ++	 */
    ++	size_t usage_len = strlen(usage_prefix) - strlen("%s");
    ++	/*
     +	 * TRANSLATORS: the colon here should align with the
     +	 * 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 *
     +	 * Russian, Chinese etc.).
     +	 *
     +	 * When a translated usage string has an embedded "\n" it's
    -+	 * because options have wrapped o the next line. The line
    ++	 * because options have wrapped to the next line. The line
     +	 * after the "\n" will then be padded to align with the
     +	 * command name, such as N_("git cmd [opt]\n<8
     +	 * spaces>[opt2]"), where the 8 spaces are the same length as
    @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
     +	 * This format string prints out that already-translated
     +	 * line. The "%*s" is whitespace padding to account for the
     +	 * padding at the start of the line that we add in this
    -+	 * function, the "%s" is a line in the (hopefully already
    ++	 * function. The "%s" is a line in the (hopefully already
     +	 * translated) N_() usage string, which contained embedded
     +	 * newlines before we split it up.
     +	 */
     +	const char *usage_continued = _("%*s%s");
    -+
    -+	/*
    -+	 * The translation could be anything, but we can count on
    -+	 * msgfmt(1)'s --check option to have asserted that "%s" is in
    -+	 * the translation. So compute the length of the " or: "
    -+	 * part. We are assuming that the translator wasn't overly
    -+	 * clever and used e.g. "%1$s" instead of "%s", there's only
    -+	 * one "%s" in "or_prefix" above, so there's no reason to do
    -+	 * so even with a RTL language.
    -+	 */
    -+	size_t or_len = strlen(or_prefix) - strlen("%s");
    -+	int i;
    -+	int saw_empty_line = 0;
    ++	const char *prefix = usage_prefix;
     +
      	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 && **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++;
    -+	for (i = 0; *usagestr; i++) {
    -+		const char *str = _(*usagestr++);
     +		struct string_list list = STRING_LIST_INIT_DUP;
     +		unsigned int j;
     +
    -+		string_list_split(&list, str, '\n', -1);
    ++		string_list_split(&list, _(*usagestr++), '\n', -1);
     +		for (j = 0; j < list.nr; j++) {
     +			const char *line = list.items[j].string;
     +
    -+			if (!saw_empty_line && !*line)
    -+				saw_empty_line = 1;
    ++			if (!*line)
    ++				BUG("empty or trailing line in usage string");
     +
    -+			if (saw_empty_line && *line)
    -+				fprintf_ln(outfile, _("    %s"), line);
    -+			else if (saw_empty_line)
    -+				fputc('\n', outfile);
    -+			else if (!j && !i)
    -+				fprintf_ln(outfile, usage_prefix, line);
    -+			else if (!j)
    -+				fprintf_ln(outfile, or_prefix, line);
    ++			if (!j)
    ++				fprintf_ln(outfile, prefix, line);
     +			else
     +				fprintf_ln(outfile, usage_continued,
    -+					   (int)or_len, "", line);
    ++					   (int)usage_len, "", line);
     +		}
     +		string_list_clear(&list, 0);
    ++
    ++		prefix = or_prefix;
      	}

      	need_newline = 1;
-- 
2.33.0.876.g423ac861752