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

[Derrick Stolee <derrickstolee@github.com>]

On 8/10/2022 12:13 PM, Victoria Dye wrote:
> Derrick Stolee wrote:
>> 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. 

You're right that these make the most sense when there can be a
non-CLI source of a setting to unset, which is a better reason than
the alias reason I gave.
 
> 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.

This change would make it easier to add a config option in the future,
though I doubt we will need it as 'git bugreport' should be used too
infrequently to want to set up such config in advance.

Thanks,
-Stolee