Re: [PATCH v12 3/5] parse-options.c: make OPTION_COUNTUP respect "unspecified" values

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Apr 3, 2016, 23:10 UTC
Message-ID: <CAPig+cS23i9XFTntLTHRbNf21DxhLAcw+9ent_ZtWj=1aO7JwQ@mail.gmail.com>
In-Reply-To: <01020153d952be68-ee055619-664d-4502-92e9-6aa524e2354b-000000@eu-west-1.amazonses.com>

On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva <pranit.bauva@gmail.com> wrote:

Show 7 quoted lines

> The reason to make it respect "unspecified" values is to give the
> ability to differentiate whether `--option` or `--no-option` was
> specified at all. "unspecified" values should be in the form of negative
> values. If initial value is set to negative and `--option` specified
> then it will reflect the number of occurrences (counting done from 0),
> if `--no-option` is specified then it will reflect 0 and if nothing at
> all is given then it will retain its negative value.

Thanks, this rewrite does a better job of explaining the aim of the change and how a client can take advantage of it. However, with my "first-time reader" hat on, I still have some trouble digesting it as a coherent whole. I wonder if a rewrite like this would help?

    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.

> This change will not affect existing users of COUNTUP, because they all
> use the initial value of 0 (or more).

    "This change does not affect behavior of existing clients of..."

Show 8 quoted lines

> 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 "verbose" is set to "unspecified" while quiet is
> set to 0 which will test the new behavior with all sets of values.

I think you need to mention here that you're talking about test-parse-options.c (and indirectly t0040) since it's otherwise too easy for the reader to think that this paragraph is a continuation of the discussion about OPT_COUNTUP()'s new behavior and how it won't impact existing tests, rather than a new topic of its own (testing the behavior). Maybe say something like this:

    To test the new behavior, augment the initial "verbose" setting
    of test-parse-options.c to be unspecified...

and then go on to say that, because "quiet" is still initialized to 0, you have complete coverage. An alternative would be to split off the new testing into its own patch, which would make this patch (which is the real meat of the change) less noisy.

I actually expected you to add new functionality to test-parse-options.c specifically to test OPT_COUNTUP() directly rather than indirectly through --verbose and --quiet, however, I think I can be sold on the approach you took for a couple reasons. First, you have a genuine use-case for an "unspecified" --verbose value, so changing test-parse-options.c's --verbose to start at -1 tests what you ultimately care about. Second, since you retained 0-initialization of --quiet, that case of OPT_COUNTUP() is still being tested.

What I find a bit disturbing about this approach is that you are getting "full coverage" only because of current *implementation*, not because you are explicitly testing for *expected* behavior. That is, you get that coverage only while both OPT__VERBOSE() and OPT__QUIET() are built atop OPT_COUNTUP(); if OPT__QUIET() is ever rewritten so it no longer uses OPT_COUNTUP(), then the tests silently lose full coverage. However, I may be over-worrying about the situation...

Show 11 quoted lines

> 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

s/was set/was encountered/

> +       all, set `int_var` to a negative value, and if it is still

s/set `int_var`/initialize `int_var`/

Show 8 quoted lines

> +       negative after parse_options(), then neither `--option` nor
> +       `--no-option` was seen.
> diff --git a/parse-options.c b/parse-options.c
> @@ -110,6 +110,8 @@ static int get_value(struct parse_opt_ctx_t *p,
>         case OPTION_COUNTUP:
> +               if (*(int *)opt->value < 0)
> +                       *(int *)opt->value = 0;
>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;

This is nicer.

Show 8 quoted lines

>                 return 0;
> 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' '
> +       test-parse-options --no-verbose >output 2>output.err &&
> +       test_must_be_empty output.err &&
> +       test_cmp expect output
> +'

If you take the advice of my 2/5 review and add new tests (in a new patch) which test --no-verbose and --no-quiet, then I think this new test here can just go away since the case it cares about will already be covered.