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
