All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH 0/2] parse-options: properly align continued usage output
@ 2021-09-01 11:12 Ævar Arnfjörð Bjarmason
  2021-09-01 11:12 ` [PATCH 1/2] built-ins: "properly" " Ævar Arnfjörð Bjarmason
                   ` (2 more replies)
  0 siblings, 3 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-01 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas,
	Ævar Arnfjörð Bjarmason

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

Before this change some built-ins using the API effectively tried to
hack around the lack of such alignment, with some getting it wrong,
and some getting it right. Others such as "git stash -h" didn't at
all, and their continued "\n" output would always look weirdly
misaligned. Now the output all looks nice!

This came about from following the thread of "why we we even have '\n'
here at all?", which is a question I had in my own mind around
https://lore.kernel.org/git/87zgszxirn.fsf@evledraar.gmail.com

Ævar Arnfjörð Bjarmason (2):
  built-ins: "properly" align continued usage output
  parse-options: properly align continued usage output

 builtin/ls-remote.c   |  4 +--
 builtin/show-branch.c |  6 ++--
 builtin/stash.c       |  2 +-
 builtin/tag.c         |  2 +-
 parse-options.c       | 79 ++++++++++++++++++++++++++++++++++++-------
 5 files changed, 73 insertions(+), 20 deletions(-)

-- 
2.33.0.807.gf14ecf9c2e9


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 1/2] built-ins: "properly" align continued usage output
  2021-09-01 11:12 [PATCH 0/2] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
@ 2021-09-01 11:12 ` Ævar Arnfjörð Bjarmason
  2021-09-01 11:12 ` [PATCH 2/2] parse-options: properly " Ævar Arnfjörð Bjarmason
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
  2 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-01 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas,
	Ævar Arnfjörð Bjarmason

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.

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
this alignment.

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:

    $ git ls-remote -h
    usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
                         [-q | --quiet] [--exit-code] [--get-url]
                         [--symref] [<repository> [<refs>...]]

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.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-remote.c   | 4 ++--
 builtin/show-branch.c | 6 +++---
 builtin/stash.c       | 2 +-
 builtin/tag.c         | 2 +-
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index 1794548c711..ef9036974ce 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -7,8 +7,8 @@
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
-	   "                     [-q | --quiet] [--exit-code] [--get-url]\n"
-	   "                     [--symref] [<repository> [<refs>...]]"),
+	   "              [-q | --quiet] [--exit-code] [--get-url]\n"
+	   "              [--symref] [<repository> [<refs>...]]"),
 	NULL
 };
 
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d77ce7aeb38..a82cd1534fc 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -11,9 +11,9 @@
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
-       "		[--current] [--color[=<when>] | --no-color] [--sparse]\n"
-       "		[--more=<n> | --list | --independent | --merge-base]\n"
-       "		[--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
+       "                [--current] [--color[=<when>] | --no-color] [--sparse]\n"
+       "                [--more=<n> | --list | --independent | --merge-base]\n"
+       "                [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
     N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"),
     NULL
 };
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..45b19007d7c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {
 
 static const char * const git_stash_save_usage[] = {
 	N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
-	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	   "               [-u|--include-untracked] [-a|--all] [<message>]"),
 	NULL
 };
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 452558ec957..9b1165d2a4e 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {
 		"\t\t<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>...]"),
+	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
-- 
2.33.0.807.gf14ecf9c2e9


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH 2/2] parse-options: properly align continued usage output
  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 ` Ævar Arnfjörð Bjarmason
  2021-09-10  7:51   ` Eric Sunshine
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
  2 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-01 11:12 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas,
	Ævar Arnfjörð Bjarmason

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.

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.

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>
---
 parse-options.c | 79 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 66 insertions(+), 13 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 2abff136a17..a06968bf4f5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -917,25 +917,78 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;
 
+	const char *usage_prefix = _("usage: %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,
+	 * Hebrew etc.), not if it's a LTR language (e.g. German,
+	 * Russian, Chinese etc.).
+	 *
+	 * When a translated usage string has an embedded "\n" it's
+	 * because options have wrapped o 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
+	 * "git cmd ".
+	 *
+	 * 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
+	 * 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;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
-	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);
 	}
 
 	need_newline = 1;
-- 
2.33.0.807.gf14ecf9c2e9


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH 2/2] parse-options: properly align continued usage output
  2021-09-01 11:12 ` [PATCH 2/2] parse-options: properly " Ævar Arnfjörð Bjarmason
@ 2021-09-10  7:51   ` Eric Sunshine
  0 siblings, 0 replies; 45+ messages in thread
From: Eric Sunshine @ 2021-09-10  7:51 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas

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.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 0/6] parse-options: properly align continued usage output & related
  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 15:38 ` Æ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
                     ` (7 more replies)
  2 siblings, 8 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 1/6] test-lib.sh: add a UNIX_SOCKETS prerequisite
  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   ` Æ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
                     ` (6 subsequent siblings)
  7 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Instead of checking $NO_UNIX_SOCKETS directly in
t/t0301-credential-cache.sh, follow the more common pattern of
creating a test prerequisite in test-lib.sh.

See 6320358e31d (Makefile: unix sockets may not available on some
platforms, 2011-12-12) for the original implementation of
NO_UNIX_SOCKETS.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 t/t0301-credential-cache.sh | 5 +++--
 t/test-lib.sh               | 1 +
 2 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh
index ebd5fa5249c..002de427984 100755
--- a/t/t0301-credential-cache.sh
+++ b/t/t0301-credential-cache.sh
@@ -4,10 +4,11 @@ test_description='credential-cache tests'
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-credential.sh
 
-test -z "$NO_UNIX_SOCKETS" || {
+if ! test_have_prereq UNIX_SOCKETS
+then
 	skip_all='skipping credential-cache tests, unix sockets not available'
 	test_done
-}
+fi
 
 # don't leave a stale daemon running
 test_atexit 'git credential-cache exit'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index abcfbed6d61..583f266b1e8 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1533,6 +1533,7 @@ test -z "$NO_PYTHON" && test_set_prereq PYTHON
 test -n "$USE_LIBPCRE2" && test_set_prereq PCRE
 test -n "$USE_LIBPCRE2" && test_set_prereq LIBPCRE2
 test -z "$NO_GETTEXT" && test_set_prereq GETTEXT
+test -z "$NO_UNIX_SOCKETS" && test_set_prereq UNIX_SOCKETS
 
 if test -z "$GIT_TEST_CHECK_CACHE_TREE"
 then
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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   ` Ævar Arnfjörð Bjarmason
  2021-09-11  0:14     ` Junio C Hamano
  2021-09-10 15:38   ` [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
                     ` (5 subsequent siblings)
  7 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Change the implementation of b5dd96b70ac (make credential helpers
builtins, 2020-08-13) to declare in the "struct cmd_struct" that
NO_UNIX_SOCKETS can't be set.

This is one of two in-tree users for the empty lines in
parse_options() usage, getting rid of that is the main motivation for
this, but it also doesn't make sense to emit these sorts of usage
messages just to appease t0012-help.sh, which seemingly b5dd96b70ac
aimed to do.

I.e. these commands don't support "[options]", or "<action>" so
emitting that at the beginning is incorrect. We should just die right
away.

The existing code also had the edge case of not emitting the die()
message if a "-h" argument was given, since parse_options() will
handle the exit() itself in that case. We could feed it
PARSE_OPT_NO_INTERNAL_HELP, but this is better.

By moving this up to the "struct cmd_struct" we can also skip these in
--list-cmds=builtins instead, as noted above we shouldn't be exiting
with code 129 in these cases.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin.h                          |  6 ++++++
 builtin/credential-cache--daemon.c | 11 +----------
 builtin/credential-cache.c         | 11 +----------
 git.c                              | 15 ++++++++++++---
 t/t0012-help.sh                    | 10 ++++++++++
 5 files changed, 30 insertions(+), 23 deletions(-)

diff --git a/builtin.h b/builtin.h
index 16ecd5586f0..66713da6a02 100644
--- a/builtin.h
+++ b/builtin.h
@@ -63,6 +63,12 @@
  *	more informed decision, e.g., by ignoring `pager.<cmd>` for
  *	certain subcommands.
  *
+ * `NEED_UNIX_SOCKETS`:
+ *
+ *	This built-in will not work if NO_UNIX_SOCKETS is defined. It
+ *	will be recognized for emitting error messages, but won't be
+ *	listed in --list-cmds=builtins.
+ *
  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
  *
  * Additionally, if `foo` is a new command, there are 4 more things to do:
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 4c6c89ab0de..d9863287a4d 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -304,16 +304,7 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache--daemon [options] <action>",
-		"",
-		"credential-cache--daemon is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache--daemon unavailable; no unix socket support"));
+	BUG("should not be called under NO_UNIX_SOCKETS");
 }
 
 #endif /* NO_UNIX_SOCKET */
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index e8a74157471..22b49b265bf 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -142,16 +142,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache [options] <action>",
-		"",
-		"credential-cache is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache unavailable; no unix socket support"));
+	BUG("should not be called under NO_UNIX_SOCKETS");
 }
 
 #endif /* NO_UNIX_SOCKETS */
diff --git a/git.c b/git.c
index 18bed9a9964..6b0248841db 100644
--- a/git.c
+++ b/git.c
@@ -17,6 +17,7 @@
 #define SUPPORT_SUPER_PREFIX	(1<<4)
 #define DELAY_PAGER_CONFIG	(1<<5)
 #define NO_PARSEOPT		(1<<6) /* parse-options is not used */
+#define NEED_UNIX_SOCKETS	(1<<7) /* Works unless -DNO_UNIX_SOCKETS */
 
 struct cmd_struct {
 	const char *cmd;
@@ -66,6 +67,10 @@ static int list_cmds(const char *spec)
 	struct string_list list = STRING_LIST_INIT_DUP;
 	int i;
 	int nongit;
+	unsigned int exclude_option = 0;
+#ifdef NO_UNIX_SOCKETS
+	exclude_option |= NEED_UNIX_SOCKETS;
+#endif
 
 	/*
 	* Set up the repository so we can pick up any repo-level config (like
@@ -78,7 +83,7 @@ static int list_cmds(const char *spec)
 		int len = sep - spec;
 
 		if (match_token(spec, len, "builtins"))
-			list_builtins(&list, 0);
+			list_builtins(&list, exclude_option);
 		else if (match_token(spec, len, "main"))
 			list_all_main_cmds(&list);
 		else if (match_token(spec, len, "others"))
@@ -423,6 +428,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
 	const char *prefix;
 
 	prefix = NULL;
+#ifdef NO_UNIX_SOCKETS
+	if (p->option & NEED_UNIX_SOCKETS)
+		die(_("%s is unavailable; there is no UNIX socket support in this build of Git"), p->cmd);
+#endif
 	help = argc == 2 && !strcmp(argv[1], "-h");
 	if (!help) {
 		if (p->option & RUN_SETUP)
@@ -513,8 +522,8 @@ static struct cmd_struct commands[] = {
 	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
 	{ "count-objects", cmd_count_objects, RUN_SETUP },
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
-	{ "credential-cache", cmd_credential_cache },
-	{ "credential-cache--daemon", cmd_credential_cache_daemon },
+	{ "credential-cache", cmd_credential_cache, NEED_UNIX_SOCKETS },
+	{ "credential-cache--daemon", cmd_credential_cache_daemon, NEED_UNIX_SOCKETS },
 	{ "credential-store", cmd_credential_store },
 	{ "describe", cmd_describe, RUN_SETUP },
 	{ "diff", cmd_diff, NO_PARSEOPT },
diff --git a/t/t0012-help.sh b/t/t0012-help.sh
index 5679e29c624..2d05dde4b90 100755
--- a/t/t0012-help.sh
+++ b/t/t0012-help.sh
@@ -85,4 +85,14 @@ do
 	'
 done <builtins
 
+test_expect_success UNIX_SOCKETS 'builtin list includes NEED_UNIX_SOCKETS under UNIX_SOCKETS' '
+	grep ^credential-cache$ builtins &&
+	grep ^credential-cache--daemon$ builtins
+'
+
+test_expect_success !UNIX_SOCKETS 'builtin list excludes NEED_UNIX_SOCKETS under !UNIX_SOCKETS' '
+	! grep ^credential-cache$ builtins &&
+	! grep ^credential-cache--daemon$ builtins
+'
+
 test_done
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array
  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-10 15:38   ` Ævar Arnfjörð Bjarmason
  2021-09-11  0:21     ` Junio C Hamano
  2021-09-10 15:38   ` [PATCH v2 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
                     ` (4 subsequent siblings)
  7 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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

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.

This gets rid of that special case, which was added in
f389c808b67 (Rework make_usage to print the usage message immediately,
2007-10-14). It was only used "blame" (the "credential-cache*" use of
it was removed in the preceding commit). Before this change we'd emit:

    $ git blame -h
    usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>

        <rev-opts> are documented in git-rev-list(1)

This changes that output to simply use "[<git-rev-list args>]" instead
of "[<rev-opts>]". This accomplishes the same, is more consistent as
"git bundle" and "git blame" use the same way of referring to these
options now.

The use of this in "blame" dated back to 5817da01434 (git-blame:
migrate to incremental parse-option [1/2], 2008-07-08), and the use in
"bundle" to 2e0afafebd8 (Add git-bundle: move objects and
references by archive, 2007-02-22).

Once we get rid of this special case we can also use usage_msg_opt()
to emit the error message we'd get on an invalid "-L <range>"
argument, which means we can get rid of the old-style "blame_usage"
variable.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/blame.c               |  9 +++------
 builtin/rev-parse.c           |  3 +++
 parse-options.c               |  8 +-------
 t/helper/test-parse-options.c |  2 --
 t/t0040-parse-options.sh      |  2 --
 t/t1502-rev-parse-parseopt.sh | 34 ++++++++++++++++++----------------
 6 files changed, 25 insertions(+), 33 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..45d9873a999 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -29,12 +29,8 @@
 #include "refs.h"
 #include "tag.h"
 
-static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
-
 static const char *blame_opt_usage[] = {
-	blame_usage,
-	"",
-	N_("<rev-opts> are documented in git-rev-list(1)"),
+	N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),
 	NULL
 };
 
@@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 				    nth_line_cb, &sb, lno, anchor,
 				    &bottom, &top, sb.path,
 				    the_repository->index))
-			usage(blame_usage);
+			usage_msg_opt(_("Invalid -L <range> parameters"),
+				      blame_opt_usage, options);
 		if ((!lno && (top || bottom)) || lno < bottom)
 			die(Q_("file %s has only %lu line",
 			       "file %s has only %lu lines",
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 22c4e1a4ff0..aeebfd52805 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -436,7 +436,10 @@ 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"));
diff --git a/parse-options.c b/parse-options.c
index 2abff136a17..950a8279beb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -924,18 +924,12 @@ 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;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 2051ce57db7..e00aef073b0 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -102,8 +102,6 @@ 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;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ad4746d899a..2910874ece5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -10,8 +10,6 @@ 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
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b29563fc997..6badc650d64 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -6,8 +6,6 @@ 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
 |
@@ -41,8 +39,6 @@ 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
 '
@@ -50,8 +46,6 @@ 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
@@ -62,8 +56,6 @@ 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
@@ -103,8 +95,6 @@ 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 &&
@@ -116,8 +106,6 @@ 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 &&
@@ -129,8 +117,6 @@ 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
@@ -144,8 +130,6 @@ 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
@@ -282,4 +266,22 @@ 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>...
+	|
+	|
+	|--
+	|h,help    show the help
+	EOF
+
+	cat >expect <<-\EOF &&
+	fatal: empty lines are not permitted before the `--'"'"' separator
+	EOF
+
+	test_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&
+	test_must_be_empty out &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 4/6] built-ins: "properly" align continued usage output
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
                     ` (2 preceding siblings ...)
  2021-09-10 15:38   ` [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
@ 2021-09-10 15:38   ` Ævar Arnfjörð Bjarmason
  2021-09-11  0:25     ` Junio C Hamano
  2021-09-10 15:38   ` [PATCH v2 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
                     ` (3 subsequent siblings)
  7 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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.

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.

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:

    $ git ls-remote -h
    usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
                         [-q | --quiet] [--exit-code] [--get-url]
                         [--symref] [<repository> [<refs>...]]

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.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-remote.c   | 4 ++--
 builtin/show-branch.c | 6 +++---
 builtin/stash.c       | 2 +-
 builtin/tag.c         | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f4fd823af83..318949c3d75 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -7,8 +7,8 @@
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
-	   "                     [-q | --quiet] [--exit-code] [--get-url]\n"
-	   "                     [--symref] [<repository> [<refs>...]]"),
+	   "              [-q | --quiet] [--exit-code] [--get-url]\n"
+	   "              [--symref] [<repository> [<refs>...]]"),
 	NULL
 };
 
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index d77ce7aeb38..a82cd1534fc 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -11,9 +11,9 @@
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
-       "		[--current] [--color[=<when>] | --no-color] [--sparse]\n"
-       "		[--more=<n> | --list | --independent | --merge-base]\n"
-       "		[--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
+       "                [--current] [--color[=<when>] | --no-color] [--sparse]\n"
+       "                [--more=<n> | --list | --independent | --merge-base]\n"
+       "                [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
     N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"),
     NULL
 };
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..45b19007d7c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {
 
 static const char * const git_stash_save_usage[] = {
 	N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
-	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	   "               [-u|--include-untracked] [-a|--all] [<message>]"),
 	NULL
 };
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 065b6bf093e..6535ed27ee9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,10 +23,10 @@
 
 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>...]"),
+	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 5/6] send-pack: properly use parse_options() API for usage string
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
                     ` (3 preceding siblings ...)
  2021-09-10 15:38   ` [PATCH v2 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
@ 2021-09-10 15:38   ` Ævar Arnfjörð Bjarmason
  2021-09-10 15:38   ` [PATCH v2 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
                     ` (2 subsequent siblings)
  7 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

When "send-pack" was changed to use the parse_options() API in
068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)
it was made to use one very long line, instead it should split them up
with newlines.

Furthermore we were including an inline explanation that you couldn't
combine "--all" and "<ref>", but unlike in the "blame" case this was
not preceded by an empty string.

Let's instead show that --all and <ref> can't be combined in the the
usual language of the usage syntax instead. We can make it clear that
one of the two options "--foo" and "--bar" is mandatory, but that the
two are mutually exclusive by referring to them as "( --foo | --bar
)".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-pack.txt | 4 ++--
 builtin/send-pack.c             | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 44fd146b912..be41f119740 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
+'git send-pack' [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
 		[--verbose] [--thin] [--atomic]
 		[--[no-]signed|--signed=(true|false|if-asked)]
-		[<host>:]<directory> [<ref>...]
+		[<host>:]<directory> (--all | <ref>...)
 
 DESCRIPTION
 -----------
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 729dea1d255..89321423125 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -17,10 +17,10 @@
 #include "protocol.h"
 
 static const char * const send_pack_usage[] = {
-	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
-	  "[--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] "
-	  "[<host>:]<directory> [<ref>...]\n"
-	  "  --all and explicit <ref> specification are mutually exclusive."),
+	N_("git send-pack [--mirror] [--dry-run] [--force]\n"
+	   "              [--receive-pack=<git-receive-pack>]\n"
+	   "              [--verbose] [--thin] [--atomic]\n"
+	   "              [<host>:]<directory> (--all | <ref>...)"),
 	NULL,
 };
 
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v2 6/6] parse-options: properly align continued usage output
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
                     ` (4 preceding siblings ...)
  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   ` Æ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
  7 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-10 15:38 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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>]
                     [...]

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
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.

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.

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 | 65 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 950a8279beb..ff28869d2c9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -917,19 +917,72 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;
 
+	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,
+	 * Hebrew etc.), not if it's a LTR language (e.g. German,
+	 * Russian, Chinese etc.).
+	 *
+	 * When a translated usage string has an embedded "\n" it's
+	 * 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
+	 * "git cmd ".
+	 *
+	 * 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
+	 * translated) N_() usage string, which contained embedded
+	 * newlines before we split it up.
+	 */
+	const char *usage_continued = _("%*s%s");
+	const char *prefix = usage_prefix;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
-	fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
 	while (*usagestr) {
-		/*
-		 * TRANSLATORS: the colon here should align with the
-		 * one in "usage: %s" translation.
-		 */
-		fprintf_ln(outfile, _("   or: %s"), _(*usagestr++));
+		struct string_list list = STRING_LIST_INIT_DUP;
+		unsigned int j;
+
+		string_list_split(&list, _(*usagestr++), '\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)
+				fprintf_ln(outfile, prefix, line);
+			else
+				fprintf_ln(outfile, usage_continued,
+					   (int)usage_len, "", line);
+		}
+		string_list_clear(&list, 0);
+
+		prefix = or_prefix;
 	}
 
 	need_newline = 1;
-- 
2.33.0.876.g423ac861752


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2021-09-11  0:14 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the implementation of b5dd96b70ac (make credential helpers
> builtins, 2020-08-13) to declare in the "struct cmd_struct" that
> NO_UNIX_SOCKETS can't be set.

It may happen to be that two credential-cache program are both
related to the same CPP macro NO_UNIX_SOCKETS, but I think the
pattern you are tackling with this topic is that a fallback
definition of a function that is designed to always die when invoked
misuses the parse-options API.  I do not want to see you invent a
new bit in cmd_struct for each and every conditional that lets us
define such a die-only fallback implementation.

I may be missing something obvious, but can't the following suffice,
and if not, why?

Thanks.

 builtin/credential-cache--daemon.c | 9 ---------
 builtin/credential-cache.c         | 9 ---------
 2 files changed, 18 deletions(-)

diff --git i/builtin/credential-cache--daemon.c w/builtin/credential-cache--daemon.c
index 4c6c89ab0d..f11a89a89b 100644
--- i/builtin/credential-cache--daemon.c
+++ w/builtin/credential-cache--daemon.c
@@ -304,15 +304,6 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache--daemon [options] <action>",
-		"",
-		"credential-cache--daemon is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
 	die(_("credential-cache--daemon unavailable; no unix socket support"));
 }
 
diff --git i/builtin/credential-cache.c w/builtin/credential-cache.c
index e8a7415747..dd794f84ce 100644
--- i/builtin/credential-cache.c
+++ w/builtin/credential-cache.c
@@ -142,15 +142,6 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 
 int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 {
-	const char * const usage[] = {
-		"git credential-cache [options] <action>",
-		"",
-		"credential-cache is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
 	die(_("credential-cache unavailable; no unix socket support"));
 }
 


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2021-09-11  0:21 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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).
>
> 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.
>
> This gets rid of that special case, which was added in
> f389c808b67 (Rework make_usage to print the usage message immediately,
> 2007-10-14). It was only used "blame" (the "credential-cache*" use of

used *by* "blame"

> it was removed in the preceding commit). Before this change we'd emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>
>         <rev-opts> are documented in git-rev-list(1)

The most crucial information is missing.  It may be shared with the
previous step but it is even worse in this step.

Why is this loss of feature a desirable thing in the first place?

What are we gaining by breaking the "after listing the alternative
forms concatenated with 'or:', we can give a general description,
before going onto the list of options"?

Without that explained, the remainder of the proposed log message
reads more like an excuse for breaking the feature, justifying that
the loss of feature can be worked around, than the description of a
solution.

Thanks.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 4/6] built-ins: "properly" align continued usage output
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2021-09-11  0:25 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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

The "A" and "B" *in* scare quotes?

> 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:
>
>     $ git ls-remote -h
>     usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
>                          [-q | --quiet] [--exit-code] [--get-url]
>                          [--symref] [<repository> [<refs>...]]

Isn't this aiming for a wrong end goal?  With an overly long
subcommand name, we'd end up with overly deep indentation of the
subsequent lines (hence too narrow a space to fit the options).

Rather, wouldn't it be better to aim for a consistent and wide
enough indentation, like say two tabs, everywhere, no matter how
long the subcommand name is?

Thanks.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 4/6] built-ins: "properly" align continued usage output
  2021-09-11  0:25     ` Junio C Hamano
@ 2021-09-11  2:40       ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11  2:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Carlo Arenas, Eric Sunshine


On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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
>
> The "A" and "B" *in* scare quotes?

Perhaps I should take a stab at rewriting this, something like:

    Let's start by aligning the strings in the C code so that their
    indentation is correct, in the end it'll still be off due to the
    indentation parse_options() itself adds, but that'll be fixed in a
    subsequent commit. That subsequent commit relies on the indentation
    in the source being consistent.

>> 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:
>>
>>     $ git ls-remote -h
>>     usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
>>                          [-q | --quiet] [--exit-code] [--get-url]
>>                          [--symref] [<repository> [<refs>...]]
>
> Isn't this aiming for a wrong end goal?  With an overly long
> subcommand name, we'd end up with overly deep indentation of the
> subsequent lines (hence too narrow a space to fit the options).
>
> Rather, wouldn't it be better to aim for a consistent and wide
> enough indentation, like say two tabs, everywhere, no matter how
> long the subcommand name is?

Similarly to the discussion on another patch about whether we should
convert these long lines to [<options>] or not, I think this is really
outside the scope of this series.

In this particular case the indentation remains exactly the same before
this series and after, for the other cases there's usually a change of
1-3 spaces or so, i.e. they were slightly off.

Should we have no indentation at all? Auto re-indent or whatever? Maybe,
but let's start by narrowly fixing code that's clearly a bit off in
functionality from what it's doing & intending to do right now.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11  2:46 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Carlo Arenas, Eric Sunshine


On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> 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).
>>
>> 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.
>>
>> This gets rid of that special case, which was added in
>> f389c808b67 (Rework make_usage to print the usage message immediately,
>> 2007-10-14). It was only used "blame" (the "credential-cache*" use of
>
> used *by* "blame"
>
>> it was removed in the preceding commit). Before this change we'd emit:
>>
>>     $ git blame -h
>>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>>
>>         <rev-opts> are documented in git-rev-list(1)
>
> The most crucial information is missing.  It may be shared with the
> previous step but it is even worse in this step.
>
> Why is this loss of feature a desirable thing in the first place?
>
> What are we gaining by breaking the "after listing the alternative
> forms concatenated with 'or:', we can give a general description,
> before going onto the list of options"?
>
> Without that explained, the remainder of the proposed log message
> reads more like an excuse for breaking the feature, justifying that
> the loss of feature can be worked around, than the description of a
> solution.

Making it the same as how "git bundle" describes it doesn't seem
anywhere close to breaking it, and that also goes to the point you had
about brevity on another patch.

I think that passing custom or generated help advice here might make
sense generally, i.e. we might eventually want to expand that to all
built-in as part of improving the -h and --help output, see e.g. the
note at the bottom of "git --help".

But if we do that let's do that with an API where we simply pass this
custom string in as another parameter to the function, rather than
having a state machine to parse it out of the array we use for the
"usage:" and "or:" list of lines.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11  2:50 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Carlo Arenas, Eric Sunshine


On Fri, Sep 10 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> Change the implementation of b5dd96b70ac (make credential helpers
>> builtins, 2020-08-13) to declare in the "struct cmd_struct" that
>> NO_UNIX_SOCKETS can't be set.
>
> It may happen to be that two credential-cache program are both
> related to the same CPP macro NO_UNIX_SOCKETS, but I think the
> pattern you are tackling with this topic is that a fallback
> definition of a function that is designed to always die when invoked
> misuses the parse-options API.  I do not want to see you invent a
> new bit in cmd_struct for each and every conditional that lets us
> define such a die-only fallback implementation.
>
> I may be missing something obvious, but can't the following suffice,
> and if not, why?

I think this is covered if you go on to read the rest of the commit
message, i.e. yes we could, but the trade-off is making the test and
users that might want to use --list-cmds=builtins hardcode these two as
special cases under the relevant prereq.

Hence doing this in the main git.c dispatch mechanism, if we can't ever
do anything useful with these it seems best to mark them as such at
compile-time in that dispatch mechanism.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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
  0 siblings, 2 replies; 45+ messages in thread
From: Carlo Marcelo Arenas Belón @ 2021-09-11  3:47 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git, Jeff King, Eric Sunshine

Sorry, if this is silly, but why is this not better (at least as a starting
point, since it obviously will need more work?

Undefined symbols for architecture x86_64:
  "_cmd_credential_cache", referenced from:
      _commands in git.o
  "_cmd_credential_cache_daemon", referenced from:
      _commands in git.o

Carlo
---- >8 ----
diff --git a/Makefile b/Makefile
index 44734f916a..2a60685c37 100644
--- a/Makefile
+++ b/Makefile
@@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
+ifndef NO_UNIX_SOCKETS
 BUILTIN_OBJS += builtin/credential-cache--daemon.o
 BUILTIN_OBJS += builtin/credential-cache.o
+endif
 BUILTIN_OBJS += builtin/credential-store.o
 BUILTIN_OBJS += builtin/credential.o
 BUILTIN_OBJS += builtin/describe.o

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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
  1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-09-11  6:12 UTC (permalink / raw)
  To: Carlo Marcelo Arenas Belón
  Cc: Ævar Arnfjörð Bjarmason, git, Jeff King, Eric Sunshine

Carlo Marcelo Arenas Belón <carenas@gmail.com> writes:

> Sorry, if this is silly, but why is this not better (at least as a starting
> point, since it obviously will need more work?
>
> Undefined symbols for architecture x86_64:
>   "_cmd_credential_cache", referenced from:
>       _commands in git.o
>   "_cmd_credential_cache_daemon", referenced from:
>       _commands in git.o
>
> Carlo
> ---- >8 ----
> diff --git a/Makefile b/Makefile
> index 44734f916a..2a60685c37 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o
>  BUILTIN_OBJS += builtin/commit.o
>  BUILTIN_OBJS += builtin/config.o
>  BUILTIN_OBJS += builtin/count-objects.o
> +ifndef NO_UNIX_SOCKETS
>  BUILTIN_OBJS += builtin/credential-cache--daemon.o
>  BUILTIN_OBJS += builtin/credential-cache.o
> +endif
>  BUILTIN_OBJS += builtin/credential-store.o
>  BUILTIN_OBJS += builtin/credential.o
>  BUILTIN_OBJS += builtin/describe.o

That smells to show a much better direction to me ;-)

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array
  2021-09-11  2:46       ` Ævar Arnfjörð Bjarmason
@ 2021-09-11  6:52         ` Junio C Hamano
  0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-09-11  6:52 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But if we do that let's do that with an API where we simply pass this
> custom string in as another parameter to the function, rather than
> having a state machine to parse it out of the array we use for the
> "usage:" and "or:" list of lines.

Sorry, but I do not follow.  A perfectly fine way to encode the
three (usage:, or:, and text) kind of information in a single array
is what this step is breaking, and then you propose to keep the two
still in that same array, with only the third kind kicked out to
"another parameter", which does not make much sense to me.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins
  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
  1 sibling, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11  7:16 UTC (permalink / raw)
  To: Carlo Marcelo Arenas Belón
  Cc: Junio C Hamano, git, Jeff King, Eric Sunshine


On Fri, Sep 10 2021, Carlo Marcelo Arenas Belón wrote:

> Sorry, if this is silly, but why is this not better (at least as a starting
> point, since it obviously will need more work?
>
> Undefined symbols for architecture x86_64:
>   "_cmd_credential_cache", referenced from:
>       _commands in git.o
>   "_cmd_credential_cache_daemon", referenced from:
>       _commands in git.o

I'm fine with it. I'm just trying to get to the end-goal of fixing the
formatting issue in "-h" output in the simplest way possible.

What you're suggesting would be effectively a revert of various parts of
b5dd96b70ac (make credential helpers builtins, 2020-08-13), which is
behavior I assumed we'd like to retain.

But yes, if changing that behavior is OK then this is simpler.

Anyway, all of this in v2 was in response to feedback on v1 to make the
v1 function in parse_options() easier, see summary at
https://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/

But it does look like Junio would like to keep the only "real" in-tree
user of the current API in builtin/blame.c, so at that point removing
these by any method becomes a moot point, so I think I'l try some
alternate approach based on v1 that doesn't touch these at all again.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v2 0/6] parse-options: properly align continued usage output & related
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
                     ` (5 preceding siblings ...)
  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   ` Eric Sunshine
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
  7 siblings, 0 replies; 45+ messages in thread
From: Eric Sunshine @ 2021-09-11  7:41 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Jeff King, Carlo Arenas

On Fri, Sep 10, 2021 at 11:38 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> 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".

To be clear, I only asked whether you really needed to add support for
embedded newlines in free-form usage strings following a "" line; I
didn't actually suggest dropping support for free-form text following
a "" line. Perhaps I didn't articulate that well enough, though, in my
review(?).

Aside: I did, however, _think_ about suggesting that free-form text
support be dropped since I only found one consumer of the feature,
however, I decided not to make that suggestion since the feature
seemed like it had potential value and the current implementation is
good enough and simple enough (though undocumented, as far as I can
tell).

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 0/6] parse-options: properly align continued usage output & related
  2021-09-10 15:38 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
                     ` (6 preceding siblings ...)
  2021-09-11  7:41   ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Eric Sunshine
@ 2021-09-11 19:08   ` Æ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
                       ` (6 more replies)
  7 siblings, 7 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:08 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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

I think this v3 will address the feedback on v2. I'm not confident the
direction will be agreed with, but let's see.

I've taken Carlo Marcelo Arenas Belón's suggestion of just not
building the credential-cache built-ins under NO_UNIX_SOCKETS, which
I'm much happier with as a solution than my v2, I just thought Junio
would veto it, but it seems he likes the direction.

The removal of the undocumented API from "git blame" is now its own
commit, which I think does a good job of justifying itself.

Then the justification in "parse-options: stop supporting" is updated
& much improved, i.e. I've had my eye on migrating git.c to
parse_options(), which wants to make use of this sort of API, but
can't because of the limitations of the API being removed here. I
think per the justification in that commit it makes sense to remove
this here, and just add something like it back later.

Ævar Arnfjörð Bjarmason (6):
  credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
  blame: replace usage end blurb with better option spec
  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 +-
 Makefile                           |  3 ++
 builtin.h                          |  2 +
 builtin/blame.c                    |  9 ++--
 builtin/credential-cache--daemon.c | 21 ---------
 builtin/credential-cache.c         | 21 ---------
 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                              |  2 +
 parse-options.c                    | 71 +++++++++++++++++++++++++-----
 t/helper/test-parse-options.c      |  2 -
 t/t0040-parse-options.sh           |  2 -
 t/t1502-rev-parse-parseopt.sh      | 34 +++++++-------
 17 files changed, 104 insertions(+), 94 deletions(-)

Range-diff against v2:
1:  9e8facb6f8c < -:  ----------- test-lib.sh: add a UNIX_SOCKETS prerequisite
2:  d6c44402715 ! 1:  ad857e80fd8 git.c: add a NEED_UNIX_SOCKETS option for built-ins
    @@ Metadata
     Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
      ## Commit message ##
    -    git.c: add a NEED_UNIX_SOCKETS option for built-ins
    +    credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
     
         Change the implementation of b5dd96b70ac (make credential helpers
    -    builtins, 2020-08-13) to declare in the "struct cmd_struct" that
    -    NO_UNIX_SOCKETS can't be set.
    +    builtins, 2020-08-13) to not build these at all under
    +    NO_UNIX_SOCKETS.
     
    -    This is one of two in-tree users for the empty lines in
    -    parse_options() usage, getting rid of that is the main motivation for
    -    this, but it also doesn't make sense to emit these sorts of usage
    -    messages just to appease t0012-help.sh, which seemingly b5dd96b70ac
    -    aimed to do.
    +    This is the easiest way to get rid of one out of two users of an
    +    obscure parse_options() API I'm trying to get rid of. It does mean
    +    that the goal of emitting a custom error message in b5dd96b70ac is
    +    being eliminated, but per [1] that seems to be an OK direction to go
    +    in.
     
    -    I.e. these commands don't support "[options]", or "<action>" so
    -    emitting that at the beginning is incorrect. We should just die right
    -    away.
    +    By not compiling it at all it won't be included in the "struct
    +    cmd_struct", and therefore will also be omitted from
    +    "--list-cmds=builtins".
     
    -    The existing code also had the edge case of not emitting the die()
    -    message if a "-h" argument was given, since parse_options() will
    -    handle the exit() itself in that case. We could feed it
    -    PARSE_OPT_NO_INTERNAL_HELP, but this is better.
    -
    -    By moving this up to the "struct cmd_struct" we can also skip these in
    -    --list-cmds=builtins instead, as noted above we shouldn't be exiting
    -    with code 129 in these cases.
    +    1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com/
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    + ## Makefile ##
    +@@ Makefile: BUILTIN_OBJS += builtin/commit-tree.o
    + BUILTIN_OBJS += builtin/commit.o
    + BUILTIN_OBJS += builtin/config.o
    + BUILTIN_OBJS += builtin/count-objects.o
    ++ifndef NO_UNIX_SOCKETS
    + BUILTIN_OBJS += builtin/credential-cache--daemon.o
    + BUILTIN_OBJS += builtin/credential-cache.o
    ++endif
    + BUILTIN_OBJS += builtin/credential-store.o
    + BUILTIN_OBJS += builtin/credential.o
    + BUILTIN_OBJS += builtin/describe.o
    +@@ Makefile: ifdef NO_INET_PTON
    + endif
    + ifdef NO_UNIX_SOCKETS
    + 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
    ++	EXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon
    + else
    + 	LIB_OBJS += unix-socket.o
    + 	LIB_OBJS += unix-stream-server.o
    +
      ## builtin.h ##
    -@@
    -  *	more informed decision, e.g., by ignoring `pager.<cmd>` for
    -  *	certain subcommands.
    -  *
    -+ * `NEED_UNIX_SOCKETS`:
    -+ *
    -+ *	This built-in will not work if NO_UNIX_SOCKETS is defined. It
    -+ *	will be recognized for emitting error messages, but won't be
    -+ *	listed in --list-cmds=builtins.
    -+ *
    -  * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.
    -  *
    -  * Additionally, if `foo` is a new command, there are 4 more things to do:
    +@@ builtin.h: int cmd_commit_tree(int argc, const char **argv, const char *prefix);
    + int cmd_config(int argc, const char **argv, const char *prefix);
    + int cmd_count_objects(int argc, const char **argv, const char *prefix);
    + int cmd_credential(int argc, const char **argv, const char *prefix);
    ++#ifndef NO_UNIX_SOCKETS
    + int cmd_credential_cache(int argc, const char **argv, const char *prefix);
    + int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);
    ++#endif
    + int cmd_credential_store(int argc, const char **argv, const char *prefix);
    + int cmd_describe(int argc, const char **argv, const char *prefix);
    + int cmd_diff_files(int argc, const char **argv, const char *prefix);
     
      ## builtin/credential-cache--daemon.c ##
    +@@
    + #include "builtin.h"
    + #include "parse-options.h"
    +-
    +-#ifndef NO_UNIX_SOCKETS
    +-
    + #include "config.h"
    + #include "tempfile.h"
    + #include "credential.h"
     @@ builtin/credential-cache--daemon.c: int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
      
    - int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
    - {
    + 	return 0;
    + }
    +-
    +-#else
    +-
    +-int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
    +-{
     -	const char * const usage[] = {
     -		"git credential-cache--daemon [options] <action>",
     -		"",
    @@ builtin/credential-cache--daemon.c: int cmd_credential_cache_daemon(int argc, co
     -
     -	argc = parse_options(argc, argv, prefix, options, usage, 0);
     -	die(_("credential-cache--daemon unavailable; no unix socket support"));
    -+	BUG("should not be called under NO_UNIX_SOCKETS");
    - }
    - 
    - #endif /* NO_UNIX_SOCKET */
    +-}
    +-
    +-#endif /* NO_UNIX_SOCKET */
     
      ## builtin/credential-cache.c ##
    +@@
    + #include "builtin.h"
    + #include "parse-options.h"
    +-
    +-#ifndef NO_UNIX_SOCKETS
    +-
    + #include "credential.h"
    + #include "string-list.h"
    + #include "unix-socket.h"
     @@ builtin/credential-cache.c: int cmd_credential_cache(int argc, const char **argv, const char *prefix)
      
    - int cmd_credential_cache(int argc, const char **argv, const char *prefix)
    - {
    + 	return 0;
    + }
    +-
    +-#else
    +-
    +-int cmd_credential_cache(int argc, const char **argv, const char *prefix)
    +-{
     -	const char * const usage[] = {
     -		"git credential-cache [options] <action>",
     -		"",
    @@ builtin/credential-cache.c: int cmd_credential_cache(int argc, const char **argv
     -
     -	argc = parse_options(argc, argv, prefix, options, usage, 0);
     -	die(_("credential-cache unavailable; no unix socket support"));
    -+	BUG("should not be called under NO_UNIX_SOCKETS");
    - }
    - 
    - #endif /* NO_UNIX_SOCKETS */
    +-}
    +-
    +-#endif /* NO_UNIX_SOCKETS */
     
      ## git.c ##
    -@@
    - #define SUPPORT_SUPER_PREFIX	(1<<4)
    - #define DELAY_PAGER_CONFIG	(1<<5)
    - #define NO_PARSEOPT		(1<<6) /* parse-options is not used */
    -+#define NEED_UNIX_SOCKETS	(1<<7) /* Works unless -DNO_UNIX_SOCKETS */
    - 
    - struct cmd_struct {
    - 	const char *cmd;
    -@@ git.c: static int list_cmds(const char *spec)
    - 	struct string_list list = STRING_LIST_INIT_DUP;
    - 	int i;
    - 	int nongit;
    -+	unsigned int exclude_option = 0;
    -+#ifdef NO_UNIX_SOCKETS
    -+	exclude_option |= NEED_UNIX_SOCKETS;
    -+#endif
    - 
    - 	/*
    - 	* Set up the repository so we can pick up any repo-level config (like
    -@@ git.c: static int list_cmds(const char *spec)
    - 		int len = sep - spec;
    - 
    - 		if (match_token(spec, len, "builtins"))
    --			list_builtins(&list, 0);
    -+			list_builtins(&list, exclude_option);
    - 		else if (match_token(spec, len, "main"))
    - 			list_all_main_cmds(&list);
    - 		else if (match_token(spec, len, "others"))
    -@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)
    - 	const char *prefix;
    - 
    - 	prefix = NULL;
    -+#ifdef NO_UNIX_SOCKETS
    -+	if (p->option & NEED_UNIX_SOCKETS)
    -+		die(_("%s is unavailable; there is no UNIX socket support in this build of Git"), p->cmd);
    -+#endif
    - 	help = argc == 2 && !strcmp(argv[1], "-h");
    - 	if (!help) {
    - 		if (p->option & RUN_SETUP)
     @@ git.c: static struct cmd_struct commands[] = {
      	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
      	{ "count-objects", cmd_count_objects, RUN_SETUP },
      	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
    --	{ "credential-cache", cmd_credential_cache },
    --	{ "credential-cache--daemon", cmd_credential_cache_daemon },
    -+	{ "credential-cache", cmd_credential_cache, NEED_UNIX_SOCKETS },
    -+	{ "credential-cache--daemon", cmd_credential_cache_daemon, NEED_UNIX_SOCKETS },
    ++#ifndef NO_UNIX_SOCKETS
    + 	{ "credential-cache", cmd_credential_cache },
    + 	{ "credential-cache--daemon", cmd_credential_cache_daemon },
    ++#endif
      	{ "credential-store", cmd_credential_store },
      	{ "describe", cmd_describe, RUN_SETUP },
      	{ "diff", cmd_diff, NO_PARSEOPT },
    -
    - ## t/t0012-help.sh ##
    -@@ t/t0012-help.sh: do
    - 	'
    - done <builtins
    - 
    -+test_expect_success UNIX_SOCKETS 'builtin list includes NEED_UNIX_SOCKETS under UNIX_SOCKETS' '
    -+	grep ^credential-cache$ builtins &&
    -+	grep ^credential-cache--daemon$ builtins
    -+'
    -+
    -+test_expect_success !UNIX_SOCKETS 'builtin list excludes NEED_UNIX_SOCKETS under !UNIX_SOCKETS' '
    -+	! grep ^credential-cache$ builtins &&
    -+	! grep ^credential-cache--daemon$ builtins
    -+'
    -+
    - test_done
-:  ----------- > 2:  036eb0efb5b blame: replace usage end blurb with better option spec
3:  11f4a119563 ! 3:  e23a8231f38 parse-options: stop supporting "" in the usagestr array
    @@ Commit message
         encountered a "" we'll emit all strings after that point verbatim
         without any "or: " prefixing.
     
    -    This gets rid of that special case, which was added in
    -    f389c808b67 (Rework make_usage to print the usage message immediately,
    -    2007-10-14). It was only used "blame" (the "credential-cache*" use of
    -    it was removed in the preceding commit). Before this change we'd emit:
    +    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().
     
    -        $ git blame -h
    -        usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
    +    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.
     
    -            <rev-opts> are documented in git-rev-list(1)
    +    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.
     
    -    This changes that output to simply use "[<git-rev-list args>]" instead
    -    of "[<rev-opts>]". This accomplishes the same, is more consistent as
    -    "git bundle" and "git blame" use the same way of referring to these
    -    options now.
    +    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().
     
    -    The use of this in "blame" dated back to 5817da01434 (git-blame:
    -    migrate to incremental parse-option [1/2], 2008-07-08), and the use in
    -    "bundle" to 2e0afafebd8 (Add git-bundle: move objects and
    -    references by archive, 2007-02-22).
    -
    -    Once we get rid of this special case we can also use usage_msg_opt()
    -    to emit the error message we'd get on an invalid "-L <range>"
    -    argument, which means we can get rid of the old-style "blame_usage"
    -    variable.
    +    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"
    @@ Commit message
     
         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
     
    - ## builtin/blame.c ##
    -@@
    - #include "refs.h"
    - #include "tag.h"
    - 
    --static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
    --
    - static const char *blame_opt_usage[] = {
    --	blame_usage,
    --	"",
    --	N_("<rev-opts> are documented in git-rev-list(1)"),
    -+	N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),
    - 	NULL
    - };
    - 
    -@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)
    - 				    nth_line_cb, &sb, lno, anchor,
    - 				    &bottom, &top, sb.path,
    - 				    the_repository->index))
    --			usage(blame_usage);
    -+			usage_msg_opt(_("Invalid -L <range> parameters"),
    -+				      blame_opt_usage, options);
    - 		if ((!lno && (top || bottom)) || lno < bottom)
    - 			die(Q_("file %s has only %lu line",
    - 			       "file %s has only %lu lines",
    -
      ## builtin/rev-parse.c ##
     @@ builtin/rev-parse.c: static int cmd_parseopt(int argc, const char **argv, const char *prefix)
      	for (;;) {
4:  4547cc968b1 = 4:  5638d2bc832 built-ins: "properly" align continued usage output
5:  b884b361ace = 5:  821e5e14132 send-pack: properly use parse_options() API for usage string
6:  e83d66da6d4 = 6:  0df2840ce4e parse-options: properly align continued usage output
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
@ 2021-09-11 19:09     ` Æ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
                       ` (5 subsequent siblings)
  6 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Change the implementation of b5dd96b70ac (make credential helpers
builtins, 2020-08-13) to not build these at all under
NO_UNIX_SOCKETS.

This is the easiest way to get rid of one out of two users of an
obscure parse_options() API I'm trying to get rid of. It does mean
that the goal of emitting a custom error message in b5dd96b70ac is
being eliminated, but per [1] that seems to be an OK direction to go
in.

By not compiling it at all it won't be included in the "struct
cmd_struct", and therefore will also be omitted from
"--list-cmds=builtins".

1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com/

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Makefile                           |  3 +++
 builtin.h                          |  2 ++
 builtin/credential-cache--daemon.c | 21 ---------------------
 builtin/credential-cache.c         | 21 ---------------------
 git.c                              |  2 ++
 5 files changed, 7 insertions(+), 42 deletions(-)

diff --git a/Makefile b/Makefile
index 429c276058d..ecde3367fa2 100644
--- a/Makefile
+++ b/Makefile
@@ -1086,8 +1086,10 @@ BUILTIN_OBJS += builtin/commit-tree.o
 BUILTIN_OBJS += builtin/commit.o
 BUILTIN_OBJS += builtin/config.o
 BUILTIN_OBJS += builtin/count-objects.o
+ifndef NO_UNIX_SOCKETS
 BUILTIN_OBJS += builtin/credential-cache--daemon.o
 BUILTIN_OBJS += builtin/credential-cache.o
+endif
 BUILTIN_OBJS += builtin/credential-store.o
 BUILTIN_OBJS += builtin/credential.o
 BUILTIN_OBJS += builtin/describe.o
@@ -1693,6 +1695,7 @@ ifdef NO_INET_PTON
 endif
 ifdef NO_UNIX_SOCKETS
 	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
+	EXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon
 else
 	LIB_OBJS += unix-socket.o
 	LIB_OBJS += unix-stream-server.o
diff --git a/builtin.h b/builtin.h
index 16ecd5586f0..9b3f173bd7c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -140,8 +140,10 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 int cmd_config(int argc, const char **argv, const char *prefix);
 int cmd_count_objects(int argc, const char **argv, const char *prefix);
 int cmd_credential(int argc, const char **argv, const char *prefix);
+#ifndef NO_UNIX_SOCKETS
 int cmd_credential_cache(int argc, const char **argv, const char *prefix);
 int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);
+#endif
 int cmd_credential_store(int argc, const char **argv, const char *prefix);
 int cmd_describe(int argc, const char **argv, const char *prefix);
 int cmd_diff_files(int argc, const char **argv, const char *prefix);
diff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c
index 4c6c89ab0de..7785412dea4 100644
--- a/builtin/credential-cache--daemon.c
+++ b/builtin/credential-cache--daemon.c
@@ -1,8 +1,5 @@
 #include "builtin.h"
 #include "parse-options.h"
-
-#ifndef NO_UNIX_SOCKETS
-
 #include "config.h"
 #include "tempfile.h"
 #include "credential.h"
@@ -299,21 +296,3 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
 
 	return 0;
 }
-
-#else
-
-int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)
-{
-	const char * const usage[] = {
-		"git credential-cache--daemon [options] <action>",
-		"",
-		"credential-cache--daemon is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache--daemon unavailable; no unix socket support"));
-}
-
-#endif /* NO_UNIX_SOCKET */
diff --git a/builtin/credential-cache.c b/builtin/credential-cache.c
index e8a74157471..64942978650 100644
--- a/builtin/credential-cache.c
+++ b/builtin/credential-cache.c
@@ -1,8 +1,5 @@
 #include "builtin.h"
 #include "parse-options.h"
-
-#ifndef NO_UNIX_SOCKETS
-
 #include "credential.h"
 #include "string-list.h"
 #include "unix-socket.h"
@@ -137,21 +134,3 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)
 
 	return 0;
 }
-
-#else
-
-int cmd_credential_cache(int argc, const char **argv, const char *prefix)
-{
-	const char * const usage[] = {
-		"git credential-cache [options] <action>",
-		"",
-		"credential-cache is disabled in this build of Git",
-		NULL
-	};
-	struct option options[] = { OPT_END() };
-
-	argc = parse_options(argc, argv, prefix, options, usage, 0);
-	die(_("credential-cache unavailable; no unix socket support"));
-}
-
-#endif /* NO_UNIX_SOCKETS */
diff --git a/git.c b/git.c
index 18bed9a9964..7c696e06ecf 100644
--- a/git.c
+++ b/git.c
@@ -513,8 +513,10 @@ static struct cmd_struct commands[] = {
 	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
 	{ "count-objects", cmd_count_objects, RUN_SETUP },
 	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
+#ifndef NO_UNIX_SOCKETS
 	{ "credential-cache", cmd_credential_cache },
 	{ "credential-cache--daemon", cmd_credential_cache_daemon },
+#endif
 	{ "credential-store", cmd_credential_store },
 	{ "describe", cmd_describe, RUN_SETUP },
 	{ "diff", cmd_diff, NO_PARSEOPT },
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 2/6] blame: replace usage end blurb with better option spec
  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-11 19:09     ` Æ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
                       ` (4 subsequent siblings)
  6 siblings, 2 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Change the "git blame -h" output to be consistent with "git bundle
-h"'s, i.e. before this we'd emit:

    $ git blame -h
    usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>

        <rev-opts> are documented in git-rev-list(1)
    [...]

Now instead of that we'll emit:

    $ git blame -h
    usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>
    [...]

This makes it consistent with the usage spec used for "git bundle":

    $ git bundle -h
    usage: git bundle create [<options>] <file> <git-rev-list args>
    [...]

The use of this in "blame" dated back to 5817da01434 (git-blame:
migrate to incremental parse-option [1/2], 2008-07-08), and the use in
"bundle" to 2e0afafebd8 (Add git-bundle: move objects and
references by archive, 2007-02-22).

Once we get rid of this special case we can also use usage_msg_opt()
to emit the error message we'd get on an invalid "-L <range>"
argument, which means we can get rid of the old-style "blame_usage"
variable entirely. This makes the output friendlier, before we'd emit say:

    $ git blame -L1,2,3,4 Makefile
    usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
    $

Now we'll instead emit:

    $ git blame -L1,2,3,4 Makefile
    fatal: Invalid -L <range> parameter

    usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>
    [...]
    $

The "[...]" there elides the "git blame" option summary.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/blame.c | 9 +++------
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 641523ff9af..e469829bc76 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -29,12 +29,8 @@
 #include "refs.h"
 #include "tag.h"
 
-static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
-
 static const char *blame_opt_usage[] = {
-	blame_usage,
-	"",
-	N_("<rev-opts> are documented in git-rev-list(1)"),
+	N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),
 	NULL
 };
 
@@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
 				    nth_line_cb, &sb, lno, anchor,
 				    &bottom, &top, sb.path,
 				    the_repository->index))
-			usage(blame_usage);
+			usage_msg_opt(_("Invalid -L <range> parameter"),
+				      blame_opt_usage, options);
 		if ((!lno && (top || bottom)) || lno < bottom)
 			die(Q_("file %s has only %lu line",
 			       "file %s has only %lu lines",
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array
  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-11 19:09     ` [PATCH v3 2/6] blame: replace usage end blurb with better option spec Ævar Arnfjörð Bjarmason
@ 2021-09-11 19:09     ` Ævar Arnfjörð Bjarmason
  2021-09-12 22:26       ` Junio C Hamano
  2021-09-11 19:09     ` [PATCH v3 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/rev-parse.c           |  3 +++
 parse-options.c               |  8 +-------
 t/helper/test-parse-options.c |  2 --
 t/t0040-parse-options.sh      |  2 --
 t/t1502-rev-parse-parseopt.sh | 34 ++++++++++++++++++----------------
 5 files changed, 22 insertions(+), 27 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 22c4e1a4ff0..aeebfd52805 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -436,7 +436,10 @@ 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"));
diff --git a/parse-options.c b/parse-options.c
index 2abff136a17..950a8279beb 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -924,18 +924,12 @@ 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;
diff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c
index 2051ce57db7..e00aef073b0 100644
--- a/t/helper/test-parse-options.c
+++ b/t/helper/test-parse-options.c
@@ -102,8 +102,6 @@ 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;
diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
index ad4746d899a..2910874ece5 100755
--- a/t/t0040-parse-options.sh
+++ b/t/t0040-parse-options.sh
@@ -10,8 +10,6 @@ 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
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b29563fc997..6badc650d64 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -6,8 +6,6 @@ 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
 |
@@ -41,8 +39,6 @@ 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
 '
@@ -50,8 +46,6 @@ 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
@@ -62,8 +56,6 @@ 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
@@ -103,8 +95,6 @@ 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 &&
@@ -116,8 +106,6 @@ 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 &&
@@ -129,8 +117,6 @@ 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
@@ -144,8 +130,6 @@ 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
@@ -282,4 +266,22 @@ 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>...
+	|
+	|
+	|--
+	|h,help    show the help
+	EOF
+
+	cat >expect <<-\EOF &&
+	fatal: empty lines are not permitted before the `--'"'"' separator
+	EOF
+
+	test_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&
+	test_must_be_empty out &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 4/6] built-ins: "properly" align continued usage output
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (2 preceding siblings ...)
  2021-09-11 19:09     ` [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
@ 2021-09-11 19:09     ` Æ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
                       ` (2 subsequent siblings)
  6 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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.

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.

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:

    $ git ls-remote -h
    usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
                         [-q | --quiet] [--exit-code] [--get-url]
                         [--symref] [<repository> [<refs>...]]

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.

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.

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 builtin/ls-remote.c   | 4 ++--
 builtin/show-branch.c | 6 +++---
 builtin/stash.c       | 2 +-
 builtin/tag.c         | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f4fd823af83..318949c3d75 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -7,8 +7,8 @@
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
-	   "                     [-q | --quiet] [--exit-code] [--get-url]\n"
-	   "                     [--symref] [<repository> [<refs>...]]"),
+	   "              [-q | --quiet] [--exit-code] [--get-url]\n"
+	   "              [--symref] [<repository> [<refs>...]]"),
 	NULL
 };
 
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index bea4bbf4680..082449293b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -11,9 +11,9 @@
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
-       "		[--current] [--color[=<when>] | --no-color] [--sparse]\n"
-       "		[--more=<n> | --list | --independent | --merge-base]\n"
-       "		[--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
+       "                [--current] [--color[=<when>] | --no-color] [--sparse]\n"
+       "                [--more=<n> | --list | --independent | --merge-base]\n"
+       "                [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
     N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"),
     NULL
 };
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..45b19007d7c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {
 
 static const char * const git_stash_save_usage[] = {
 	N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
-	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	   "               [-u|--include-untracked] [-a|--all] [<message>]"),
 	NULL
 };
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 065b6bf093e..6535ed27ee9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,10 +23,10 @@
 
 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>...]"),
+	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 5/6] send-pack: properly use parse_options() API for usage string
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (3 preceding siblings ...)
  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     ` Æ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
  6 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

When "send-pack" was changed to use the parse_options() API in
068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)
it was made to use one very long line, instead it should split them up
with newlines.

Furthermore we were including an inline explanation that you couldn't
combine "--all" and "<ref>", but unlike in the "blame" case this was
not preceded by an empty string.

Let's instead show that --all and <ref> can't be combined in the the
usual language of the usage syntax instead. We can make it clear that
one of the two options "--foo" and "--bar" is mandatory, but that the
two are mutually exclusive by referring to them as "( --foo | --bar
)".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-pack.txt | 4 ++--
 builtin/send-pack.c             | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 44fd146b912..be41f119740 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
+'git send-pack' [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
 		[--verbose] [--thin] [--atomic]
 		[--[no-]signed|--signed=(true|false|if-asked)]
-		[<host>:]<directory> [<ref>...]
+		[<host>:]<directory> (--all | <ref>...)
 
 DESCRIPTION
 -----------
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 729dea1d255..89321423125 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -17,10 +17,10 @@
 #include "protocol.h"
 
 static const char * const send_pack_usage[] = {
-	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
-	  "[--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] "
-	  "[<host>:]<directory> [<ref>...]\n"
-	  "  --all and explicit <ref> specification are mutually exclusive."),
+	N_("git send-pack [--mirror] [--dry-run] [--force]\n"
+	   "              [--receive-pack=<git-receive-pack>]\n"
+	   "              [--verbose] [--thin] [--atomic]\n"
+	   "              [<host>:]<directory> (--all | <ref>...)"),
 	NULL,
 };
 
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v3 6/6] parse-options: properly align continued usage output
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (4 preceding siblings ...)
  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     ` Ævar Arnfjörð Bjarmason
  2021-09-13  0:13     ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
  6 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-11 19:09 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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>]
                     [...]

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
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.

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.

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 | 65 ++++++++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 59 insertions(+), 6 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 950a8279beb..ff28869d2c9 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -917,19 +917,72 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;
 
+	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,
+	 * Hebrew etc.), not if it's a LTR language (e.g. German,
+	 * Russian, Chinese etc.).
+	 *
+	 * When a translated usage string has an embedded "\n" it's
+	 * 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
+	 * "git cmd ".
+	 *
+	 * 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
+	 * translated) N_() usage string, which contained embedded
+	 * newlines before we split it up.
+	 */
+	const char *usage_continued = _("%*s%s");
+	const char *prefix = usage_prefix;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
-	fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
 	while (*usagestr) {
-		/*
-		 * TRANSLATORS: the colon here should align with the
-		 * one in "usage: %s" translation.
-		 */
-		fprintf_ln(outfile, _("   or: %s"), _(*usagestr++));
+		struct string_list list = STRING_LIST_INIT_DUP;
+		unsigned int j;
+
+		string_list_split(&list, _(*usagestr++), '\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)
+				fprintf_ln(outfile, prefix, line);
+			else
+				fprintf_ln(outfile, usage_continued,
+					   (int)usage_len, "", line);
+		}
+		string_list_clear(&list, 0);
+
+		prefix = or_prefix;
 	}
 
 	need_newline = 1;
-- 
2.33.0.995.ga5ea46173a2


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec
  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
  1 sibling, 0 replies; 45+ messages in thread
From: Eric Sunshine @ 2021-09-12  4:45 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Git List, Junio C Hamano, Jeff King, Carlo Arenas

On Sat, Sep 11, 2021 at 3:10 PM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> Change the "git blame -h" output to be consistent with "git bundle
> -h"'s, i.e. before this we'd emit:

Just a couple tiny, tiny nits (which may or may not be worth a re-roll)...

>     $ git blame -h
>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>
>         <rev-opts> are documented in git-rev-list(1)
>     [...]
>
> Now instead of that we'll emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>

This is lacking a hyphen between `git` and `rev-list`...

> This makes it consistent with the usage spec used for "git bundle":
>
>     $ git bundle -h
>     usage: git bundle create [<options>] <file> <git-rev-list args>

...whereas this has a hyphen between the two.

> [...]
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
> diff --git a/builtin/blame.c b/builtin/blame.c
> @@ -29,12 +29,8 @@
> -       N_("<rev-opts> are documented in git-rev-list(1)"),
> +       N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),

Ditto regarding missing hyphen.

> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>                                     nth_line_cb, &sb, lno, anchor,
>                                     &bottom, &top, sb.path,
>                                     the_repository->index))
> -                       usage(blame_usage);
> +                       usage_msg_opt(_("Invalid -L <range> parameter"),
> +                                     blame_opt_usage, options);

builltin/blame.c seems to be pretty consistent about starting error
and warning messages with a lowercase letter, so this perhaps should
follow suit. Also, I think you can drop "parameter" without losing
clarity:

    invalid -L range

would likely be good enough.

>                 if ((!lno && (top || bottom)) || lno < bottom)
>                         die(Q_("file %s has only %lu line",
>                                "file %s has only %lu lines",

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
  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
  0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-09-12 21:48 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> @@ -1693,6 +1695,7 @@ ifdef NO_INET_PTON
>  endif
>  ifdef NO_UNIX_SOCKETS
>  	BASIC_CFLAGS += -DNO_UNIX_SOCKETS
> +	EXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon

Yah, I forgot about EXCLUDED_PROGRAMS ;-)  

Sounds like a sensible direction to go.

> diff --git a/git.c b/git.c
> index 18bed9a9964..7c696e06ecf 100644
> --- a/git.c
> +++ b/git.c
> @@ -513,8 +513,10 @@ static struct cmd_struct commands[] = {
>  	{ "config", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },
>  	{ "count-objects", cmd_count_objects, RUN_SETUP },
>  	{ "credential", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },
> +#ifndef NO_UNIX_SOCKETS
>  	{ "credential-cache", cmd_credential_cache },
>  	{ "credential-cache--daemon", cmd_credential_cache_daemon },
> +#endif
>  	{ "credential-store", cmd_credential_store },
>  	{ "describe", cmd_describe, RUN_SETUP },
>  	{ "diff", cmd_diff, NO_PARSEOPT },

OK.  Looks good.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec
  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
  1 sibling, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-09-12 22:22 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> Change the "git blame -h" output to be consistent with "git bundle
> -h"'s, i.e. before this we'd emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<rev-opts>] [<rev>] [--] <file>
>
>         <rev-opts> are documented in git-rev-list(1)
>     [...]
>
> Now instead of that we'll emit:
>
>     $ git blame -h
>     usage: git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>
>     [...]

What we take are options that rev-list takes, not arguments like A..B,
so the updated text seems to be more wrong.

It does not even make much sense as a goal to make "blame" and
"bundle" similar to begin with, does it?  It may make sense for
"bundle" to be more similar to "pack-objects", in that the
information the command needs ultimately is about what is needed by
"rev-list --objects" (i.e. object enumeration), while "blame" is
more similar to "log", in that it is interested in walking commit
DAG but not about the trees and blobs connected to the commits in
the DAG.  From the end-users' point of view, they do not care if
"bundle" and "blame" are explained using similar terms.

Also, not all <rev-opts> you can see from "git rev-list -h" would
make sense in the context of "git blame".  "--no-merges" and any
options that are related to the number of parents make no sense,
--all, --branches and the friends, when used to give multiple
positive ends (i.e. starting points) of traversal, would not make
sense at all, options about ordering and formatting output of
rev-list of course would not make any difference.

At the very least, we should say <rev-list-options> there, or we
should just drop the mention of "we also take some options meant for
rev-list" from here, leaving:

	usage: git blame [<options>] [<rev>] [--] <file>

and nothing else?

I am sympathetic to the original reasoning why we wanted to add a
parenthetical "by the way, we also take (some) options rev-list
takes" here.  This is because not all relevant options are described
in the options[] array we have there (we delegate what we do not
know to parse_revisions_opt()), and it is sort of understandable
that they wanted to leave a hint that the list of options given here
is not exhaustive.

But in the longer run, if we really wanted to make the -h output
useful to end users, what we should aim for is not to make it
similar to "git bundle", but to ensure that the option summary
includes the relevant options shared with rev-list (in other words,
make the list of options cover all the options the command takes,
not just the ones in today's options[] array).

When that happens, users do not have to care which ones happen to be
parsed by us via our call to parse_options_step(), and which other
ones are given to parse_revision_opt().  There won't be any need to
say "we also take some options..." with <rev-opts> in the original.

> diff --git a/builtin/blame.c b/builtin/blame.c
> index 641523ff9af..e469829bc76 100644
> --- a/builtin/blame.c
> +++ b/builtin/blame.c
> @@ -29,12 +29,8 @@
>  #include "refs.h"
>  #include "tag.h"
>  
> -static char blame_usage[] = N_("git blame [<options>] [<rev-opts>] [<rev>] [--] <file>");
> -
>  static const char *blame_opt_usage[] = {
> -	blame_usage,
> -	"",
> -	N_("<rev-opts> are documented in git-rev-list(1)"),
> +	N_("git blame [<options>] [<git rev-list args>] [<rev>] [--] <file>"),
>  	NULL
>  };
>  
> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)
>  				    nth_line_cb, &sb, lno, anchor,
>  				    &bottom, &top, sb.path,
>  				    the_repository->index))
> -			usage(blame_usage);
> +			usage_msg_opt(_("Invalid -L <range> parameter"),
> +				      blame_opt_usage, options);

"invalid -L <range>" is fine, but can't we parrot what the user gave
us here?  You can give more than one -L to specify two ranges in the
same file that are not contiguous:

	git blame -L1,10 -L100.112 master..seen -- foo.c

and it would be helpful to tell which one is broken.

>  		if ((!lno && (top || bottom)) || lno < bottom)
>  			die(Q_("file %s has only %lu line",
>  			       "file %s has only %lu lines",

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array
  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
  0 siblings, 1 reply; 45+ messages in thread
From: Junio C Hamano @ 2021-09-12 22:26 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
> index ad4746d899a..2910874ece5 100755
> --- a/t/t0040-parse-options.sh
> +++ b/t/t0040-parse-options.sh
> @@ -10,8 +10,6 @@ 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

Isn't this, and a lot more importantly the next one ...

> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
> index b29563fc997..6badc650d64 100755
> --- a/t/t1502-rev-parse-parseopt.sh
> +++ b/t/t1502-rev-parse-parseopt.sh
> @@ -6,8 +6,6 @@ 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
>  |

... coalmine canaries that tell us that end-user's scripts using the
"git rev-parse --parseopt" in the documented way will be broken?

I'd rather not have to sorry about breaking end-user scripts this
way.  Unlike a very small number of in-tree parse_options() call in
C programs, we have unbounded of them.


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 0/4] parse-options: properly align continued usage output
  2021-09-11 19:08   ` [PATCH v3 " Ævar Arnfjörð Bjarmason
                       ` (5 preceding siblings ...)
  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     ` Æ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
                         ` (4 more replies)
  6 siblings, 5 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/
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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
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 <avarab@gmail.com>
     
      ## 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 <avarab@gmail.com>
     
    - ## 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 <avarab@gmail.com>
     
      ## 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,
-- 
2.33.0.1001.g3ab2ac1eaae


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 1/4] parse-options API users: align usage output in C-strings
  2021-09-13  0:13     ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
@ 2021-09-13  0:13       ` Æ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
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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.

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.

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:

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

Would emit:

   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 <avarab@gmail.com>
---
 builtin/ls-remote.c   | 4 ++--
 builtin/show-branch.c | 6 +++---
 builtin/stash.c       | 2 +-
 builtin/tag.c         | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f4fd823af83..318949c3d75 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -7,8 +7,8 @@
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
-	   "                     [-q | --quiet] [--exit-code] [--get-url]\n"
-	   "                     [--symref] [<repository> [<refs>...]]"),
+	   "              [-q | --quiet] [--exit-code] [--get-url]\n"
+	   "              [--symref] [<repository> [<refs>...]]"),
 	NULL
 };
 
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index bea4bbf4680..082449293b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -11,9 +11,9 @@
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
-       "		[--current] [--color[=<when>] | --no-color] [--sparse]\n"
-       "		[--more=<n> | --list | --independent | --merge-base]\n"
-       "		[--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
+       "                [--current] [--color[=<when>] | --no-color] [--sparse]\n"
+       "                [--more=<n> | --list | --independent | --merge-base]\n"
+       "                [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
     N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"),
     NULL
 };
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..45b19007d7c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {
 
 static const char * const git_stash_save_usage[] = {
 	N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
-	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	   "               [-u|--include-untracked] [-a|--all] [<message>]"),
 	NULL
 };
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 065b6bf093e..6535ed27ee9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,10 +23,10 @@
 
 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>...]"),
+	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
-- 
2.33.0.1001.g3ab2ac1eaae


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 2/4] send-pack: properly use parse_options() API for usage string
  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       ` Æ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
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

When "send-pack" was changed to use the parse_options() API in
068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)
it was made to use one very long line, instead it should split them up
with newlines.

Furthermore we were including an inline explanation that you couldn't
combine "--all" and "<ref>", but unlike in the "blame" case this was
not preceded by an empty string.

Let's instead show that --all and <ref> can't be combined in the the
usual language of the usage syntax instead. We can make it clear that
one of the two options "--foo" and "--bar" is mandatory, but that the
two are mutually exclusive by referring to them as "( --foo | --bar
)".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-pack.txt | 4 ++--
 builtin/send-pack.c             | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 44fd146b912..be41f119740 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
+'git send-pack' [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
 		[--verbose] [--thin] [--atomic]
 		[--[no-]signed|--signed=(true|false|if-asked)]
-		[<host>:]<directory> [<ref>...]
+		[<host>:]<directory> (--all | <ref>...)
 
 DESCRIPTION
 -----------
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 729dea1d255..89321423125 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -17,10 +17,10 @@
 #include "protocol.h"
 
 static const char * const send_pack_usage[] = {
-	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
-	  "[--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] "
-	  "[<host>:]<directory> [<ref>...]\n"
-	  "  --all and explicit <ref> specification are mutually exclusive."),
+	N_("git send-pack [--mirror] [--dry-run] [--force]\n"
+	   "              [--receive-pack=<git-receive-pack>]\n"
+	   "              [--verbose] [--thin] [--atomic]\n"
+	   "              [<host>:]<directory> (--all | <ref>...)"),
 	NULL,
 };
 
-- 
2.33.0.1001.g3ab2ac1eaae


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 3/4] git rev-parse --parseopt tests: add more usagestr tests
  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       ` Æ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-21 13:30       ` [PATCH v5 0/4] " Ævar Arnfjörð Bjarmason
  4 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Add tests for the "usagestr" passed to parse-options.c
usage_with_options_internal() through cmd_parseopt().

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 <avarab@gmail.com>
---
 t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b29563fc997..284fe18e726 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -282,4 +282,58 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op
 	test_cmp expect output
 '
 
+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
+
+	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 <spec >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.1001.g3ab2ac1eaae


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v4 4/4] parse-options: properly align continued usage output
  2021-09-13  0:13     ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
                         ` (2 preceding siblings ...)
  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       ` Æ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
  4 siblings, 1 reply; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:13 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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>]
                     [...]

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
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.

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>
---
 parse-options.c | 76 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 2abff136a17..75f0a6c81c5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -917,25 +917,77 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;
 
+	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,
+	 * Hebrew etc.), not if it's a LTR language (e.g. German,
+	 * Russian, Chinese etc.).
+	 *
+	 * When a translated usage string has an embedded "\n" it's
+	 * 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
+	 * "git cmd ".
+	 *
+	 * 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
+	 * translated) N_() usage string, which contained embedded
+	 * newlines before we split it up.
+	 */
+	const char *usage_continued = _("%*s%s");
+	const char *prefix = usage_prefix;
+	int saw_empty_line = 0;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
-	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++;
+		const char *str = _(*usagestr++);
+		struct string_list list = STRING_LIST_INIT_DUP;
+		unsigned int j;
+
+		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 (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,
+					   (int)usage_len, "", line);
+		}
+		string_list_clear(&list, 0);
+
+		prefix = or_prefix;
 	}
 
 	need_newline = 1;
-- 
2.33.0.1001.g3ab2ac1eaae


^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array
  2021-09-12 22:26       ` Junio C Hamano
@ 2021-09-13  0:21         ` Ævar Arnfjörð Bjarmason
  0 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-13  0:21 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, Carlo Arenas, Eric Sunshine


On Sun, Sep 12 2021, Junio C Hamano wrote:

> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:
>
>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh
>> index ad4746d899a..2910874ece5 100755
>> --- a/t/t0040-parse-options.sh
>> +++ b/t/t0040-parse-options.sh
>> @@ -10,8 +10,6 @@ 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
>
> Isn't this, and a lot more importantly the next one ...
>
>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
>> index b29563fc997..6badc650d64 100755
>> --- a/t/t1502-rev-parse-parseopt.sh
>> +++ b/t/t1502-rev-parse-parseopt.sh
>> @@ -6,8 +6,6 @@ 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
>>  |
>
> ... coalmine canaries that tell us that end-user's scripts using the
> "git rev-parse --parseopt" in the documented way will be broken?
>
> I'd rather not have to sorry about breaking end-user scripts this
> way.  Unlike a very small number of in-tree parse_options() call in
> C programs, we have unbounded of them.

I re-rolled this in v4 to not change how the function works at all:
https://lore.kernel.org/git/cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com/

I do think we're going above & beyond what's reasonable in maintaining
these interfaces, i.e. per 21d47835386 (Add a parseopt mode to
git-rev-parse to bring parse-options to shell scripts., 2007-11-04) I
don't think anyone thought that interface would be used outside of
git.git.

But then perhaps I'm wrong, or it has been, and since we ended up
sticking it in rev-parse which was marked as plumbing we can't change
how it or the underlying C APIs it relies on work.

In practice I doubt it would break things for anyone, but per the result
of the discussion to "remove dead shell code" [1] I don't think I'll get
anywhere with that.

I do think that getting some answer to [2] & how we think about the
likes of "git rev-parse --parseopt" going forward would be very
useful.

I.e. would you be OK with us marking these as deprecated / warn on their
out-of-tree use? If so we could after some time remove a lot of code &
simplify things once the last in-tree shellscript user goes away.

Or if we don't see that they can ever be changed that's also useful to
know (and per [2] I think I'd feel obligated to send in a revert
bringing git-parse-remote back).

1. https://lore.kernel.org/git/cover-v3-0.4-00000000000-20210911T111435Z-avarab@gmail.com/
2. https://lore.kernel.org/git/87tuiwjfvi.fsf@evledraar.gmail.com/

^ permalink raw reply	[flat|nested] 45+ messages in thread

* Re: [PATCH v4 4/4] parse-options: properly align continued usage output
  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
  0 siblings, 0 replies; 45+ messages in thread
From: Junio C Hamano @ 2021-09-13  6:41 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: git, Jeff King, Carlo Arenas, Eric Sunshine

Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:

> 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>]
>                      [...]
>
> 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

A "be" is missing here.

> would make that non-trivial.
>
> Even if it was I still think this approach is better, because this way

"was" -> "were,", I think.

Even though this step has quite a lot of comment strings meant for
human consumption, I didn't hunt for grammos and typos and only
looked at the code.  Somebody else may want to proof-read it.

The new code looked OK.  Do we want to have some new test in
t/helper/test-*.c with fixed "command help string"?

Thanks.

^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v5 0/4] parse-options: properly align continued usage output
  2021-09-13  0:13     ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
                         ` (3 preceding siblings ...)
  2021-09-13  0:13       ` [PATCH v4 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:30       ` Æ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
                           ` (3 more replies)
  4 siblings, 4 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:30 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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

There's only a very small typo changes in this v5 in response to
https://lore.kernel.org/git/xmqq8s01geov.fsf@gitster.g/; I also
re-read the rest over & spellchecked the comments I added, for
whatever that's worth.

I didn't add a test in t/helper/ as Junio suggested, since it's the
usage message itself it doesn't slot nicely into the control flow of
t/helper/test-run-command.c, we could of course make it work, but I'd
prefer to punt on it.

I think having manually tested the output should do in this
case. I.e. I manually looked at all the "-h" emitted by t0012-help.sh
and it's all nicely formatted now (aside from some overly long lines,
addressing that is left for another day).

Æ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 v4:
1:  64d8647340d = 1:  352662be92d parse-options API users: align usage output in C-strings
2:  f10ff775c69 = 2:  d3767214d22 send-pack: properly use parse_options() API for usage string
3:  05a0c7cac37 = 3:  8262999b456 git rev-parse --parseopt tests: add more usagestr tests
4:  55480dee680 ! 4:  9f7f3f8b4ed parse-options: properly align continued usage output
    @@ Commit message
     
         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
    +    might be 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
    +    Even if it were 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.
-- 
2.33.0.1098.gf02a64c1a2d


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v5 1/4] parse-options API users: align usage output in C-strings
  2021-09-21 13:30       ` [PATCH v5 0/4] " Ævar Arnfjörð Bjarmason
@ 2021-09-21 13:30         ` Æ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
                           ` (2 subsequent siblings)
  3 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:30 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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.

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.

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:

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

Would emit:

   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 <avarab@gmail.com>
---
 builtin/ls-remote.c   | 4 ++--
 builtin/show-branch.c | 6 +++---
 builtin/stash.c       | 2 +-
 builtin/tag.c         | 4 ++--
 4 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/builtin/ls-remote.c b/builtin/ls-remote.c
index f4fd823af83..318949c3d75 100644
--- a/builtin/ls-remote.c
+++ b/builtin/ls-remote.c
@@ -7,8 +7,8 @@
 
 static const char * const ls_remote_usage[] = {
 	N_("git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]\n"
-	   "                     [-q | --quiet] [--exit-code] [--get-url]\n"
-	   "                     [--symref] [<repository> [<refs>...]]"),
+	   "              [-q | --quiet] [--exit-code] [--get-url]\n"
+	   "              [--symref] [<repository> [<refs>...]]"),
 	NULL
 };
 
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index bea4bbf4680..082449293b5 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -11,9 +11,9 @@
 
 static const char* show_branch_usage[] = {
     N_("git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\n"
-       "		[--current] [--color[=<when>] | --no-color] [--sparse]\n"
-       "		[--more=<n> | --list | --independent | --merge-base]\n"
-       "		[--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
+       "                [--current] [--color[=<when>] | --no-color] [--sparse]\n"
+       "                [--more=<n> | --list | --independent | --merge-base]\n"
+       "                [--no-name | --sha1-name] [--topics] [(<rev> | <glob>)...]"),
     N_("git show-branch (-g | --reflog)[=<n>[,<base>]] [--list] [<ref>]"),
     NULL
 };
diff --git a/builtin/stash.c b/builtin/stash.c
index 8f42360ca91..45b19007d7c 100644
--- a/builtin/stash.c
+++ b/builtin/stash.c
@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {
 
 static const char * const git_stash_save_usage[] = {
 	N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
-	   "          [-u|--include-untracked] [-a|--all] [<message>]"),
+	   "               [-u|--include-untracked] [-a|--all] [<message>]"),
 	NULL
 };
 
diff --git a/builtin/tag.c b/builtin/tag.c
index 065b6bf093e..6535ed27ee9 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -23,10 +23,10 @@
 
 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>...]"),
+	   "        [--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
 	N_("git tag -v [--format=<format>] <tagname>..."),
 	NULL
 };
-- 
2.33.0.1098.gf02a64c1a2d


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v5 2/4] send-pack: properly use parse_options() API for usage string
  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         ` Æ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
  3 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:30 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

When "send-pack" was changed to use the parse_options() API in
068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)
it was made to use one very long line, instead it should split them up
with newlines.

Furthermore we were including an inline explanation that you couldn't
combine "--all" and "<ref>", but unlike in the "blame" case this was
not preceded by an empty string.

Let's instead show that --all and <ref> can't be combined in the the
usual language of the usage syntax instead. We can make it clear that
one of the two options "--foo" and "--bar" is mandatory, but that the
two are mutually exclusive by referring to them as "( --foo | --bar
)".

Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/git-send-pack.txt | 4 ++--
 builtin/send-pack.c             | 8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt
index 44fd146b912..be41f119740 100644
--- a/Documentation/git-send-pack.txt
+++ b/Documentation/git-send-pack.txt
@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository
 SYNOPSIS
 --------
 [verse]
-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
+'git send-pack' [--dry-run] [--force] [--receive-pack=<git-receive-pack>]
 		[--verbose] [--thin] [--atomic]
 		[--[no-]signed|--signed=(true|false|if-asked)]
-		[<host>:]<directory> [<ref>...]
+		[<host>:]<directory> (--all | <ref>...)
 
 DESCRIPTION
 -----------
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index 729dea1d255..89321423125 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -17,10 +17,10 @@
 #include "protocol.h"
 
 static const char * const send_pack_usage[] = {
-	N_("git send-pack [--all | --mirror] [--dry-run] [--force] "
-	  "[--receive-pack=<git-receive-pack>] [--verbose] [--thin] [--atomic] "
-	  "[<host>:]<directory> [<ref>...]\n"
-	  "  --all and explicit <ref> specification are mutually exclusive."),
+	N_("git send-pack [--mirror] [--dry-run] [--force]\n"
+	   "              [--receive-pack=<git-receive-pack>]\n"
+	   "              [--verbose] [--thin] [--atomic]\n"
+	   "              [<host>:]<directory> (--all | <ref>...)"),
 	NULL,
 };
 
-- 
2.33.0.1098.gf02a64c1a2d


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v5 3/4] git rev-parse --parseopt tests: add more usagestr tests
  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         ` Ævar Arnfjörð Bjarmason
  2021-09-21 13:30         ` [PATCH v5 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:30 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

Add tests for the "usagestr" passed to parse-options.c
usage_with_options_internal() through cmd_parseopt().

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 <avarab@gmail.com>
---
 t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++++++++++++++
 1 file changed, 54 insertions(+)

diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index b29563fc997..284fe18e726 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -282,4 +282,58 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op
 	test_cmp expect output
 '
 
+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
+
+	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 <spec >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.33.0.1098.gf02a64c1a2d


^ permalink raw reply	[flat|nested] 45+ messages in thread

* [PATCH v5 4/4] parse-options: properly align continued usage output
  2021-09-21 13:30       ` [PATCH v5 0/4] " Ævar Arnfjörð Bjarmason
                           ` (2 preceding siblings ...)
  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         ` Ævar Arnfjörð Bjarmason
  3 siblings, 0 replies; 45+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-09-21 13:30 UTC (permalink / raw)
  To: git
  Cc: Junio C Hamano, Jeff King, Carlo Arenas, Eric Sunshine,
	Ævar Arnfjörð Bjarmason

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>]
                     [...]

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 be cases with sub-commands or "git" itself taking arguments that
would make that non-trivial.

Even if it were 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
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.

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>
---
 parse-options.c | 76 +++++++++++++++++++++++++++++++++++++++++--------
 1 file changed, 64 insertions(+), 12 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 2abff136a17..75f0a6c81c5 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -917,25 +917,77 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
 	FILE *outfile = err ? stderr : stdout;
 	int need_newline;
 
+	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,
+	 * Hebrew etc.), not if it's a LTR language (e.g. German,
+	 * Russian, Chinese etc.).
+	 *
+	 * When a translated usage string has an embedded "\n" it's
+	 * 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
+	 * "git cmd ".
+	 *
+	 * 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
+	 * translated) N_() usage string, which contained embedded
+	 * newlines before we split it up.
+	 */
+	const char *usage_continued = _("%*s%s");
+	const char *prefix = usage_prefix;
+	int saw_empty_line = 0;
+
 	if (!usagestr)
 		return PARSE_OPT_HELP;
 
 	if (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)
 		fprintf(outfile, "cat <<\\EOF\n");
 
-	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++;
+		const char *str = _(*usagestr++);
+		struct string_list list = STRING_LIST_INIT_DUP;
+		unsigned int j;
+
+		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 (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,
+					   (int)usage_len, "", line);
+		}
+		string_list_clear(&list, 0);
+
+		prefix = or_prefix;
 	}
 
 	need_newline = 1;
-- 
2.33.0.1098.gf02a64c1a2d


^ permalink raw reply	[flat|nested] 45+ messages in thread

end of thread, other threads:[~2021-09-21 13:30 UTC | newest]

Thread overview: 45+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
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
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

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.