From: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
To: git@vger.kernel.org
Cc: "Junio C Hamano" <gitster@pobox.com>, "Jeff King" <peff@peff.net>,
"Carlo Arenas" <carenas@gmail.com>,
"Eric Sunshine" <sunshine@sunshineco.com>,
"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: [PATCH v2 0/6] parse-options: properly align continued usage output & related
Date: Fri, 10 Sep 2021 17:38:30 +0200 [thread overview]
Message-ID: <cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-0.2-00000000000-20210901T110917Z-avarab@gmail.com>
This series changes usage_with_options_internal() in parse-options.c
to properly align continued "\n" usage output.
This v2 now also gets rid of the support for "" in the usage string
array. Eric Sunshine had ideas[1] for how to simplify the code in v1,
along with a suggestion that we could just get rid of the "" from
"builtin/blame.c".
I'd done some experiments with that before submitting v1, but decided
to try to submit something simpler to begin with. But let's just grow
this in scope a bit and do that, as shown here we also need to do the
same in builtin/credential-cache*.c.
This gives us a much nicer end-state, and as an added bonus
effectively brings back the check removed with the removal of
USE_PARENS_AROUND_GETTEXT_N in in-flight ]2], which is what brought
this alignment issue & edge cases in parse_options() usage to my
attention in the first place.
1. https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/
2. https://lore.kernel.org/git/878s0gwmvq.fsf@evledraar.gmail.com/
Ævar Arnfjörð Bjarmason (6):
test-lib.sh: add a UNIX_SOCKETS prerequisite
git.c: add a NEED_UNIX_SOCKETS option for built-ins
parse-options: stop supporting "" in the usagestr array
built-ins: "properly" align continued usage output
send-pack: properly use parse_options() API for usage string
parse-options: properly align continued usage output
Documentation/git-send-pack.txt | 4 +-
builtin.h | 6 +++
builtin/blame.c | 9 ++--
builtin/credential-cache--daemon.c | 11 +----
builtin/credential-cache.c | 11 +----
builtin/ls-remote.c | 4 +-
builtin/rev-parse.c | 3 ++
builtin/send-pack.c | 8 ++--
builtin/show-branch.c | 6 +--
builtin/stash.c | 2 +-
builtin/tag.c | 4 +-
git.c | 15 +++++--
parse-options.c | 71 +++++++++++++++++++++++++-----
t/helper/test-parse-options.c | 2 -
t/t0012-help.sh | 10 +++++
t/t0040-parse-options.sh | 2 -
t/t0301-credential-cache.sh | 5 ++-
t/t1502-rev-parse-parseopt.sh | 34 +++++++-------
t/test-lib.sh | 1 +
19 files changed, 131 insertions(+), 77 deletions(-)
Range-diff against v1:
-: ----------- > 1: 9e8facb6f8c test-lib.sh: add a UNIX_SOCKETS prerequisite
-: ----------- > 2: d6c44402715 git.c: add a NEED_UNIX_SOCKETS option for built-ins
-: ----------- > 3: 11f4a119563 parse-options: stop supporting "" in the usagestr array
1: ccc024c414f ! 4: 4547cc968b1 built-ins: "properly" align continued usage output
@@ Commit message
output.
But two wrongs don't make a right, let's "fix" this by making it worse
- temporarily, in anticipating of improving parse-options.c to handle
+ temporarily, in anticipation of improving parse-options.c to handle
this alignment.
The issue is that we should have whitespace corresponding to the
@@ builtin/stash.c: static const char * const git_stash_push_usage[] = {
## builtin/tag.c ##
-@@ builtin/tag.c: static const char * const git_tag_usage[] = {
- "\t\t<tagname> [<head>]"),
+@@
+
+ static const char * const git_tag_usage[] = {
+ N_("git tag [-a | -s | -u <key-id>] [-f] [-m <msg> | -F <file>]\n"
+- "\t\t<tagname> [<head>]"),
++ " <tagname> [<head>]"),
N_("git tag -d <tagname>..."),
N_("git tag -l [-n[<num>]] [--contains <commit>] [--no-contains <commit>] [--points-at <object>]\n"
- "\t\t[--format=<format>] [--merged <commit>] [--no-merged <commit>] [<pattern>...]"),
-: ----------- > 5: b884b361ace send-pack: properly use parse_options() API for usage string
2: ab4bb70902b ! 6: e83d66da6d4 parse-options: properly align continued usage output
@@ Commit message
N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
" [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
+ [...]
We'd like to have that output aligned with the length of the initial
"git stash " output, but since usage_with_options_internal() adds its
@@ Commit message
[-u|--include-untracked] [-a|--all] [-m|--message <message>]
[...]
- In making this change we can can fold the two for-loops over *usagestr
- into one. We had two of them purely to account for the case where an
- empty string in the array delimits the usage output from free-form
- text output.
+ We could also go for an approach where we have the caller support no
+ padding of their own, i.e. (same as the first example, except for the
+ padding on the second line):
+
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
+ "[-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
+ [...]
+
+ But to do that we'll need to find the length of "git stash". We can
+ discover that from the "cmd" in the "struct cmd_struct", but there
+ might cases with sub-commands or "git" itself taking arguments that
+ would make that non-trivial.
+
+ Even if it was I still think this approach is better, because this way
+ we'll get the same legible alignment in the C code. The fact that
+ usage_with_options_internal() is adding its own prefix padding is an
+ implementation detail that callers shouldn't need to worry about.
+
+ Implementation notes:
We could skip the string_list_split() with a strchr(str, '\n') check,
but we'd then need to duplicate our state machine for strings that do
@@ Commit message
automatically, 2007-10-15) which isn't RTL-safe, but that code would
be easy to fix. Let's not introduce new RTL translation problems here.
+ I'm also adding a check to catch the mistake of needlessly adding a
+ trailing "\n", such as:
+
+ N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [<message>]\n"),
+
+ Or even a mistake like adding just one "\n" in a string with no other
+ newlines:
+
+ N_("git stash list [<options>]\n"),
+
+ This catches the cases already tested for in cmd_parseopt(), but this
+ covers the purely C API. As noted a preceding commit that added the
+ die() to cmd_parseopt() I'm fairly confident that this will be
+ triggered by no in-tree user I've missed.
+
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## parse-options.c ##
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
+ const char *usage_prefix = _("usage: %s");
+ /*
++ * The translation could be anything, but we can count on
++ * msgfmt(1)'s --check option to have asserted that "%s" is in
++ * the translation. So compute the length of the "usage: "
++ * part. We are assuming that the translator wasn't overly
++ * clever and used e.g. "%1$s" instead of "%s", there's only
++ * one "%s" in "usage_prefix" above, so there's no reason to
++ * do so even with a RTL language.
++ */
++ size_t usage_len = strlen(usage_prefix) - strlen("%s");
++ /*
+ * TRANSLATORS: the colon here should align with the
+ * one in "usage: %s" translation.
+ */
+ const char *or_prefix = _(" or: %s");
++
+ /*
+ * TRANSLATORS: You should only need to translate this format
+ * string if your language is a RTL language (e.g. Arabic,
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
+ * Russian, Chinese etc.).
+ *
+ * When a translated usage string has an embedded "\n" it's
-+ * because options have wrapped o the next line. The line
++ * because options have wrapped to the next line. The line
+ * after the "\n" will then be padded to align with the
+ * command name, such as N_("git cmd [opt]\n<8
+ * spaces>[opt2]"), where the 8 spaces are the same length as
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
+ * This format string prints out that already-translated
+ * line. The "%*s" is whitespace padding to account for the
+ * padding at the start of the line that we add in this
-+ * function, the "%s" is a line in the (hopefully already
++ * function. The "%s" is a line in the (hopefully already
+ * translated) N_() usage string, which contained embedded
+ * newlines before we split it up.
+ */
+ const char *usage_continued = _("%*s%s");
-+
-+ /*
-+ * The translation could be anything, but we can count on
-+ * msgfmt(1)'s --check option to have asserted that "%s" is in
-+ * the translation. So compute the length of the " or: "
-+ * part. We are assuming that the translator wasn't overly
-+ * clever and used e.g. "%1$s" instead of "%s", there's only
-+ * one "%s" in "or_prefix" above, so there's no reason to do
-+ * so even with a RTL language.
-+ */
-+ size_t or_len = strlen(or_prefix) - strlen("%s");
-+ int i;
-+ int saw_empty_line = 0;
++ const char *prefix = usage_prefix;
+
if (!usagestr)
return PARSE_OPT_HELP;
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
fprintf(outfile, "cat <<\\EOF\n");
- fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
-- while (*usagestr && **usagestr)
+ while (*usagestr) {
- /*
- * TRANSLATORS: the colon here should align with the
- * one in "usage: %s" translation.
- */
- fprintf_ln(outfile, _(" or: %s"), _(*usagestr++));
-- while (*usagestr) {
-- if (**usagestr)
-- fprintf_ln(outfile, _(" %s"), _(*usagestr));
-- else
-- fputc('\n', outfile);
-- usagestr++;
-+ for (i = 0; *usagestr; i++) {
-+ const char *str = _(*usagestr++);
+ struct string_list list = STRING_LIST_INIT_DUP;
+ unsigned int j;
+
-+ string_list_split(&list, str, '\n', -1);
++ string_list_split(&list, _(*usagestr++), '\n', -1);
+ for (j = 0; j < list.nr; j++) {
+ const char *line = list.items[j].string;
+
-+ if (!saw_empty_line && !*line)
-+ saw_empty_line = 1;
++ if (!*line)
++ BUG("empty or trailing line in usage string");
+
-+ if (saw_empty_line && *line)
-+ fprintf_ln(outfile, _(" %s"), line);
-+ else if (saw_empty_line)
-+ fputc('\n', outfile);
-+ else if (!j && !i)
-+ fprintf_ln(outfile, usage_prefix, line);
-+ else if (!j)
-+ fprintf_ln(outfile, or_prefix, line);
++ if (!j)
++ fprintf_ln(outfile, prefix, line);
+ else
+ fprintf_ln(outfile, usage_continued,
-+ (int)or_len, "", line);
++ (int)usage_len, "", line);
+ }
+ string_list_clear(&list, 0);
++
++ prefix = or_prefix;
}
need_newline = 1;
--
2.33.0.876.g423ac861752
next prev parent reply other threads:[~2021-09-10 15:38 UTC|newest]
Thread overview: 45+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-09-01 11:12 [PATCH 0/2] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-01 11:12 ` [PATCH 1/2] built-ins: "properly" " Ævar Arnfjörð Bjarmason
2021-09-01 11:12 ` [PATCH 2/2] parse-options: properly " Ævar Arnfjörð Bjarmason
2021-09-10 7:51 ` Eric Sunshine
2021-09-10 15:38 ` Ævar Arnfjörð Bjarmason [this message]
2021-09-10 15:38 ` [PATCH v2 1/6] test-lib.sh: add a UNIX_SOCKETS prerequisite Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins Ævar Arnfjörð Bjarmason
2021-09-11 0:14 ` Junio C Hamano
2021-09-11 2:50 ` Ævar Arnfjörð Bjarmason
2021-09-11 3:47 ` Carlo Marcelo Arenas Belón
2021-09-11 6:12 ` Junio C Hamano
2021-09-11 7:16 ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-11 0:21 ` Junio C Hamano
2021-09-11 2:46 ` Ævar Arnfjörð Bjarmason
2021-09-11 6:52 ` Junio C Hamano
2021-09-10 15:38 ` [PATCH v2 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 0:25 ` Junio C Hamano
2021-09-11 2:40 ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 7:41 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Eric Sunshine
2021-09-11 19:08 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS Ævar Arnfjörð Bjarmason
2021-09-12 21:48 ` Junio C Hamano
2021-09-11 19:09 ` [PATCH v3 2/6] blame: replace usage end blurb with better option spec Ævar Arnfjörð Bjarmason
2021-09-12 4:45 ` Eric Sunshine
2021-09-12 22:22 ` Junio C Hamano
2021-09-11 19:09 ` [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-12 22:26 ` Junio C Hamano
2021-09-13 0:21 ` Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` [PATCH v4 0/4] " Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` [PATCH v4 1/4] parse-options API users: align usage output in C-strings Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` [PATCH v4 2/4] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` [PATCH v4 3/4] git rev-parse --parseopt tests: add more usagestr tests Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` [PATCH v4 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-13 6:41 ` Junio C Hamano
2021-09-21 13:30 ` [PATCH v5 0/4] " Ævar Arnfjörð Bjarmason
2021-09-21 13:30 ` [PATCH v5 1/4] parse-options API users: align usage output in C-strings Ævar Arnfjörð Bjarmason
2021-09-21 13:30 ` [PATCH v5 2/4] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-21 13:30 ` [PATCH v5 3/4] git rev-parse --parseopt tests: add more usagestr tests Ævar Arnfjörð Bjarmason
2021-09-21 13:30 ` [PATCH v5 4/4] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=cover-v2-0.6-00000000000-20210910T153146Z-avarab@gmail.com \
--to=avarab@gmail.com \
--cc=carenas@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=peff@peff.net \
--cc=sunshine@sunshineco.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).