patch, 2 partsparse-options: properly align continued usage output

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

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

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

On 9/1/21 7:12 AM, Ævar Arnfjörð Bjarmason wrote:

More on this below...

Makes sense.

"wrapped o the next line"?

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.

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;

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

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

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

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

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

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.

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

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

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

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

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.

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

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

used *by* "blame"

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.

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

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.

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

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.

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

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

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

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.

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

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.

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.

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

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

On Fri, Sep 10, 2021 at 11:38 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

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

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

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

Yah, I forgot about EXCLUDED_PROGRAMS ;-)  

Sounds like a sensible direction to go.

OK.  Looks good.

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

On Sat, Sep 11, 2021 at 3:10 PM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

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

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

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

Ditto regarding missing hyphen.

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.

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

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.

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

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

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

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

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

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

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/

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

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

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

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,

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

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

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(+)

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

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

A "be" is missing here.

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

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.

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

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(+)

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

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