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 v4 0/4] parse-options: properly align continued usage output
Date: Mon, 13 Sep 2021 02:13:18 +0200 [thread overview]
Message-ID: <cover-v4-0.4-00000000000-20210912T235347Z-avarab@gmail.com> (raw)
In-Reply-To: <cover-v3-0.6-00000000000-20210911T190239Z-avarab@gmail.com>
This series changes usage_with_options_internal() in parse-options.c
to properly align continued "\n" usage output.
This v4 essentially goes back to the version of this in v1. In v2 I'd
addressed Eric's comments in [1] by trying to remove the last three
in-tree users of this part of the API.
But if "git rev-parse --parseopt" is going to be bug-for-bug
compatible with existing out-of-tree users[2] then changing that
becomes a non-starter.
So let's keep the existing behavior (which also had at least one other
weird & undocumented edge case, which we now test for).
This v4 also has a fix for the "saw_empty_line" bug Eric noted in [2]
(which wasn't applicable to v2..v3), and takes some of his suggestions
for simplifying the loop (but not all, I still kept it as one loop).
1. https://lore.kernel.org/git/f8560b11-ba56-0a52-8bb4-5b71f0657764@sunshineco.com/
2. https://lore.kernel.org/git/xmqqpmtdjuqj.fsf@gitster.g/
Ævar Arnfjörð Bjarmason (4):
parse-options API users: align usage output in C-strings
send-pack: properly use parse_options() API for usage string
git rev-parse --parseopt tests: add more usagestr tests
parse-options: properly align continued usage output
Documentation/git-send-pack.txt | 4 +-
builtin/ls-remote.c | 4 +-
builtin/send-pack.c | 8 ++--
builtin/show-branch.c | 6 +--
builtin/stash.c | 2 +-
builtin/tag.c | 4 +-
parse-options.c | 76 +++++++++++++++++++++++++++------
t/t1502-rev-parse-parseopt.sh | 54 +++++++++++++++++++++++
8 files changed, 132 insertions(+), 26 deletions(-)
Range-diff against v3:
1: ad857e80fd8 < -: ----------- credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS
2: 036eb0efb5b < -: ----------- blame: replace usage end blurb with better option spec
4: 5638d2bc832 ! 1: 64d8647340d built-ins: "properly" align continued usage output
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- built-ins: "properly" align continued usage output
+ parse-options API users: align usage output in C-strings
- Let's "fix" various "git <cmd> -h" output by "properly" aligning the
- output in cases where we continue usage output after a "\n". The "fix"
- and "properly" scare quotes are because this actually makes things
- worse in some cases, because e.g. in the case of "git tag -h" the
- "\t\t" effectively works around how parse-options.c aligns this
- output.
+ In preparation for having continued usage lines properly aligned in
+ "git <cmd> -h" output, let's have the "[" on the second such lines
+ align with the "[" on the first line.
- But two wrongs don't make a right, let's "fix" this by making it worse
- temporarily, in anticipation of improving parse-options.c to handle
- this alignment.
+ In some cases this makes the output worse, because e.g. the "git
+ ls-remote -h" output had been aligned to account for the extra
+ whitespace that the usage_with_options_internal() function in
+ parse-options.c would add.
- The issue is that we should have whitespace corresponding to the
- length of the command name here, e.g. in the case of "git ls-remote"
- it should be 14 characters, or the length of ""git ls-remote
- ". Instead we had 21 characters in builtin/ls-remote.c, those extra 7
- characters are the length of "usage: " (and also " or:"). So in the C
- locale the resulting output aligns nicely:
+ In other cases such as builtin/stash.c (not changed here), we were
+ aligned in the C strings, but since that didn't account for the extra
+ padding in usage_with_options_internal() it would come out looking
+ misaligned, e.g. code like this:
- $ git ls-remote -h
- usage: git ls-remote [--heads] [--tags] [--refs] [--upload-pack=<exec>]
- [-q | --quiet] [--exit-code] [--get-url]
- [--symref] [<repository> [<refs>...]]
+ N_("git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
+ " [-u|--include-untracked] [-a|--all] [-m|--message <message>]\n"
- But that's fragile, we might not be under the C locale. We really
- should have parse-options.c itself add this padding. In a subsequent
- commit I'll make it do that.
+ Would emit:
- In the case of "tag" and "show-branch" and "stash save" the output was
- not properly aligned, although in the "git tag" case it was
- near-enough (aligned with the "-" in "git tag -l") to look good,
- assuming C locale & a tab-width of 8. In any case, let's align this in
- a way that looks obviously correct when looking at the source itself,
- and then improve parse-options.c itself.
+ or: git stash [push [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]
+ [-u|--include-untracked] [-a|--all] [-m|--message <message>]
+
+ Let's change all the usage arrays which use such continued usage
+ output via "\n"-embedding to be like builtin/stash.c.
+
+ This makes the output worse temporarily, but in a subsequent change
+ I'll improve the usage_with_options_internal() to take this into
+ account, at which point all of the strings being changed here will
+ emit prettier output.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
5: 821e5e14132 = 2: f10ff775c69 send-pack: properly use parse_options() API for usage string
3: e23a8231f38 ! 3: 05a0c7cac37 parse-options: stop supporting "" in the usagestr array
@@ Metadata
Author: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## Commit message ##
- parse-options: stop supporting "" in the usagestr array
+ git rev-parse --parseopt tests: add more usagestr tests
- The strings in the the "usagestr" array have special handling for the
- empty string dating back to f389c808b67 (Rework make_usage to print
- the usage message immediately, 2007-10-14).
+ Add tests for the "usagestr" passed to parse-options.c
+ usage_with_options_internal() through cmd_parseopt().
- We'll prefix all strings after the first one with "or: ". Then if we
- encountered a "" we'll emit all strings after that point verbatim
- without any "or: " prefixing.
-
- In the preceding commits we got rid of the two users of this
- undocumented part of the API. Let's remove it in preparation for
- improving the output emitted by usage_with_options_internal().
-
- I think we might want to use this in the future, but in that case
- we'll be much better off with an API that emulates the
- non-parse_options() way that git.c does this.
-
- That git.c code uses a separate "git_usage_string" and
- "git_more_info_string". See b7d9681974e (Print info about "git help
- COMMAND" on git's main usage pages, 2008-06-06). By splitting the two
- up we can emit something in the middle, as indeed git.c does.
-
- I'd like our "git <cmd> -h" info to be more helpful, and I'd also like
- parse_options() to handle the "git" command itself, because of the
- limitations of how this was done in usage_with_options_internal() we
- couldn't migrate a caller like git.c to parse_options().
-
- So let's just remove this for now, it has no users, and once we want
- to do this again we can simply add another argument to the relevant
- functions, or otherwise hook into things so that we can print
- something at the end and/or middle.
-
- It's possible that this change introduce breakage somewhere. We'd only
- catch these cases at runtime, and the "git rev-parse --parseopt"
- command is used by shellscripts, see bac199b7b17 (Update
- git-sh-setup(1) to allow transparent use of git-rev-parse --parseopt,
- 2007-11-04). I've grepped the codebase for "OPTIONS_SPEC",
- "char.*\*.*usage\[\]" etc. I'm fairly sure there no outstanding users
- of this functionality.
+ These test for edge cases in the existing behavior related to the
+ "--parseopt" interface doing its own line-splitting with
+ strbuf_getline(), and the native C interface expecting and potentially
+ needing to handle newlines within the strings in the array it
+ accepts. The results are probably something that wasn't anticipated,
+ but let's make sure we stay backwards compatible with it.
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- ## builtin/rev-parse.c ##
-@@ builtin/rev-parse.c: static int cmd_parseopt(int argc, const char **argv, const char *prefix)
- for (;;) {
- if (strbuf_getline(&sb, stdin) == EOF)
- die(_("premature end of input"));
-+ if (!sb.len)
-+ die(_("empty lines are not permitted before the `--' separator"));
- ALLOC_GROW(usage, unb + 1, usz);
-+
- if (!strcmp("--", sb.buf)) {
- if (unb < 1)
- die(_("no usage string given before the `--' separator"));
-
- ## parse-options.c ##
-@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *ctx,
- fprintf(outfile, "cat <<\\EOF\n");
-
- fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
-- while (*usagestr && **usagestr)
-+ while (*usagestr) {
- /*
- * TRANSLATORS: the colon here should align with the
- * one in "usage: %s" translation.
- */
- fprintf_ln(outfile, _(" or: %s"), _(*usagestr++));
-- while (*usagestr) {
-- if (**usagestr)
-- fprintf_ln(outfile, _(" %s"), _(*usagestr));
-- else
-- fputc('\n', outfile);
-- usagestr++;
- }
-
- need_newline = 1;
-
- ## t/helper/test-parse-options.c ##
-@@ t/helper/test-parse-options.c: int cmd__parse_options(int argc, const char **argv)
- const char *prefix = "prefix/";
- const char *usage[] = {
- "test-tool parse-options <options>",
-- "",
-- "A helper function for the parse-options API.",
- NULL
- };
- struct string_list expect = STRING_LIST_INIT_NODUP;
-
- ## t/t0040-parse-options.sh ##
-@@ t/t0040-parse-options.sh: test_description='our own option parser'
- cat >expect <<\EOF
- usage: test-tool parse-options <options>
-
-- A helper function for the parse-options API.
--
- --yes get a boolean
- -D, --no-doubt begins with 'no-'
- -B, --no-fear be brave
-
## t/t1502-rev-parse-parseopt.sh ##
-@@ t/t1502-rev-parse-parseopt.sh: test_description='test git rev-parse --parseopt'
- test_expect_success 'setup optionspec' '
- sed -e "s/^|//" >optionspec <<\EOF
- |some-command [options] <args>...
--|
--|some-command does foo and bar!
- |--
- |h,help show the help
- |
-@@ t/t1502-rev-parse-parseopt.sh: EOF
- test_expect_success 'setup optionspec-no-switches' '
- sed -e "s/^|//" >optionspec_no_switches <<\EOF
- |some-command [options] <args>...
--|
--|some-command does foo and bar!
- |--
- EOF
- '
-@@ t/t1502-rev-parse-parseopt.sh: EOF
- test_expect_success 'setup optionspec-only-hidden-switches' '
- sed -e "s/^|//" >optionspec_only_hidden_switches <<\EOF
- |some-command [options] <args>...
--|
--|some-command does foo and bar!
- |--
- |hidden1* A hidden switch
- EOF
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output' '
- |cat <<\EOF
- |usage: some-command [options] <args>...
- |
--| some-command does foo and bar!
--|
- | -h, --help show the help
- | --foo some nifty option --foo
- | --bar ... some cool option --bar with an argument
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output no switches' '
- |cat <<\EOF
- |usage: some-command [options] <args>...
- |
--| some-command does foo and bar!
--|
- |EOF
- END_EXPECT
- test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_no_switches &&
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help output hidden switches' '
- |cat <<\EOF
- |usage: some-command [options] <args>...
- |
--| some-command does foo and bar!
--|
- |EOF
- END_EXPECT
- test_expect_code 129 git rev-parse --parseopt -- -h > output < optionspec_only_hidden_switches &&
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt help-all output hidden switches' '
- |cat <<\EOF
- |usage: some-command [options] <args>...
- |
--| some-command does foo and bar!
--|
- | --hidden1 A hidden switch
- |
- |EOF
-@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt invalid switch help output' '
- |error: unknown option `does-not-exist'\''
- |usage: some-command [options] <args>...
- |
--| some-command does foo and bar!
--|
- | -h, --help show the help
- | --foo some nifty option --foo
- | --bar ... some cool option --bar with an argument
@@ t/t1502-rev-parse-parseopt.sh: test_expect_success 'test --parseopt --stuck-long and short option with unset op
test_cmp expect output
'
-+test_expect_success 'test --parseopt help output hidden switches' '
-+ sed -e "s/^|//" >optionspec-trailing-line <<-\EOF &&
-+ |some-command [options] <args>...
++test_expect_success 'test --parseopt help output: "wrapped" options normal "or:" lines' '
++ sed -e "s/^|//" >spec <<-\EOF &&
++ |cmd [--some-option]
++ | [--another-option]
++ |cmd [--yet-another-option]
++ |--
++ |h,help show the help
++ EOF
++
++ sed -e "s/^|//" >expect <<-\END_EXPECT &&
++ |cat <<\EOF
++ |usage: cmd [--some-option]
++ | or: [--another-option]
++ | or: cmd [--yet-another-option]
++ |
++ | -h, --help show the help
+ |
++ |EOF
++ END_EXPECT
++
++ test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
++ test_cmp expect actual
++'
++
++test_expect_success 'test --parseopt help output: multi-line blurb after empty line' '
++ sed -e "s/^|//" >spec <<-\EOF &&
++ |cmd [--some-option]
++ | [--another-option]
+ |
++ |multi
++ |line
++ |blurb
+ |--
+ |h,help show the help
+ EOF
+
-+ cat >expect <<-\EOF &&
-+ fatal: empty lines are not permitted before the `--'"'"' separator
-+ EOF
++ sed -e "s/^|//" >expect <<-\END_EXPECT &&
++ |cat <<\EOF
++ |usage: cmd [--some-option]
++ | or: [--another-option]
++ |
++ | multi
++ | line
++ | blurb
++ |
++ | -h, --help show the help
++ |
++ |EOF
++ END_EXPECT
+
-+ test_must_fail git rev-parse --parseopt -- -h >out < optionspec-trailing-line 2>actual &&
-+ test_must_be_empty out &&
++ test_must_fail git rev-parse --parseopt -- -h >out <spec >actual &&
+ test_cmp expect actual
+'
+
6: 0df2840ce4e ! 4: 55480dee680 parse-options: properly align continued usage output
@@ Commit message
automatically, 2007-10-15) which isn't RTL-safe, but that code would
be easy to fix. Let's not introduce new RTL translation problems here.
- I'm also adding a check to catch the mistake of needlessly adding a
- trailing "\n", such as:
-
- N_("git stash save [-p|--patch] [-k|--[no-]keep-index] [-q|--quiet]\n"
- " [-u|--include-untracked] [-a|--all] [<message>]\n"),
-
- Or even a mistake like adding just one "\n" in a string with no other
- newlines:
-
- N_("git stash list [<options>]\n"),
-
- This catches the cases already tested for in cmd_parseopt(), but this
- covers the purely C API. As noted a preceding commit that added the
- die() to cmd_parseopt() I'm fairly confident that this will be
- triggered by no in-tree user I've missed.
-
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
## parse-options.c ##
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
+ * one in "usage: %s" translation.
+ */
+ const char *or_prefix = _(" or: %s");
-+
+ /*
+ * TRANSLATORS: You should only need to translate this format
+ * string if your language is a RTL language (e.g. Arabic,
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
+ */
+ const char *usage_continued = _("%*s%s");
+ const char *prefix = usage_prefix;
++ int saw_empty_line = 0;
+
if (!usagestr)
return PARSE_OPT_HELP;
@@ parse-options.c: static int usage_with_options_internal(struct parse_opt_ctx_t *
fprintf(outfile, "cat <<\\EOF\n");
- fprintf_ln(outfile, _("usage: %s"), _(*usagestr++));
- while (*usagestr) {
+- while (*usagestr && **usagestr)
- /*
- * TRANSLATORS: the colon here should align with the
- * one in "usage: %s" translation.
- */
- fprintf_ln(outfile, _(" or: %s"), _(*usagestr++));
+ while (*usagestr) {
+- if (**usagestr)
+- fprintf_ln(outfile, _(" %s"), _(*usagestr));
+- else
+- fputc('\n', outfile);
+- usagestr++;
++ const char *str = _(*usagestr++);
+ struct string_list list = STRING_LIST_INIT_DUP;
+ unsigned int j;
+
-+ string_list_split(&list, _(*usagestr++), '\n', -1);
++ if (!saw_empty_line && !*str)
++ saw_empty_line = 1;
++
++ string_list_split(&list, str, '\n', -1);
+ for (j = 0; j < list.nr; j++) {
+ const char *line = list.items[j].string;
+
-+ if (!*line)
-+ BUG("empty or trailing line in usage string");
-+
-+ if (!j)
++ if (saw_empty_line && *line)
++ fprintf_ln(outfile, _(" %s"), line);
++ else if (saw_empty_line)
++ fputc('\n', outfile);
++ else if (!j)
+ fprintf_ln(outfile, prefix, line);
+ else
+ fprintf_ln(outfile, usage_continued,
--
2.33.0.1001.g3ab2ac1eaae
next prev parent reply other threads:[~2021-09-13 0:13 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 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 1/6] test-lib.sh: add a UNIX_SOCKETS prerequisite Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 2/6] git.c: add a NEED_UNIX_SOCKETS option for built-ins Ævar Arnfjörð Bjarmason
2021-09-11 0:14 ` Junio C Hamano
2021-09-11 2:50 ` Ævar Arnfjörð Bjarmason
2021-09-11 3:47 ` Carlo Marcelo Arenas Belón
2021-09-11 6:12 ` Junio C Hamano
2021-09-11 7:16 ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-11 0:21 ` Junio C Hamano
2021-09-11 2:46 ` Ævar Arnfjörð Bjarmason
2021-09-11 6:52 ` Junio C Hamano
2021-09-10 15:38 ` [PATCH v2 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 0:25 ` Junio C Hamano
2021-09-11 2:40 ` Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-10 15:38 ` [PATCH v2 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 7:41 ` [PATCH v2 0/6] parse-options: properly align continued usage output & related Eric Sunshine
2021-09-11 19:08 ` [PATCH v3 " Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 1/6] credential-cache{,--daemon}: don't build under NO_UNIX_SOCKETS Ævar Arnfjörð Bjarmason
2021-09-12 21:48 ` Junio C Hamano
2021-09-11 19:09 ` [PATCH v3 2/6] blame: replace usage end blurb with better option spec Ævar Arnfjörð Bjarmason
2021-09-12 4:45 ` Eric Sunshine
2021-09-12 22:22 ` Junio C Hamano
2021-09-11 19:09 ` [PATCH v3 3/6] parse-options: stop supporting "" in the usagestr array Ævar Arnfjörð Bjarmason
2021-09-12 22:26 ` Junio C Hamano
2021-09-13 0:21 ` Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 4/6] built-ins: "properly" align continued usage output Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 5/6] send-pack: properly use parse_options() API for usage string Ævar Arnfjörð Bjarmason
2021-09-11 19:09 ` [PATCH v3 6/6] parse-options: properly align continued usage output Ævar Arnfjörð Bjarmason
2021-09-13 0:13 ` Ævar Arnfjörð Bjarmason [this message]
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-v4-0.4-00000000000-20210912T235347Z-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).