Re: [PATCH] Teach 'git merge' and 'git pull' the option --ff-only

From: Björn Gustavsson <bgustavsson@gmail.com>
Date: Oct 29, 2009, 06:23 UTC
Message-ID: <4AE934DA.7080907@gmail.com>
In-Reply-To: <7vk4yfi1dd.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

Show 8 quoted lines

> Björn Gustavsson <bgustavsson@gmail.com> writes:
>>  
>> +--ff-only::
>> +	Refuse to merge unless the merge can be resolved as a
>> +	fast-forward.
> 
> Do you or do you not allow "already up to date"?  I think it makes sense
> to allow it, but it is unclear from these two lines.

I do allow it. I will change the description to the following in the re-roll:

--ff-only::
	Refuse to merge and exit with a non-zero status unless the
	current `HEAD` is already up-to-date or the merge can be
	resolved as a fast-forward.

Show 10 quoted lines

> 
>> @@ -874,6 +877,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  		option_commit = 0;
>>  	}
>>  
>> +	if (!allow_fast_forward && fast_forward_only)
>> +		die("You cannot combine --no-ff with --ff-only.");
> 
> Are these the only nonsensical combinations?  How should this interact
> with other options, e.g. --squash or --message?

They are the only options I can think of that flatly contradict each other.

Combining --squash and --ff-only will succeed if the current HEAD can be fast-forwarded and will abort otherwise. I don't know how useful that would be in practice, but I see no strong reason to forbid it.

The -m option will always be ignored, of course, and there will be the usual warning if fast-forward is possible:

   Fast forward (no commit created; -m option ignored)

I don't think there is any need to explicitly forbid the combination of -m and --ff-only.

I should probably update the commit message in the re-roll to include the information in the previous paragraphs.

Show 19 quoted lines

>> @@ -969,8 +975,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>>  	}
>>  
>>  	for (i = 0; i < use_strategies_nr; i++) {
>> -		if (use_strategies[i]->attr & NO_FAST_FORWARD)
>> +		if (use_strategies[i]->attr & NO_FAST_FORWARD) {
>>  			allow_fast_forward = 0;
>> +			if (fast_forward_only)
>> +				die("You cannot combine --ff-only with the merge strategy '%s'.", use_strategies[i]->name);
>> +		}
> 
> I am not convinced this tests the right condition nor it is placed at the
> right place in the codepath---even if a specified strategy happens to
> allow fast-forward, wouldn't it be nonsense to say
> 
>     $ git merge --ff-only -s resolve that-one
> 
> in the first place?  Note that I am not saying "I am convinced this is
> wrong."

Re-thinking it, I think that the test should be removed. It seemed like a good idea at the time to point out which strategy that prevented the fast-forward, but if there is a list of merge strategies, the test will prevent --ff-only to succeed if *any* of merge strategies cannot fast-forward. (Also, but I am not sure about this, a merge strategy that does not allow fast-forward might allow up-to-date.)

Therefore, I will remove the test in the re-roll.

Thanks for the comments!

/Björn