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