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

> Junio C Hamano <gitster@pobox.com> 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().