Re: parse-options does not recognize "unspecified" behavior

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Mar 17, 2016 at 2:19 AM, Jeff King <peff@xxxxxxxx> wrote:
> On Thu, Mar 17, 2016 at 01:21:49AM +0530, Pranit Bauva wrote:
>
>> I noticed that parse-options does not recognize the variable which is
>> set to -1 so as to denote the "unspecified" value.
>
> Right. Like all of the stock parse-options handlers, it does not ever
> read or understand the value passed to it by the caller. It only
> increments or decrements.
>
>> I did the following changes in builtin/commit.c (in master branch not
>> the patch I am working on) :
>>  - static int verbose = -1
>>  - introduced a printf statement after parsing the options to print
>> the value of verbose.
>>
>> When I ran `git commit` :
>>  I get the output that verbose is set to -1.
>>
>> When I ran `git commit -v` :
>> I get the output that verbose is set to 0.
>>
>> When I ran `git commit -v -v` :
>> I get the output that verbose is set to 1.
>>
>> When I ran `git commit --no-verbose` :
>> I get the out that verbose is set to 0.
>> [...]
>> It seems that parse-options just increments the value without
>> considering the -1 flag to denote "unspecified value".
>>
>> Is this a bug?
>
> Not in parse-options, though I think setting verbose to "-1" in the
> first place is wrong.
>
> In general, parse-options does not know or care about the default values
> that callers assign to variables; it just writes to them based on the
> option-type specified by the caller. So the behavior for "commit",
> "commit -v", and "commit -v -v" you show are perfectly reasonable.
>
> But the one for "--no-verbose" is wrong. Parse-options has to write some
> "reset" value, and it does not know what the initial default was. So it
> writes 0. This is the same for options like OPT_SET_INT, and similar for
> string options (where we set it to NULL).
>
> So I think the caller choosing "-1" here as the "not set" value is the
> bug.
>
> -Peff

I agree to you on the point that parse-options should not care about
the value passed to it. But I think plainly incrementing the value of
the variable is not a very nice way. I have an another approach to it.
The parse-options will first store a temporary structure. If there is
some changes (not the "--no-" ones) then it sets the respective
variable in temporary structure to the set value. If "--no-" is passed
then it writes the "reset" value to the respective variable in
temporary structure. If nothing about that options is specified then
it copies the respective variable from original to temporary. After
completing the entire process, it can copy temporary structure to the
original structure.

What are your opinions about this?
--
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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]