On Fri, 27 Feb 2009, Junio C Hamano wrote: > Junio C Hamano <gitster@xxxxxxxxx> writes: > > > It triggers this codepath in get_value(): > > > > case OPTION_BOOLEAN: > > *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > > return 0; > > > > and ends up incrementing it to zero. > > > > I wonder what would break if we simply change this to: > > > > case OPTION_BOOLEAN: > > *(int *)opt->value = !unset; > > return 0; > > > > Damn it, it is called BOOLEAN, and naïvely I would think --option would > > set it to 1, --no-option would set it to 0, and not having --option nor > > --no-option would leave the associated variable alone, but apparently that > > is not what is happening. > > > > Pierre, do you remember why this code is implemented this way? The > > "increment if we have --option" semantics seems to date back to 4a59fd1 > > (Add a simple option parser., 2007-10-15) which is the day one of the > > history of parse-options. > > I think that this came from a misguided attempt to do: > > -v > -v -v > -v -v -v > > to cumulatively record the desired verbosity levels. > > Originally we did not have OPT__VERBOSITY() nor OPT_VERBOSE() so it is > understandable that OPT_VERBOSE() was implemented in terms of this > OPT_BOOLEAN() construct. > > I think all parse_options() users that do support the verbosity level is > either using OPT__VERBOSE() or OPT_VERBOSITY() these days, and we could > probably fix this by doing something like: > > (1) Introduce OPTION_CUMULATIVE_LEVEL whose behaviour is "--no-option to > reset to zero, --option to raise by one", and fix OPTION_BOOLEAN to > "--no-option to zero, --option to one": > > case OPTION_BOOLEAN: > *(int *)opt->value = !unset; > return 0; > case OPTION_CUMULATIVE_LEVEL: > *(int *)opt->value = unset ? 0 : *(int *)opt->value + 1; > return 0; > > (2) Use OPT_CUMULATIVE_LEVEL() instead of OPT_BOOLEAN() to implement > OPT__VERBOSE() and OPT__QUIET(). I think there are a few other options that work like that, such as -C for diff and the like; I don't know if there are any others that go through parse_options, but I wouldn't count on there not being any. So I think the conversion of OPT_BOOLEAN needs to go with an audit of current users. -Daniel *This .sig left intentionally blank*
