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

On Thu, Mar 17, 2016 at 2:19 AM, Jeff King [off-list ref] wrote:

On Thu, Mar 17, 2016 at 01:21:49AM +0530, Pranit Bauva wrote:

quoted

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.

quoted

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?