Re: incorrect 'git (checkout|branch) -h' output with new --track modes (was: [PATCH v8 2/3] branch: add flags and config to inherit tracking)

From: René Scharfe <l.s.r@web.de>
Date: Jan 19, 2022, 10:20 UTC
Message-ID: <55d5327a-9c6c-7fd8-b540-e710259c0694@web.de>
In-Reply-To: <220111.86a6g3yqf9.gmgdl@evledraar.gmail.com>

Am 11.01.22 um 02:57 schrieb Ævar Arnfjörð Bjarmason:

Show 11 quoted lines

>
> On Mon, Dec 20 2021, Josh Steadmon wrote:
>
>> Since we've added an argument to "--track", also add "--track=direct" as
>> another way to explicitly get the original "--track" behavior ("--track"
>> without an argument still works as well).
>> [...]
>> -'git branch' [--track | --no-track] [-f] <branchname> [<start-point>]
>> +'git branch' [--track [direct|inherit] | --no-track] [-f] <branchname> [<start-point>]
>
> The usage info here is correct...

Actually it isn't, because optional arguments need the equal sign. I.e. this works as expected:

   git branch --track=direct branch

But this here interprets "direct" as a branch name (and branch as a start point):

   git branch --track direct branch

The usage string could start with:

  'git branch' [--track | --track=direct | --track=inherit | --no-track]

... or the less repetitive:

  'git branch' [--track[=(direct|inherit)] | --no-track]

Options with required arguments accept either whitespace or an equal sign between option name and arg. With PARSE_OPT_OPTARG we cannot accept whitespace because we cannot decide whether the next thing after the option name is an argument or the next parameter.

Show 5 quoted lines

>
>> ---track::
>> +--track [inherit|direct]::
>
> ...as is this...

Same here:

   --track[=(direct|inherit)]

Show 22 quoted lines

>
>> -		OPT_SET_INT('t', "track",  &track, N_("set up tracking mode (see git-pull(1))"),
>> -			BRANCH_TRACK_EXPLICIT),
>> +		OPT_CALLBACK_F('t', "track",  &track, "direct|inherit",
>> +			N_("set branch tracking configuration"),
>> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>> +			parse_opt_tracking_mode),
> ....
>
>>  	struct option options[] = {
>>  		OPT_BOOL('d', "detach", &opts->force_detach, N_("detach HEAD at named commit")),
>> -		OPT_SET_INT('t', "track",  &opts->track, N_("set upstream info for new branch"),
>> -			BRANCH_TRACK_EXPLICIT),
>> +		OPT_CALLBACK_F('t', "track",  &opts->track, "direct|inherit",
>
> ... but these are not. I.e. we'll emit:
>
>     -t, --track[=direct|inherit]
>                           set branch tracking configuration
>
> I.e. implying that the valid uses are --track, --track=direct, and
> --trackinherit.

Well spotted. It should be specified as "(direct|inherit)" (i.e. with parens).

> It looks like the problem is (ab)use of PARSE_OPT_OPTARG, i.e. it was
> never meant for an enumeration of possible values, but for something
> like N_("mode"). It could be made to support that, but it would require
> some light patching of the releant bits of parse-options.c.

Could you please elaborate that point? AFAIU PARSE_OPT_OPTARG just requires arguments to be attached with equal signs and there is no limitation regarding the number of possible argument values.

> The PARSE_OPT_LITERAL_ARGHELP should also be dropped if it's fixed to
> use a string like "mode".

That's true. And it's also enabled automatically if the argument help string contains any of the following characters: ()<>[]|. So basically it's never needed explicitly..

Show 13 quoted lines

>
>> +			N_("set up tracking mode (see git-pull(1))"),
>
> Aside: In v5 the other reference was changed from using the
> "git-pull(1)" manpage syntax. See
> https://lore.kernel.org/git/cover.1638859949.git.steadmon@google.com/;
> It looks like this one should be changed too, but was missed.
>
>> +			PARSE_OPT_OPTARG | PARSE_OPT_LITERAL_ARGHELP,
>> +			parse_opt_tracking_mode),
>
>
>