On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: > OPT_COUNTUP() merely increments the counter upon --option, and resets it > to 0 upon --no-option, which means that there is no "unspecified" value > with which a client can initialize the counter to determine whether or > not --[no]-option was seen at all. > > Make OPT_COUNTUP() treat any negative number as an "unspecified" value > to address this shortcoming. In particular, if a client initializes the > counter to -1, then if it is still -1 after parse_options(), then > neither --option nor --no-option was seen; If it is 0, then --no-option > was seen last, and if it is 1 or greater, than --option was seen last. Nit: I'm pretty sure that when I wrote this commit message for you[1] the "if" following the semicolon was correctly lowercase. It's not clear why it got incorrectly capitalized here. More below... > This change does not affect the behavior of existing clients because > they all use the initial value of 0 (or more). > > Note that builtin/clean.c initializes the variable used with > OPT__FORCE (which uses OPT_COUNTUP()) to a negative value, but it is set > to either 0 or 1 by reading the configuration before the code calls > parse_options(), i.e. as far as parse_options() is concerned, the > initial value of the variable is not negative. > > To test this behavior, in test-parse-options.c, "verbose" is set to > "unspecified" while quiet is set to 0 which will test the new behavior > with all sets of values. > > Signed-off-by: Pranit Bauva <pranit.bauva@xxxxxxxxx> > --- > diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt > @@ -144,8 +144,12 @@ There are some macros to easily define options: > `OPT_COUNTUP(short, long, &int_var, description)`:: > Introduce a count-up option. > - `int_var` is incremented on each use of `--option`, and > - reset to zero with `--no-option`. > + Each use of `--option` increments `int_var`, starting from zero > + (even if initially negative), and `--no-option` resets it to > + zero. To determine if `--option` or `--no-option` was set at Repeating from [1]: s/was set/was encountered/ > + all, set `int_var` to a negative value, and if it is still Repeating from [1] and [2]: s/set `int_var`/initialize `int_var`/ > + negative after parse_options(), then neither `--option` nor > + `--no-option` was seen. > diff --git a/t/t0040-parse-options.sh b/t/t0040-parse-options.sh > @@ -454,6 +454,25 @@ dry run: no > +test_expect_success 'OPT_COUNTUP() resets to 0 with --no- flag' ' What is "--no- flag"? > + test-parse-options --no-verbose >output 2>output.err && > + test_must_be_empty output.err && > + test_cmp expect output > +' In my v12 review, I noticed that neither --no-verbose nor --no-quiet was being tested by t0040 (which is conceptually independent of the OPT__COUNTUP change) and suggested[3] that you add a new patch to address that shortcoming. This idea was followed up by [1] saying that this test (here) could then be dropped since the case it checks would already be covered by the new patch. My impression was that you agreed[4] that that made sense, however, this test is still here. Did I misunderstand your response[4]? [1]: http://article.gmane.org/gmane.comp.version-control.git/290662 [2]: http://article.gmane.org/gmane.comp.version-control.git/289991 [3]: http://article.gmane.org/gmane.comp.version-control.git/290655 [4]: http://article.gmane.org/gmane.comp.version-control.git/290787 -- 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