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

From: Junio C Hamano <[email-masked]>
Date: Tue, 23 Jan 2018 10:51:41 -0800

Johannes Schindelin <[email-masked]> writes:

> +	/*
> +	 * 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???

>  	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.

> +		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);