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

> Pranit Bauva <pranit.bauva@gmail.com> writes:
>
>> This change will not affect existing users of COUNTUP at all, as long as
>> they use the initial value of 0 (or more).
>>
>> Force uses the "unspecified" value in conjunction with OPT__FORCE in
>> builtin/clean.c in a different way to handle its config which
>> munges the "-1" into 0 before we get to parse_options.
>
> These two paragraphs leave the readers wondering what the conclusion
> is.  "it is OK as long as..." makes us wonder "so are there users
> that do not satisfy that condition?"  "in a different way" makes us
> wonder "Does this change break builtin/clean.c because COUNTUP is
> used in a different way?".
>
>         This change does not affect existing users of COUNTUP,
>         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.
>
> perhaps?

>>  `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
>> +     all, set `int_var` to a negative value, and if it is still
>> +     negative after parse_options(), then neither `--option` nor
>> +     `--no-option` was seen.
>>
>>  `OPT_BIT(short, long, &int_var, description, mask)`::
>>       Introduce a boolean option.
>> diff --git a/parse-options.c b/parse-options.c
>> index 47a9192..86d30bd 100644
>> --- a/parse-options.c
>> +++ b/parse-options.c
>> @@ -110,7 +110,13 @@ static int get_value(struct parse_opt_ctx_t *p,
>>               return 0;
>>
>>       case OPTION_COUNTUP:
>> -             *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>> +             if (unset)
>> +                     *(int *)opt->value = 0;
>> +             else {
>> +                     if (*(int *)opt->value < 0)
>> +                             *(int *)opt->value = 0;
>> +                     *(int *)opt->value += 1;
>> +             }
>>               return 0;
>>
>>       case OPTION_SET_INT:
>
> The new behaviour given by the patch looks very sensible, but I
> wonder if this is easier to reason about:
>
>         case OPTION_COUNTUP:
> +               if (*(int *)opt->value < 0)
> +                       *(int *)opt->value = 0;
>                 *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, upon hitting this case arm, we know that an explicit option
> was given from the command line, so the "unspecified" initial value,
> if it is still there, gets reset to 0, and after doing that, we just
> do the usual thing.