Re: [PATCH] Add --ff-only flag to git-merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 29, 2009, 22:29 UTC
Message-ID: <7vbptpeoge.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <a891e1bd0901291257t774af061s84497cde8f4bf61c@mail.gmail.com>

Yuval Kogman <nothingmuch@woobling.org> writes:

Show 11 quoted lines

> I started incorperating your feedback but before I send a new patch I
> have several questions about the trickier bits:
>
> 2009/1/28 Junio C Hamano <gitster@pobox.com>:
>
>>  * The placement of this misses the case where a merge of two unrelated
>>   histories is attempted.  You would need to also have a check at "No
>>   common ancestors found. We need a real merge." part.
>
> Won't that fall through? The if (!common) is above, and this is
> eventually an else if for it (see line 978)

When "if (!common)" is true, the empty statement ";" is executed, and all its "else if" conditional arms will be skipped. Is that what you want to happen?

Show 7 quoted lines

>> The octopus
>>   codepath could also end up with a fast forward or up-to-date.
>
> So this case is obviously more convoluted... If an octopus merge is
> chosen should it just pass --ff-only to git-merge-octopus? Or maybe it
> should always pass --ff-only and the various different strategies
> would just die unconditionally?

I was referring to this part:

	} else {
		/*
		 * An octopus.  If we can reach all the remote we are up
		 * to date.
		 */
		int up_to_date = 1;
		...
		if (up_to_date) {
			finish_up_to_date("Already up-to-date. Yeeah!");
			return 0;
		}
	}

You do not want to fail this case, where you tried to merge others that have already been merged, when --ff-only is given, do you? After all, all that you are interested in is "do not create a new merge commit".

If you scroll down a bit from there, you will see:

	/* We are going to make a new commit. */
	git_committer_info(IDENT_ERROR_ON_NO_NAME);

	/*
	 * At this point, we need a real merge.  No matter what strategy
	 * we use, it would operate on the index, possibly affecting the

This is where "if (!common) ;" will fall through to.

If your goal is to prevent the user from creating a new merge commit, the logical place to do so would be immediately before that comment, independent from all the if..elseif..fi conditional arms that come before it, I think.

You also need to disable allow_trivial when ff-only is given, but I think that goes without saying. If you do not want to allow creating a new merge commit, you do not want even a trivial merge to happen.