Re: [PATCH 3/8] sequencer: fast-forward merge commits, if possible

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 23, 2018, 18:51 UTC
Message-ID: <xmqqvafsh0hu.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <9878bd57cc82daf08309943305460c1e8a050518.1516225925.git.johannes.schindelin@gmx.de>

Johannes Schindelin <johannes.schindelin@gmx.de> writes:

Show 8 quoted lines

> +	/*
> +	 * If HEAD is not identical to the parent of the original merge commit,
> +	 * we cannot fast-forward.
> +	 */
> +	can_fast_forward = commit && commit->parents &&
> +		!oidcmp(&commit->parents->item->object.oid,
> +			&head_commit->object.oid);
> +

I think this expression and assignment should better be done much later. Are you going to update commit, commit->parents, etc. that are involved in the computation in the meantime???

Show 12 quoted lines

>  	strbuf_addf(&ref_name, "refs/rewritten/%.*s", merge_arg_len, arg);
>  	merge_commit = lookup_commit_reference_by_name(ref_name.buf);
>  	if (!merge_commit) {
> @@ -2164,6 +2172,17 @@ static int do_merge(struct commit *commit, const char *arg, int arg_len,
>  		rollback_lock_file(&lock);
>  		return -1;
>  	}
> +
> +	if (can_fast_forward && commit->parents->next &&
> +	    !commit->parents->next->next &&
> +	    !oidcmp(&commit->parents->next->item->object.oid,
> +		    &merge_commit->object.oid)) {

... Namely, here. Because the earlier one is computing "are we replaying exactly the same commit on top of exactly the same state?", which is merely one half of "can we fast-forward", and storing it in a variable whose name is over-promising way before it becomes necessary. The other half of "can we fast-forward?" logic is the remainder of the if() condition we see above. IOW, when fully spelled, this code can fast-forward when we are replaying a commit on top of exactly the same first-parent and the commit being replayed is a single parent merge.

We may even want to get rid of can_fast_forward variable.

Show 9 quoted lines

> +		strbuf_release(&ref_name);
> +		rollback_lock_file(&lock);
> +		return fast_forward_to(&commit->object.oid,
> +				       &head_commit->object.oid, 0, opts);
> +	}
> +
>  	write_message(oid_to_hex(&merge_commit->object.oid), GIT_SHA1_HEXSZ,
>  		      git_path_merge_head(), 0);
>  	write_message("no-ff", 5, git_path_merge_mode(), 0);