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