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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 13, 2016, 05:56 UTC
Message-ID: <CAPig+cQM_E9VgsxifBcT_XGyTCCaanNcfQEm6rU85pSNaQG7MQ@mail.gmail.com>
In-Reply-To: <010201540cb60971-0bf3b786-4acd-4317-bd3c-3397dc2b51f9-000000@eu-west-1.amazonses.com>

On Tue, Apr 12, 2016 at 7:02 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 10 quoted lines

> 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...

Show 24 quoted lines

> 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`/

Show 5 quoted lines

> +       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