On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Pranit Bauva <pranit.bauva@xxxxxxxxx> 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?
Thanks again for the help with the comit message. I sometimes fail to
see how first time readers will infer from the message.
>> `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.
This does look cleaner. Nice thinking that there is no need to
actually specify when it gets 0. It gets 0 no matter what as long as
OPTION_COUNTUP is speficied in any format (with or without --no) and
variable is "unspecified".
--
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