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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Wed, Jan 19 2022, René Scharfe wrote:

> Am 11.01.22 um 02:57 schrieb Ævar Arnfjörð Bjarmason:
>>
>> 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.

Well spotted. Your downthread patch LGTM (with the small nit I noted
that having an optbug() check for this would be even better).

>>
>>> ---track::
>>> +--track [inherit|direct]::
>>
>> ...as is this...
>
> Same here:
>
>    --track[=(direct|inherit)]
>
>>
>>> -		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).

*nod*

>> 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.

I'd skimmed the code & -h generation, but see on a second reading that I
was just wrong about this.

I.e. I thought it would always misformat alternate args, but as your
downthread patch shows where we'll now for "git am -h" emit e.g.:

    --show-current-patch[=(diff|raw)]

The output is now correct (and was before, we were just giving the flag
rudendantly).

>> 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..