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

From: Jeff King <peff@peff.net>
Date: Mar 17, 2016, 01:50 UTC
Message-ID: <20160317015044.GB12830@sigill.intra.peff.net>
In-Reply-To: <0102015381b7c7b5-5b57a780-369b-478b-94d2-7094f0befdad-000000@eu-west-1.amazonses.com>

On Wed, Mar 16, 2016 at 11:16:58PM +0000, Pranit Bauva wrote:

Show 12 quoted lines

> 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
> 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?

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.

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.

-Peff