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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2009, 08:13 UTC
Message-ID: <7vmyc8mh56.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vr61kmht9.fsf@gitster.siamese.dyndns.org>

Junio C Hamano <gitster@pobox.com> writes:

Show 23 quoted lines

> 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().

But I am inclined to postpone such a change til after 1.6.2.