Re: [PATCH] merge: handle --ff/--no-ff/--ff-only as a tri-state option

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 2, 2013, 18:46 UTC
Message-ID: <7vfvvw94v4.fsf@alter.siamese.dyndns.org>
In-Reply-To: <51D2927F.3040207@alum.mit.edu>

Michael Haggerty <mhagger@alum.mit.edu> writes:

Show 6 quoted lines

> You allow --no-ff-only but ignore it, which I think is incorrect.  In
>
>     git merge --ff-only --no-ff-only [...]
>
> , the --no-ff-only should presumably cancel the effect of the previous
> --ff-only (i.e., be equivalent to "--ff").

Ideally, if we were starting from scratch and living in the "you pick one out of three" world, we should forbid "--no-ff-only". The "--no-ff" option is spelled as if it is a negation of "--ff", and it did start as such before "--ff-only" was introduced, but "--no-ff" would have been better named "--always-create-merge", which is what the option really means.

And in the tristate world, with mutually exclusive "--A", "--B", and "--C" options, "--no-C" does not mean "I want to do A" at all.

If the existing code had allowed with "--no-ff-only" to defeat configured merge.ff=only from the command line, then there may have been users who are used to that behaviour, and we cannot break them, but luckily or unluckily it does not work, so...

Show 16 quoted lines

>> @@ -1157,14 +1181,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  		show_diffstat = 0;
>>  
>>  	if (squash) {
>> -		if (!allow_fast_forward)
>> +		if (fast_forward == FF_NO)
>>  			die(_("You cannot combine --squash with --no-ff."));
>>  		option_commit = 0;
>>  	}
>>  
>
> So there is still a problem with setting merge.ff=false, namely that it
> prevents the use of --squash.  That's not good.  (I realize that you are
> not to blame for this pre-existing behavior.)
>
> How should --squash and the ff-related options interact?

Interesting point.

Show 15 quoted lines

>     git merge --ff --squash
>     git merge --no-ff --squash
>
> I think these should just squash.
>
>     git merge --ff-only --squash
>
> I think this should definitely squash.  But perhaps it should require
> that HEAD be an ancestor of the branch to be merged?
>
>     git merge --squash --ff
>     git merge --squash --no-ff
>     git merge --squash --ff-only
>
> Should these do the same as the versions with the option order reversed?

As "--squash" is about _not_ moving the head but only updating the working tree and the index, I personally think it should be treated as an error if any of these "ff" options is explicitly given from the command line.