Re: [PATCH v2 08/10] builtin/bugreport.c: create '--diagnose' option

From: Victoria Dye <vdye@github.com>
Date: Aug 10, 2022, 16:13 UTC
Message-ID: <19fe9b29-a860-fb59-ecb4-0d72b61167fe@github.com>
In-Reply-To: <77f445f7-3934-c165-3160-e09d7b884774@github.com>

Derrick Stolee wrote:

Show 30 quoted lines

> On 8/9/22 7:53 PM, Victoria Dye wrote:
>> Derrick Stolee wrote:
>>> On 8/3/2022 9:45 PM, Victoria Dye via GitGitGadget wrote:
> 
>>>> +static int option_parse_diagnose(const struct option *opt,
>>>> +				 const char *arg, int unset)
>>>> +{
>>>> +	enum diagnose_mode *diagnose = opt->value;
>>>> +
>>>> +	BUG_ON_OPT_NEG(unset);
>>>> +
>>>> +	if (!arg || !strcmp(arg, "basic"))
>>>> +		*diagnose = DIAGNOSE_BASIC;
>>>> +	else if (!strcmp(arg, "all"))
>>>> +		*diagnose = DIAGNOSE_ALL;
>>>
>>> Should we allow "none" to reset the value to DIAGNOSE_NONE?
>>
>> As far as I can tell, while some builtins have options that  match the
>> default behavior of the command (e.g., '--no-autosquash' in 'git rebase'),
>> those options typically exist to override a config setting (e.g.,
>> 'rebase.autosquash'). No config exists for 'bugreport --diagnose' (and I
>> don't think it would make sense to add one), so '--diagnose=none' would only
>> be used to override another '--diagnose' specification in the same
>> command/alias (e.g., 'git bugreport --diagnose=basic --diagnose=none'). 
> 
> Ah, so --diagnose=none isn't valuable because --no-diagnose would be
> the better way to write the same thing. You would need to remove the
> PARSE_OPT_NONEG from your OPT_CALLBACK_F() to allow that (and then do
> the appropriate logic with the "unset" parameter).

I'm not sure I follow. I wasn't suggesting a difference in value between '--no-diagnose' and '--diagnose=none'. My point was that, when there's an option variant that "resets" the value to the default (like '--no-autosquash', '--no-recurse-submodules', etc.), it usually *also* corresponds to an overridable config setting ('rebase.autosquash', 'push.recurseSubmodules'). No such 'bugreport.diagnose' config exists (or, IMO, should exist), so the need for a "reset to default" option seemed weaker.

I used boolean options as my examples, but they aren't intended to imply a meaningful difference between '--no-diagnose' amd '--diagnose=none'.

> 
> The reason to have these things is basically so users can create
> aliases (say 'git br' expands to 'git bugreport --diagnose=all', but
> they want to run 'git br --no-diagnose' to clear that --diagnose=all).

I considered that usage ("'--diagnose=none' would only be used to override another '--diagnose' specification in the same command/alias"), but wasn't sure how common it would be for this particular option. It sounds like you can see it being useful, so I'll include '--diagnose=none' in the next version.

> 
> Thanks,
> -Stolee