Re: [PATCH v9 1/3] parse-options.c: make OPTION__COUNTUP consider negative values
On Thu, Mar 24, 2016 at 4:03 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
Show 31 quoted lines
>> The reason to make it consider negative values or more specifically
>> "unspecified" values is to give the ability to differentiate between
>> once, multiple time or with --no-option.
>>
>> Eg. :
>> initialize verbose = -1
>> `git commit` => verbose = -1
>> `git commit -v` => verbose = 1
>> `git commit -v -v` => verbose = 2
>> `git commit --no-verbose` => verbose = 0
>>
>> Signed-off-by: Pranit Bauva <pranit.bauva@gmail.com>
>>
>> ---
>> The discussion about this patch:
>> [1] : http://thread.gmane.org/gmane.comp.version-control.git/289027
>>
>> Previous version of the patch:
>> [v1] : http://thread.gmane.org/gmane.comp.version-control.git/289061
>>
>> Changes introduced:
>> Use a different language in commit message to make the change and its
>> utility more clear.
>
> I don't see the mentioned change in the commit message. In
> particular:
>
> - As Eric pointed out in the previous round, the commit message
> misses the single most important point of justification: to
> determine whether '--option' or '--no-option' was specified at
> all. Explaining this would also make the examples unnecessary.
I will justify it in the commit message. Also will remove the examples.
> - OPT_COUNTUP is used in several places, mostly indirecty, but the
> commit message doesn't explain that possible side-effects to these
> callers were considered and that they are not harmed by this
> change.
I will include bits from my and Jeff's conversation into this commit message.
Show 50 quoted lines
>> ---
>> Documentation/technical/api-parse-options.txt | 6 ++++--
>> parse-options.c | 8 +++++++-
>> 2 files changed, 11 insertions(+), 3 deletions(-)
>
> A couple of new tests to t0040-parse-options.sh would be great to
> ensure that starting from a negative value works as advertised, i.e.
> at least that '--option' jumps to 1 and '--no-option' resets to 0.
>
>> diff --git a/Documentation/technical/api-parse-options.txt b/Documentation/technical/api-parse-options.txt
>> index 5f0757d..a908d8a 100644
>> --- a/Documentation/technical/api-parse-options.txt
>> +++ b/Documentation/technical/api-parse-options.txt
>> @@ -144,8 +144,10 @@ There are some macros to easily define options:
>>
>> `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`.
>> + If the `int_var` is negative and `--option` is absent,
>> + then it will retain its value. Otherwise it will first set
>> + `int_var` to 0 and then increment it on each use of `--option`,
>> + and reset to zero with `--no-option`.
>>
>> `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:
>>
>> --
>> https://github.com/git/git/pull/218
>>
>>Pranit Bauva <pranit.bauva@gmail.com>