Re: "Not possible to fast-forward" when pull.ff=only and new commits on remote

From: Jeff King <peff@peff.net>
Date: Oct 20, 2021, 17:09 UTC
Message-ID: <YXBNY7/mWyxvAo/r@coredump.intra.peff.net>
In-Reply-To: <xmqqzgr3pso7.fsf@gitster.g>

On Wed, Oct 20, 2021 at 09:28:08AM -0700, Junio C Hamano wrote:

Show 12 quoted lines

> Jeff King <peff@peff.net> writes:
> 
> > Thanks for reporting, this is an interesting case. I agree that it
> > probably ought to continue to be a noop. There is nothing to pull, and
> > so the question of ff-versus-merge should not even enter into it.
> 
> Probably something along this line?  Hasn't been tested beyond
> compiling and passing
> 
>     $ git checkout master && ./git pull --ff-only -v . maint
> 
> but that should be sufficient, I hope.

Yeah, this direction makes sense to me. Just looking over the patch...

Show 12 quoted lines

> +/*
> + * Is orig_head is a descendant of _all_ merge_heads?
> + * Unfortunately is_descendant_of() cannot be used as it 
> + * asks if orig_head is a descendant of at least one of them.
> + */
> +static int already_up_to_date(struct object_id *orig_head,
> +			      struct oid_array *merge_heads)
> +{
> +	int i;
> +	struct commit *ours;
> +
> +	ours = lookup_commit_reference(the_repository, orig_head);

I think orig_head can be the null oid if we're on an unborn HEAD. I guess you'd want to return "1" in that case (but I could be wrong; it looks like get_can_ff() assumes it's valid, so perhaps that case is handled earlier).

I'd expect that merge_heads can never be empty here, or we'd bail earlier in the command, but I didn't check (though again, get_can_ff() seems to assume there's at least one).

Show 12 quoted lines

> +	for (i = 0; i < merge_heads->nr; i++) {
> +		struct commit_list *list = NULL;
> +		struct commit *theirs;
> +		int ok;
> +
> +		theirs = lookup_commit_reference(the_repository, &merge_heads->oid[i]);
> +		commit_list_insert(theirs, &list);
> +		ok = repo_is_descendant_of(the_repository, ours, list);
> +		free_commit_list(list);
> +		if (!ok)
> +			return 0;
> +	}

Running a sequence of traversals like this can be slow, because we may walk over the same history again and again. But I think in the usual non-octopus cases we'd only have one entry, so we'd only be adding a single extra merge-base traversal in most cases.

It does feel like this could be combined with get_can_ff() somehow so that we're not adding even that single traversal. But I expect that may be hard to do because of the multiple heads (e.g., we cannot use the usual ahead/behind code).

-Peff