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

[Junio C Hamano]

Pranit Bauva  writes:

> Also, when such an idea for new feature comes up, it is better to
> implement it because let's say some developer is stuck in future and
> this new feature could help him but he doesn't have a clue that such a
> discussion happened some time ago. Thus saving him time and further
> effort.
>
> Anyways, How is the convention in git for these type of futuristic ideas?

Just keep it in mind without complicating the code, unless the idea
truly makes sense and has immediate use.

For example, I do not know how it would be useful to be able to
count up starting from 2 with an option --more that is COUNTUP, if
your --no-more would reset it to 0.

 git cmd --more ;# sets more=2
 git cmd --more --more ;# sets more=3
 git cmd --no-more --more ;# sets more=1, not more=2


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

On Fri, Apr 1, 2016 at 1:36 AM, Junio C Hamano  wrote:
> Pranit Bauva  writes:
>
>> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
>>>
>>> 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".
>
> I do not think there is any planned users of such an enhancement,
> but the above points us into a future possibility to be able to do
> this:
>
> case OPTION_COUNTUP:
> +   if (*(int *)opt->value < 0)
> +   *(int *)opt->value = -*(int *)opt->value - 1;
> *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
>
> That is, by using "-2" as the "unspecified" value, you can start
> counting up from 2 (i.e. the presence of the option resets the
> variable to 1 and then the option being not "unset" increments it)
> if your application wants to.

I am not very familiar with Git community but I think including a new
feature to use our existing library (who's functionality isn't
required as for now) can trigger some creative thoughts in minds of
other developers which will make them think "How could this be used?".
This could lead to some exciting ideas on improving the UI of git.
Having something actually in hand gives you a confidence, rather than
knowing that you could grab it whenever it is needed.

Also, when such an idea for new feature comes up, it is better to
implement it because let's say some developer is stuck in future and
this new feature could help him but he doesn't have a clue that such a
discussion happened some time ago. Thus saving him time and further
effort.

Anyways, How is the convention in git for these type of futuristic ideas?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  writes:

> On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
>>
>> 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".

I do not think there is any planned users of such an enhancement,
but the above points us into a future possibility to be able to do
this:

case OPTION_COUNTUP:
+   if (*(int *)opt->value < 0)
+   *(int *)opt->value = -*(int *)opt->value - 1;
*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;

That is, by using "-2" as the "unspecified" value, you can start
counting up from 2 (i.e. the presence of the option resets the
variable to 1 and then the option being not "unset" increments it)
if your application wants to.

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Pranit Bauva]

On Fri, Apr 1, 2016 at 12:11 AM, Junio C Hamano  wrote:
> Pranit Bauva  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, _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, _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 majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

[Junio C Hamano]

Pranit Bauva  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, _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, _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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html