On Thu, Mar 17, 2016 at 12:58 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Wed, Mar 16, 2016 at 9:50 PM, Jeff King <peff@xxxxxxxx> 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. I guess the language wasn't very clear. I will change this. >>> 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. My bad, it should be 2. > > 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 majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html