git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Eric Sunshine <sunshine@sunshineco.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>, git@vger.kernel.org
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Carlo Arenas <carenas@gmail.com>
Subject: Re: [PATCH 2/2] parse-options: properly align continued usage output
Date: Fri, 10 Sep 2021 03:51:45 -0400	[thread overview]
Message-ID: <f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com> (raw)
In-Reply-To: <patch-2.2-ab4bb70902b-20210901T110917Z-avarab@gmail.com>

On 9/1/21 7:12 AM, Ævar Arnfjörð Bjarmason wrote:
> Some commands such as "git stash" emit continued options output with
> e.g. "git stash -h", because usage_with_options_internal() prefixes
> with its own whitespace the resulting output wasn't properly
> aligned. Let's account for the added whitespace, which properly aligns
> the output.
> 
> The "git stash" command has usage output with a N_() translation that
> legitimately stretches across multiple lines;
> 
> 	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
> own whitespace prefixing we fell short, before this change we'd emit:
> 
>      $ git stash -h
>      usage: git stash list [<options>]
>         or: git stash show [<options>] [<stash>]
>         [...]
>         or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
>                [-u|--include-untracked] [-a|--all] [-m|--message <message>]
>                [...]
> 
> Now we'll properly emit aligned output.  I.e. the last four lines
> above will instead be (a whitespace-only change to the above):
> 
>         [...]
>         or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
>                       [-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.

More on this below...

> 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
> and don't contain a "\n". It's simpler to just always split into a
> "struct string_list", even though the common case is that that "struct
> string_list" will contain only one element. This is not
> performance-sensitive code.

Makes sense.

> This change is relatively more complex since I've accounted for making
> it future-proof for RTL translation support. Later in
> usage_with_options_internal() we have some existing padding code
> dating back to d7a38c54a6c (parse-options: be able to generate usages
> 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.
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/parse-options.c b/parse-options.c
> @@ -917,25 +917,78 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
> +	 * When a translated usage string has an embedded "\n" it's
> +	 * because options have wrapped o the next line. The line

"wrapped o the next line"?

> +	size_t or_len = strlen(or_prefix) - strlen("%s");
> +	int i;
> +	int saw_empty_line = 0;
> +
> -	fprintf_ln(outfile, _("usage: %s"), _(*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++;
> +	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);
> +		for (j = 0; j < list.nr; j++) {
> +			const char *line = list.items[j].string;
> +
> +			if (!saw_empty_line && !*line)
> +				saw_empty_line = 1;
> +
> +			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);
> +			else
> +				fprintf_ln(outfile, usage_continued,
> +					   (int)or_len, "", line);
> +		}
> +		string_list_clear(&list, 0);

I may be missing something obvious, but I'm having trouble understanding 
why this single loop is better than the two loops it replaces. The 
cognitive load of the new code is much higher than that of the original. 
With the original code, the logic was obvious at a glance. On the other 
hand, I had to concentrate hard to figure out what the new code is 
trying to do and to wrap my brain around all the cases it is handling. I 
suppose you went with the single loop to avoid code duplication (in 
particular, the call to string_list_split() and the loop over the split 
elements)?

There are other ways this might be accomplished which don't carry such a 
high cognitive load. One (typed-in-email) possibility which closely 
resembles the existing code:

     const char *pfx = usage_prefix;
     while (*usagestr && **usagestr) {
         string_list_split(&list, _(*usagestr++), ...);
         fprintf_ln(outfile, pfx, list.items[0].string);
         for (i = 1; i < list.nr; i++)
             fprintf_ln(outfile, usage_continued,
                 (int)or_len, "", list.items[i].string);
         pfx = or_prefix;
     }
     while (*usagestr) {
         string_list_split(&list, _(*usagestr++), ...);
         for (i = 0; i < list.nr; i++) {
             const char *line = list.items[i].string;
             if (*line)
                 fprintf_ln(outfile, _("    %s"), line);
             else
                 fputc('\n', outfile);
         }
     }

I also wonder if you really need to support the embedded-newline case 
for the free-form text loop. For free-form text, it's just as easy for 
each line of text to be a distinct item in the usage[] array, if I 
understand correctly, so there isn't really a good reason for clients to 
embed newlines in the free-form text portion. Given that there's only a 
single client in the entire project which takes advantage of the 
free-form text support -- and that client doesn't embed newlines -- it 
may be simpler to not bother supporting embedded newlines for the 
free-form text, in which case you don't even need to modify that loop; 
the existing code is good enough.

Anyhow, the above observations are subjective, thus not necessarily 
actionable, however, there is also a subtle yet dramatic behavior change 
in the new code, if I understand correctly. It's not clear if this 
behavior change is intentional (it isn't mentioned in the commit 
message), but it does seem potentially dangerous. Specifically, with the:

     if (!saw_empty_line && !*line)
         saw_empty_line = 1;

check inside the inner loop which iterates over the split lines, this 
means that if someone accidentally embeds an extra newline in some usage 
line:

     static const char *foo_usage[] = {
         N_("git foo --bar\n\n" /* <-- accidental extra newline */
            "        --baz"),
         N_("git boo"),
         NULL
     };

then _all_ following usage lines will incorrectly be treated as 
free-form text lines rather than as the "or:" lines they are intended to 
be. Moving the:

     if (!saw_empty_line && !*line)
         saw_empty_line = 1;

check from the inner loop to the outer loop should restore the original 
(intended) behavior, I believe.

  reply	other threads:[~2021-09-10  7:51 UTC|newest]

Thread overview: 45+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2021-09-01 11:12 [PATCH 0/2] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-01 11:12 ` [PATCH 1/2] built-ins: "properly" " Ævar Arnfjörð Bjarmason
2021-09-01 11:12 ` [PATCH 2/2] parse-options: properly " Ævar Arnfjörð Bjarmason
2021-09-10  7:51   ` Eric Sunshine [this message]
2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
2021-09-10 15:38   ` [PATCH v2 1/6] test-lib.sh: add a UNIX_SOCKETS prerequisite Ævar Arnfjörð Bjarmason
2021-09-10 15:38   ` [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins Ævar Arnfjörð Bjarmason
2021-09-11  0:14     ` Junio C Hamano
2021-09-11  2:50       ` Ævar Arnfjörð Bjarmason
2021-09-11  3:47         ` Carlo Marcelo Arenas Belón
2021-09-11  6:12           ` Junio C Hamano
2021-09-11  7:16           ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38   ` [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-11  0:21     ` Junio C Hamano
2021-09-11  2:46       ` Ævar Arnfjörð Bjarmason
2021-09-11  6:52         ` Junio C Hamano
2021-09-10 15:38   ` [PATCH v2 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11  0:25     ` Junio C Hamano
2021-09-11  2:40       ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38   ` [PATCH v2 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-10 15:38   ` [PATCH v2 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11  7:41   ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Eric Sunshine
2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2021-09-11 19:09     ` [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS Ævar Arnfjörð Bjarmason
2021-09-12 21:48       ` Junio C Hamano
2021-09-11 19:09     ` [PATCH v3 2/6] blame: replace usage end blurb with better option spec Ævar Arnfjörð Bjarmason
2021-09-12  4:45       ` Eric Sunshine
2021-09-12 22:22       ` Junio C Hamano
2021-09-11 19:09     ` [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-12 22:26       ` Junio C Hamano
2021-09-13  0:21         ` Ævar Arnfjörð Bjarmason
2021-09-11 19:09     ` [PATCH v3 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 19:09     ` [PATCH v3 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-11 19:09     ` [PATCH v3 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-13  0:13     ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
2021-09-13  0:13       ` [PATCH v4 1/4] parse-options API users: align usage output in C-strings Ævar Arnfjörð Bjarmason
2021-09-13  0:13       ` [PATCH v4 2/4] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-13  0:13       ` [PATCH v4 3/4] git rev-parse --parseopt tests: add more usagestr tests Ævar Arnfjörð Bjarmason
2021-09-13  0:13       ` [PATCH v4 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-13  6:41         ` Junio C Hamano
2021-09-21 13:30       ` [PATCH v5 0/4] " Ævar Arnfjörð Bjarmason
2021-09-21 13:30         ` [PATCH v5 1/4] parse-options API users: align usage output in C-strings Ævar Arnfjörð Bjarmason
2021-09-21 13:30         ` [PATCH v5 2/4] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-21 13:30         ` [PATCH v5 3/4] git rev-parse --parseopt tests: add more usagestr tests Ævar Arnfjörð Bjarmason
2021-09-21 13:30         ` [PATCH v5 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com \
    --to=sunshine@sunshineco.com \
    --cc=avarab@gmail.com \
    --cc=carenas@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).