Hi Junio,
On Tue, 23 Jan 2018, Junio C Hamano wrote:
> Johannes Schindelin <johannes.schindelin@xxxxxx> 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???
No, it is in the exact right spot.
What you are missing is that there will be some more code here, to support
octopus merges.
That's why it is exactly the right spot.
> > @@ -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.
... which the octopus merge code will never reach. That's why this is the
wrong spot for that initial can_fast_forward assignment.
> 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.
Absolutely not.
Ciao,
Dscho
