Re: [PATCH] branch,checkout: fix --track usage strings

[Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx>]

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@xxxxxxxxxxxxxxxxxxx/
>
> Signed-off-by: Josh Steadmon <steadmon@xxxxxxxxxx>
> ---
>  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@xxxxxxxxxxxxxxxxxxx/;
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@xxxxxxxxxxxxxxxxxxx/
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.