{"thread":{"id":"56423","subject":"[PATCH 0/2] parse-options: properly align continued usage output","startedAt":"2021-09-01T11:13:07Z","lastAt":"2021-09-21T13:30:46Z","messageCount":45,"participants":["Ævar Arnfjörð Bjarmason","Eric Sunshine","Junio C Hamano","Carlo Marcelo Arenas Belón"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"434409","messageId":"cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com","threadId":"56423","inReplyTo":null,"subject":"[PATCH 0/2] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-01T11:12:53Z","receivedAt":"2021-09-01T11:13:07Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes usage_with_options_internal() in parse-options.c\nto properly align continued \"\\n\" usage output.\n\nBefore this change some built-ins using the API effectively tried to\nhack around the lack of such alignment, with some getting it wrong,\nand some getting it right. Others such as \"git stash -h\" didn't at\nall, and their continued \"\\n\" output would always look weirdly\nmisaligned. Now the output all looks nice!\n\nThis came about from following the thread of \"why we we even have '\\n'\nhere at all?\", which is a question I had in my own mind around\nhttps://lore.kernel.org/git/87zgszxirn.fsf@evledraar.gmail.com\n\nÆvar Arnfjörð Bjarmason (2):\n built-ins: \"properly\" align continued usage output\n parse-options: properly align continued usage output\n\n builtin/ls-remote.c | 4 +--\n builtin/show-branch.c | 6 ++--\n builtin/stash.c | 2 +-\n builtin/tag.c | 2 +-\n parse-options.c | 79 ++++++++++++++++++++++++++++++++++++-------\n 5 files changed, 73 insertions(+), 20 deletions(-)\n\n-- \n2.33.0.807.gf14ecf9c2e9\n\n"},{"id":"434410","messageId":"patch-1.2-ccc024c414f-20210901T110917Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com","subject":"[PATCH 1/2] built-ins: \"properly\" align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-01T11:12:54Z","receivedAt":"2021-09-01T11:13:08Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\noutput in cases where we continue usage output after a \"\\n\". The \"fix\"\nand \"properly\" scare quotes are because this actually makes things\nworse in some cases, because e.g. in the case of \"git tag -h\" the\n\"\\t\\t\" effectively works around how parse-options.c aligns this\noutput.\n\nBut two wrongs don't make a right, let's \"fix\" this by making it worse\ntemporarily, in anticipating of improving parse-options.c to handle\nthis alignment.\n\nThe issue is that we should have whitespace corresponding to the\nlength of the command name here, e.g. in the case of \"git ls-remote\"\nit should be 14 characters, or the length of \"\"git ls-remote\n\". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\ncharacters are the length of \"usage: \" (and also \" or:\"). So in the C\nlocale the resulting output aligns nicely:\n\n $ git ls-remote -h\n usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n [-q | --quiet] [--exit-code] [--get-url]\n [--symref] [ [...]]\n\nBut that's fragile, we might not be under the C locale. We really\nshould have parse-options.c itself add this padding. In a subsequent\ncommit I'll make it do that.\n\nIn the case of \"tag\" and \"show-branch\" and \"stash save\" the output was\nnot properly aligned, although in the \"git tag\" case it was\nnear-enough (aligned with the \"-\" in \"git tag -l\") to look good,\nassuming C locale & a tab-width of 8. In any case, let's align this in\na way that looks obviously correct when looking at the source itself,\nand then improve parse-options.c itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/ls-remote.c | 4 ++--\n builtin/show-branch.c | 6 +++---\n builtin/stash.c | 2 +-\n builtin/tag.c | 2 +-\n 4 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex 1794548c711..ef9036974ce 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -7,8 +7,8 @@\n \n static const char * const ls_remote_usage[] = {\n \tN_(\"git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\\n\"\n-\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n-\t \" [--symref] [ [...]]\"),\n+\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n+\t \" [--symref] [ [...]]\"),\n \tNULL\n };\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d77ce7aeb38..a82cd1534fc 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -11,9 +11,9 @@\n \n static const char* show_branch_usage[] = {\n N_(\"git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\\n\"\n- \"\t\t[--current] [--color[=] | --no-color] [--sparse]\\n\"\n- \"\t\t[--more= | --list | --independent | --merge-base]\\n\"\n- \"\t\t[--no-name | --sha1-name] [--topics] [( | )...]\"),\n+ \" [--current] [--color[=] | --no-color] [--sparse]\\n\"\n+ \" [--more= | --list | --independent | --merge-base]\\n\"\n+ \" [--no-name | --sha1-name] [--topics] [( | )...]\"),\n N_(\"git show-branch (-g | --reflog)[=[,]] [--list] []\"),\n NULL\n };\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..45b19007d7c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {\n \n static const char * const git_stash_save_usage[] = {\n \tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n-\t \" [-u|--include-untracked] [-a|--all] []\"),\n+\t \" [-u|--include-untracked] [-a|--all] []\"),\n \tNULL\n };\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 452558ec957..9b1165d2a4e 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -26,7 +26,7 @@ static const char * const git_tag_usage[] = {\n \t\t\"\\t\\t []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n+\t \" [--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n-- \n2.33.0.807.gf14ecf9c2e9\n\n"},{"id":"434411","messageId":"patch-2.2-ab4bb70902b-20210901T110917Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com","subject":"[PATCH 2/2] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-01T11:12:55Z","receivedAt":"2021-09-01T11:13:10Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some commands such as \"git stash\" emit continued options output with\ne.g. \"git stash -h\", because usage_with_options_internal() prefixes\nwith its own whitespace the resulting output wasn't properly\naligned. Let's account for the added whitespace, which properly aligns\nthe output.\n\nThe \"git stash\" command has usage output with a N_() translation that\nlegitimately stretches across multiple lines;\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n\nWe'd like to have that output aligned with the length of the initial\n\"git stash \" output, but since usage_with_options_internal() adds its\nown whitespace prefixing we fell short, before this change we'd emit:\n\n $ git stash -h\n usage: git stash list []\n or: git stash show [] []\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nNow we'll properly emit aligned output. I.e. the last four lines\nabove will instead be (a whitespace-only change to the above):\n\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nIn making this change we can can fold the two for-loops over *usagestr\ninto one. We had two of them purely to account for the case where an\nempty string in the array delimits the usage output from free-form\ntext output.\n\nWe could skip the string_list_split() with a strchr(str, '\\n') check,\nbut we'd then need to duplicate our state machine for strings that do\nand don't contain a \"\\n\". It's simpler to just always split into a\n\"struct string_list\", even though the common case is that that \"struct\nstring_list\" will contain only one element. This is not\nperformance-sensitive code.\n\nThis change is relatively more complex since I've accounted for making\nit future-proof for RTL translation support. Later in\nusage_with_options_internal() we have some existing padding code\ndating back to d7a38c54a6c (parse-options: be able to generate usages\nautomatically, 2007-10-15) which isn't RTL-safe, but that code would\nbe easy to fix. Let's not introduce new RTL translation problems here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 79 +++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 66 insertions(+), 13 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..a06968bf4f5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -917,25 +917,78 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n \n+\tconst char *usage_prefix = _(\"usage: %s\");\n+\t/*\n+\t * TRANSLATORS: the colon here should align with the\n+\t * one in \"usage: %s\" translation.\n+\t */\n+\tconst char *or_prefix = _(\" or: %s\");\n+\t/*\n+\t * TRANSLATORS: You should only need to translate this format\n+\t * string if your language is a RTL language (e.g. Arabic,\n+\t * Hebrew etc.), not if it's a LTR language (e.g. German,\n+\t * Russian, Chinese etc.).\n+\t *\n+\t * When a translated usage string has an embedded \"\\n\" it's\n+\t * because options have wrapped o the next line. The line\n+\t * after the \"\\n\" will then be padded to align with the\n+\t * command name, such as N_(\"git cmd [opt]\\n<8\n+\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n+\t * \"git cmd \".\n+\t *\n+\t * This format string prints out that already-translated\n+\t * line. The \"%*s\" is whitespace padding to account for the\n+\t * padding at the start of the line that we add in this\n+\t * function, the \"%s\" is a line in the (hopefully already\n+\t * translated) N_() usage string, which contained embedded\n+\t * newlines before we split it up.\n+\t */\n+\tconst char *usage_continued = _(\"%*s%s\");\n+\n+\t/*\n+\t * The translation could be anything, but we can count on\n+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n+\t * the translation. So compute the length of the \" or: \"\n+\t * part. We are assuming that the translator wasn't overly\n+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n+\t * one \"%s\" in \"or_prefix\" above, so there's no reason to do\n+\t * so even with a RTL language.\n+\t */\n+\tsize_t or_len = strlen(or_prefix) - strlen(\"%s\");\n+\tint i;\n+\tint saw_empty_line = 0;\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n \tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n-\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n-\twhile (*usagestr && **usagestr)\n-\t\t/*\n-\t\t * TRANSLATORS: the colon here should align with the\n-\t\t * one in \"usage: %s\" translation.\n-\t\t */\n-\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n-\twhile (*usagestr) {\n-\t\tif (**usagestr)\n-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n-\t\telse\n-\t\t\tfputc('\\n', outfile);\n-\t\tusagestr++;\n+\tfor (i = 0; *usagestr; i++) {\n+\t\tconst char *str = _(*usagestr++);\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tunsigned int j;\n+\n+\t\tstring_list_split(&list, str, '\\n', -1);\n+\t\tfor (j = 0; j < list.nr; j++) {\n+\t\t\tconst char *line = list.items[j].string;\n+\n+\t\t\tif (!saw_empty_line && !*line)\n+\t\t\t\tsaw_empty_line = 1;\n+\n+\t\t\tif (saw_empty_line && *line)\n+\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n+\t\t\telse if (saw_empty_line)\n+\t\t\t\tfputc('\\n', outfile);\n+\t\t\telse if (!j && !i)\n+\t\t\t\tfprintf_ln(outfile, usage_prefix, line);\n+\t\t\telse if (!j)\n+\t\t\t\tfprintf_ln(outfile, or_prefix, line);\n+\t\t\telse\n+\t\t\t\tfprintf_ln(outfile, usage_continued,\n+\t\t\t\t\t (int)or_len, \"\", line);\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.807.gf14ecf9c2e9\n\n"},{"id":"435370","messageId":"f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com","threadId":"56423","inReplyTo":"patch-2.2-ab4bb70902b-20210901T110917Z-avarab@gmail.com","subject":"Re: [PATCH 2/2] parse-options: properly align continued usage output","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-10T07:51:45Z","receivedAt":"2021-09-10T07:51:51Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On 9/1/21 7:12 AM, Ævar Arnfjörð Bjarmason wrote:\n> Some commands such as \"git stash\" emit continued options output with\n> e.g. \"git stash -h\", because usage_with_options_internal() prefixes\n> with its own whitespace the resulting output wasn't properly\n> aligned. Let's account for the added whitespace, which properly aligns\n> the output.\n> \n> The \"git stash\" command has usage output with a N_() translation that\n> legitimately stretches across multiple lines;\n> \n> \tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n> \t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n> \n> We'd like to have that output aligned with the length of the initial\n> \"git stash \" output, but since usage_with_options_internal() adds its\n> own whitespace prefixing we fell short, before this change we'd emit:\n> \n> $ git stash -h\n> usage: git stash list []\n> or: git stash show [] []\n> [...]\n> or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n> [-u|--include-untracked] [-a|--all] [-m|--message ]\n> [...]\n> \n> Now we'll properly emit aligned output. I.e. the last four lines\n> above will instead be (a whitespace-only change to the above):\n> \n> [...]\n> or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n> [-u|--include-untracked] [-a|--all] [-m|--message ]\n> [...]\n> \n> In making this change we can can fold the two for-loops over *usagestr\n> into one. We had two of them purely to account for the case where an\n> empty string in the array delimits the usage output from free-form\n> text output.\n\nMore on this below...\n\n> We could skip the string_list_split() with a strchr(str, '\\n') check,\n> but we'd then need to duplicate our state machine for strings that do\n> and don't contain a \"\\n\". It's simpler to just always split into a\n> \"struct string_list\", even though the common case is that that \"struct\n> string_list\" will contain only one element. This is not\n> performance-sensitive code.\n\nMakes sense.\n\n> This change is relatively more complex since I've accounted for making\n> it future-proof for RTL translation support. Later in\n> usage_with_options_internal() we have some existing padding code\n> dating back to d7a38c54a6c (parse-options: be able to generate usages\n> automatically, 2007-10-15) which isn't RTL-safe, but that code would\n> be easy to fix. Let's not introduce new RTL translation problems here.\n> \n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/parse-options.c b/parse-options.c\n> @@ -917,25 +917,78 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n> +\t * When a translated usage string has an embedded \"\\n\" it's\n> +\t * because options have wrapped o the next line. The line\n\n\"wrapped o the next line\"?\n\n> +\tsize_t or_len = strlen(or_prefix) - strlen(\"%s\");\n> +\tint i;\n> +\tint saw_empty_line = 0;\n> +\n> -\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n> -\twhile (*usagestr && **usagestr)\n> -\t\t/*\n> -\t\t * TRANSLATORS: the colon here should align with the\n> -\t\t * one in \"usage: %s\" translation.\n> -\t\t */\n> -\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n> -\twhile (*usagestr) {\n> -\t\tif (**usagestr)\n> -\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n> -\t\telse\n> -\t\t\tfputc('\\n', outfile);\n> -\t\tusagestr++;\n> +\tfor (i = 0; *usagestr; i++) {\n> +\t\tconst char *str = _(*usagestr++);\n> +\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n> +\t\tunsigned int j;\n> +\n> +\t\tstring_list_split(&list, str, '\\n', -1);\n> +\t\tfor (j = 0; j < list.nr; j++) {\n> +\t\t\tconst char *line = list.items[j].string;\n> +\n> +\t\t\tif (!saw_empty_line && !*line)\n> +\t\t\t\tsaw_empty_line = 1;\n> +\n> +\t\t\tif (saw_empty_line && *line)\n> +\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n> +\t\t\telse if (saw_empty_line)\n> +\t\t\t\tfputc('\\n', outfile);\n> +\t\t\telse if (!j && !i)\n> +\t\t\t\tfprintf_ln(outfile, usage_prefix, line);\n> +\t\t\telse if (!j)\n> +\t\t\t\tfprintf_ln(outfile, or_prefix, line);\n> +\t\t\telse\n> +\t\t\t\tfprintf_ln(outfile, usage_continued,\n> +\t\t\t\t\t (int)or_len, \"\", line);\n> +\t\t}\n> +\t\tstring_list_clear(&list, 0);\n\nI may be missing something obvious, but I'm having trouble understanding \nwhy this single loop is better than the two loops it replaces. The \ncognitive load of the new code is much higher than that of the original. \nWith the original code, the logic was obvious at a glance. On the other \nhand, I had to concentrate hard to figure out what the new code is \ntrying to do and to wrap my brain around all the cases it is handling. I \nsuppose you went with the single loop to avoid code duplication (in \nparticular, the call to string_list_split() and the loop over the split \nelements)?\n\nThere are other ways this might be accomplished which don't carry such a \nhigh cognitive load. One (typed-in-email) possibility which closely \nresembles the existing code:\n\n const char *pfx = usage_prefix;\n while (*usagestr && **usagestr) {\n string_list_split(&list, _(*usagestr++), ...);\n fprintf_ln(outfile, pfx, list.items[0].string);\n for (i = 1; i < list.nr; i++)\n fprintf_ln(outfile, usage_continued,\n (int)or_len, \"\", list.items[i].string);\n pfx = or_prefix;\n }\n while (*usagestr) {\n string_list_split(&list, _(*usagestr++), ...);\n for (i = 0; i < list.nr; i++) {\n const char *line = list.items[i].string;\n if (*line)\n fprintf_ln(outfile, _(\" %s\"), line);\n else\n fputc('\\n', outfile);\n }\n }\n\nI also wonder if you really need to support the embedded-newline case \nfor the free-form text loop. For free-form text, it's just as easy for \neach line of text to be a distinct item in the usage[] array, if I \nunderstand correctly, so there isn't really a good reason for clients to \nembed newlines in the free-form text portion. Given that there's only a \nsingle client in the entire project which takes advantage of the \nfree-form text support -- and that client doesn't embed newlines -- it \nmay be simpler to not bother supporting embedded newlines for the \nfree-form text, in which case you don't even need to modify that loop; \nthe existing code is good enough.\n\nAnyhow, the above observations are subjective, thus not necessarily \nactionable, however, there is also a subtle yet dramatic behavior change \nin the new code, if I understand correctly. It's not clear if this \nbehavior change is intentional (it isn't mentioned in the commit \nmessage), but it does seem potentially dangerous. Specifically, with the:\n\n if (!saw_empty_line && !*line)\n saw_empty_line = 1;\n\ncheck inside the inner loop which iterates over the split lines, this \nmeans that if someone accidentally embeds an extra newline in some usage \nline:\n\n static const char *foo_usage[] = {\n N_(\"git foo --bar\\n\\n\" /* <-- accidental extra newline */\n \" --baz\"),\n N_(\"git boo\"),\n NULL\n };\n\nthen _all_ following usage lines will incorrectly be treated as \nfree-form text lines rather than as the \"or:\" lines they are intended to \nbe. Moving the:\n\n if (!saw_empty_line && !*line)\n saw_empty_line = 1;\n\ncheck from the inner loop to the outer loop should restore the original \n(intended) behavior, I believe.\n"},{"id":"435433","messageId":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com","subject":"[PATCH v2 0/6] parse-options: properly align continued usage output & related","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:30Z","receivedAt":"2021-09-10T15:38:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes usage_with_options_internal() in parse-options.c\nto properly align continued \"\\n\" usage output.\n\nThis v2 now also gets rid of the support for \"\" in the usage string\narray. Eric Sunshine had ideas[1] for how to simplify the code in v1,\nalong with a suggestion that we could just get rid of the \"\" from\n\"builtin/blame.c\".\n\nI'd done some experiments with that before submitting v1, but decided\nto try to submit something simpler to begin with. But let's just grow\nthis in scope a bit and do that, as shown here we also need to do the\nsame in builtin/credential-cache*.c.\n\nThis gives us a much nicer end-state, and as an added bonus\neffectively brings back the check removed with the removal of\nUSE_PARENS_AROUND_GETTEXT_N in in-flight ]2], which is what brought\nthis alignment issue & edge cases in parse_options() usage to my\nattention in the first place.\n\n1. https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/\n2. https://lore.kernel.org/git/878s0gwmvq.fsf@evledraar.gmail.com/\n\nÆvar Arnfjörð Bjarmason (6):\n test-lib.sh: add a UNIX_SOCKETS prerequisite\n git.c: add a NEED_UNIX_SOCKETS option for built-ins\n parse-options: stop supporting \"\" in the usagestr array\n built-ins: \"properly\" align continued usage output\n send-pack: properly use parse_options() API for usage string\n parse-options: properly align continued usage output\n\n Documentation/git-send-pack.txt | 4 +-\n builtin.h | 6 +++\n builtin/blame.c | 9 ++--\n builtin/credential-cache--daemon.c | 11 +----\n builtin/credential-cache.c | 11 +----\n builtin/ls-remote.c | 4 +-\n builtin/rev-parse.c | 3 ++\n builtin/send-pack.c | 8 ++--\n builtin/show-branch.c | 6 +--\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 +-\n git.c | 15 +++++--\n parse-options.c | 71 +++++++++++++++++++++++++-----\n t/helper/test-parse-options.c | 2 -\n t/t0012-help.sh | 10 +++++\n t/t0040-parse-options.sh | 2 -\n t/t0301-credential-cache.sh | 5 ++-\n t/t1502-rev-parse-parseopt.sh | 34 +++++++-------\n t/test-lib.sh | 1 +\n 19 files changed, 131 insertions(+), 77 deletions(-)\n\nRange-diff against v1:\n-: ----------- > 1: 9e8facb6f8c test-lib.sh: add a UNIX_SOCKETS prerequisite\n-: ----------- > 2: d6c44402715 git.c: add a NEED_UNIX_SOCKETS option for built-ins\n-: ----------- > 3: 11f4a119563 parse-options: stop supporting \"\" in the usagestr array\n1: ccc024c414f ! 4: 4547cc968b1 built-ins: \"properly\" align continued usage output\n @@ Commit message\n output.\n \n But two wrongs don't make a right, let's \"fix\" this by making it worse\n - temporarily, in anticipating of improving parse-options.c to handle\n + temporarily, in anticipation of improving parse-options.c to handle\n this alignment.\n \n The issue is that we should have whitespace corresponding to the\n @@ builtin/stash.c: static const char * const git_stash_push_usage[] = {\n \n \n ## builtin/tag.c ##\n -@@ builtin/tag.c: static const char * const git_tag_usage[] = {\n - \t\t\"\\t\\t []\"),\n +@@\n + \n + static const char * const git_tag_usage[] = {\n + \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ]\\n\"\n +-\t\t\"\\t\\t []\"),\n ++\t \" []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n -\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n-: ----------- > 5: b884b361ace send-pack: properly use parse_options() API for usage string\n2: ab4bb70902b ! 6: e83d66da6d4 parse-options: properly align continued usage output\n @@ Commit message\n \n N_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n + [...]\n \n We'd like to have that output aligned with the length of the initial\n \"git stash \" output, but since usage_with_options_internal() adds its\n @@ Commit message\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n \n - In making this change we can can fold the two for-loops over *usagestr\n - into one. We had two of them purely to account for the case where an\n - empty string in the array delimits the usage output from free-form\n - text output.\n + We could also go for an approach where we have the caller support no\n + padding of their own, i.e. (same as the first example, except for the\n + padding on the second line):\n +\n + N_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n + \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n + [...]\n +\n + But to do that we'll need to find the length of \"git stash\". We can\n + discover that from the \"cmd\" in the \"struct cmd_struct\", but there\n + might cases with sub-commands or \"git\" itself taking arguments that\n + would make that non-trivial.\n +\n + Even if it was I still think this approach is better, because this way\n + we'll get the same legible alignment in the C code. The fact that\n + usage_with_options_internal() is adding its own prefix padding is an\n + implementation detail that callers shouldn't need to worry about.\n +\n + Implementation notes:\n \n We could skip the string_list_split() with a strchr(str, '\\n') check,\n but we'd then need to duplicate our state machine for strings that do\n @@ Commit message\n automatically, 2007-10-15) which isn't RTL-safe, but that code would\n be easy to fix. Let's not introduce new RTL translation problems here.\n \n + I'm also adding a check to catch the mistake of needlessly adding a\n + trailing \"\\n\", such as:\n +\n + N_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n + \" [-u|--include-untracked] [-a|--all] []\\n\"),\n +\n + Or even a mistake like adding just one \"\\n\" in a string with no other\n + newlines:\n +\n + N_(\"git stash list []\\n\"),\n +\n + This catches the cases already tested for in cmd_parseopt(), but this\n + covers the purely C API. As noted a preceding commit that added the\n + die() to cmd_parseopt() I'm fairly confident that this will be\n + triggered by no in-tree user I've missed.\n +\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## parse-options.c ##\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n \n +\tconst char *usage_prefix = _(\"usage: %s\");\n +\t/*\n ++\t * The translation could be anything, but we can count on\n ++\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n ++\t * the translation. So compute the length of the \"usage: \"\n ++\t * part. We are assuming that the translator wasn't overly\n ++\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n ++\t * one \"%s\" in \"usage_prefix\" above, so there's no reason to\n ++\t * do so even with a RTL language.\n ++\t */\n ++\tsize_t usage_len = strlen(usage_prefix) - strlen(\"%s\");\n ++\t/*\n +\t * TRANSLATORS: the colon here should align with the\n +\t * one in \"usage: %s\" translation.\n +\t */\n +\tconst char *or_prefix = _(\" or: %s\");\n ++\n +\t/*\n +\t * TRANSLATORS: You should only need to translate this format\n +\t * string if your language is a RTL language (e.g. Arabic,\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n +\t * Russian, Chinese etc.).\n +\t *\n +\t * When a translated usage string has an embedded \"\\n\" it's\n -+\t * because options have wrapped o the next line. The line\n ++\t * because options have wrapped to the next line. The line\n +\t * after the \"\\n\" will then be padded to align with the\n +\t * command name, such as N_(\"git cmd [opt]\\n<8\n +\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n +\t * This format string prints out that already-translated\n +\t * line. The \"%*s\" is whitespace padding to account for the\n +\t * padding at the start of the line that we add in this\n -+\t * function, the \"%s\" is a line in the (hopefully already\n ++\t * function. The \"%s\" is a line in the (hopefully already\n +\t * translated) N_() usage string, which contained embedded\n +\t * newlines before we split it up.\n +\t */\n +\tconst char *usage_continued = _(\"%*s%s\");\n -+\n -+\t/*\n -+\t * The translation could be anything, but we can count on\n -+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n -+\t * the translation. So compute the length of the \" or: \"\n -+\t * part. We are assuming that the translator wasn't overly\n -+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n -+\t * one \"%s\" in \"or_prefix\" above, so there's no reason to do\n -+\t * so even with a RTL language.\n -+\t */\n -+\tsize_t or_len = strlen(or_prefix) - strlen(\"%s\");\n -+\tint i;\n -+\tint saw_empty_line = 0;\n ++\tconst char *prefix = usage_prefix;\n +\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n -\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n --\twhile (*usagestr && **usagestr)\n + \twhile (*usagestr) {\n -\t\t/*\n -\t\t * TRANSLATORS: the colon here should align with the\n -\t\t * one in \"usage: %s\" translation.\n -\t\t */\n -\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n --\twhile (*usagestr) {\n --\t\tif (**usagestr)\n --\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n --\t\telse\n --\t\t\tfputc('\\n', outfile);\n --\t\tusagestr++;\n -+\tfor (i = 0; *usagestr; i++) {\n -+\t\tconst char *str = _(*usagestr++);\n +\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n +\t\tunsigned int j;\n +\n -+\t\tstring_list_split(&list, str, '\\n', -1);\n ++\t\tstring_list_split(&list, _(*usagestr++), '\\n', -1);\n +\t\tfor (j = 0; j < list.nr; j++) {\n +\t\t\tconst char *line = list.items[j].string;\n +\n -+\t\t\tif (!saw_empty_line && !*line)\n -+\t\t\t\tsaw_empty_line = 1;\n ++\t\t\tif (!*line)\n ++\t\t\t\tBUG(\"empty or trailing line in usage string\");\n +\n -+\t\t\tif (saw_empty_line && *line)\n -+\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n -+\t\t\telse if (saw_empty_line)\n -+\t\t\t\tfputc('\\n', outfile);\n -+\t\t\telse if (!j && !i)\n -+\t\t\t\tfprintf_ln(outfile, usage_prefix, line);\n -+\t\t\telse if (!j)\n -+\t\t\t\tfprintf_ln(outfile, or_prefix, line);\n ++\t\t\tif (!j)\n ++\t\t\t\tfprintf_ln(outfile, prefix, line);\n +\t\t\telse\n +\t\t\t\tfprintf_ln(outfile, usage_continued,\n -+\t\t\t\t\t (int)or_len, \"\", line);\n ++\t\t\t\t\t (int)usage_len, \"\", line);\n +\t\t}\n +\t\tstring_list_clear(&list, 0);\n ++\n ++\t\tprefix = or_prefix;\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435434","messageId":"patch-v2-1.6-9e8facb6f8c-20210910T153147Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v2 1/6] test-lib.sh: add a UNIX_SOCKETS prerequisite","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:31Z","receivedAt":"2021-09-10T15:38:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Instead of checking $NO_UNIX_SOCKETS directly in\nt/t0301-credential-cache.sh, follow the more common pattern of\ncreating a test prerequisite in test-lib.sh.\n\nSee 6320358e31d (Makefile: unix sockets may not available on some\nplatforms, 2011-12-12) for the original implementation of\nNO_UNIX_SOCKETS.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t0301-credential-cache.sh | 5 +++--\n t/test-lib.sh | 1 +\n 2 files changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t0301-credential-cache.sh b/t/t0301-credential-cache.sh\nindex ebd5fa5249c..002de427984 100755\n--- a/t/t0301-credential-cache.sh\n+++ b/t/t0301-credential-cache.sh\n@@ -4,10 +4,11 @@ test_description='credential-cache tests'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-credential.sh\n \n-test -z \"$NO_UNIX_SOCKETS\" || {\n+if ! test_have_prereq UNIX_SOCKETS\n+then\n \tskip_all='skipping credential-cache tests, unix sockets not available'\n \ttest_done\n-}\n+fi\n \n # don't leave a stale daemon running\n test_atexit 'git credential-cache exit'\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex abcfbed6d61..583f266b1e8 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1533,6 +1533,7 @@ test -z \"$NO_PYTHON\" && test_set_prereq PYTHON\n test -n \"$USE_LIBPCRE2\" && test_set_prereq PCRE\n test -n \"$USE_LIBPCRE2\" && test_set_prereq LIBPCRE2\n test -z \"$NO_GETTEXT\" && test_set_prereq GETTEXT\n+test -z \"$NO_UNIX_SOCKETS\" && test_set_prereq UNIX_SOCKETS\n \n if test -z \"$GIT_TEST_CHECK_CACHE_TREE\"\n then\n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435435","messageId":"patch-v2-2.6-d6c44402715-20210910T153147Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:32Z","receivedAt":"2021-09-10T15:38:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the implementation of b5dd96b70ac (make credential helpers\nbuiltins, 2020-08-13) to declare in the \"struct cmd_struct\" that\nNO_UNIX_SOCKETS can't be set.\n\nThis is one of two in-tree users for the empty lines in\nparse_options() usage, getting rid of that is the main motivation for\nthis, but it also doesn't make sense to emit these sorts of usage\nmessages just to appease t0012-help.sh, which seemingly b5dd96b70ac\naimed to do.\n\nI.e. these commands don't support \"[options]\", or \"\" so\nemitting that at the beginning is incorrect. We should just die right\naway.\n\nThe existing code also had the edge case of not emitting the die()\nmessage if a \"-h\" argument was given, since parse_options() will\nhandle the exit() itself in that case. We could feed it\nPARSE_OPT_NO_INTERNAL_HELP, but this is better.\n\nBy moving this up to the \"struct cmd_struct\" we can also skip these in\n--list-cmds=builtins instead, as noted above we shouldn't be exiting\nwith code 129 in these cases.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin.h | 6 ++++++\n builtin/credential-cache--daemon.c | 11 +----------\n builtin/credential-cache.c | 11 +----------\n git.c | 15 ++++++++++++---\n t/t0012-help.sh | 10 ++++++++++\n 5 files changed, 30 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin.h b/builtin.h\nindex 16ecd5586f0..66713da6a02 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -63,6 +63,12 @@\n *\tmore informed decision, e.g., by ignoring `pager.` for\n *\tcertain subcommands.\n *\n+ * `NEED_UNIX_SOCKETS`:\n+ *\n+ *\tThis built-in will not work if NO_UNIX_SOCKETS is defined. It\n+ *\twill be recognized for emitting error messages, but won't be\n+ *\tlisted in --list-cmds=builtins.\n+ *\n * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n *\n * Additionally, if `foo` is a new command, there are 4 more things to do:\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 4c6c89ab0de..d9863287a4d 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -304,16 +304,7 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n \n int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n {\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache--daemon [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache--daemon is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n-\tdie(_(\"credential-cache--daemon unavailable; no unix socket support\"));\n+\tBUG(\"should not be called under NO_UNIX_SOCKETS\");\n }\n \n #endif /* NO_UNIX_SOCKET */\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex e8a74157471..22b49b265bf 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -142,16 +142,7 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n \n int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n {\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n-\tdie(_(\"credential-cache unavailable; no unix socket support\"));\n+\tBUG(\"should not be called under NO_UNIX_SOCKETS\");\n }\n \n #endif /* NO_UNIX_SOCKETS */\ndiff --git a/git.c b/git.c\nindex 18bed9a9964..6b0248841db 100644\n--- a/git.c\n+++ b/git.c\n@@ -17,6 +17,7 @@\n #define SUPPORT_SUPER_PREFIX\t(1<<4)\n #define DELAY_PAGER_CONFIG\t(1<<5)\n #define NO_PARSEOPT\t\t(1<<6) /* parse-options is not used */\n+#define NEED_UNIX_SOCKETS\t(1<<7) /* Works unless -DNO_UNIX_SOCKETS */\n \n struct cmd_struct {\n \tconst char *cmd;\n@@ -66,6 +67,10 @@ static int list_cmds(const char *spec)\n \tstruct string_list list = STRING_LIST_INIT_DUP;\n \tint i;\n \tint nongit;\n+\tunsigned int exclude_option = 0;\n+#ifdef NO_UNIX_SOCKETS\n+\texclude_option |= NEED_UNIX_SOCKETS;\n+#endif\n \n \t/*\n \t* Set up the repository so we can pick up any repo-level config (like\n@@ -78,7 +83,7 @@ static int list_cmds(const char *spec)\n \t\tint len = sep - spec;\n \n \t\tif (match_token(spec, len, \"builtins\"))\n-\t\t\tlist_builtins(&list, 0);\n+\t\t\tlist_builtins(&list, exclude_option);\n \t\telse if (match_token(spec, len, \"main\"))\n \t\t\tlist_all_main_cmds(&list);\n \t\telse if (match_token(spec, len, \"others\"))\n@@ -423,6 +428,10 @@ static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n \tconst char *prefix;\n \n \tprefix = NULL;\n+#ifdef NO_UNIX_SOCKETS\n+\tif (p->option & NEED_UNIX_SOCKETS)\n+\t\tdie(_(\"%s is unavailable; there is no UNIX socket support in this build of Git\"), p->cmd);\n+#endif\n \thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n \tif (!help) {\n \t\tif (p->option & RUN_SETUP)\n@@ -513,8 +522,8 @@ static struct cmd_struct commands[] = {\n \t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },\n-\t{ \"credential-cache\", cmd_credential_cache },\n-\t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n+\t{ \"credential-cache\", cmd_credential_cache, NEED_UNIX_SOCKETS },\n+\t{ \"credential-cache--daemon\", cmd_credential_cache_daemon, NEED_UNIX_SOCKETS },\n \t{ \"credential-store\", cmd_credential_store },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n \t{ \"diff\", cmd_diff, NO_PARSEOPT },\ndiff --git a/t/t0012-help.sh b/t/t0012-help.sh\nindex 5679e29c624..2d05dde4b90 100755\n--- a/t/t0012-help.sh\n+++ b/t/t0012-help.sh\n@@ -85,4 +85,14 @@ do\n \t'\n done ] [] [] [--] \n\n are documented in git-rev-list(1)\n\nThis changes that output to simply use \"[]\" instead\nof \"[]\". This accomplishes the same, is more consistent as\n\"git bundle\" and \"git blame\" use the same way of referring to these\noptions now.\n\nThe use of this in \"blame\" dated back to 5817da01434 (git-blame:\nmigrate to incremental parse-option [1/2], 2008-07-08), and the use in\n\"bundle\" to 2e0afafebd8 (Add git-bundle: move objects and\nreferences by archive, 2007-02-22).\n\nOnce we get rid of this special case we can also use usage_msg_opt()\nto emit the error message we'd get on an invalid \"-L \"\nargument, which means we can get rid of the old-style \"blame_usage\"\nvariable.\n\nIt's possible that this change introduce breakage somewhere. We'd only\ncatch these cases at runtime, and the \"git rev-parse --parseopt\"\ncommand is used by shellscripts, see bac199b7b17 (Update\ngit-sh-setup(1) to allow transparent use of git-rev-parse --parseopt,\n2007-11-04). I've grepped the codebase for \"OPTIONS_SPEC\",\n\"char.*\\*.*usage\\[\\]\" etc. I'm fairly sure there no outstanding users\nof this functionality.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/blame.c | 9 +++------\n builtin/rev-parse.c | 3 +++\n parse-options.c | 8 +-------\n t/helper/test-parse-options.c | 2 --\n t/t0040-parse-options.sh | 2 --\n t/t1502-rev-parse-parseopt.sh | 34 ++++++++++++++++++----------------\n 6 files changed, 25 insertions(+), 33 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..45d9873a999 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -29,12 +29,8 @@\n #include \"refs.h\"\n #include \"tag.h\"\n \n-static char blame_usage[] = N_(\"git blame [] [] [] [--] \");\n-\n static const char *blame_opt_usage[] = {\n-\tblame_usage,\n-\t\"\",\n-\tN_(\" are documented in git-rev-list(1)\"),\n+\tN_(\"git blame [] [] [] [--] \"),\n \tNULL\n };\n \n@@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t\t nth_line_cb, &sb, lno, anchor,\n \t\t\t\t &bottom, &top, sb.path,\n \t\t\t\t the_repository->index))\n-\t\t\tusage(blame_usage);\n+\t\t\tusage_msg_opt(_(\"Invalid -L parameters\"),\n+\t\t\t\t blame_opt_usage, options);\n \t\tif ((!lno && (top || bottom)) || lno < bottom)\n \t\t\tdie(Q_(\"file %s has only %lu line\",\n \t\t\t \"file %s has only %lu lines\",\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 22c4e1a4ff0..aeebfd52805 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -436,7 +436,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tfor (;;) {\n \t\tif (strbuf_getline(&sb, stdin) == EOF)\n \t\t\tdie(_(\"premature end of input\"));\n+\t\tif (!sb.len)\n+\t\t\tdie(_(\"empty lines are not permitted before the `--' separator\"));\n \t\tALLOC_GROW(usage, unb + 1, usz);\n+\n \t\tif (!strcmp(\"--\", sb.buf)) {\n \t\t\tif (unb < 1)\n \t\t\t\tdie(_(\"no usage string given before the `--' separator\"));\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..950a8279beb 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -924,18 +924,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n \tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n-\twhile (*usagestr && **usagestr)\n+\twhile (*usagestr) {\n \t\t/*\n \t\t * TRANSLATORS: the colon here should align with the\n \t\t * one in \"usage: %s\" translation.\n \t\t */\n \t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n-\twhile (*usagestr) {\n-\t\tif (**usagestr)\n-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n-\t\telse\n-\t\t\tfputc('\\n', outfile);\n-\t\tusagestr++;\n \t}\n \n \tneed_newline = 1;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 2051ce57db7..e00aef073b0 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -102,8 +102,6 @@ int cmd__parse_options(int argc, const char **argv)\n \tconst char *prefix = \"prefix/\";\n \tconst char *usage[] = {\n \t\t\"test-tool parse-options \",\n-\t\t\"\",\n-\t\t\"A helper function for the parse-options API.\",\n \t\tNULL\n \t};\n \tstruct string_list expect = STRING_LIST_INIT_NODUP;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ad4746d899a..2910874ece5 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -10,8 +10,6 @@ test_description='our own option parser'\n cat >expect <<\\EOF\n usage: test-tool parse-options \n \n- A helper function for the parse-options API.\n-\n --yes get a boolean\n -D, --no-doubt begins with 'no-'\n -B, --no-fear be brave\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex b29563fc997..6badc650d64 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -6,8 +6,6 @@ test_description='test git rev-parse --parseopt'\n test_expect_success 'setup optionspec' '\n \tsed -e \"s/^|//\" >optionspec <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n |h,help show the help\n |\n@@ -41,8 +39,6 @@ EOF\n test_expect_success 'setup optionspec-no-switches' '\n \tsed -e \"s/^|//\" >optionspec_no_switches <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n EOF\n '\n@@ -50,8 +46,6 @@ EOF\n test_expect_success 'setup optionspec-only-hidden-switches' '\n \tsed -e \"s/^|//\" >optionspec_only_hidden_switches <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n |hidden1* A hidden switch\n EOF\n@@ -62,8 +56,6 @@ test_expect_success 'test --parseopt help output' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | -h, --help show the help\n | --foo some nifty option --foo\n | --bar ... some cool option --bar with an argument\n@@ -103,8 +95,6 @@ test_expect_success 'test --parseopt help output no switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n |EOF\n END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&\n@@ -116,8 +106,6 @@ test_expect_success 'test --parseopt help output hidden switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n |EOF\n END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&\n@@ -129,8 +117,6 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | --hidden1 A hidden switch\n |\n |EOF\n@@ -144,8 +130,6 @@ test_expect_success 'test --parseopt invalid switch help output' '\n |error: unknown option `does-not-exist'\\''\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | -h, --help show the help\n | --foo some nifty option --foo\n | --bar ... some cool option --bar with an argument\n@@ -282,4 +266,22 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op\n \ttest_cmp expect output\n '\n \n+test_expect_success 'test --parseopt help output hidden switches' '\n+\tsed -e \"s/^|//\" >optionspec-trailing-line <<-\\EOF &&\n+\t|some-command [options] ...\n+\t|\n+\t|\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tfatal: empty lines are not permitted before the `--'\"'\"' separator\n+\tEOF\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435437","messageId":"patch-v2-4.6-4547cc968b1-20210910T153147Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v2 4/6] built-ins: \"properly\" align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:34Z","receivedAt":"2021-09-10T15:38:53Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\noutput in cases where we continue usage output after a \"\\n\". The \"fix\"\nand \"properly\" scare quotes are because this actually makes things\nworse in some cases, because e.g. in the case of \"git tag -h\" the\n\"\\t\\t\" effectively works around how parse-options.c aligns this\noutput.\n\nBut two wrongs don't make a right, let's \"fix\" this by making it worse\ntemporarily, in anticipation of improving parse-options.c to handle\nthis alignment.\n\nThe issue is that we should have whitespace corresponding to the\nlength of the command name here, e.g. in the case of \"git ls-remote\"\nit should be 14 characters, or the length of \"\"git ls-remote\n\". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\ncharacters are the length of \"usage: \" (and also \" or:\"). So in the C\nlocale the resulting output aligns nicely:\n\n $ git ls-remote -h\n usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n [-q | --quiet] [--exit-code] [--get-url]\n [--symref] [ [...]]\n\nBut that's fragile, we might not be under the C locale. We really\nshould have parse-options.c itself add this padding. In a subsequent\ncommit I'll make it do that.\n\nIn the case of \"tag\" and \"show-branch\" and \"stash save\" the output was\nnot properly aligned, although in the \"git tag\" case it was\nnear-enough (aligned with the \"-\" in \"git tag -l\") to look good,\nassuming C locale & a tab-width of 8. In any case, let's align this in\na way that looks obviously correct when looking at the source itself,\nand then improve parse-options.c itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/ls-remote.c | 4 ++--\n builtin/show-branch.c | 6 +++---\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 ++--\n 4 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex f4fd823af83..318949c3d75 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -7,8 +7,8 @@\n \n static const char * const ls_remote_usage[] = {\n \tN_(\"git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\\n\"\n-\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n-\t \" [--symref] [ [...]]\"),\n+\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n+\t \" [--symref] [ [...]]\"),\n \tNULL\n };\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex d77ce7aeb38..a82cd1534fc 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -11,9 +11,9 @@\n \n static const char* show_branch_usage[] = {\n N_(\"git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\\n\"\n- \"\t\t[--current] [--color[=] | --no-color] [--sparse]\\n\"\n- \"\t\t[--more= | --list | --independent | --merge-base]\\n\"\n- \"\t\t[--no-name | --sha1-name] [--topics] [( | )...]\"),\n+ \" [--current] [--color[=] | --no-color] [--sparse]\\n\"\n+ \" [--more= | --list | --independent | --merge-base]\\n\"\n+ \" [--no-name | --sha1-name] [--topics] [( | )...]\"),\n N_(\"git show-branch (-g | --reflog)[=[,]] [--list] []\"),\n NULL\n };\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..45b19007d7c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {\n \n static const char * const git_stash_save_usage[] = {\n \tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n-\t \" [-u|--include-untracked] [-a|--all] []\"),\n+\t \" [-u|--include-untracked] [-a|--all] []\"),\n \tNULL\n };\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 065b6bf093e..6535ed27ee9 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,10 +23,10 @@\n \n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ]\\n\"\n-\t\t\"\\t\\t []\"),\n+\t \" []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n+\t \" [--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435438","messageId":"patch-v2-5.6-b884b361ace-20210910T153147Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v2 5/6] send-pack: properly use parse_options() API for usage string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:35Z","receivedAt":"2021-09-10T15:38:58Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When \"send-pack\" was changed to use the parse_options() API in\n068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)\nit was made to use one very long line, instead it should split them up\nwith newlines.\n\nFurthermore we were including an inline explanation that you couldn't\ncombine \"--all\" and \"\", but unlike in the \"blame\" case this was\nnot preceded by an empty string.\n\nLet's instead show that --all and can't be combined in the the\nusual language of the usage syntax instead. We can make it clear that\none of the two options \"--foo\" and \"--bar\" is mandatory, but that the\ntwo are mutually exclusive by referring to them as \"( --foo | --bar\n)\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-send-pack.txt | 4 ++--\n builtin/send-pack.c | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 44fd146b912..be41f119740 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=]\n+'git send-pack' [--dry-run] [--force] [--receive-pack=]\n \t\t[--verbose] [--thin] [--atomic]\n \t\t[--[no-]signed|--signed=(true|false|if-asked)]\n-\t\t[:] [...]\n+\t\t[:] (--all | ...)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 729dea1d255..89321423125 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -17,10 +17,10 @@\n #include \"protocol.h\"\n \n static const char * const send_pack_usage[] = {\n-\tN_(\"git send-pack [--all | --mirror] [--dry-run] [--force] \"\n-\t \"[--receive-pack=] [--verbose] [--thin] [--atomic] \"\n-\t \"[:] [...]\\n\"\n-\t \" --all and explicit specification are mutually exclusive.\"),\n+\tN_(\"git send-pack [--mirror] [--dry-run] [--force]\\n\"\n+\t \" [--receive-pack=]\\n\"\n+\t \" [--verbose] [--thin] [--atomic]\\n\"\n+\t \" [:] (--all | ...)\"),\n \tNULL,\n };\n \n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435439","messageId":"patch-v2-6.6-e83d66da6d4-20210910T153147Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v2 6/6] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-10T15:38:36Z","receivedAt":"2021-09-10T15:38:59Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some commands such as \"git stash\" emit continued options output with\ne.g. \"git stash -h\", because usage_with_options_internal() prefixes\nwith its own whitespace the resulting output wasn't properly\naligned. Let's account for the added whitespace, which properly aligns\nthe output.\n\nThe \"git stash\" command has usage output with a N_() translation that\nlegitimately stretches across multiple lines;\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nWe'd like to have that output aligned with the length of the initial\n\"git stash \" output, but since usage_with_options_internal() adds its\nown whitespace prefixing we fell short, before this change we'd emit:\n\n $ git stash -h\n usage: git stash list []\n or: git stash show [] []\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nNow we'll properly emit aligned output. I.e. the last four lines\nabove will instead be (a whitespace-only change to the above):\n\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nWe could also go for an approach where we have the caller support no\npadding of their own, i.e. (same as the first example, except for the\npadding on the second line):\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nBut to do that we'll need to find the length of \"git stash\". We can\ndiscover that from the \"cmd\" in the \"struct cmd_struct\", but there\nmight cases with sub-commands or \"git\" itself taking arguments that\nwould make that non-trivial.\n\nEven if it was I still think this approach is better, because this way\nwe'll get the same legible alignment in the C code. The fact that\nusage_with_options_internal() is adding its own prefix padding is an\nimplementation detail that callers shouldn't need to worry about.\n\nImplementation notes:\n\nWe could skip the string_list_split() with a strchr(str, '\\n') check,\nbut we'd then need to duplicate our state machine for strings that do\nand don't contain a \"\\n\". It's simpler to just always split into a\n\"struct string_list\", even though the common case is that that \"struct\nstring_list\" will contain only one element. This is not\nperformance-sensitive code.\n\nThis change is relatively more complex since I've accounted for making\nit future-proof for RTL translation support. Later in\nusage_with_options_internal() we have some existing padding code\ndating back to d7a38c54a6c (parse-options: be able to generate usages\nautomatically, 2007-10-15) which isn't RTL-safe, but that code would\nbe easy to fix. Let's not introduce new RTL translation problems here.\n\nI'm also adding a check to catch the mistake of needlessly adding a\ntrailing \"\\n\", such as:\n\n\tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] []\\n\"),\n\nOr even a mistake like adding just one \"\\n\" in a string with no other\nnewlines:\n\n\tN_(\"git stash list []\\n\"),\n\nThis catches the cases already tested for in cmd_parseopt(), but this\ncovers the purely C API. As noted a preceding commit that added the\ndie() to cmd_parseopt() I'm fairly confident that this will be\ntriggered by no in-tree user I've missed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 59 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 950a8279beb..ff28869d2c9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -917,19 +917,72 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n \n+\tconst char *usage_prefix = _(\"usage: %s\");\n+\t/*\n+\t * The translation could be anything, but we can count on\n+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n+\t * the translation. So compute the length of the \"usage: \"\n+\t * part. We are assuming that the translator wasn't overly\n+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n+\t * one \"%s\" in \"usage_prefix\" above, so there's no reason to\n+\t * do so even with a RTL language.\n+\t */\n+\tsize_t usage_len = strlen(usage_prefix) - strlen(\"%s\");\n+\t/*\n+\t * TRANSLATORS: the colon here should align with the\n+\t * one in \"usage: %s\" translation.\n+\t */\n+\tconst char *or_prefix = _(\" or: %s\");\n+\n+\t/*\n+\t * TRANSLATORS: You should only need to translate this format\n+\t * string if your language is a RTL language (e.g. Arabic,\n+\t * Hebrew etc.), not if it's a LTR language (e.g. German,\n+\t * Russian, Chinese etc.).\n+\t *\n+\t * When a translated usage string has an embedded \"\\n\" it's\n+\t * because options have wrapped to the next line. The line\n+\t * after the \"\\n\" will then be padded to align with the\n+\t * command name, such as N_(\"git cmd [opt]\\n<8\n+\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n+\t * \"git cmd \".\n+\t *\n+\t * This format string prints out that already-translated\n+\t * line. The \"%*s\" is whitespace padding to account for the\n+\t * padding at the start of the line that we add in this\n+\t * function. The \"%s\" is a line in the (hopefully already\n+\t * translated) N_() usage string, which contained embedded\n+\t * newlines before we split it up.\n+\t */\n+\tconst char *usage_continued = _(\"%*s%s\");\n+\tconst char *prefix = usage_prefix;\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n \tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n-\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n \twhile (*usagestr) {\n-\t\t/*\n-\t\t * TRANSLATORS: the colon here should align with the\n-\t\t * one in \"usage: %s\" translation.\n-\t\t */\n-\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tunsigned int j;\n+\n+\t\tstring_list_split(&list, _(*usagestr++), '\\n', -1);\n+\t\tfor (j = 0; j < list.nr; j++) {\n+\t\t\tconst char *line = list.items[j].string;\n+\n+\t\t\tif (!*line)\n+\t\t\t\tBUG(\"empty or trailing line in usage string\");\n+\n+\t\t\tif (!j)\n+\t\t\t\tfprintf_ln(outfile, prefix, line);\n+\t\t\telse\n+\t\t\t\tfprintf_ln(outfile, usage_continued,\n+\t\t\t\t\t (int)usage_len, \"\", line);\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\n+\t\tprefix = or_prefix;\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.876.g423ac861752\n\n"},{"id":"435489","messageId":"xmqqr1dwotmn.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v2-2.6-d6c44402715-20210910T153147Z-avarab@gmail.com","subject":"Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T00:14:56Z","receivedAt":"2021-09-11T00:15:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the implementation of b5dd96b70ac (make credential helpers\n> builtins, 2020-08-13) to declare in the \"struct cmd_struct\" that\n> NO_UNIX_SOCKETS can't be set.\n\nIt may happen to be that two credential-cache program are both\nrelated to the same CPP macro NO_UNIX_SOCKETS, but I think the\npattern you are tackling with this topic is that a fallback\ndefinition of a function that is designed to always die when invoked\nmisuses the parse-options API. I do not want to see you invent a\nnew bit in cmd_struct for each and every conditional that lets us\ndefine such a die-only fallback implementation.\n\nI may be missing something obvious, but can't the following suffice,\nand if not, why?\n\nThanks.\n\n builtin/credential-cache--daemon.c | 9 ---------\n builtin/credential-cache.c | 9 ---------\n 2 files changed, 18 deletions(-)\n\ndiff --git i/builtin/credential-cache--daemon.c w/builtin/credential-cache--daemon.c\nindex 4c6c89ab0d..f11a89a89b 100644\n--- i/builtin/credential-cache--daemon.c\n+++ w/builtin/credential-cache--daemon.c\n@@ -304,15 +304,6 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n \n int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n {\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache--daemon [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache--daemon is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n \tdie(_(\"credential-cache--daemon unavailable; no unix socket support\"));\n }\n \ndiff --git i/builtin/credential-cache.c w/builtin/credential-cache.c\nindex e8a7415747..dd794f84ce 100644\n--- i/builtin/credential-cache.c\n+++ w/builtin/credential-cache.c\n@@ -142,15 +142,6 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n \n int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n {\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n \tdie(_(\"credential-cache unavailable; no unix socket support\"));\n }\n \n\n"},{"id":"435490","messageId":"xmqqlf44otbz.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v2-3.6-11f4a119563-20210910T153147Z-avarab@gmail.com","subject":"Re: [PATCH v2 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T00:21:20Z","receivedAt":"2021-09-11T00:21:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> The strings in the the \"usagestr\" array have special handling for the\n> empty string dating back to f389c808b67 (Rework make_usage to print\n> the usage message immediately, 2007-10-14).\n>\n> We'll prefix all strings after the first one with \"or: \". Then if we\n> encountered a \"\" we'll emit all strings after that point verbatim\n> without any \"or: \" prefixing.\n>\n> This gets rid of that special case, which was added in\n> f389c808b67 (Rework make_usage to print the usage message immediately,\n> 2007-10-14). It was only used \"blame\" (the \"credential-cache*\" use of\n\nused *by* \"blame\"\n\n> it was removed in the preceding commit). Before this change we'd emit:\n>\n> $ git blame -h\n> usage: git blame [] [] [] [--] \n>\n> are documented in git-rev-list(1)\n\nThe most crucial information is missing. It may be shared with the\nprevious step but it is even worse in this step.\n\nWhy is this loss of feature a desirable thing in the first place?\n\nWhat are we gaining by breaking the \"after listing the alternative\nforms concatenated with 'or:', we can give a general description,\nbefore going onto the list of options\"?\n\nWithout that explained, the remainder of the proposed log message\nreads more like an excuse for breaking the feature, justifying that\nthe loss of feature can be worked around, than the description of a\nsolution.\n\nThanks.\n"},{"id":"435492","messageId":"xmqqfsucot4f.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v2-4.6-4547cc968b1-20210910T153147Z-avarab@gmail.com","subject":"Re: [PATCH v2 4/6] built-ins: \"properly\" align continued usage output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T00:25:52Z","receivedAt":"2021-09-11T00:25:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\n> output in cases where we continue usage output after a \"\\n\". The \"fix\"\n> and \"properly\" scare quotes are because this actually makes things\n\nThe \"A\" and \"B\" *in* scare quotes?\n\n> The issue is that we should have whitespace corresponding to the\n> length of the command name here, e.g. in the case of \"git ls-remote\"\n> it should be 14 characters, or the length of \"\"git ls-remote\n> \". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\n> characters are the length of \"usage: \" (and also \" or:\"). So in the C\n> locale the resulting output aligns nicely:\n>\n> $ git ls-remote -h\n> usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n> [-q | --quiet] [--exit-code] [--get-url]\n> [--symref] [ [...]]\n\nIsn't this aiming for a wrong end goal? With an overly long\nsubcommand name, we'd end up with overly deep indentation of the\nsubsequent lines (hence too narrow a space to fit the options).\n\nRather, wouldn't it be better to aim for a consistent and wide\nenough indentation, like say two tabs, everywhere, no matter how\nlong the subcommand name is?\n\nThanks.\n\n"},{"id":"435503","messageId":"87v937akzw.fsf@evledraar.gmail.com","threadId":"56423","inReplyTo":"xmqqfsucot4f.fsf@gitster.g","subject":"Re: [PATCH v2 4/6] built-ins: \"properly\" align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T02:40:44Z","receivedAt":"2021-09-11T02:45:12Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\n>> output in cases where we continue usage output after a \"\\n\". The \"fix\"\n>> and \"properly\" scare quotes are because this actually makes things\n>\n> The \"A\" and \"B\" *in* scare quotes?\n\nPerhaps I should take a stab at rewriting this, something like:\n\n Let's start by aligning the strings in the C code so that their\n indentation is correct, in the end it'll still be off due to the\n indentation parse_options() itself adds, but that'll be fixed in a\n subsequent commit. That subsequent commit relies on the indentation\n in the source being consistent.\n\n>> The issue is that we should have whitespace corresponding to the\n>> length of the command name here, e.g. in the case of \"git ls-remote\"\n>> it should be 14 characters, or the length of \"\"git ls-remote\n>> \". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\n>> characters are the length of \"usage: \" (and also \" or:\"). So in the C\n>> locale the resulting output aligns nicely:\n>>\n>> $ git ls-remote -h\n>> usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n>> [-q | --quiet] [--exit-code] [--get-url]\n>> [--symref] [ [...]]\n>\n> Isn't this aiming for a wrong end goal? With an overly long\n> subcommand name, we'd end up with overly deep indentation of the\n> subsequent lines (hence too narrow a space to fit the options).\n>\n> Rather, wouldn't it be better to aim for a consistent and wide\n> enough indentation, like say two tabs, everywhere, no matter how\n> long the subcommand name is?\n\nSimilarly to the discussion on another patch about whether we should\nconvert these long lines to [] or not, I think this is really\noutside the scope of this series.\n\nIn this particular case the indentation remains exactly the same before\nthis series and after, for the other cases there's usually a change of\n1-3 spaces or so, i.e. they were slightly off.\n\nShould we have no indentation at all? Auto re-indent or whatever? Maybe,\nbut let's start by narrowly fixing code that's clearly a bit off in\nfunctionality from what it's doing & intending to do right now.\n"},{"id":"435504","messageId":"87r1dvaksr.fsf@evledraar.gmail.com","threadId":"56423","inReplyTo":"xmqqlf44otbz.fsf@gitster.g","subject":"Re: [PATCH v2 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T02:46:39Z","receivedAt":"2021-09-11T02:49:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> The strings in the the \"usagestr\" array have special handling for the\n>> empty string dating back to f389c808b67 (Rework make_usage to print\n>> the usage message immediately, 2007-10-14).\n>>\n>> We'll prefix all strings after the first one with \"or: \". Then if we\n>> encountered a \"\" we'll emit all strings after that point verbatim\n>> without any \"or: \" prefixing.\n>>\n>> This gets rid of that special case, which was added in\n>> f389c808b67 (Rework make_usage to print the usage message immediately,\n>> 2007-10-14). It was only used \"blame\" (the \"credential-cache*\" use of\n>\n> used *by* \"blame\"\n>\n>> it was removed in the preceding commit). Before this change we'd emit:\n>>\n>> $ git blame -h\n>> usage: git blame [] [] [] [--] \n>>\n>> are documented in git-rev-list(1)\n>\n> The most crucial information is missing. It may be shared with the\n> previous step but it is even worse in this step.\n>\n> Why is this loss of feature a desirable thing in the first place?\n>\n> What are we gaining by breaking the \"after listing the alternative\n> forms concatenated with 'or:', we can give a general description,\n> before going onto the list of options\"?\n>\n> Without that explained, the remainder of the proposed log message\n> reads more like an excuse for breaking the feature, justifying that\n> the loss of feature can be worked around, than the description of a\n> solution.\n\nMaking it the same as how \"git bundle\" describes it doesn't seem\nanywhere close to breaking it, and that also goes to the point you had\nabout brevity on another patch.\n\nI think that passing custom or generated help advice here might make\nsense generally, i.e. we might eventually want to expand that to all\nbuilt-in as part of improving the -h and --help output, see e.g. the\nnote at the bottom of \"git --help\".\n\nBut if we do that let's do that with an API where we simply pass this\ncustom string in as another parameter to the function, rather than\nhaving a state machine to parse it out of the array we use for the\n\"usage:\" and \"or:\" list of lines.\n"},{"id":"435505","messageId":"87mtojaklj.fsf@evledraar.gmail.com","threadId":"56423","inReplyTo":"xmqqr1dwotmn.fsf@gitster.g","subject":"Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T02:50:54Z","receivedAt":"2021-09-11T02:53:49Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> Change the implementation of b5dd96b70ac (make credential helpers\n>> builtins, 2020-08-13) to declare in the \"struct cmd_struct\" that\n>> NO_UNIX_SOCKETS can't be set.\n>\n> It may happen to be that two credential-cache program are both\n> related to the same CPP macro NO_UNIX_SOCKETS, but I think the\n> pattern you are tackling with this topic is that a fallback\n> definition of a function that is designed to always die when invoked\n> misuses the parse-options API. I do not want to see you invent a\n> new bit in cmd_struct for each and every conditional that lets us\n> define such a die-only fallback implementation.\n>\n> I may be missing something obvious, but can't the following suffice,\n> and if not, why?\n\nI think this is covered if you go on to read the rest of the commit\nmessage, i.e. yes we could, but the trade-off is making the test and\nusers that might want to use --list-cmds=builtins hardcode these two as\nspecial cases under the relevant prereq.\n\nHence doing this in the main git.c dispatch mechanism, if we can't ever\ndo anything useful with these it seems best to mark them as such at\ncompile-time in that dispatch mechanism.\n"},{"id":"435515","messageId":"YTwm57pp/cvfR5rg@carlos-mbp.lan","threadId":"56423","inReplyTo":"87mtojaklj.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Carlo Marcelo Arenas Belón","fromEmail":"carenas@gmail.com","sentAt":"2021-09-11T03:47:51Z","receivedAt":"2021-09-11T03:47:59Z","isPatch":true,"sender":{"key":"carenas@gmail.com","avatar":"https://avatars.githubusercontent.com/u/76036?v=4"},"body":"Sorry, if this is silly, but why is this not better (at least as a starting\npoint, since it obviously will need more work?\n\nUndefined symbols for architecture x86_64:\n \"_cmd_credential_cache\", referenced from:\n _commands in git.o\n \"_cmd_credential_cache_daemon\", referenced from:\n _commands in git.o\n\nCarlo\n---- >8 ----\ndiff --git a/Makefile b/Makefile\nindex 44734f916a..2a60685c37 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o\n BUILTIN_OBJS += builtin/commit.o\n BUILTIN_OBJS += builtin/config.o\n BUILTIN_OBJS += builtin/count-objects.o\n+ifndef NO_UNIX_SOCKETS\n BUILTIN_OBJS += builtin/credential-cache--daemon.o\n BUILTIN_OBJS += builtin/credential-cache.o\n+endif\n BUILTIN_OBJS += builtin/credential-store.o\n BUILTIN_OBJS += builtin/credential.o\n BUILTIN_OBJS += builtin/describe.o\n"},{"id":"435517","messageId":"xmqqee9vmyhp.fsf@gitster.g","threadId":"56423","inReplyTo":"YTwm57pp/cvfR5rg@carlos-mbp.lan","subject":"Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T06:12:50Z","receivedAt":"2021-09-11T06:12:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Carlo Marcelo Arenas Belón writes:\n\n> Sorry, if this is silly, but why is this not better (at least as a starting\n> point, since it obviously will need more work?\n>\n> Undefined symbols for architecture x86_64:\n> \"_cmd_credential_cache\", referenced from:\n> _commands in git.o\n> \"_cmd_credential_cache_daemon\", referenced from:\n> _commands in git.o\n>\n> Carlo\n> ---- >8 ----\n> diff --git a/Makefile b/Makefile\n> index 44734f916a..2a60685c37 100644\n> --- a/Makefile\n> +++ b/Makefile\n> @@ -1098,8 +1098,10 @@ BUILTIN_OBJS += builtin/commit-tree.o\n> BUILTIN_OBJS += builtin/commit.o\n> BUILTIN_OBJS += builtin/config.o\n> BUILTIN_OBJS += builtin/count-objects.o\n> +ifndef NO_UNIX_SOCKETS\n> BUILTIN_OBJS += builtin/credential-cache--daemon.o\n> BUILTIN_OBJS += builtin/credential-cache.o\n> +endif\n> BUILTIN_OBJS += builtin/credential-store.o\n> BUILTIN_OBJS += builtin/credential.o\n> BUILTIN_OBJS += builtin/describe.o\n\nThat smells to show a much better direction to me ;-)\n"},{"id":"435519","messageId":"xmqq8s03mwn1.fsf@gitster.g","threadId":"56423","inReplyTo":"87r1dvaksr.fsf@evledraar.gmail.com","subject":"Re: [PATCH v2 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-11T06:52:50Z","receivedAt":"2021-09-11T06:52:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> But if we do that let's do that with an API where we simply pass this\n> custom string in as another parameter to the function, rather than\n> having a state machine to parse it out of the array we use for the\n> \"usage:\" and \"or:\" list of lines.\n\nSorry, but I do not follow. A perfectly fine way to encode the\nthree (usage:, or:, and text) kind of information in a single array\nis what this step is breaking, and then you propose to keep the two\nstill in that same array, with only the third kind kicked out to\n\"another parameter\", which does not make much sense to me.\n"},{"id":"435521","messageId":"87ilz7a88o.fsf@evledraar.gmail.com","threadId":"56423","inReplyTo":"YTwm57pp/cvfR5rg@carlos-mbp.lan","subject":"Re: [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T07:16:56Z","receivedAt":"2021-09-11T07:20:47Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Fri, Sep 10 2021, Carlo Marcelo Arenas Belón wrote:\n\n> Sorry, if this is silly, but why is this not better (at least as a starting\n> point, since it obviously will need more work?\n>\n> Undefined symbols for architecture x86_64:\n> \"_cmd_credential_cache\", referenced from:\n> _commands in git.o\n> \"_cmd_credential_cache_daemon\", referenced from:\n> _commands in git.o\n\nI'm fine with it. I'm just trying to get to the end-goal of fixing the\nformatting issue in \"-h\" output in the simplest way possible.\n\nWhat you're suggesting would be effectively a revert of various parts of\nb5dd96b70ac (make credential helpers builtins, 2020-08-13), which is\nbehavior I assumed we'd like to retain.\n\nBut yes, if changing that behavior is OK then this is simpler.\n\nAnyway, all of this in v2 was in response to feedback on v1 to make the\nv1 function in parse_options() easier, see summary at\nhttps://lore.kernel.org/git/cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com/\n\nBut it does look like Junio would like to keep the only \"real\" in-tree\nuser of the current API in builtin/blame.c, so at that point removing\nthese by any method becomes a moot point, so I think I'l try some\nalternate approach based on v1 that doesn't touch these at all again.\n"},{"id":"435522","messageId":"CAPig+cSrjx=okjqyOtD7=wwcAt2QZ30OMr8rKNoy9Z42_Ux4ew@mail.gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"Re: [PATCH v2 0/6] parse-options: properly align continued usage output & related","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-11T07:41:09Z","receivedAt":"2021-09-11T07:41:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, Sep 10, 2021 at 11:38 AM Ævar Arnfjörð Bjarmason\n wrote:\n> This series changes usage_with_options_internal() in parse-options.c\n> to properly align continued \"\\n\" usage output.\n>\n> This v2 now also gets rid of the support for \"\" in the usage string\n> array. Eric Sunshine had ideas[1] for how to simplify the code in v1,\n> along with a suggestion that we could just get rid of the \"\" from\n> \"builtin/blame.c\".\n\nTo be clear, I only asked whether you really needed to add support for\nembedded newlines in free-form usage strings following a \"\" line; I\ndidn't actually suggest dropping support for free-form text following\na \"\" line. Perhaps I didn't articulate that well enough, though, in my\nreview(?).\n\nAside: I did, however, _think_ about suggesting that free-form text\nsupport be dropped since I only found one consumer of the feature,\nhowever, I decided not to make that suggestion since the feature\nseemed like it had potential value and the current implementation is\ngood enough and simple enough (though undocumented, as far as I can\ntell).\n"},{"id":"435583","messageId":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com","subject":"[PATCH v3 0/6] parse-options: properly align continued usage output & related","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:08:59Z","receivedAt":"2021-09-11T19:10:32Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes usage_with_options_internal() in parse-options.c\nto properly align continued \"\\n\" usage output.\n\nI think this v3 will address the feedback on v2. I'm not confident the\ndirection will be agreed with, but let's see.\n\nI've taken Carlo Marcelo Arenas Belón's suggestion of just not\nbuilding the credential-cache built-ins under NO_UNIX_SOCKETS, which\nI'm much happier with as a solution than my v2, I just thought Junio\nwould veto it, but it seems he likes the direction.\n\nThe removal of the undocumented API from \"git blame\" is now its own\ncommit, which I think does a good job of justifying itself.\n\nThen the justification in \"parse-options: stop supporting\" is updated\n& much improved, i.e. I've had my eye on migrating git.c to\nparse_options(), which wants to make use of this sort of API, but\ncan't because of the limitations of the API being removed here. I\nthink per the justification in that commit it makes sense to remove\nthis here, and just add something like it back later.\n\nÆvar Arnfjörð Bjarmason (6):\n credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS\n blame: replace usage end blurb with better option spec\n parse-options: stop supporting \"\" in the usagestr array\n built-ins: \"properly\" align continued usage output\n send-pack: properly use parse_options() API for usage string\n parse-options: properly align continued usage output\n\n Documentation/git-send-pack.txt | 4 +-\n Makefile | 3 ++\n builtin.h | 2 +\n builtin/blame.c | 9 ++--\n builtin/credential-cache--daemon.c | 21 ---------\n builtin/credential-cache.c | 21 ---------\n builtin/ls-remote.c | 4 +-\n builtin/rev-parse.c | 3 ++\n builtin/send-pack.c | 8 ++--\n builtin/show-branch.c | 6 +--\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 +-\n git.c | 2 +\n parse-options.c | 71 +++++++++++++++++++++++++-----\n t/helper/test-parse-options.c | 2 -\n t/t0040-parse-options.sh | 2 -\n t/t1502-rev-parse-parseopt.sh | 34 +++++++-------\n 17 files changed, 104 insertions(+), 94 deletions(-)\n\nRange-diff against v2:\n1: 9e8facb6f8c < -: ----------- test-lib.sh: add a UNIX_SOCKETS prerequisite\n2: d6c44402715 ! 1: ad857e80fd8 git.c: add a NEED_UNIX_SOCKETS option for built-ins\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - git.c: add a NEED_UNIX_SOCKETS option for built-ins\n + credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS\n \n Change the implementation of b5dd96b70ac (make credential helpers\n - builtins, 2020-08-13) to declare in the \"struct cmd_struct\" that\n - NO_UNIX_SOCKETS can't be set.\n + builtins, 2020-08-13) to not build these at all under\n + NO_UNIX_SOCKETS.\n \n - This is one of two in-tree users for the empty lines in\n - parse_options() usage, getting rid of that is the main motivation for\n - this, but it also doesn't make sense to emit these sorts of usage\n - messages just to appease t0012-help.sh, which seemingly b5dd96b70ac\n - aimed to do.\n + This is the easiest way to get rid of one out of two users of an\n + obscure parse_options() API I'm trying to get rid of. It does mean\n + that the goal of emitting a custom error message in b5dd96b70ac is\n + being eliminated, but per [1] that seems to be an OK direction to go\n + in.\n \n - I.e. these commands don't support \"[options]\", or \"\" so\n - emitting that at the beginning is incorrect. We should just die right\n - away.\n + By not compiling it at all it won't be included in the \"struct\n + cmd_struct\", and therefore will also be omitted from\n + \"--list-cmds=builtins\".\n \n - The existing code also had the edge case of not emitting the die()\n - message if a \"-h\" argument was given, since parse_options() will\n - handle the exit() itself in that case. We could feed it\n - PARSE_OPT_NO_INTERNAL_HELP, but this is better.\n -\n - By moving this up to the \"struct cmd_struct\" we can also skip these in\n - --list-cmds=builtins instead, as noted above we shouldn't be exiting\n - with code 129 in these cases.\n + 1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com/\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n + ## Makefile ##\n +@@ Makefile: BUILTIN_OBJS += builtin/commit-tree.o\n + BUILTIN_OBJS += builtin/commit.o\n + BUILTIN_OBJS += builtin/config.o\n + BUILTIN_OBJS += builtin/count-objects.o\n ++ifndef NO_UNIX_SOCKETS\n + BUILTIN_OBJS += builtin/credential-cache--daemon.o\n + BUILTIN_OBJS += builtin/credential-cache.o\n ++endif\n + BUILTIN_OBJS += builtin/credential-store.o\n + BUILTIN_OBJS += builtin/credential.o\n + BUILTIN_OBJS += builtin/describe.o\n +@@ Makefile: ifdef NO_INET_PTON\n + endif\n + ifdef NO_UNIX_SOCKETS\n + \tBASIC_CFLAGS += -DNO_UNIX_SOCKETS\n ++\tEXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon\n + else\n + \tLIB_OBJS += unix-socket.o\n + \tLIB_OBJS += unix-stream-server.o\n +\n ## builtin.h ##\n -@@\n - *\tmore informed decision, e.g., by ignoring `pager.` for\n - *\tcertain subcommands.\n - *\n -+ * `NEED_UNIX_SOCKETS`:\n -+ *\n -+ *\tThis built-in will not work if NO_UNIX_SOCKETS is defined. It\n -+ *\twill be recognized for emitting error messages, but won't be\n -+ *\tlisted in --list-cmds=builtins.\n -+ *\n - * . Add `builtin/foo.o` to `BUILTIN_OBJS` in `Makefile`.\n - *\n - * Additionally, if `foo` is a new command, there are 4 more things to do:\n +@@ builtin.h: int cmd_commit_tree(int argc, const char **argv, const char *prefix);\n + int cmd_config(int argc, const char **argv, const char *prefix);\n + int cmd_count_objects(int argc, const char **argv, const char *prefix);\n + int cmd_credential(int argc, const char **argv, const char *prefix);\n ++#ifndef NO_UNIX_SOCKETS\n + int cmd_credential_cache(int argc, const char **argv, const char *prefix);\n + int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);\n ++#endif\n + int cmd_credential_store(int argc, const char **argv, const char *prefix);\n + int cmd_describe(int argc, const char **argv, const char *prefix);\n + int cmd_diff_files(int argc, const char **argv, const char *prefix);\n \n ## builtin/credential-cache--daemon.c ##\n +@@\n + #include \"builtin.h\"\n + #include \"parse-options.h\"\n +-\n +-#ifndef NO_UNIX_SOCKETS\n +-\n + #include \"config.h\"\n + #include \"tempfile.h\"\n + #include \"credential.h\"\n @@ builtin/credential-cache--daemon.c: int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n \n - int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n - {\n + \treturn 0;\n + }\n +-\n +-#else\n +-\n +-int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n +-{\n -\tconst char * const usage[] = {\n -\t\t\"git credential-cache--daemon [options] \",\n -\t\t\"\",\n @@ builtin/credential-cache--daemon.c: int cmd_credential_cache_daemon(int argc, co\n -\n -\targc = parse_options(argc, argv, prefix, options, usage, 0);\n -\tdie(_(\"credential-cache--daemon unavailable; no unix socket support\"));\n -+\tBUG(\"should not be called under NO_UNIX_SOCKETS\");\n - }\n - \n - #endif /* NO_UNIX_SOCKET */\n +-}\n +-\n +-#endif /* NO_UNIX_SOCKET */\n \n ## builtin/credential-cache.c ##\n +@@\n + #include \"builtin.h\"\n + #include \"parse-options.h\"\n +-\n +-#ifndef NO_UNIX_SOCKETS\n +-\n + #include \"credential.h\"\n + #include \"string-list.h\"\n + #include \"unix-socket.h\"\n @@ builtin/credential-cache.c: int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n \n - int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n - {\n + \treturn 0;\n + }\n +-\n +-#else\n +-\n +-int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n +-{\n -\tconst char * const usage[] = {\n -\t\t\"git credential-cache [options] \",\n -\t\t\"\",\n @@ builtin/credential-cache.c: int cmd_credential_cache(int argc, const char **argv\n -\n -\targc = parse_options(argc, argv, prefix, options, usage, 0);\n -\tdie(_(\"credential-cache unavailable; no unix socket support\"));\n -+\tBUG(\"should not be called under NO_UNIX_SOCKETS\");\n - }\n - \n - #endif /* NO_UNIX_SOCKETS */\n +-}\n +-\n +-#endif /* NO_UNIX_SOCKETS */\n \n ## git.c ##\n -@@\n - #define SUPPORT_SUPER_PREFIX\t(1<<4)\n - #define DELAY_PAGER_CONFIG\t(1<<5)\n - #define NO_PARSEOPT\t\t(1<<6) /* parse-options is not used */\n -+#define NEED_UNIX_SOCKETS\t(1<<7) /* Works unless -DNO_UNIX_SOCKETS */\n - \n - struct cmd_struct {\n - \tconst char *cmd;\n -@@ git.c: static int list_cmds(const char *spec)\n - \tstruct string_list list = STRING_LIST_INIT_DUP;\n - \tint i;\n - \tint nongit;\n -+\tunsigned int exclude_option = 0;\n -+#ifdef NO_UNIX_SOCKETS\n -+\texclude_option |= NEED_UNIX_SOCKETS;\n -+#endif\n - \n - \t/*\n - \t* Set up the repository so we can pick up any repo-level config (like\n -@@ git.c: static int list_cmds(const char *spec)\n - \t\tint len = sep - spec;\n - \n - \t\tif (match_token(spec, len, \"builtins\"))\n --\t\t\tlist_builtins(&list, 0);\n -+\t\t\tlist_builtins(&list, exclude_option);\n - \t\telse if (match_token(spec, len, \"main\"))\n - \t\t\tlist_all_main_cmds(&list);\n - \t\telse if (match_token(spec, len, \"others\"))\n -@@ git.c: static int run_builtin(struct cmd_struct *p, int argc, const char **argv)\n - \tconst char *prefix;\n - \n - \tprefix = NULL;\n -+#ifdef NO_UNIX_SOCKETS\n -+\tif (p->option & NEED_UNIX_SOCKETS)\n -+\t\tdie(_(\"%s is unavailable; there is no UNIX socket support in this build of Git\"), p->cmd);\n -+#endif\n - \thelp = argc == 2 && !strcmp(argv[1], \"-h\");\n - \tif (!help) {\n - \t\tif (p->option & RUN_SETUP)\n @@ git.c: static struct cmd_struct commands[] = {\n \t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },\n --\t{ \"credential-cache\", cmd_credential_cache },\n --\t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n -+\t{ \"credential-cache\", cmd_credential_cache, NEED_UNIX_SOCKETS },\n -+\t{ \"credential-cache--daemon\", cmd_credential_cache_daemon, NEED_UNIX_SOCKETS },\n ++#ifndef NO_UNIX_SOCKETS\n + \t{ \"credential-cache\", cmd_credential_cache },\n + \t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n ++#endif\n \t{ \"credential-store\", cmd_credential_store },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n \t{ \"diff\", cmd_diff, NO_PARSEOPT },\n -\n - ## t/t0012-help.sh ##\n -@@ t/t0012-help.sh: do\n - \t'\n - done 2: 036eb0efb5b blame: replace usage end blurb with better option spec\n3: 11f4a119563 ! 3: e23a8231f38 parse-options: stop supporting \"\" in the usagestr array\n @@ Commit message\n encountered a \"\" we'll emit all strings after that point verbatim\n without any \"or: \" prefixing.\n \n - This gets rid of that special case, which was added in\n - f389c808b67 (Rework make_usage to print the usage message immediately,\n - 2007-10-14). It was only used \"blame\" (the \"credential-cache*\" use of\n - it was removed in the preceding commit). Before this change we'd emit:\n + In the preceding commits we got rid of the two users of this\n + undocumented part of the API. Let's remove it in preparation for\n + improving the output emitted by usage_with_options_internal().\n \n - $ git blame -h\n - usage: git blame [] [] [] [--] \n + I think we might want to use this in the future, but in that case\n + we'll be much better off with an API that emulates the\n + non-parse_options() way that git.c does this.\n \n - are documented in git-rev-list(1)\n + That git.c code uses a separate \"git_usage_string\" and\n + \"git_more_info_string\". See b7d9681974e (Print info about \"git help\n + COMMAND\" on git's main usage pages, 2008-06-06). By splitting the two\n + up we can emit something in the middle, as indeed git.c does.\n \n - This changes that output to simply use \"[]\" instead\n - of \"[]\". This accomplishes the same, is more consistent as\n - \"git bundle\" and \"git blame\" use the same way of referring to these\n - options now.\n + I'd like our \"git -h\" info to be more helpful, and I'd also like\n + parse_options() to handle the \"git\" command itself, because of the\n + limitations of how this was done in usage_with_options_internal() we\n + couldn't migrate a caller like git.c to parse_options().\n \n - The use of this in \"blame\" dated back to 5817da01434 (git-blame:\n - migrate to incremental parse-option [1/2], 2008-07-08), and the use in\n - \"bundle\" to 2e0afafebd8 (Add git-bundle: move objects and\n - references by archive, 2007-02-22).\n -\n - Once we get rid of this special case we can also use usage_msg_opt()\n - to emit the error message we'd get on an invalid \"-L \"\n - argument, which means we can get rid of the old-style \"blame_usage\"\n - variable.\n + So let's just remove this for now, it has no users, and once we want\n + to do this again we can simply add another argument to the relevant\n + functions, or otherwise hook into things so that we can print\n + something at the end and/or middle.\n \n It's possible that this change introduce breakage somewhere. We'd only\n catch these cases at runtime, and the \"git rev-parse --parseopt\"\n @@ Commit message\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## builtin/blame.c ##\n -@@\n - #include \"refs.h\"\n - #include \"tag.h\"\n - \n --static char blame_usage[] = N_(\"git blame [] [] [] [--] \");\n --\n - static const char *blame_opt_usage[] = {\n --\tblame_usage,\n --\t\"\",\n --\tN_(\" are documented in git-rev-list(1)\"),\n -+\tN_(\"git blame [] [] [] [--] \"),\n - \tNULL\n - };\n - \n -@@ builtin/blame.c: int cmd_blame(int argc, const char **argv, const char *prefix)\n - \t\t\t\t nth_line_cb, &sb, lno, anchor,\n - \t\t\t\t &bottom, &top, sb.path,\n - \t\t\t\t the_repository->index))\n --\t\t\tusage(blame_usage);\n -+\t\t\tusage_msg_opt(_(\"Invalid -L parameters\"),\n -+\t\t\t\t blame_opt_usage, options);\n - \t\tif ((!lno && (top || bottom)) || lno < bottom)\n - \t\t\tdie(Q_(\"file %s has only %lu line\",\n - \t\t\t \"file %s has only %lu lines\",\n -\n ## builtin/rev-parse.c ##\n @@ builtin/rev-parse.c: static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tfor (;;) {\n4: 4547cc968b1 = 4: 5638d2bc832 built-ins: \"properly\" align continued usage output\n5: b884b361ace = 5: 821e5e14132 send-pack: properly use parse_options() API for usage string\n6: e83d66da6d4 = 6: 0df2840ce4e parse-options: properly align continued usage output\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435584","messageId":"patch-v3-1.6-ad857e80fd8-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:00Z","receivedAt":"2021-09-11T19:10:34Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the implementation of b5dd96b70ac (make credential helpers\nbuiltins, 2020-08-13) to not build these at all under\nNO_UNIX_SOCKETS.\n\nThis is the easiest way to get rid of one out of two users of an\nobscure parse_options() API I'm trying to get rid of. It does mean\nthat the goal of emitting a custom error message in b5dd96b70ac is\nbeing eliminated, but per [1] that seems to be an OK direction to go\nin.\n\nBy not compiling it at all it won't be included in the \"struct\ncmd_struct\", and therefore will also be omitted from\n\"--list-cmds=builtins\".\n\n1. https://lore.kernel.org/git/cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com/\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Makefile | 3 +++\n builtin.h | 2 ++\n builtin/credential-cache--daemon.c | 21 ---------------------\n builtin/credential-cache.c | 21 ---------------------\n git.c | 2 ++\n 5 files changed, 7 insertions(+), 42 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 429c276058d..ecde3367fa2 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -1086,8 +1086,10 @@ BUILTIN_OBJS += builtin/commit-tree.o\n BUILTIN_OBJS += builtin/commit.o\n BUILTIN_OBJS += builtin/config.o\n BUILTIN_OBJS += builtin/count-objects.o\n+ifndef NO_UNIX_SOCKETS\n BUILTIN_OBJS += builtin/credential-cache--daemon.o\n BUILTIN_OBJS += builtin/credential-cache.o\n+endif\n BUILTIN_OBJS += builtin/credential-store.o\n BUILTIN_OBJS += builtin/credential.o\n BUILTIN_OBJS += builtin/describe.o\n@@ -1693,6 +1695,7 @@ ifdef NO_INET_PTON\n endif\n ifdef NO_UNIX_SOCKETS\n \tBASIC_CFLAGS += -DNO_UNIX_SOCKETS\n+\tEXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon\n else\n \tLIB_OBJS += unix-socket.o\n \tLIB_OBJS += unix-stream-server.o\ndiff --git a/builtin.h b/builtin.h\nindex 16ecd5586f0..9b3f173bd7c 100644\n--- a/builtin.h\n+++ b/builtin.h\n@@ -140,8 +140,10 @@ int cmd_commit_tree(int argc, const char **argv, const char *prefix);\n int cmd_config(int argc, const char **argv, const char *prefix);\n int cmd_count_objects(int argc, const char **argv, const char *prefix);\n int cmd_credential(int argc, const char **argv, const char *prefix);\n+#ifndef NO_UNIX_SOCKETS\n int cmd_credential_cache(int argc, const char **argv, const char *prefix);\n int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix);\n+#endif\n int cmd_credential_store(int argc, const char **argv, const char *prefix);\n int cmd_describe(int argc, const char **argv, const char *prefix);\n int cmd_diff_files(int argc, const char **argv, const char *prefix);\ndiff --git a/builtin/credential-cache--daemon.c b/builtin/credential-cache--daemon.c\nindex 4c6c89ab0de..7785412dea4 100644\n--- a/builtin/credential-cache--daemon.c\n+++ b/builtin/credential-cache--daemon.c\n@@ -1,8 +1,5 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n-\n-#ifndef NO_UNIX_SOCKETS\n-\n #include \"config.h\"\n #include \"tempfile.h\"\n #include \"credential.h\"\n@@ -299,21 +296,3 @@ int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n \n \treturn 0;\n }\n-\n-#else\n-\n-int cmd_credential_cache_daemon(int argc, const char **argv, const char *prefix)\n-{\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache--daemon [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache--daemon is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n-\tdie(_(\"credential-cache--daemon unavailable; no unix socket support\"));\n-}\n-\n-#endif /* NO_UNIX_SOCKET */\ndiff --git a/builtin/credential-cache.c b/builtin/credential-cache.c\nindex e8a74157471..64942978650 100644\n--- a/builtin/credential-cache.c\n+++ b/builtin/credential-cache.c\n@@ -1,8 +1,5 @@\n #include \"builtin.h\"\n #include \"parse-options.h\"\n-\n-#ifndef NO_UNIX_SOCKETS\n-\n #include \"credential.h\"\n #include \"string-list.h\"\n #include \"unix-socket.h\"\n@@ -137,21 +134,3 @@ int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n \n \treturn 0;\n }\n-\n-#else\n-\n-int cmd_credential_cache(int argc, const char **argv, const char *prefix)\n-{\n-\tconst char * const usage[] = {\n-\t\t\"git credential-cache [options] \",\n-\t\t\"\",\n-\t\t\"credential-cache is disabled in this build of Git\",\n-\t\tNULL\n-\t};\n-\tstruct option options[] = { OPT_END() };\n-\n-\targc = parse_options(argc, argv, prefix, options, usage, 0);\n-\tdie(_(\"credential-cache unavailable; no unix socket support\"));\n-}\n-\n-#endif /* NO_UNIX_SOCKETS */\ndiff --git a/git.c b/git.c\nindex 18bed9a9964..7c696e06ecf 100644\n--- a/git.c\n+++ b/git.c\n@@ -513,8 +513,10 @@ static struct cmd_struct commands[] = {\n \t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },\n+#ifndef NO_UNIX_SOCKETS\n \t{ \"credential-cache\", cmd_credential_cache },\n \t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n+#endif\n \t{ \"credential-store\", cmd_credential_store },\n \t{ \"describe\", cmd_describe, RUN_SETUP },\n \t{ \"diff\", cmd_diff, NO_PARSEOPT },\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435585","messageId":"patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 2/6] blame: replace usage end blurb with better option spec","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:01Z","receivedAt":"2021-09-11T19:10:38Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Change the \"git blame -h\" output to be consistent with \"git bundle\n-h\"'s, i.e. before this we'd emit:\n\n $ git blame -h\n usage: git blame [] [] [] [--] \n\n are documented in git-rev-list(1)\n [...]\n\nNow instead of that we'll emit:\n\n $ git blame -h\n usage: git blame [] [] [] [--] \n [...]\n\nThis makes it consistent with the usage spec used for \"git bundle\":\n\n $ git bundle -h\n usage: git bundle create [] \n [...]\n\nThe use of this in \"blame\" dated back to 5817da01434 (git-blame:\nmigrate to incremental parse-option [1/2], 2008-07-08), and the use in\n\"bundle\" to 2e0afafebd8 (Add git-bundle: move objects and\nreferences by archive, 2007-02-22).\n\nOnce we get rid of this special case we can also use usage_msg_opt()\nto emit the error message we'd get on an invalid \"-L \"\nargument, which means we can get rid of the old-style \"blame_usage\"\nvariable entirely. This makes the output friendlier, before we'd emit say:\n\n $ git blame -L1,2,3,4 Makefile\n usage: git blame [] [] [] [--] \n $\n\nNow we'll instead emit:\n\n $ git blame -L1,2,3,4 Makefile\n fatal: Invalid -L parameter\n\n usage: git blame [] [] [] [--] \n [...]\n $\n\nThe \"[...]\" there elides the \"git blame\" option summary.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/blame.c | 9 +++------\n 1 file changed, 3 insertions(+), 6 deletions(-)\n\ndiff --git a/builtin/blame.c b/builtin/blame.c\nindex 641523ff9af..e469829bc76 100644\n--- a/builtin/blame.c\n+++ b/builtin/blame.c\n@@ -29,12 +29,8 @@\n #include \"refs.h\"\n #include \"tag.h\"\n \n-static char blame_usage[] = N_(\"git blame [] [] [] [--] \");\n-\n static const char *blame_opt_usage[] = {\n-\tblame_usage,\n-\t\"\",\n-\tN_(\" are documented in git-rev-list(1)\"),\n+\tN_(\"git blame [] [] [] [--] \"),\n \tNULL\n };\n \n@@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n \t\t\t\t nth_line_cb, &sb, lno, anchor,\n \t\t\t\t &bottom, &top, sb.path,\n \t\t\t\t the_repository->index))\n-\t\t\tusage(blame_usage);\n+\t\t\tusage_msg_opt(_(\"Invalid -L parameter\"),\n+\t\t\t\t blame_opt_usage, options);\n \t\tif ((!lno && (top || bottom)) || lno < bottom)\n \t\t\tdie(Q_(\"file %s has only %lu line\",\n \t\t\t \"file %s has only %lu lines\",\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435586","messageId":"patch-v3-3.6-e23a8231f38-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:02Z","receivedAt":"2021-09-11T19:10:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"The strings in the the \"usagestr\" array have special handling for the\nempty string dating back to f389c808b67 (Rework make_usage to print\nthe usage message immediately, 2007-10-14).\n\nWe'll prefix all strings after the first one with \"or: \". Then if we\nencountered a \"\" we'll emit all strings after that point verbatim\nwithout any \"or: \" prefixing.\n\nIn the preceding commits we got rid of the two users of this\nundocumented part of the API. Let's remove it in preparation for\nimproving the output emitted by usage_with_options_internal().\n\nI think we might want to use this in the future, but in that case\nwe'll be much better off with an API that emulates the\nnon-parse_options() way that git.c does this.\n\nThat git.c code uses a separate \"git_usage_string\" and\n\"git_more_info_string\". See b7d9681974e (Print info about \"git help\nCOMMAND\" on git's main usage pages, 2008-06-06). By splitting the two\nup we can emit something in the middle, as indeed git.c does.\n\nI'd like our \"git -h\" info to be more helpful, and I'd also like\nparse_options() to handle the \"git\" command itself, because of the\nlimitations of how this was done in usage_with_options_internal() we\ncouldn't migrate a caller like git.c to parse_options().\n\nSo let's just remove this for now, it has no users, and once we want\nto do this again we can simply add another argument to the relevant\nfunctions, or otherwise hook into things so that we can print\nsomething at the end and/or middle.\n\nIt's possible that this change introduce breakage somewhere. We'd only\ncatch these cases at runtime, and the \"git rev-parse --parseopt\"\ncommand is used by shellscripts, see bac199b7b17 (Update\ngit-sh-setup(1) to allow transparent use of git-rev-parse --parseopt,\n2007-11-04). I've grepped the codebase for \"OPTIONS_SPEC\",\n\"char.*\\*.*usage\\[\\]\" etc. I'm fairly sure there no outstanding users\nof this functionality.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/rev-parse.c | 3 +++\n parse-options.c | 8 +-------\n t/helper/test-parse-options.c | 2 --\n t/t0040-parse-options.sh | 2 --\n t/t1502-rev-parse-parseopt.sh | 34 ++++++++++++++++++----------------\n 5 files changed, 22 insertions(+), 27 deletions(-)\n\ndiff --git a/builtin/rev-parse.c b/builtin/rev-parse.c\nindex 22c4e1a4ff0..aeebfd52805 100644\n--- a/builtin/rev-parse.c\n+++ b/builtin/rev-parse.c\n@@ -436,7 +436,10 @@ static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n \tfor (;;) {\n \t\tif (strbuf_getline(&sb, stdin) == EOF)\n \t\t\tdie(_(\"premature end of input\"));\n+\t\tif (!sb.len)\n+\t\t\tdie(_(\"empty lines are not permitted before the `--' separator\"));\n \t\tALLOC_GROW(usage, unb + 1, usz);\n+\n \t\tif (!strcmp(\"--\", sb.buf)) {\n \t\t\tif (unb < 1)\n \t\t\t\tdie(_(\"no usage string given before the `--' separator\"));\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..950a8279beb 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -924,18 +924,12 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n \tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n-\twhile (*usagestr && **usagestr)\n+\twhile (*usagestr) {\n \t\t/*\n \t\t * TRANSLATORS: the colon here should align with the\n \t\t * one in \"usage: %s\" translation.\n \t\t */\n \t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n-\twhile (*usagestr) {\n-\t\tif (**usagestr)\n-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n-\t\telse\n-\t\t\tfputc('\\n', outfile);\n-\t\tusagestr++;\n \t}\n \n \tneed_newline = 1;\ndiff --git a/t/helper/test-parse-options.c b/t/helper/test-parse-options.c\nindex 2051ce57db7..e00aef073b0 100644\n--- a/t/helper/test-parse-options.c\n+++ b/t/helper/test-parse-options.c\n@@ -102,8 +102,6 @@ int cmd__parse_options(int argc, const char **argv)\n \tconst char *prefix = \"prefix/\";\n \tconst char *usage[] = {\n \t\t\"test-tool parse-options \",\n-\t\t\"\",\n-\t\t\"A helper function for the parse-options API.\",\n \t\tNULL\n \t};\n \tstruct string_list expect = STRING_LIST_INIT_NODUP;\ndiff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\nindex ad4746d899a..2910874ece5 100755\n--- a/t/t0040-parse-options.sh\n+++ b/t/t0040-parse-options.sh\n@@ -10,8 +10,6 @@ test_description='our own option parser'\n cat >expect <<\\EOF\n usage: test-tool parse-options \n \n- A helper function for the parse-options API.\n-\n --yes get a boolean\n -D, --no-doubt begins with 'no-'\n -B, --no-fear be brave\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex b29563fc997..6badc650d64 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -6,8 +6,6 @@ test_description='test git rev-parse --parseopt'\n test_expect_success 'setup optionspec' '\n \tsed -e \"s/^|//\" >optionspec <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n |h,help show the help\n |\n@@ -41,8 +39,6 @@ EOF\n test_expect_success 'setup optionspec-no-switches' '\n \tsed -e \"s/^|//\" >optionspec_no_switches <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n EOF\n '\n@@ -50,8 +46,6 @@ EOF\n test_expect_success 'setup optionspec-only-hidden-switches' '\n \tsed -e \"s/^|//\" >optionspec_only_hidden_switches <<\\EOF\n |some-command [options] ...\n-|\n-|some-command does foo and bar!\n |--\n |hidden1* A hidden switch\n EOF\n@@ -62,8 +56,6 @@ test_expect_success 'test --parseopt help output' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | -h, --help show the help\n | --foo some nifty option --foo\n | --bar ... some cool option --bar with an argument\n@@ -103,8 +95,6 @@ test_expect_success 'test --parseopt help output no switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n |EOF\n END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&\n@@ -116,8 +106,6 @@ test_expect_success 'test --parseopt help output hidden switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n |EOF\n END_EXPECT\n \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&\n@@ -129,8 +117,6 @@ test_expect_success 'test --parseopt help-all output hidden switches' '\n |cat <<\\EOF\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | --hidden1 A hidden switch\n |\n |EOF\n@@ -144,8 +130,6 @@ test_expect_success 'test --parseopt invalid switch help output' '\n |error: unknown option `does-not-exist'\\''\n |usage: some-command [options] ...\n |\n-| some-command does foo and bar!\n-|\n | -h, --help show the help\n | --foo some nifty option --foo\n | --bar ... some cool option --bar with an argument\n@@ -282,4 +266,22 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op\n \ttest_cmp expect output\n '\n \n+test_expect_success 'test --parseopt help output hidden switches' '\n+\tsed -e \"s/^|//\" >optionspec-trailing-line <<-\\EOF &&\n+\t|some-command [options] ...\n+\t|\n+\t|\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tfatal: empty lines are not permitted before the `--'\"'\"' separator\n+\tEOF\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&\n+\ttest_must_be_empty out &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435587","messageId":"patch-v3-4.6-5638d2bc832-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 4/6] built-ins: \"properly\" align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:03Z","receivedAt":"2021-09-11T19:10:44Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\noutput in cases where we continue usage output after a \"\\n\". The \"fix\"\nand \"properly\" scare quotes are because this actually makes things\nworse in some cases, because e.g. in the case of \"git tag -h\" the\n\"\\t\\t\" effectively works around how parse-options.c aligns this\noutput.\n\nBut two wrongs don't make a right, let's \"fix\" this by making it worse\ntemporarily, in anticipation of improving parse-options.c to handle\nthis alignment.\n\nThe issue is that we should have whitespace corresponding to the\nlength of the command name here, e.g. in the case of \"git ls-remote\"\nit should be 14 characters, or the length of \"\"git ls-remote\n\". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\ncharacters are the length of \"usage: \" (and also \" or:\"). So in the C\nlocale the resulting output aligns nicely:\n\n $ git ls-remote -h\n usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n [-q | --quiet] [--exit-code] [--get-url]\n [--symref] [ [...]]\n\nBut that's fragile, we might not be under the C locale. We really\nshould have parse-options.c itself add this padding. In a subsequent\ncommit I'll make it do that.\n\nIn the case of \"tag\" and \"show-branch\" and \"stash save\" the output was\nnot properly aligned, although in the \"git tag\" case it was\nnear-enough (aligned with the \"-\" in \"git tag -l\") to look good,\nassuming C locale & a tab-width of 8. In any case, let's align this in\na way that looks obviously correct when looking at the source itself,\nand then improve parse-options.c itself.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/ls-remote.c | 4 ++--\n builtin/show-branch.c | 6 +++---\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 ++--\n 4 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex f4fd823af83..318949c3d75 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -7,8 +7,8 @@\n \n static const char * const ls_remote_usage[] = {\n \tN_(\"git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\\n\"\n-\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n-\t \" [--symref] [ [...]]\"),\n+\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n+\t \" [--symref] [ [...]]\"),\n \tNULL\n };\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex bea4bbf4680..082449293b5 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -11,9 +11,9 @@\n \n static const char* show_branch_usage[] = {\n N_(\"git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\\n\"\n- \"\t\t[--current] [--color[=] | --no-color] [--sparse]\\n\"\n- \"\t\t[--more= | --list | --independent | --merge-base]\\n\"\n- \"\t\t[--no-name | --sha1-name] [--topics] [( | )...]\"),\n+ \" [--current] [--color[=] | --no-color] [--sparse]\\n\"\n+ \" [--more= | --list | --independent | --merge-base]\\n\"\n+ \" [--no-name | --sha1-name] [--topics] [( | )...]\"),\n N_(\"git show-branch (-g | --reflog)[=[,]] [--list] []\"),\n NULL\n };\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..45b19007d7c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {\n \n static const char * const git_stash_save_usage[] = {\n \tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n-\t \" [-u|--include-untracked] [-a|--all] []\"),\n+\t \" [-u|--include-untracked] [-a|--all] []\"),\n \tNULL\n };\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 065b6bf093e..6535ed27ee9 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,10 +23,10 @@\n \n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ]\\n\"\n-\t\t\"\\t\\t []\"),\n+\t \" []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n+\t \" [--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435588","messageId":"patch-v3-5.6-821e5e14132-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 5/6] send-pack: properly use parse_options() API for usage string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:04Z","receivedAt":"2021-09-11T19:10:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When \"send-pack\" was changed to use the parse_options() API in\n068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)\nit was made to use one very long line, instead it should split them up\nwith newlines.\n\nFurthermore we were including an inline explanation that you couldn't\ncombine \"--all\" and \"\", but unlike in the \"blame\" case this was\nnot preceded by an empty string.\n\nLet's instead show that --all and can't be combined in the the\nusual language of the usage syntax instead. We can make it clear that\none of the two options \"--foo\" and \"--bar\" is mandatory, but that the\ntwo are mutually exclusive by referring to them as \"( --foo | --bar\n)\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-send-pack.txt | 4 ++--\n builtin/send-pack.c | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 44fd146b912..be41f119740 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=]\n+'git send-pack' [--dry-run] [--force] [--receive-pack=]\n \t\t[--verbose] [--thin] [--atomic]\n \t\t[--[no-]signed|--signed=(true|false|if-asked)]\n-\t\t[:] [...]\n+\t\t[:] (--all | ...)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 729dea1d255..89321423125 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -17,10 +17,10 @@\n #include \"protocol.h\"\n \n static const char * const send_pack_usage[] = {\n-\tN_(\"git send-pack [--all | --mirror] [--dry-run] [--force] \"\n-\t \"[--receive-pack=] [--verbose] [--thin] [--atomic] \"\n-\t \"[:] [...]\\n\"\n-\t \" --all and explicit specification are mutually exclusive.\"),\n+\tN_(\"git send-pack [--mirror] [--dry-run] [--force]\\n\"\n+\t \" [--receive-pack=]\\n\"\n+\t \" [--verbose] [--thin] [--atomic]\\n\"\n+\t \" [:] (--all | ...)\"),\n \tNULL,\n };\n \n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435589","messageId":"patch-v3-6.6-0df2840ce4e-20210911T190239Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v3 6/6] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-11T19:09:05Z","receivedAt":"2021-09-11T19:10:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some commands such as \"git stash\" emit continued options output with\ne.g. \"git stash -h\", because usage_with_options_internal() prefixes\nwith its own whitespace the resulting output wasn't properly\naligned. Let's account for the added whitespace, which properly aligns\nthe output.\n\nThe \"git stash\" command has usage output with a N_() translation that\nlegitimately stretches across multiple lines;\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nWe'd like to have that output aligned with the length of the initial\n\"git stash \" output, but since usage_with_options_internal() adds its\nown whitespace prefixing we fell short, before this change we'd emit:\n\n $ git stash -h\n usage: git stash list []\n or: git stash show [] []\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nNow we'll properly emit aligned output. I.e. the last four lines\nabove will instead be (a whitespace-only change to the above):\n\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nWe could also go for an approach where we have the caller support no\npadding of their own, i.e. (same as the first example, except for the\npadding on the second line):\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nBut to do that we'll need to find the length of \"git stash\". We can\ndiscover that from the \"cmd\" in the \"struct cmd_struct\", but there\nmight cases with sub-commands or \"git\" itself taking arguments that\nwould make that non-trivial.\n\nEven if it was I still think this approach is better, because this way\nwe'll get the same legible alignment in the C code. The fact that\nusage_with_options_internal() is adding its own prefix padding is an\nimplementation detail that callers shouldn't need to worry about.\n\nImplementation notes:\n\nWe could skip the string_list_split() with a strchr(str, '\\n') check,\nbut we'd then need to duplicate our state machine for strings that do\nand don't contain a \"\\n\". It's simpler to just always split into a\n\"struct string_list\", even though the common case is that that \"struct\nstring_list\" will contain only one element. This is not\nperformance-sensitive code.\n\nThis change is relatively more complex since I've accounted for making\nit future-proof for RTL translation support. Later in\nusage_with_options_internal() we have some existing padding code\ndating back to d7a38c54a6c (parse-options: be able to generate usages\nautomatically, 2007-10-15) which isn't RTL-safe, but that code would\nbe easy to fix. Let's not introduce new RTL translation problems here.\n\nI'm also adding a check to catch the mistake of needlessly adding a\ntrailing \"\\n\", such as:\n\n\tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] []\\n\"),\n\nOr even a mistake like adding just one \"\\n\" in a string with no other\nnewlines:\n\n\tN_(\"git stash list []\\n\"),\n\nThis catches the cases already tested for in cmd_parseopt(), but this\ncovers the purely C API. As noted a preceding commit that added the\ndie() to cmd_parseopt() I'm fairly confident that this will be\ntriggered by no in-tree user I've missed.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 65 ++++++++++++++++++++++++++++++++++++++++++++-----\n 1 file changed, 59 insertions(+), 6 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 950a8279beb..ff28869d2c9 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -917,19 +917,72 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n \n+\tconst char *usage_prefix = _(\"usage: %s\");\n+\t/*\n+\t * The translation could be anything, but we can count on\n+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n+\t * the translation. So compute the length of the \"usage: \"\n+\t * part. We are assuming that the translator wasn't overly\n+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n+\t * one \"%s\" in \"usage_prefix\" above, so there's no reason to\n+\t * do so even with a RTL language.\n+\t */\n+\tsize_t usage_len = strlen(usage_prefix) - strlen(\"%s\");\n+\t/*\n+\t * TRANSLATORS: the colon here should align with the\n+\t * one in \"usage: %s\" translation.\n+\t */\n+\tconst char *or_prefix = _(\" or: %s\");\n+\n+\t/*\n+\t * TRANSLATORS: You should only need to translate this format\n+\t * string if your language is a RTL language (e.g. Arabic,\n+\t * Hebrew etc.), not if it's a LTR language (e.g. German,\n+\t * Russian, Chinese etc.).\n+\t *\n+\t * When a translated usage string has an embedded \"\\n\" it's\n+\t * because options have wrapped to the next line. The line\n+\t * after the \"\\n\" will then be padded to align with the\n+\t * command name, such as N_(\"git cmd [opt]\\n<8\n+\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n+\t * \"git cmd \".\n+\t *\n+\t * This format string prints out that already-translated\n+\t * line. The \"%*s\" is whitespace padding to account for the\n+\t * padding at the start of the line that we add in this\n+\t * function. The \"%s\" is a line in the (hopefully already\n+\t * translated) N_() usage string, which contained embedded\n+\t * newlines before we split it up.\n+\t */\n+\tconst char *usage_continued = _(\"%*s%s\");\n+\tconst char *prefix = usage_prefix;\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n \tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n-\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n \twhile (*usagestr) {\n-\t\t/*\n-\t\t * TRANSLATORS: the colon here should align with the\n-\t\t * one in \"usage: %s\" translation.\n-\t\t */\n-\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tunsigned int j;\n+\n+\t\tstring_list_split(&list, _(*usagestr++), '\\n', -1);\n+\t\tfor (j = 0; j < list.nr; j++) {\n+\t\t\tconst char *line = list.items[j].string;\n+\n+\t\t\tif (!*line)\n+\t\t\t\tBUG(\"empty or trailing line in usage string\");\n+\n+\t\t\tif (!j)\n+\t\t\t\tfprintf_ln(outfile, prefix, line);\n+\t\t\telse\n+\t\t\t\tfprintf_ln(outfile, usage_continued,\n+\t\t\t\t\t (int)usage_len, \"\", line);\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\n+\t\tprefix = or_prefix;\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.995.ga5ea46173a2\n\n"},{"id":"435625","messageId":"CAPig+cRzzMEFKMEW1dMB=Y++kuoFUnLgRatV7_hFZ5BnSbpD+A@mail.gmail.com","threadId":"56423","inReplyTo":"patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2021-09-12T04:45:50Z","receivedAt":"2021-09-12T04:46:05Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Sep 11, 2021 at 3:10 PM Ævar Arnfjörð Bjarmason\n wrote:\n> Change the \"git blame -h\" output to be consistent with \"git bundle\n> -h\"'s, i.e. before this we'd emit:\n\nJust a couple tiny, tiny nits (which may or may not be worth a re-roll)...\n\n> $ git blame -h\n> usage: git blame [] [] [] [--] \n>\n> are documented in git-rev-list(1)\n> [...]\n>\n> Now instead of that we'll emit:\n>\n> $ git blame -h\n> usage: git blame [] [] [] [--] \n\nThis is lacking a hyphen between `git` and `rev-list`...\n\n> This makes it consistent with the usage spec used for \"git bundle\":\n>\n> $ git bundle -h\n> usage: git bundle create [] \n\n...whereas this has a hyphen between the two.\n\n> [...]\n> Signed-off-by: Ævar Arnfjörð Bjarmason \n> ---\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> @@ -29,12 +29,8 @@\n> - N_(\" are documented in git-rev-list(1)\"),\n> + N_(\"git blame [] [] [] [--] \"),\n\nDitto regarding missing hyphen.\n\n> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> nth_line_cb, &sb, lno, anchor,\n> &bottom, &top, sb.path,\n> the_repository->index))\n> - usage(blame_usage);\n> + usage_msg_opt(_(\"Invalid -L parameter\"),\n> + blame_opt_usage, options);\n\nbuilltin/blame.c seems to be pretty consistent about starting error\nand warning messages with a lowercase letter, so this perhaps should\nfollow suit. Also, I think you can drop \"parameter\" without losing\nclarity:\n\n invalid -L range\n\nwould likely be good enough.\n\n> if ((!lno && (top || bottom)) || lno < bottom)\n> die(Q_(\"file %s has only %lu line\",\n> \"file %s has only %lu lines\",\n"},{"id":"435657","messageId":"xmqqczpdlb2s.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v3-1.6-ad857e80fd8-20210911T190239Z-avarab@gmail.com","subject":"Re: [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-12T21:48:27Z","receivedAt":"2021-09-12T21:48:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> @@ -1693,6 +1695,7 @@ ifdef NO_INET_PTON\n> endif\n> ifdef NO_UNIX_SOCKETS\n> \tBASIC_CFLAGS += -DNO_UNIX_SOCKETS\n> +\tEXCLUDED_PROGRAMS += git-credential-cache git-credential-cache--daemon\n\nYah, I forgot about EXCLUDED_PROGRAMS ;-) \n\nSounds like a sensible direction to go.\n\n> diff --git a/git.c b/git.c\n> index 18bed9a9964..7c696e06ecf 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -513,8 +513,10 @@ static struct cmd_struct commands[] = {\n> \t{ \"config\", cmd_config, RUN_SETUP_GENTLY | DELAY_PAGER_CONFIG },\n> \t{ \"count-objects\", cmd_count_objects, RUN_SETUP },\n> \t{ \"credential\", cmd_credential, RUN_SETUP_GENTLY | NO_PARSEOPT },\n> +#ifndef NO_UNIX_SOCKETS\n> \t{ \"credential-cache\", cmd_credential_cache },\n> \t{ \"credential-cache--daemon\", cmd_credential_cache_daemon },\n> +#endif\n> \t{ \"credential-store\", cmd_credential_store },\n> \t{ \"describe\", cmd_describe, RUN_SETUP },\n> \t{ \"diff\", cmd_diff, NO_PARSEOPT },\n\nOK. Looks good.\n"},{"id":"435663","messageId":"xmqqtuipjuxc.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v3-2.6-036eb0efb5b-20210911T190239Z-avarab@gmail.com","subject":"Re: [PATCH v3 2/6] blame: replace usage end blurb with better option spec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-12T22:22:39Z","receivedAt":"2021-09-12T22:22:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Change the \"git blame -h\" output to be consistent with \"git bundle\n> -h\"'s, i.e. before this we'd emit:\n>\n> $ git blame -h\n> usage: git blame [] [] [] [--] \n>\n> are documented in git-rev-list(1)\n> [...]\n>\n> Now instead of that we'll emit:\n>\n> $ git blame -h\n> usage: git blame [] [] [] [--] \n> [...]\n\nWhat we take are options that rev-list takes, not arguments like A..B,\nso the updated text seems to be more wrong.\n\nIt does not even make much sense as a goal to make \"blame\" and\n\"bundle\" similar to begin with, does it? It may make sense for\n\"bundle\" to be more similar to \"pack-objects\", in that the\ninformation the command needs ultimately is about what is needed by\n\"rev-list --objects\" (i.e. object enumeration), while \"blame\" is\nmore similar to \"log\", in that it is interested in walking commit\nDAG but not about the trees and blobs connected to the commits in\nthe DAG. From the end-users' point of view, they do not care if\n\"bundle\" and \"blame\" are explained using similar terms.\n\nAlso, not all you can see from \"git rev-list -h\" would\nmake sense in the context of \"git blame\". \"--no-merges\" and any\noptions that are related to the number of parents make no sense,\n--all, --branches and the friends, when used to give multiple\npositive ends (i.e. starting points) of traversal, would not make\nsense at all, options about ordering and formatting output of\nrev-list of course would not make any difference.\n\nAt the very least, we should say there, or we\nshould just drop the mention of \"we also take some options meant for\nrev-list\" from here, leaving:\n\n\tusage: git blame [] [] [--] \n\nand nothing else?\n\nI am sympathetic to the original reasoning why we wanted to add a\nparenthetical \"by the way, we also take (some) options rev-list\ntakes\" here. This is because not all relevant options are described\nin the options[] array we have there (we delegate what we do not\nknow to parse_revisions_opt()), and it is sort of understandable\nthat they wanted to leave a hint that the list of options given here\nis not exhaustive.\n\nBut in the longer run, if we really wanted to make the -h output\nuseful to end users, what we should aim for is not to make it\nsimilar to \"git bundle\", but to ensure that the option summary\nincludes the relevant options shared with rev-list (in other words,\nmake the list of options cover all the options the command takes,\nnot just the ones in today's options[] array).\n\nWhen that happens, users do not have to care which ones happen to be\nparsed by us via our call to parse_options_step(), and which other\nones are given to parse_revision_opt(). There won't be any need to\nsay \"we also take some options...\" with in the original.\n\n> diff --git a/builtin/blame.c b/builtin/blame.c\n> index 641523ff9af..e469829bc76 100644\n> --- a/builtin/blame.c\n> +++ b/builtin/blame.c\n> @@ -29,12 +29,8 @@\n> #include \"refs.h\"\n> #include \"tag.h\"\n> \n> -static char blame_usage[] = N_(\"git blame [] [] [] [--] \");\n> -\n> static const char *blame_opt_usage[] = {\n> -\tblame_usage,\n> -\t\"\",\n> -\tN_(\" are documented in git-rev-list(1)\"),\n> +\tN_(\"git blame [] [] [] [--] \"),\n> \tNULL\n> };\n> \n> @@ -1107,7 +1103,8 @@ int cmd_blame(int argc, const char **argv, const char *prefix)\n> \t\t\t\t nth_line_cb, &sb, lno, anchor,\n> \t\t\t\t &bottom, &top, sb.path,\n> \t\t\t\t the_repository->index))\n> -\t\t\tusage(blame_usage);\n> +\t\t\tusage_msg_opt(_(\"Invalid -L parameter\"),\n> +\t\t\t\t blame_opt_usage, options);\n\n\"invalid -L \" is fine, but can't we parrot what the user gave\nus here? You can give more than one -L to specify two ranges in the\nsame file that are not contiguous:\n\n\tgit blame -L1,10 -L100.112 master..seen -- foo.c\n\nand it would be helpful to tell which one is broken.\n\n> \t\tif ((!lno && (top || bottom)) || lno < bottom)\n> \t\t\tdie(Q_(\"file %s has only %lu line\",\n> \t\t\t \"file %s has only %lu lines\",\n"},{"id":"435665","messageId":"xmqqpmtdjuqj.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v3-3.6-e23a8231f38-20210911T190239Z-avarab@gmail.com","subject":"Re: [PATCH v3 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-12T22:26:44Z","receivedAt":"2021-09-12T22:26:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n> index ad4746d899a..2910874ece5 100755\n> --- a/t/t0040-parse-options.sh\n> +++ b/t/t0040-parse-options.sh\n> @@ -10,8 +10,6 @@ test_description='our own option parser'\n> cat >expect <<\\EOF\n> usage: test-tool parse-options \n> \n> - A helper function for the parse-options API.\n> -\n> --yes get a boolean\n> -D, --no-doubt begins with 'no-'\n> -B, --no-fear be brave\n\nIsn't this, and a lot more importantly the next one ...\n\n> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n> index b29563fc997..6badc650d64 100755\n> --- a/t/t1502-rev-parse-parseopt.sh\n> +++ b/t/t1502-rev-parse-parseopt.sh\n> @@ -6,8 +6,6 @@ test_description='test git rev-parse --parseopt'\n> test_expect_success 'setup optionspec' '\n> \tsed -e \"s/^|//\" >optionspec <<\\EOF\n> |some-command [options] ...\n> -|\n> -|some-command does foo and bar!\n> |--\n> |h,help show the help\n> |\n\n... coalmine canaries that tell us that end-user's scripts using the\n\"git rev-parse --parseopt\" in the documented way will be broken?\n\nI'd rather not have to sorry about breaking end-user scripts this\nway. Unlike a very small number of in-tree parse_options() call in\nC programs, we have unbounded of them.\n\n"},{"id":"435672","messageId":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com","subject":"[PATCH v4 0/4] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:13:18Z","receivedAt":"2021-09-13T00:13:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes usage_with_options_internal() in parse-options.c\nto properly align continued \"\\n\" usage output.\n\nThis v4 essentially goes back to the version of this in v1. In v2 I'd\naddressed Eric's comments in [1] by trying to remove the last three\nin-tree users of this part of the API.\n\nBut if \"git rev-parse --parseopt\" is going to be bug-for-bug\ncompatible with existing out-of-tree users[2] then changing that\nbecomes a non-starter.\n\nSo let's keep the existing behavior (which also had at least one other\nweird & undocumented edge case, which we now test for).\n\nThis v4 also has a fix for the \"saw_empty_line\" bug Eric noted in [2]\n(which wasn't applicable to v2..v3), and takes some of his suggestions\nfor simplifying the loop (but not all, I still kept it as one loop).\n\n1. https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/\n2. https://lore.kernel.org/git/xmqqpmtdjuqj.fsf@gitster.g/\n\nÆvar Arnfjörð Bjarmason (4):\n parse-options API users: align usage output in C-strings\n send-pack: properly use parse_options() API for usage string\n git rev-parse --parseopt tests: add more usagestr tests\n parse-options: properly align continued usage output\n\n Documentation/git-send-pack.txt | 4 +-\n builtin/ls-remote.c | 4 +-\n builtin/send-pack.c | 8 ++--\n builtin/show-branch.c | 6 +--\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 +-\n parse-options.c | 76 +++++++++++++++++++++++++++------\n t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++\n 8 files changed, 132 insertions(+), 26 deletions(-)\n\nRange-diff against v3:\n1: ad857e80fd8 < -: ----------- credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS\n2: 036eb0efb5b < -: ----------- blame: replace usage end blurb with better option spec\n4: 5638d2bc832 ! 1: 64d8647340d built-ins: \"properly\" align continued usage output\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - built-ins: \"properly\" align continued usage output\n + parse-options API users: align usage output in C-strings\n \n - Let's \"fix\" various \"git -h\" output by \"properly\" aligning the\n - output in cases where we continue usage output after a \"\\n\". The \"fix\"\n - and \"properly\" scare quotes are because this actually makes things\n - worse in some cases, because e.g. in the case of \"git tag -h\" the\n - \"\\t\\t\" effectively works around how parse-options.c aligns this\n - output.\n + In preparation for having continued usage lines properly aligned in\n + \"git -h\" output, let's have the \"[\" on the second such lines\n + align with the \"[\" on the first line.\n \n - But two wrongs don't make a right, let's \"fix\" this by making it worse\n - temporarily, in anticipation of improving parse-options.c to handle\n - this alignment.\n + In some cases this makes the output worse, because e.g. the \"git\n + ls-remote -h\" output had been aligned to account for the extra\n + whitespace that the usage_with_options_internal() function in\n + parse-options.c would add.\n \n - The issue is that we should have whitespace corresponding to the\n - length of the command name here, e.g. in the case of \"git ls-remote\"\n - it should be 14 characters, or the length of \"\"git ls-remote\n - \". Instead we had 21 characters in builtin/ls-remote.c, those extra 7\n - characters are the length of \"usage: \" (and also \" or:\"). So in the C\n - locale the resulting output aligns nicely:\n + In other cases such as builtin/stash.c (not changed here), we were\n + aligned in the C strings, but since that didn't account for the extra\n + padding in usage_with_options_internal() it would come out looking\n + misaligned, e.g. code like this:\n \n - $ git ls-remote -h\n - usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\n - [-q | --quiet] [--exit-code] [--get-url]\n - [--symref] [ [...]]\n + N_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n + \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n \n - But that's fragile, we might not be under the C locale. We really\n - should have parse-options.c itself add this padding. In a subsequent\n - commit I'll make it do that.\n + Would emit:\n \n - In the case of \"tag\" and \"show-branch\" and \"stash save\" the output was\n - not properly aligned, although in the \"git tag\" case it was\n - near-enough (aligned with the \"-\" in \"git tag -l\") to look good,\n - assuming C locale & a tab-width of 8. In any case, let's align this in\n - a way that looks obviously correct when looking at the source itself,\n - and then improve parse-options.c itself.\n + or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n + [-u|--include-untracked] [-a|--all] [-m|--message ]\n +\n + Let's change all the usage arrays which use such continued usage\n + output via \"\\n\"-embedding to be like builtin/stash.c.\n +\n + This makes the output worse temporarily, but in a subsequent change\n + I'll improve the usage_with_options_internal() to take this into\n + account, at which point all of the strings being changed here will\n + emit prettier output.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n5: 821e5e14132 = 2: f10ff775c69 send-pack: properly use parse_options() API for usage string\n3: e23a8231f38 ! 3: 05a0c7cac37 parse-options: stop supporting \"\" in the usagestr array\n @@ Metadata\n Author: Ævar Arnfjörð Bjarmason \n \n ## Commit message ##\n - parse-options: stop supporting \"\" in the usagestr array\n + git rev-parse --parseopt tests: add more usagestr tests\n \n - The strings in the the \"usagestr\" array have special handling for the\n - empty string dating back to f389c808b67 (Rework make_usage to print\n - the usage message immediately, 2007-10-14).\n + Add tests for the \"usagestr\" passed to parse-options.c\n + usage_with_options_internal() through cmd_parseopt().\n \n - We'll prefix all strings after the first one with \"or: \". Then if we\n - encountered a \"\" we'll emit all strings after that point verbatim\n - without any \"or: \" prefixing.\n -\n - In the preceding commits we got rid of the two users of this\n - undocumented part of the API. Let's remove it in preparation for\n - improving the output emitted by usage_with_options_internal().\n -\n - I think we might want to use this in the future, but in that case\n - we'll be much better off with an API that emulates the\n - non-parse_options() way that git.c does this.\n -\n - That git.c code uses a separate \"git_usage_string\" and\n - \"git_more_info_string\". See b7d9681974e (Print info about \"git help\n - COMMAND\" on git's main usage pages, 2008-06-06). By splitting the two\n - up we can emit something in the middle, as indeed git.c does.\n -\n - I'd like our \"git -h\" info to be more helpful, and I'd also like\n - parse_options() to handle the \"git\" command itself, because of the\n - limitations of how this was done in usage_with_options_internal() we\n - couldn't migrate a caller like git.c to parse_options().\n -\n - So let's just remove this for now, it has no users, and once we want\n - to do this again we can simply add another argument to the relevant\n - functions, or otherwise hook into things so that we can print\n - something at the end and/or middle.\n -\n - It's possible that this change introduce breakage somewhere. We'd only\n - catch these cases at runtime, and the \"git rev-parse --parseopt\"\n - command is used by shellscripts, see bac199b7b17 (Update\n - git-sh-setup(1) to allow transparent use of git-rev-parse --parseopt,\n - 2007-11-04). I've grepped the codebase for \"OPTIONS_SPEC\",\n - \"char.*\\*.*usage\\[\\]\" etc. I'm fairly sure there no outstanding users\n - of this functionality.\n + These test for edge cases in the existing behavior related to the\n + \"--parseopt\" interface doing its own line-splitting with\n + strbuf_getline(), and the native C interface expecting and potentially\n + needing to handle newlines within the strings in the array it\n + accepts. The results are probably something that wasn't anticipated,\n + but let's make sure we stay backwards compatible with it.\n \n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n - ## builtin/rev-parse.c ##\n -@@ builtin/rev-parse.c: static int cmd_parseopt(int argc, const char **argv, const char *prefix)\n - \tfor (;;) {\n - \t\tif (strbuf_getline(&sb, stdin) == EOF)\n - \t\t\tdie(_(\"premature end of input\"));\n -+\t\tif (!sb.len)\n -+\t\t\tdie(_(\"empty lines are not permitted before the `--' separator\"));\n - \t\tALLOC_GROW(usage, unb + 1, usz);\n -+\n - \t\tif (!strcmp(\"--\", sb.buf)) {\n - \t\t\tif (unb < 1)\n - \t\t\t\tdie(_(\"no usage string given before the `--' separator\"));\n -\n - ## parse-options.c ##\n -@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n - \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n - \n - \tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n --\twhile (*usagestr && **usagestr)\n -+\twhile (*usagestr) {\n - \t\t/*\n - \t\t * TRANSLATORS: the colon here should align with the\n - \t\t * one in \"usage: %s\" translation.\n - \t\t */\n - \t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n --\twhile (*usagestr) {\n --\t\tif (**usagestr)\n --\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n --\t\telse\n --\t\t\tfputc('\\n', outfile);\n --\t\tusagestr++;\n - \t}\n - \n - \tneed_newline = 1;\n -\n - ## t/helper/test-parse-options.c ##\n -@@ t/helper/test-parse-options.c: int cmd__parse_options(int argc, const char **argv)\n - \tconst char *prefix = \"prefix/\";\n - \tconst char *usage[] = {\n - \t\t\"test-tool parse-options \",\n --\t\t\"\",\n --\t\t\"A helper function for the parse-options API.\",\n - \t\tNULL\n - \t};\n - \tstruct string_list expect = STRING_LIST_INIT_NODUP;\n -\n - ## t/t0040-parse-options.sh ##\n -@@ t/t0040-parse-options.sh: test_description='our own option parser'\n - cat >expect <<\\EOF\n - usage: test-tool parse-options \n - \n -- A helper function for the parse-options API.\n --\n - --yes get a boolean\n - -D, --no-doubt begins with 'no-'\n - -B, --no-fear be brave\n -\n ## t/t1502-rev-parse-parseopt.sh ##\n -@@ t/t1502-rev-parse-parseopt.sh: test_description='test git rev-parse --parseopt'\n - test_expect_success 'setup optionspec' '\n - \tsed -e \"s/^|//\" >optionspec <<\\EOF\n - |some-command [options] ...\n --|\n --|some-command does foo and bar!\n - |--\n - |h,help show the help\n - |\n -@@ t/t1502-rev-parse-parseopt.sh: EOF\n - test_expect_success 'setup optionspec-no-switches' '\n - \tsed -e \"s/^|//\" >optionspec_no_switches <<\\EOF\n - |some-command [options] ...\n --|\n --|some-command does foo and bar!\n - |--\n - EOF\n - '\n -@@ t/t1502-rev-parse-parseopt.sh: EOF\n - test_expect_success 'setup optionspec-only-hidden-switches' '\n - \tsed -e \"s/^|//\" >optionspec_only_hidden_switches <<\\EOF\n - |some-command [options] ...\n --|\n --|some-command does foo and bar!\n - |--\n - |hidden1* A hidden switch\n - EOF\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output' '\n - |cat <<\\EOF\n - |usage: some-command [options] ...\n - |\n --| some-command does foo and bar!\n --|\n - | -h, --help show the help\n - | --foo some nifty option --foo\n - | --bar ... some cool option --bar with an argument\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output no switches' '\n - |cat <<\\EOF\n - |usage: some-command [options] ...\n - |\n --| some-command does foo and bar!\n --|\n - |EOF\n - END_EXPECT\n - \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output hidden switches' '\n - |cat <<\\EOF\n - |usage: some-command [options] ...\n - |\n --| some-command does foo and bar!\n --|\n - |EOF\n - END_EXPECT\n - \ttest_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '\n - |cat <<\\EOF\n - |usage: some-command [options] ...\n - |\n --| some-command does foo and bar!\n --|\n - | --hidden1 A hidden switch\n - |\n - |EOF\n -@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt invalid switch help output' '\n - |error: unknown option `does-not-exist'\\''\n - |usage: some-command [options] ...\n - |\n --| some-command does foo and bar!\n --|\n - | -h, --help show the help\n - | --foo some nifty option --foo\n - | --bar ... some cool option --bar with an argument\n @@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt --stuck-long and short option with unset op\n \ttest_cmp expect output\n '\n \n -+test_expect_success 'test --parseopt help output hidden switches' '\n -+\tsed -e \"s/^|//\" >optionspec-trailing-line <<-\\EOF &&\n -+\t|some-command [options] ...\n ++test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\" lines' '\n ++\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n ++\t|cmd [--some-option]\n ++\t| [--another-option]\n ++\t|cmd [--yet-another-option]\n ++\t|--\n ++\t|h,help show the help\n ++\tEOF\n ++\n ++\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n ++\t|cat <<\\EOF\n ++\t|usage: cmd [--some-option]\n ++\t| or: [--another-option]\n ++\t| or: cmd [--yet-another-option]\n ++\t|\n ++\t| -h, --help show the help\n +\t|\n ++\t|EOF\n ++\tEND_EXPECT\n ++\n ++\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n ++\ttest_cmp expect actual\n ++'\n ++\n ++test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n ++\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n ++\t|cmd [--some-option]\n ++\t| [--another-option]\n +\t|\n ++\t|multi\n ++\t|line\n ++\t|blurb\n +\t|--\n +\t|h,help show the help\n +\tEOF\n +\n -+\tcat >expect <<-\\EOF &&\n -+\tfatal: empty lines are not permitted before the `--'\"'\"' separator\n -+\tEOF\n ++\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n ++\t|cat <<\\EOF\n ++\t|usage: cmd [--some-option]\n ++\t| or: [--another-option]\n ++\t|\n ++\t| multi\n ++\t| line\n ++\t| blurb\n ++\t|\n ++\t| -h, --help show the help\n ++\t|\n ++\t|EOF\n ++\tEND_EXPECT\n +\n -+\ttest_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&\n -+\ttest_must_be_empty out &&\n ++\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n +\ttest_cmp expect actual\n +'\n +\n6: 0df2840ce4e ! 4: 55480dee680 parse-options: properly align continued usage output\n @@ Commit message\n automatically, 2007-10-15) which isn't RTL-safe, but that code would\n be easy to fix. Let's not introduce new RTL translation problems here.\n \n - I'm also adding a check to catch the mistake of needlessly adding a\n - trailing \"\\n\", such as:\n -\n - N_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n - \" [-u|--include-untracked] [-a|--all] []\\n\"),\n -\n - Or even a mistake like adding just one \"\\n\" in a string with no other\n - newlines:\n -\n - N_(\"git stash list []\\n\"),\n -\n - This catches the cases already tested for in cmd_parseopt(), but this\n - covers the purely C API. As noted a preceding commit that added the\n - die() to cmd_parseopt() I'm fairly confident that this will be\n - triggered by no in-tree user I've missed.\n -\n Signed-off-by: Ævar Arnfjörð Bjarmason \n \n ## parse-options.c ##\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n +\t * one in \"usage: %s\" translation.\n +\t */\n +\tconst char *or_prefix = _(\" or: %s\");\n -+\n +\t/*\n +\t * TRANSLATORS: You should only need to translate this format\n +\t * string if your language is a RTL language (e.g. Arabic,\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n +\t */\n +\tconst char *usage_continued = _(\"%*s%s\");\n +\tconst char *prefix = usage_prefix;\n ++\tint saw_empty_line = 0;\n +\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n @@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n -\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n - \twhile (*usagestr) {\n +-\twhile (*usagestr && **usagestr)\n -\t\t/*\n -\t\t * TRANSLATORS: the colon here should align with the\n -\t\t * one in \"usage: %s\" translation.\n -\t\t */\n -\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n + \twhile (*usagestr) {\n +-\t\tif (**usagestr)\n +-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n +-\t\telse\n +-\t\t\tfputc('\\n', outfile);\n +-\t\tusagestr++;\n ++\t\tconst char *str = _(*usagestr++);\n +\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n +\t\tunsigned int j;\n +\n -+\t\tstring_list_split(&list, _(*usagestr++), '\\n', -1);\n ++\t\tif (!saw_empty_line && !*str)\n ++\t\t\tsaw_empty_line = 1;\n ++\n ++\t\tstring_list_split(&list, str, '\\n', -1);\n +\t\tfor (j = 0; j < list.nr; j++) {\n +\t\t\tconst char *line = list.items[j].string;\n +\n -+\t\t\tif (!*line)\n -+\t\t\t\tBUG(\"empty or trailing line in usage string\");\n -+\n -+\t\t\tif (!j)\n ++\t\t\tif (saw_empty_line && *line)\n ++\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n ++\t\t\telse if (saw_empty_line)\n ++\t\t\t\tfputc('\\n', outfile);\n ++\t\t\telse if (!j)\n +\t\t\t\tfprintf_ln(outfile, prefix, line);\n +\t\t\telse\n +\t\t\t\tfprintf_ln(outfile, usage_continued,\n-- \n2.33.0.1001.g3ab2ac1eaae\n\n"},{"id":"435673","messageId":"patch-v4-1.4-64d8647340d-20210912T235347Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","subject":"[PATCH v4 1/4] parse-options API users: align usage output in C-strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:13:19Z","receivedAt":"2021-09-13T00:13:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for having continued usage lines properly aligned in\n\"git -h\" output, let's have the \"[\" on the second such lines\nalign with the \"[\" on the first line.\n\nIn some cases this makes the output worse, because e.g. the \"git\nls-remote -h\" output had been aligned to account for the extra\nwhitespace that the usage_with_options_internal() function in\nparse-options.c would add.\n\nIn other cases such as builtin/stash.c (not changed here), we were\naligned in the C strings, but since that didn't account for the extra\npadding in usage_with_options_internal() it would come out looking\nmisaligned, e.g. code like this:\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n\nWould emit:\n\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n\nLet's change all the usage arrays which use such continued usage\noutput via \"\\n\"-embedding to be like builtin/stash.c.\n\nThis makes the output worse temporarily, but in a subsequent change\nI'll improve the usage_with_options_internal() to take this into\naccount, at which point all of the strings being changed here will\nemit prettier output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/ls-remote.c | 4 ++--\n builtin/show-branch.c | 6 +++---\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 ++--\n 4 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex f4fd823af83..318949c3d75 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -7,8 +7,8 @@\n \n static const char * const ls_remote_usage[] = {\n \tN_(\"git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\\n\"\n-\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n-\t \" [--symref] [ [...]]\"),\n+\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n+\t \" [--symref] [ [...]]\"),\n \tNULL\n };\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex bea4bbf4680..082449293b5 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -11,9 +11,9 @@\n \n static const char* show_branch_usage[] = {\n N_(\"git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\\n\"\n- \"\t\t[--current] [--color[=] | --no-color] [--sparse]\\n\"\n- \"\t\t[--more= | --list | --independent | --merge-base]\\n\"\n- \"\t\t[--no-name | --sha1-name] [--topics] [( | )...]\"),\n+ \" [--current] [--color[=] | --no-color] [--sparse]\\n\"\n+ \" [--more= | --list | --independent | --merge-base]\\n\"\n+ \" [--no-name | --sha1-name] [--topics] [( | )...]\"),\n N_(\"git show-branch (-g | --reflog)[=[,]] [--list] []\"),\n NULL\n };\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..45b19007d7c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {\n \n static const char * const git_stash_save_usage[] = {\n \tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n-\t \" [-u|--include-untracked] [-a|--all] []\"),\n+\t \" [-u|--include-untracked] [-a|--all] []\"),\n \tNULL\n };\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 065b6bf093e..6535ed27ee9 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,10 +23,10 @@\n \n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ]\\n\"\n-\t\t\"\\t\\t []\"),\n+\t \" []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n+\t \" [--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n-- \n2.33.0.1001.g3ab2ac1eaae\n\n"},{"id":"435674","messageId":"patch-v4-2.4-f10ff775c69-20210912T235347Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","subject":"[PATCH v4 2/4] send-pack: properly use parse_options() API for usage string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:13:20Z","receivedAt":"2021-09-13T00:13:45Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When \"send-pack\" was changed to use the parse_options() API in\n068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)\nit was made to use one very long line, instead it should split them up\nwith newlines.\n\nFurthermore we were including an inline explanation that you couldn't\ncombine \"--all\" and \"\", but unlike in the \"blame\" case this was\nnot preceded by an empty string.\n\nLet's instead show that --all and can't be combined in the the\nusual language of the usage syntax instead. We can make it clear that\none of the two options \"--foo\" and \"--bar\" is mandatory, but that the\ntwo are mutually exclusive by referring to them as \"( --foo | --bar\n)\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-send-pack.txt | 4 ++--\n builtin/send-pack.c | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 44fd146b912..be41f119740 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=]\n+'git send-pack' [--dry-run] [--force] [--receive-pack=]\n \t\t[--verbose] [--thin] [--atomic]\n \t\t[--[no-]signed|--signed=(true|false|if-asked)]\n-\t\t[:] [...]\n+\t\t[:] (--all | ...)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 729dea1d255..89321423125 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -17,10 +17,10 @@\n #include \"protocol.h\"\n \n static const char * const send_pack_usage[] = {\n-\tN_(\"git send-pack [--all | --mirror] [--dry-run] [--force] \"\n-\t \"[--receive-pack=] [--verbose] [--thin] [--atomic] \"\n-\t \"[:] [...]\\n\"\n-\t \" --all and explicit specification are mutually exclusive.\"),\n+\tN_(\"git send-pack [--mirror] [--dry-run] [--force]\\n\"\n+\t \" [--receive-pack=]\\n\"\n+\t \" [--verbose] [--thin] [--atomic]\\n\"\n+\t \" [:] (--all | ...)\"),\n \tNULL,\n };\n \n-- \n2.33.0.1001.g3ab2ac1eaae\n\n"},{"id":"435675","messageId":"patch-v4-3.4-05a0c7cac37-20210912T235347Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","subject":"[PATCH v4 3/4] git rev-parse --parseopt tests: add more usagestr tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:13:21Z","receivedAt":"2021-09-13T00:13:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests for the \"usagestr\" passed to parse-options.c\nusage_with_options_internal() through cmd_parseopt().\n\nThese test for edge cases in the existing behavior related to the\n\"--parseopt\" interface doing its own line-splitting with\nstrbuf_getline(), and the native C interface expecting and potentially\nneeding to handle newlines within the strings in the array it\naccepts. The results are probably something that wasn't anticipated,\nbut let's make sure we stay backwards compatible with it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex b29563fc997..284fe18e726 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -282,4 +282,58 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op\n \ttest_cmp expect output\n '\n \n+test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\" lines' '\n+\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n+\t|cmd [--some-option]\n+\t| [--another-option]\n+\t|cmd [--yet-another-option]\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n+\t|cat <<\\EOF\n+\t|usage: cmd [--some-option]\n+\t| or: [--another-option]\n+\t| or: cmd [--yet-another-option]\n+\t|\n+\t| -h, --help show the help\n+\t|\n+\t|EOF\n+\tEND_EXPECT\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n+\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n+\t|cmd [--some-option]\n+\t| [--another-option]\n+\t|\n+\t|multi\n+\t|line\n+\t|blurb\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n+\t|cat <<\\EOF\n+\t|usage: cmd [--some-option]\n+\t| or: [--another-option]\n+\t|\n+\t| multi\n+\t| line\n+\t| blurb\n+\t|\n+\t| -h, --help show the help\n+\t|\n+\t|EOF\n+\tEND_EXPECT\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.1001.g3ab2ac1eaae\n\n"},{"id":"435676","messageId":"patch-v4-4.4-55480dee680-20210912T235347Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","subject":"[PATCH v4 4/4] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:13:22Z","receivedAt":"2021-09-13T00:13:48Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some commands such as \"git stash\" emit continued options output with\ne.g. \"git stash -h\", because usage_with_options_internal() prefixes\nwith its own whitespace the resulting output wasn't properly\naligned. Let's account for the added whitespace, which properly aligns\nthe output.\n\nThe \"git stash\" command has usage output with a N_() translation that\nlegitimately stretches across multiple lines;\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nWe'd like to have that output aligned with the length of the initial\n\"git stash \" output, but since usage_with_options_internal() adds its\nown whitespace prefixing we fell short, before this change we'd emit:\n\n $ git stash -h\n usage: git stash list []\n or: git stash show [] []\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nNow we'll properly emit aligned output. I.e. the last four lines\nabove will instead be (a whitespace-only change to the above):\n\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nWe could also go for an approach where we have the caller support no\npadding of their own, i.e. (same as the first example, except for the\npadding on the second line):\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nBut to do that we'll need to find the length of \"git stash\". We can\ndiscover that from the \"cmd\" in the \"struct cmd_struct\", but there\nmight cases with sub-commands or \"git\" itself taking arguments that\nwould make that non-trivial.\n\nEven if it was I still think this approach is better, because this way\nwe'll get the same legible alignment in the C code. The fact that\nusage_with_options_internal() is adding its own prefix padding is an\nimplementation detail that callers shouldn't need to worry about.\n\nImplementation notes:\n\nWe could skip the string_list_split() with a strchr(str, '\\n') check,\nbut we'd then need to duplicate our state machine for strings that do\nand don't contain a \"\\n\". It's simpler to just always split into a\n\"struct string_list\", even though the common case is that that \"struct\nstring_list\" will contain only one element. This is not\nperformance-sensitive code.\n\nThis change is relatively more complex since I've accounted for making\nit future-proof for RTL translation support. Later in\nusage_with_options_internal() we have some existing padding code\ndating back to d7a38c54a6c (parse-options: be able to generate usages\nautomatically, 2007-10-15) which isn't RTL-safe, but that code would\nbe easy to fix. Let's not introduce new RTL translation problems here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 76 +++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 64 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..75f0a6c81c5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -917,25 +917,77 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n \n+\tconst char *usage_prefix = _(\"usage: %s\");\n+\t/*\n+\t * The translation could be anything, but we can count on\n+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n+\t * the translation. So compute the length of the \"usage: \"\n+\t * part. We are assuming that the translator wasn't overly\n+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n+\t * one \"%s\" in \"usage_prefix\" above, so there's no reason to\n+\t * do so even with a RTL language.\n+\t */\n+\tsize_t usage_len = strlen(usage_prefix) - strlen(\"%s\");\n+\t/*\n+\t * TRANSLATORS: the colon here should align with the\n+\t * one in \"usage: %s\" translation.\n+\t */\n+\tconst char *or_prefix = _(\" or: %s\");\n+\t/*\n+\t * TRANSLATORS: You should only need to translate this format\n+\t * string if your language is a RTL language (e.g. Arabic,\n+\t * Hebrew etc.), not if it's a LTR language (e.g. German,\n+\t * Russian, Chinese etc.).\n+\t *\n+\t * When a translated usage string has an embedded \"\\n\" it's\n+\t * because options have wrapped to the next line. The line\n+\t * after the \"\\n\" will then be padded to align with the\n+\t * command name, such as N_(\"git cmd [opt]\\n<8\n+\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n+\t * \"git cmd \".\n+\t *\n+\t * This format string prints out that already-translated\n+\t * line. The \"%*s\" is whitespace padding to account for the\n+\t * padding at the start of the line that we add in this\n+\t * function. The \"%s\" is a line in the (hopefully already\n+\t * translated) N_() usage string, which contained embedded\n+\t * newlines before we split it up.\n+\t */\n+\tconst char *usage_continued = _(\"%*s%s\");\n+\tconst char *prefix = usage_prefix;\n+\tint saw_empty_line = 0;\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n \tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n-\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n-\twhile (*usagestr && **usagestr)\n-\t\t/*\n-\t\t * TRANSLATORS: the colon here should align with the\n-\t\t * one in \"usage: %s\" translation.\n-\t\t */\n-\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n \twhile (*usagestr) {\n-\t\tif (**usagestr)\n-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n-\t\telse\n-\t\t\tfputc('\\n', outfile);\n-\t\tusagestr++;\n+\t\tconst char *str = _(*usagestr++);\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tunsigned int j;\n+\n+\t\tif (!saw_empty_line && !*str)\n+\t\t\tsaw_empty_line = 1;\n+\n+\t\tstring_list_split(&list, str, '\\n', -1);\n+\t\tfor (j = 0; j < list.nr; j++) {\n+\t\t\tconst char *line = list.items[j].string;\n+\n+\t\t\tif (saw_empty_line && *line)\n+\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n+\t\t\telse if (saw_empty_line)\n+\t\t\t\tfputc('\\n', outfile);\n+\t\t\telse if (!j)\n+\t\t\t\tfprintf_ln(outfile, prefix, line);\n+\t\t\telse\n+\t\t\t\tfprintf_ln(outfile, usage_continued,\n+\t\t\t\t\t (int)usage_len, \"\", line);\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\n+\t\tprefix = or_prefix;\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.1001.g3ab2ac1eaae\n\n"},{"id":"435678","messageId":"878s015mtg.fsf@evledraar.gmail.com","threadId":"56423","inReplyTo":"xmqqpmtdjuqj.fsf@gitster.g","subject":"Re: [PATCH v3 3/6] parse-options: stop supporting \"\" in the usagestr array","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-13T00:21:33Z","receivedAt":"2021-09-13T00:41:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Sun, Sep 12 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason writes:\n>\n>> diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh\n>> index ad4746d899a..2910874ece5 100755\n>> --- a/t/t0040-parse-options.sh\n>> +++ b/t/t0040-parse-options.sh\n>> @@ -10,8 +10,6 @@ test_description='our own option parser'\n>> cat >expect <<\\EOF\n>> usage: test-tool parse-options \n>> \n>> - A helper function for the parse-options API.\n>> -\n>> --yes get a boolean\n>> -D, --no-doubt begins with 'no-'\n>> -B, --no-fear be brave\n>\n> Isn't this, and a lot more importantly the next one ...\n>\n>> diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\n>> index b29563fc997..6badc650d64 100755\n>> --- a/t/t1502-rev-parse-parseopt.sh\n>> +++ b/t/t1502-rev-parse-parseopt.sh\n>> @@ -6,8 +6,6 @@ test_description='test git rev-parse --parseopt'\n>> test_expect_success 'setup optionspec' '\n>> \tsed -e \"s/^|//\" >optionspec <<\\EOF\n>> |some-command [options] ...\n>> -|\n>> -|some-command does foo and bar!\n>> |--\n>> |h,help show the help\n>> |\n>\n> ... coalmine canaries that tell us that end-user's scripts using the\n> \"git rev-parse --parseopt\" in the documented way will be broken?\n>\n> I'd rather not have to sorry about breaking end-user scripts this\n> way. Unlike a very small number of in-tree parse_options() call in\n> C programs, we have unbounded of them.\n\nI re-rolled this in v4 to not change how the function works at all:\nhttps://lore.kernel.org/git/cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com/\n\nI do think we're going above & beyond what's reasonable in maintaining\nthese interfaces, i.e. per 21d47835386 (Add a parseopt mode to\ngit-rev-parse to bring parse-options to shell scripts., 2007-11-04) I\ndon't think anyone thought that interface would be used outside of\ngit.git.\n\nBut then perhaps I'm wrong, or it has been, and since we ended up\nsticking it in rev-parse which was marked as plumbing we can't change\nhow it or the underlying C APIs it relies on work.\n\nIn practice I doubt it would break things for anyone, but per the result\nof the discussion to \"remove dead shell code\" [1] I don't think I'll get\nanywhere with that.\n\nI do think that getting some answer to [2] & how we think about the\nlikes of \"git rev-parse --parseopt\" going forward would be very\nuseful.\n\nI.e. would you be OK with us marking these as deprecated / warn on their\nout-of-tree use? If so we could after some time remove a lot of code &\nsimplify things once the last in-tree shellscript user goes away.\n\nOr if we don't see that they can ever be changed that's also useful to\nknow (and per [2] I think I'd feel obligated to send in a revert\nbringing git-parse-remote back).\n\n1. https://lore.kernel.org/git/cover-v3-0.4-00000000000-20210911T111435Z-avarab@gmail.com/\n2. https://lore.kernel.org/git/87tuiwjfvi.fsf@evledraar.gmail.com/\n"},{"id":"435694","messageId":"xmqq8s01geov.fsf@gitster.g","threadId":"56423","inReplyTo":"patch-v4-4.4-55480dee680-20210912T235347Z-avarab@gmail.com","subject":"Re: [PATCH v4 4/4] parse-options: properly align continued usage output","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-13T06:41:36Z","receivedAt":"2021-09-13T06:41:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason writes:\n\n> Some commands such as \"git stash\" emit continued options output with\n> e.g. \"git stash -h\", because usage_with_options_internal() prefixes\n> with its own whitespace the resulting output wasn't properly\n> aligned. Let's account for the added whitespace, which properly aligns\n> the output.\n>\n> The \"git stash\" command has usage output with a N_() translation that\n> legitimately stretches across multiple lines;\n>\n> \tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n> \t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n> [...]\n>\n> We'd like to have that output aligned with the length of the initial\n> \"git stash \" output, but since usage_with_options_internal() adds its\n> own whitespace prefixing we fell short, before this change we'd emit:\n>\n> $ git stash -h\n> usage: git stash list []\n> or: git stash show [] []\n> [...]\n> or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n> [-u|--include-untracked] [-a|--all] [-m|--message ]\n> [...]\n>\n> Now we'll properly emit aligned output. I.e. the last four lines\n> above will instead be (a whitespace-only change to the above):\n>\n> [...]\n> or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n> [-u|--include-untracked] [-a|--all] [-m|--message ]\n> [...]\n>\n> We could also go for an approach where we have the caller support no\n> padding of their own, i.e. (same as the first example, except for the\n> padding on the second line):\n>\n> \tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n> \t \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n> [...]\n>\n> But to do that we'll need to find the length of \"git stash\". We can\n> discover that from the \"cmd\" in the \"struct cmd_struct\", but there\n> might cases with sub-commands or \"git\" itself taking arguments that\n\nA \"be\" is missing here.\n\n> would make that non-trivial.\n>\n> Even if it was I still think this approach is better, because this way\n\n\"was\" -> \"were,\", I think.\n\nEven though this step has quite a lot of comment strings meant for\nhuman consumption, I didn't hunt for grammos and typos and only\nlooked at the code. Somebody else may want to proof-read it.\n\nThe new code looked OK. Do we want to have some new test in\nt/helper/test-*.c with fixed \"command help string\"?\n\nThanks.\n"},{"id":"436608","messageId":"cover-v5-0.4-00000000000-20210921T132350Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com","subject":"[PATCH v5 0/4] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:30:07Z","receivedAt":"2021-09-21T13:30:21Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"This series changes usage_with_options_internal() in parse-options.c\nto properly align continued \"\\n\" usage output.\n\nThere's only a very small typo changes in this v5 in response to\nhttps://lore.kernel.org/git/xmqq8s01geov.fsf@gitster.g/; I also\nre-read the rest over & spellchecked the comments I added, for\nwhatever that's worth.\n\nI didn't add a test in t/helper/ as Junio suggested, since it's the\nusage message itself it doesn't slot nicely into the control flow of\nt/helper/test-run-command.c, we could of course make it work, but I'd\nprefer to punt on it.\n\nI think having manually tested the output should do in this\ncase. I.e. I manually looked at all the \"-h\" emitted by t0012-help.sh\nand it's all nicely formatted now (aside from some overly long lines,\naddressing that is left for another day).\n\nÆvar Arnfjörð Bjarmason (4):\n parse-options API users: align usage output in C-strings\n send-pack: properly use parse_options() API for usage string\n git rev-parse --parseopt tests: add more usagestr tests\n parse-options: properly align continued usage output\n\n Documentation/git-send-pack.txt | 4 +-\n builtin/ls-remote.c | 4 +-\n builtin/send-pack.c | 8 ++--\n builtin/show-branch.c | 6 +--\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 +-\n parse-options.c | 76 +++++++++++++++++++++++++++------\n t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++\n 8 files changed, 132 insertions(+), 26 deletions(-)\n\nRange-diff against v4:\n1: 64d8647340d = 1: 352662be92d parse-options API users: align usage output in C-strings\n2: f10ff775c69 = 2: d3767214d22 send-pack: properly use parse_options() API for usage string\n3: 05a0c7cac37 = 3: 8262999b456 git rev-parse --parseopt tests: add more usagestr tests\n4: 55480dee680 ! 4: 9f7f3f8b4ed parse-options: properly align continued usage output\n @@ Commit message\n \n But to do that we'll need to find the length of \"git stash\". We can\n discover that from the \"cmd\" in the \"struct cmd_struct\", but there\n - might cases with sub-commands or \"git\" itself taking arguments that\n + might be cases with sub-commands or \"git\" itself taking arguments that\n would make that non-trivial.\n \n - Even if it was I still think this approach is better, because this way\n + Even if it were I still think this approach is better, because this way\n we'll get the same legible alignment in the C code. The fact that\n usage_with_options_internal() is adding its own prefix padding is an\n implementation detail that callers shouldn't need to worry about.\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436609","messageId":"patch-v5-2.4-d3767214d22-20210921T132350Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v5-0.4-00000000000-20210921T132350Z-avarab@gmail.com","subject":"[PATCH v5 2/4] send-pack: properly use parse_options() API for usage string","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:30:09Z","receivedAt":"2021-09-21T13:30:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"When \"send-pack\" was changed to use the parse_options() API in\n068c77a5189 (builtin/send-pack.c: use parse_options API, 2015-08-19)\nit was made to use one very long line, instead it should split them up\nwith newlines.\n\nFurthermore we were including an inline explanation that you couldn't\ncombine \"--all\" and \"\", but unlike in the \"blame\" case this was\nnot preceded by an empty string.\n\nLet's instead show that --all and can't be combined in the the\nusual language of the usage syntax instead. We can make it clear that\none of the two options \"--foo\" and \"--bar\" is mandatory, but that the\ntwo are mutually exclusive by referring to them as \"( --foo | --bar\n)\".\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n Documentation/git-send-pack.txt | 4 ++--\n builtin/send-pack.c | 8 ++++----\n 2 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/git-send-pack.txt b/Documentation/git-send-pack.txt\nindex 44fd146b912..be41f119740 100644\n--- a/Documentation/git-send-pack.txt\n+++ b/Documentation/git-send-pack.txt\n@@ -9,10 +9,10 @@ git-send-pack - Push objects over Git protocol to another repository\n SYNOPSIS\n --------\n [verse]\n-'git send-pack' [--all] [--dry-run] [--force] [--receive-pack=]\n+'git send-pack' [--dry-run] [--force] [--receive-pack=]\n \t\t[--verbose] [--thin] [--atomic]\n \t\t[--[no-]signed|--signed=(true|false|if-asked)]\n-\t\t[:] [...]\n+\t\t[:] (--all | ...)\n \n DESCRIPTION\n -----------\ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex 729dea1d255..89321423125 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -17,10 +17,10 @@\n #include \"protocol.h\"\n \n static const char * const send_pack_usage[] = {\n-\tN_(\"git send-pack [--all | --mirror] [--dry-run] [--force] \"\n-\t \"[--receive-pack=] [--verbose] [--thin] [--atomic] \"\n-\t \"[:] [...]\\n\"\n-\t \" --all and explicit specification are mutually exclusive.\"),\n+\tN_(\"git send-pack [--mirror] [--dry-run] [--force]\\n\"\n+\t \" [--receive-pack=]\\n\"\n+\t \" [--verbose] [--thin] [--atomic]\\n\"\n+\t \" [:] (--all | ...)\"),\n \tNULL,\n };\n \n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436610","messageId":"patch-v5-3.4-8262999b456-20210921T132350Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v5-0.4-00000000000-20210921T132350Z-avarab@gmail.com","subject":"[PATCH v5 3/4] git rev-parse --parseopt tests: add more usagestr tests","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:30:10Z","receivedAt":"2021-09-21T13:30:28Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Add tests for the \"usagestr\" passed to parse-options.c\nusage_with_options_internal() through cmd_parseopt().\n\nThese test for edge cases in the existing behavior related to the\n\"--parseopt\" interface doing its own line-splitting with\nstrbuf_getline(), and the native C interface expecting and potentially\nneeding to handle newlines within the strings in the array it\naccepts. The results are probably something that wasn't anticipated,\nbut let's make sure we stay backwards compatible with it.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++++++++++++++\n 1 file changed, 54 insertions(+)\n\ndiff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh\nindex b29563fc997..284fe18e726 100755\n--- a/t/t1502-rev-parse-parseopt.sh\n+++ b/t/t1502-rev-parse-parseopt.sh\n@@ -282,4 +282,58 @@ test_expect_success 'test --parseopt --stuck-long and short option with unset op\n \ttest_cmp expect output\n '\n \n+test_expect_success 'test --parseopt help output: \"wrapped\" options normal \"or:\" lines' '\n+\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n+\t|cmd [--some-option]\n+\t| [--another-option]\n+\t|cmd [--yet-another-option]\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n+\t|cat <<\\EOF\n+\t|usage: cmd [--some-option]\n+\t| or: [--another-option]\n+\t| or: cmd [--yet-another-option]\n+\t|\n+\t| -h, --help show the help\n+\t|\n+\t|EOF\n+\tEND_EXPECT\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '\n+\tsed -e \"s/^|//\" >spec <<-\\EOF &&\n+\t|cmd [--some-option]\n+\t| [--another-option]\n+\t|\n+\t|multi\n+\t|line\n+\t|blurb\n+\t|--\n+\t|h,help show the help\n+\tEOF\n+\n+\tsed -e \"s/^|//\" >expect <<-\\END_EXPECT &&\n+\t|cat <<\\EOF\n+\t|usage: cmd [--some-option]\n+\t| or: [--another-option]\n+\t|\n+\t| multi\n+\t| line\n+\t| blurb\n+\t|\n+\t| -h, --help show the help\n+\t|\n+\t|EOF\n+\tEND_EXPECT\n+\n+\ttest_must_fail git rev-parse --parseopt -- -h >out actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436611","messageId":"patch-v5-4.4-9f7f3f8b4ed-20210921T132350Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v5-0.4-00000000000-20210921T132350Z-avarab@gmail.com","subject":"[PATCH v5 4/4] parse-options: properly align continued usage output","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:30:11Z","receivedAt":"2021-09-21T13:30:36Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Some commands such as \"git stash\" emit continued options output with\ne.g. \"git stash -h\", because usage_with_options_internal() prefixes\nwith its own whitespace the resulting output wasn't properly\naligned. Let's account for the added whitespace, which properly aligns\nthe output.\n\nThe \"git stash\" command has usage output with a N_() translation that\nlegitimately stretches across multiple lines;\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nWe'd like to have that output aligned with the length of the initial\n\"git stash \" output, but since usage_with_options_internal() adds its\nown whitespace prefixing we fell short, before this change we'd emit:\n\n $ git stash -h\n usage: git stash list []\n or: git stash show [] []\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nNow we'll properly emit aligned output. I.e. the last four lines\nabove will instead be (a whitespace-only change to the above):\n\n [...]\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n [...]\n\nWe could also go for an approach where we have the caller support no\npadding of their own, i.e. (same as the first example, except for the\npadding on the second line):\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \"[-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n [...]\n\nBut to do that we'll need to find the length of \"git stash\". We can\ndiscover that from the \"cmd\" in the \"struct cmd_struct\", but there\nmight be cases with sub-commands or \"git\" itself taking arguments that\nwould make that non-trivial.\n\nEven if it were I still think this approach is better, because this way\nwe'll get the same legible alignment in the C code. The fact that\nusage_with_options_internal() is adding its own prefix padding is an\nimplementation detail that callers shouldn't need to worry about.\n\nImplementation notes:\n\nWe could skip the string_list_split() with a strchr(str, '\\n') check,\nbut we'd then need to duplicate our state machine for strings that do\nand don't contain a \"\\n\". It's simpler to just always split into a\n\"struct string_list\", even though the common case is that that \"struct\nstring_list\" will contain only one element. This is not\nperformance-sensitive code.\n\nThis change is relatively more complex since I've accounted for making\nit future-proof for RTL translation support. Later in\nusage_with_options_internal() we have some existing padding code\ndating back to d7a38c54a6c (parse-options: be able to generate usages\nautomatically, 2007-10-15) which isn't RTL-safe, but that code would\nbe easy to fix. Let's not introduce new RTL translation problems here.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n parse-options.c | 76 +++++++++++++++++++++++++++++++++++++++++--------\n 1 file changed, 64 insertions(+), 12 deletions(-)\n\ndiff --git a/parse-options.c b/parse-options.c\nindex 2abff136a17..75f0a6c81c5 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -917,25 +917,77 @@ static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,\n \tFILE *outfile = err ? stderr : stdout;\n \tint need_newline;\n \n+\tconst char *usage_prefix = _(\"usage: %s\");\n+\t/*\n+\t * The translation could be anything, but we can count on\n+\t * msgfmt(1)'s --check option to have asserted that \"%s\" is in\n+\t * the translation. So compute the length of the \"usage: \"\n+\t * part. We are assuming that the translator wasn't overly\n+\t * clever and used e.g. \"%1$s\" instead of \"%s\", there's only\n+\t * one \"%s\" in \"usage_prefix\" above, so there's no reason to\n+\t * do so even with a RTL language.\n+\t */\n+\tsize_t usage_len = strlen(usage_prefix) - strlen(\"%s\");\n+\t/*\n+\t * TRANSLATORS: the colon here should align with the\n+\t * one in \"usage: %s\" translation.\n+\t */\n+\tconst char *or_prefix = _(\" or: %s\");\n+\t/*\n+\t * TRANSLATORS: You should only need to translate this format\n+\t * string if your language is a RTL language (e.g. Arabic,\n+\t * Hebrew etc.), not if it's a LTR language (e.g. German,\n+\t * Russian, Chinese etc.).\n+\t *\n+\t * When a translated usage string has an embedded \"\\n\" it's\n+\t * because options have wrapped to the next line. The line\n+\t * after the \"\\n\" will then be padded to align with the\n+\t * command name, such as N_(\"git cmd [opt]\\n<8\n+\t * spaces>[opt2]\"), where the 8 spaces are the same length as\n+\t * \"git cmd \".\n+\t *\n+\t * This format string prints out that already-translated\n+\t * line. The \"%*s\" is whitespace padding to account for the\n+\t * padding at the start of the line that we add in this\n+\t * function. The \"%s\" is a line in the (hopefully already\n+\t * translated) N_() usage string, which contained embedded\n+\t * newlines before we split it up.\n+\t */\n+\tconst char *usage_continued = _(\"%*s%s\");\n+\tconst char *prefix = usage_prefix;\n+\tint saw_empty_line = 0;\n+\n \tif (!usagestr)\n \t\treturn PARSE_OPT_HELP;\n \n \tif (!err && ctx && ctx->flags & PARSE_OPT_SHELL_EVAL)\n \t\tfprintf(outfile, \"cat <<\\\\EOF\\n\");\n \n-\tfprintf_ln(outfile, _(\"usage: %s\"), _(*usagestr++));\n-\twhile (*usagestr && **usagestr)\n-\t\t/*\n-\t\t * TRANSLATORS: the colon here should align with the\n-\t\t * one in \"usage: %s\" translation.\n-\t\t */\n-\t\tfprintf_ln(outfile, _(\" or: %s\"), _(*usagestr++));\n \twhile (*usagestr) {\n-\t\tif (**usagestr)\n-\t\t\tfprintf_ln(outfile, _(\" %s\"), _(*usagestr));\n-\t\telse\n-\t\t\tfputc('\\n', outfile);\n-\t\tusagestr++;\n+\t\tconst char *str = _(*usagestr++);\n+\t\tstruct string_list list = STRING_LIST_INIT_DUP;\n+\t\tunsigned int j;\n+\n+\t\tif (!saw_empty_line && !*str)\n+\t\t\tsaw_empty_line = 1;\n+\n+\t\tstring_list_split(&list, str, '\\n', -1);\n+\t\tfor (j = 0; j < list.nr; j++) {\n+\t\t\tconst char *line = list.items[j].string;\n+\n+\t\t\tif (saw_empty_line && *line)\n+\t\t\t\tfprintf_ln(outfile, _(\" %s\"), line);\n+\t\t\telse if (saw_empty_line)\n+\t\t\t\tfputc('\\n', outfile);\n+\t\t\telse if (!j)\n+\t\t\t\tfprintf_ln(outfile, prefix, line);\n+\t\t\telse\n+\t\t\t\tfprintf_ln(outfile, usage_continued,\n+\t\t\t\t\t (int)usage_len, \"\", line);\n+\t\t}\n+\t\tstring_list_clear(&list, 0);\n+\n+\t\tprefix = or_prefix;\n \t}\n \n \tneed_newline = 1;\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"},{"id":"436612","messageId":"patch-v5-1.4-352662be92d-20210921T132350Z-avarab@gmail.com","threadId":"56423","inReplyTo":"cover-v5-0.4-00000000000-20210921T132350Z-avarab@gmail.com","subject":"[PATCH v5 1/4] parse-options API users: align usage output in C-strings","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-09-21T13:30:08Z","receivedAt":"2021-09-21T13:30:46Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"In preparation for having continued usage lines properly aligned in\n\"git -h\" output, let's have the \"[\" on the second such lines\nalign with the \"[\" on the first line.\n\nIn some cases this makes the output worse, because e.g. the \"git\nls-remote -h\" output had been aligned to account for the extra\nwhitespace that the usage_with_options_internal() function in\nparse-options.c would add.\n\nIn other cases such as builtin/stash.c (not changed here), we were\naligned in the C strings, but since that didn't account for the extra\npadding in usage_with_options_internal() it would come out looking\nmisaligned, e.g. code like this:\n\n\tN_(\"git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n\t \" [-u|--include-untracked] [-a|--all] [-m|--message ]\\n\"\n\nWould emit:\n\n or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n [-u|--include-untracked] [-a|--all] [-m|--message ]\n\nLet's change all the usage arrays which use such continued usage\noutput via \"\\n\"-embedding to be like builtin/stash.c.\n\nThis makes the output worse temporarily, but in a subsequent change\nI'll improve the usage_with_options_internal() to take this into\naccount, at which point all of the strings being changed here will\nemit prettier output.\n\nSigned-off-by: Ævar Arnfjörð Bjarmason \n---\n builtin/ls-remote.c | 4 ++--\n builtin/show-branch.c | 6 +++---\n builtin/stash.c | 2 +-\n builtin/tag.c | 4 ++--\n 4 files changed, 8 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/ls-remote.c b/builtin/ls-remote.c\nindex f4fd823af83..318949c3d75 100644\n--- a/builtin/ls-remote.c\n+++ b/builtin/ls-remote.c\n@@ -7,8 +7,8 @@\n \n static const char * const ls_remote_usage[] = {\n \tN_(\"git ls-remote [--heads] [--tags] [--refs] [--upload-pack=]\\n\"\n-\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n-\t \" [--symref] [ [...]]\"),\n+\t \" [-q | --quiet] [--exit-code] [--get-url]\\n\"\n+\t \" [--symref] [ [...]]\"),\n \tNULL\n };\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex bea4bbf4680..082449293b5 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -11,9 +11,9 @@\n \n static const char* show_branch_usage[] = {\n N_(\"git show-branch [-a | --all] [-r | --remotes] [--topo-order | --date-order]\\n\"\n- \"\t\t[--current] [--color[=] | --no-color] [--sparse]\\n\"\n- \"\t\t[--more= | --list | --independent | --merge-base]\\n\"\n- \"\t\t[--no-name | --sha1-name] [--topics] [( | )...]\"),\n+ \" [--current] [--color[=] | --no-color] [--sparse]\\n\"\n+ \" [--more= | --list | --independent | --merge-base]\\n\"\n+ \" [--no-name | --sha1-name] [--topics] [( | )...]\"),\n N_(\"git show-branch (-g | --reflog)[=[,]] [--list] []\"),\n NULL\n };\ndiff --git a/builtin/stash.c b/builtin/stash.c\nindex 8f42360ca91..45b19007d7c 100644\n--- a/builtin/stash.c\n+++ b/builtin/stash.c\n@@ -85,7 +85,7 @@ static const char * const git_stash_push_usage[] = {\n \n static const char * const git_stash_save_usage[] = {\n \tN_(\"git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\\n\"\n-\t \" [-u|--include-untracked] [-a|--all] []\"),\n+\t \" [-u|--include-untracked] [-a|--all] []\"),\n \tNULL\n };\n \ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 065b6bf093e..6535ed27ee9 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -23,10 +23,10 @@\n \n static const char * const git_tag_usage[] = {\n \tN_(\"git tag [-a | -s | -u ] [-f] [-m | -F ]\\n\"\n-\t\t\"\\t\\t []\"),\n+\t \" []\"),\n \tN_(\"git tag -d ...\"),\n \tN_(\"git tag -l [-n[]] [--contains ] [--no-contains ] [--points-at ]\\n\"\n-\t\t\"\\t\\t[--format=] [--merged ] [--no-merged ] [...]\"),\n+\t \" [--format=] [--merged ] [--no-merged ] [...]\"),\n \tN_(\"git tag -v [--format=] ...\"),\n \tNULL\n };\n-- \n2.33.0.1098.gf02a64c1a2d\n\n"}]}