Re: [PATCH/RFC] parse-options.c: make OPTION__COUNTUP consider negative values

> On Wed, Mar 16, 2016 at 9:50 PM, Jeff King <peff@peff.net> wrote:
>> On Wed, Mar 16, 2016 at 11:16:58PM +0000, Pranit Bauva wrote:
>>> The reason to make it consider negative values or more specifically
>>> "unspecified" values is to differentiate between the option passed
>>> once, multiple times or with --no-option. This makes the receiver
>
> This is inaccurate and rather confusing. It's not that an
> "unspecified" value gives you the ability to "differentiate between
> once, multiple time or with --no-option", but rather that it allows
> you to determine wether --option or --no-option was encountered at
> all.

>>> know what actually happened with the arguments which is particularly
>>> required with option have multiple levels of that option.
>>>
>>> Eg. :
>>> initialize verbose = -1
>>> `git commit` => verbose = -1
>>> `git commit -v` => verbose = 1
>>> `git commit -v -v` => verbose = 1
>>> `git commit --no-verbose` => verbose = 0
>>
>> This second to last example would be 2, right?
>
> Right.

>
> I'm not sure that this example block is helpful, though. A clearer
> commit message which does a better job of explaining the reason for
> the change would likely eliminate the need for an example.
>
>> That aside, this patch does mean that one can no longer use
>> OPT_COUNTUP() for negative values (i.e., the caller must start it at
>> either 0 or 1, and it must always go up from there).
>>
>> And we would need to verify that all of the existing callers are OK with
>> this. Did you check that that (not rhetorical; I suspect they are all
>> OK, but somebody needs to check)?
>>
>> We are also changing semantics without changing the interface, which
>> means any topics in flight (that you _cannot_ review, because you have
>> not seen them yet) may be subtly broken. To me that is not an absolute
>> deal-breaker, but something to weigh against the utility of the change.
>
> Indeed, I was envisioning a more conservative approach of having
> OPT__VERBOSE use a custom callback or perhaps introducing a new
> 'flags' value or even (probably too ugly) abusing the 'defval' field
> to specially indicate that it wants the "negative means unspecified"
> behavior; the other consumers of OPT_COUNTUP would not request this
> special behavior. But, as you say, changing the behavior of
> OPT_COUNTUP unconditionally may not be a deal-breaker.
>
> I also realized that Pranit can achieve the desired behavior without
> modifying OPT__VERBOSE at all. Specifically, rather than initializing
> his opt_verbose variable to -1, he can instead initialize it to 1.
> Then:
>
> * if --verbose is seen (one or more times), opt_verbose will be >=2,
> and the real verbosity level will be (opt_verbose - 1)
>
> * if --no-verbose is seen, opt_verbose will be 0
>
> * if neither is seen, then opt_verbose will remain 1
>
> However, I think this approach is far too ugly and non-obvious to
> seriously suggest using it, whereas the change to OPT__VERBOSE is
> easily understood and could prove useful in the future for other
> commands with multiple verbosity levels.
>
>> When looking more carefully at builtin/commit.c for the other thread, it
>> occurred to me that OPT_BOOL might be a better fit for commit's "-v". It
>> really is a boolean "show the diff or not" and thus unlike the other
>> "make me more verbose". And OPT_BOOL already has the behavior you want,
>> I think.
>
> For completeness (for readers of this thread), it was pointed out in
> the other thread[1] that git-commit does indeed recognize multiple
> verbosity levels, so changing it to use OPT_BOOL would be undesirable
> (wrong).
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/289027/focus=289074