On Thu, Mar 24, 2016 at 6:33 AM, SZEDER Gábor <szeder@xxxxxxxxxx> wrote:
>> 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@xxxxxxxxx>
>> ---
>> 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.
Agreed. It would be more helpful to explain that you are changing
OPT_COUNTUP so that --option always counts up from zero, even if the
initial value was negative, in order to allow the caller to determine
whether --option or --no-option was seen at all, by setting the
initial value to -1 and then checking if it still -1 after
parse_options(). If you explain that well, then the tabular example in
your commit message can go away altogether.
The subject ("make OPTION__COUNTUP consider negative values") could
use a little work too. OPTION__COUNTUP already works with negative
values, but not in the way you want. Instead, you want it to treat
negative values specially as "unspecified", and that's the real gist
of this patch, thus the subject ought, at the very least, have the
word "unspecified" in it.
> - 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.
>
>> diff --git 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`.
This reads a little bit backward, but, more importantly, doesn't do a
good job of conveying to the reader that a negative value can
represent "unspecified". The best way to convey that would probably be
via example. For instance, something like this:
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 seen 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.
--
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