On Wed, Mar 16, 2016 at 9:50 PM, Jeff King <p...@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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to