On Sat, Apr 2, 2016 at 7:33 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote:
> 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..."
> 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...
> 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
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`/
> + 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.
> 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.
--
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