Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

On Fri, 27 Feb 2009, Junio C Hamano wrote:

Junio C Hamano [off-list ref] writes:

quoted

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*