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

From: Derrick Stolee <[email-masked]>
Date: Wed, 10 Aug 2022 08:52:23 -0400

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

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

Thanks,
-Stolee