On Wed, Apr 13, 2016 at 11:26 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > 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. A typo. Will fix it if re-rolled > 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`/ Will include this if re-rolled. > >> + 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 I actually did include the tests in the patch 3/6[1]. I mentioned in my response[2] that I will include the tests between 2/5 and 3/5. Since, after that no email was exchanged, I thought you agreed. [1]: http://article.gmane.org/gmane.comp.version-control.git/291310 [2]: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