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

On Wed, Mar 16, 2016 at 9:50 PM, Jeff King [off-list ref] wrote:

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

quoted

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.

quoted

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