Re: [PATCH v14 4/6] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

> On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> 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@gmail.com>
>> ---
>> 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