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

Pranit Bauva [off-list ref] 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?

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.

On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano [off-list ref] wrote:

Pranit Bauva [off-list ref] writes:

quoted

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.

quoted

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