| Message ID | 3de40324bea6a1dd9bca2654721471e3809e87d8.1642538935.git.steadmon@google.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 6a6a1b2bbf3fd76dfcc49c25589f0deb785f21d2 |
| Headers | show |
| Series | branch,checkout: fix --track usage strings | expand |
Josh Steadmon <steadmon@google.com> writes: > As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a > list of allowed parameters is not recommended. Both git-branch and > git-checkout were changed in d311566 (branch: add flags and config to > inherit tracking, 2021-12-20) to use this discouraged combination for > their --track flags. (tl;dr) I'll take this as-is and hopefully I can fast-track it in time before tagging -rc2 tomorrow. Having said that, here is what parse-options.h describes this flag bit: * PARSE_OPT_LITERAL_ARGHELP: says that argh shouldn't be enclosed in brackets * (i.e. '<argh>') in the help message. * Useful for options with multiple parameters. Notice that this bit actually is meant "for options with multiple parameters"? The recommendation given to you might not be showing the right way. Looking at "git grep -C2 OPT_LITERAL_ARGHELP \*.c" output, I suspect that a better solution may be to enclose "direct|inherit" in a pair of parentheses, i.e. "(direct|inherit)". That mimics the way how "git am --show-current-patch[=(diff|raw)]" does it. Then we would show --track[=(direct|inherit)] instead of --track[=<mode>] which means that ... > Fix this by removing PARSE_OPT_LITERAL_ARGHELP, and changing the arghelp > to simply be "mode". Users may discover allowed values in the manual > pages. ... users won't have to visit the manual page only to find out what modes we support. In any case, the lesson should not be lost in the list archive. To help future developers from the same trouble, we should leave a note to revisit the above description of the flag bit in parse-options.h later, possibly after the 2.35 final, to see if we can improve it (both the description and/or the behaviour of the code when it sees the flag bit). Thanks.
On Tue, Jan 18 2022, Josh Steadmon wrote: > As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a > list of allowed parameters is not recommended. Both git-branch and > git-checkout were changed in d311566 (branch: add flags and config to > inherit tracking, 2021-12-20) to use this discouraged combination for > their --track flags. > > Fix this by removing PARSE_OPT_LITERAL_ARGHELP, and changing the arghelp > to simply be "mode". Users may discover allowed values in the manual > pages. > > [1]: https://lore.kernel.org/git/220111.86a6g3yqf9.gmgdl@evledraar.gmail.com/ > > Signed-off-by: Josh Steadmon <steadmon@google.com> > --- > builtin/branch.c | 4 ++-- > builtin/checkout.c | 6 +++--- > 2 files changed, 5 insertions(+), 5 deletions(-) > > diff --git a/builtin/branch.c b/builtin/branch.c > index 2251e6a54f..0c8d8a8827 100644 > --- a/builtin/branch.c > +++ b/builtin/branch.c > @@ -638,9 +638,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) > OPT__VERBOSE(&filter.verbose, > N_("show hash and subject, give twice for upstream branch")), > OPT__QUIET(&quiet, N_("suppress informational messages")), > - OPT_CALLBACK_F('t', "track", &track, "direct|inherit", > + OPT_CALLBACK_F('t', "track", &track, N_("mode"), > N_("set branch tracking configuration"), > - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, > + PARSE_OPT_OPTARG, > parse_opt_tracking_mode), > OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"), > BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN), > diff --git a/builtin/checkout.c b/builtin/checkout.c > index 94814c37b4..6a5dd2a2a2 100644 > --- a/builtin/checkout.c > +++ b/builtin/checkout.c > @@ -1549,9 +1549,9 @@ static struct option *add_common_switch_branch_options( > { > struct option options[] = { > OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")), > - OPT_CALLBACK_F('t', "track", &opts->track, "direct|inherit", > - N_("set up tracking mode (see git-pull(1))"), > - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, > + OPT_CALLBACK_F('t', "track", &opts->track, N_("mode"), > + N_("set branch tracking configuration"), > + PARSE_OPT_OPTARG, > parse_opt_tracking_mode), > OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"), > PARSE_OPT_NOCOMPLETE), > > base-commit: b56bd95bbc8f716cb8cbb5fdc18b9b0f00323c6a Additional comment: I think this change is correct, as noted in my just-sent https://lore.kernel.org/git/220120.864k5ymx55.gmgdl@evledraar.gmail.com/; it would be nice but not necessary to follow-up with an optbug() test as noted in https://lore.kernel.org/git/220119.867davokff.gmgdl@evledraar.gmail.com/ though. But to not merely repeat myself, I also saw that we're emitting the wrong output from usage_argh() in cases of !PARSE_OPT_NOARG. I.e. we need this fix too: diff --git a/parse-options.c b/parse-options.c index a8283037be9..2be1eabd84e 100644 --- a/parse-options.c +++ b/parse-options.c @@ -915,8 +915,10 @@ static int usage_argh(const struct option *opts, FILE *outfile) s = literal ? "[=%s]" : "[=<%s>]"; else s = literal ? "[%s]" : "[<%s>]"; - else + else if (opts->flags & PARSE_OPT_NOARG) s = literal ? " %s" : " <%s>"; + else + s = literal ? "[=]%s" : "[=]<%s>"; return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("...")); } With that we'll now emit: $ ./git add -h 2>&1|grep chmod --chmod[=](+|-)x override the executable bit of the listed files Which is correct, as we accept both of: git add --chmod +x git add --chmod=+x But not: $ git add --chmod error: parse-options.c:58: option `chmod' requires a value But the usage output stated that the "=" was mandatory before.
On Jan 20 2022, Ævar Arnfjörð Bjarmason wrote: > With that we'll now emit: > > $ ./git add -h 2>&1|grep chmod > --chmod[=](+|-)x override the executable bit of the listed files That looks like --chmod+x is valid, which isn't.
On Thu, Jan 20 2022, Andreas Schwab wrote: > On Jan 20 2022, Ævar Arnfjörð Bjarmason wrote: > >> With that we'll now emit: >> >> $ ./git add -h 2>&1|grep chmod >> --chmod[=](+|-)x override the executable bit of the listed files > > That looks like --chmod+x is valid, which isn't. Indeed, it should be --chmod(=| )(+|-)x in that "else" case. But I think the rest of what I pointed out still applies with that amendmend. Thanks!
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > With that we'll now emit: > > $ ./git add -h 2>&1|grep chmod > --chmod[=](+|-)x override the executable bit of the listed files > ... > But the usage output stated that the "=" was mandatory before. I am not sure if it is healthy to be _that_ strict when interpreting the boilerplate elements in the output. Between "git add -h" that gives (1) git add --chmod( |=)(+|-)x (2) git add --chmod=(+|-)x I would prefer the latter 10x as much as the former. The choice "You can give either plus or minus" is very much what the reader must understand and it is worth reminding in the help. Compared to that, "You can use the stuck form that is recommended by gitcli documentation when giving the argument to the --chmod option, or you can give the argument to the option as a separate command line argument", while technically correct, is not a choice that is worth cluttering the output and making it harder to read. To put it differently, the choice (+|-) is something the user needs to pick correctly to make what they want to happen happen. On the other hand, the choice ( |=) is not. As this is a boilerplate choice that is shared by any and all options that take an argument, once you are aware that stuck form is recommended but that separate form is also accepted, you'd see "git add --chmod=blah" in the help and would not hesitate to type "git add --chmod blah". And if you are not aware of the existence of the alternative, nothing is lost. You can type '=' and see what you want to see happen happen. Not cluttering the help text with an extra choice that the user does not have to make has a value.
On Thu, Jan 20 2022, Junio C Hamano wrote: > Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> With that we'll now emit: >> >> $ ./git add -h 2>&1|grep chmod >> --chmod[=](+|-)x override the executable bit of the listed files >> ... >> But the usage output stated that the "=" was mandatory before. > > I am not sure if it is healthy to be _that_ strict when interpreting > the boilerplate elements in the output. Between "git add -h" that > gives > > (1) git add --chmod( |=)(+|-)x > (2) git add --chmod=(+|-)x > > I would prefer the latter 10x as much as the former. The choice > "You can give either plus or minus" is very much what the reader > must understand and it is worth reminding in the help. Compared to > that, "You can use the stuck form that is recommended by gitcli > documentation when giving the argument to the --chmod option, or you > can give the argument to the option as a separate command line > argument", while technically correct, is not a choice that is worth > cluttering the output and making it harder to read. > > To put it differently, the choice (+|-) is something the user needs > to pick correctly to make what they want to happen happen. On the > other hand, the choice ( |=) is not. As this is a boilerplate > choice that is shared by any and all options that take an argument, > once you are aware that stuck form is recommended but that separate > form is also accepted, you'd see "git add --chmod=blah" in the help > and would not hesitate to type "git add --chmod blah". And if you > are not aware of the existence of the alternative, nothing is lost. > You can type '=' and see what you want to see happen happen. > > Not cluttering the help text with an extra choice that the user does > not have to make has a value. I.e. if we're not going for pedantic accuracy you'd prefer the below diff instead? I.e. when an option doesn't take an optional argument we're going out of our way to say that you can omit the "=", but we can instead just include it and have the the explanation in gitcli(7) about when "=" is optional suffice. Also, with the sh completion if you do "git add --chm<TAB>" it expands it to "git add --chmod=", i.e. the cursor is left after the "=" that's not shown in the "git add -h". So always including it would be consistent with that. diff --git a/parse-options.c b/parse-options.c index a8283037be9..75c345bb738 100644 --- a/parse-options.c +++ b/parse-options.c @@ -916,7 +916,7 @@ static int usage_argh(const struct option *opts, FILE *outfile) else s = literal ? "[%s]" : "[<%s>]"; else - s = literal ? " %s" : " <%s>"; + s = literal ? "=%s" : "=<%s>"; return utf8_fprintf(outfile, s, opts->argh ? _(opts->argh) : _("...")); } Just this patch will make getopts test fail, because we'll need e.g. this test_cmp adjusted: + diff -u expect output --- expect 2022-01-21 11:31:17.395492260 +0000 +++ output 2022-01-21 11:31:17.395492260 +0000 @@ -5,7 +5,7 @@ -h, --help show the help --foo some nifty option --foo - --bar ... some cool option --bar with an argument + --bar=... some cool option --bar with an argument -b, --baz a short and long option An option group Header @@ -13,20 +13,20 @@ -d, --data[=...] short and long option with an optional argument Argument hints - -B <arg> short option required argument - --bar2 <arg> long option required argument - -e, --fuz <with-space> + -B=<arg> short option required argument + --bar2=<arg> long option required argument + -e, --fuz=<with-space> short and long option required argument -s[<some>] short option optional argument --long[=<data>] long option optional argument -g, --fluf[=<path>] short and long option optional argument - --longest <very-long-argument-hint> + --longest=<very-long-argument-hint> a very long argument hint - --pair <key=value> with an equals sign in the hint + --pair=<key=value> with an equals sign in the hint --aswitch help te=t contains? fl*g characters!` - --bswitch <hint> hint has trailing tab character + --bswitch=<hint> hint has trailing tab character --cswitch switch has trailing tab character - --short-hint <a> with a one symbol hint + --short-hint=<a> with a one symbol hint It's arguably a bit odd to see the "=" for those that have !opts->long_name, but on the other hand I can't think of a reason other than it looking unusual to me for why we wouldn't include the "=" there for consistency. It's odd that we don't have the short options in --git-completion-helper at all, so "git am -C<tab>" isn't completed", but looking at show_gitcomp() we'd need some further adjusting to make that work.
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > Also, with the sh completion if you do "git add --chm<TAB>" it expands > it to "git add --chmod=", i.e. the cursor is left after the "=" that's > not shown in the "git add -h". So always including it would be > consistent with that. > > diff --git a/parse-options.c b/parse-options.c > index a8283037be9..75c345bb738 100644 > --- a/parse-options.c > +++ b/parse-options.c > @@ -916,7 +916,7 @@ static int usage_argh(const struct option *opts, FILE *outfile) > else > s = literal ? "[%s]" : "[<%s>]"; > else > - s = literal ? " %s" : " <%s>"; > + s = literal ? "=%s" : "=<%s>"; If the option has a long name, I think it is a good change. I do not offhand know if it is a good change for a short option, though. $ git diff -B=20/60 error: break-rewrites expects <n>/<m> form $ git diff -B 20/60 fatal: ambiguous argument '20/60': unknown revision or path not in the working tree. $ git diff -B20/60 gitcli.txt has this (I didn't check with the parse-options implementation, though): * when a command-line option takes an argument, use the 'stuck' form. In other words, write `git foo -oArg` instead of `git foo -o Arg` for short options, and `git foo --long-opt=Arg` instead of `git foo --long-opt Arg` for long options. An option that takes optional option-argument must be written in the 'stuck' form. So probably you'd need the same "show differently depending on which between short and long we will show" on this side of the if/else. else { if (opts->long_name) s = literal ? "=%s" : "=<%s>"; else s = literal ? "%s" : "<%s>"; } perhaps?
diff --git a/builtin/branch.c b/builtin/branch.c index 2251e6a54f..0c8d8a8827 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -638,9 +638,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix) OPT__VERBOSE(&filter.verbose, N_("show hash and subject, give twice for upstream branch")), OPT__QUIET(&quiet, N_("suppress informational messages")), - OPT_CALLBACK_F('t', "track", &track, "direct|inherit", + OPT_CALLBACK_F('t', "track", &track, N_("mode"), N_("set branch tracking configuration"), - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + PARSE_OPT_OPTARG, parse_opt_tracking_mode), OPT_SET_INT_F(0, "set-upstream", &track, N_("do not use"), BRANCH_TRACK_OVERRIDE, PARSE_OPT_HIDDEN), diff --git a/builtin/checkout.c b/builtin/checkout.c index 94814c37b4..6a5dd2a2a2 100644 --- a/builtin/checkout.c +++ b/builtin/checkout.c @@ -1549,9 +1549,9 @@ static struct option *add_common_switch_branch_options( { struct option options[] = { OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")), - OPT_CALLBACK_F('t', "track", &opts->track, "direct|inherit", - N_("set up tracking mode (see git-pull(1))"), - PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP, + OPT_CALLBACK_F('t', "track", &opts->track, N_("mode"), + N_("set branch tracking configuration"), + PARSE_OPT_OPTARG, parse_opt_tracking_mode), OPT__FORCE(&opts->force, N_("force checkout (throw away local modifications)"), PARSE_OPT_NOCOMPLETE),
As Ævar pointed out in [1], the use of PARSE_OPT_LITERAL_ARGHELP with a list of allowed parameters is not recommended. Both git-branch and git-checkout were changed in d311566 (branch: add flags and config to inherit tracking, 2021-12-20) to use this discouraged combination for their --track flags. Fix this by removing PARSE_OPT_LITERAL_ARGHELP, and changing the arghelp to simply be "mode". Users may discover allowed values in the manual pages. [1]: https://lore.kernel.org/git/220111.86a6g3yqf9.gmgdl@evledraar.gmail.com/ Signed-off-by: Josh Steadmon <steadmon@google.com> --- builtin/branch.c | 4 ++-- builtin/checkout.c | 6 +++--- 2 files changed, 5 insertions(+), 5 deletions(-) base-commit: b56bd95bbc8f716cb8cbb5fdc18b9b0f00323c6a