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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 31, 2016, 18:41 UTC
Message-ID: <xmqq60w2o5g9.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <01020153cd2341e2-2616ba0b-9271-4a97-87d1-b6dedbf31828-000000@eu-west-1.amazonses.com>

Pranit Bauva <pranit.bauva@gmail.com> writes:

Show 6 quoted lines

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

Show 32 quoted lines

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