Re: [PATCH] branch,checkout: fix --track usage strings
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-01-20 12:10:41
On Tue, Jan 18 2022, Josh Steadmon wrote:
quoted hunk ↗ jump to hunk
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/ (local) Signed-off-by: Josh Steadmon <redacted> --- 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/ (local); 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/ (local) 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.