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

> After upgrade to 2.33.1, the behavior of `pull.ff=only` has changed in a way that breaks some workflows, notably the default used in VSCode.
> 
> Example (specific repo doesn't matter)
> 
> ```
> $ git clone git@github.com:kcarnold/util.git
> ...
> $ cd util
> $ echo "" > test
> $ git add test
> $ git commit -m "Test"
> $ git -c pull.ff=only pull
> fatal: Not possible to fast-forward, aborting.
> $ git status
> On branch master
> Your branch is ahead of 'origin/master' by 1 commit.
>   (use "git push" to publish your local commits)
> ```
> 
> Previously this pull succeeded without error, which was as expected because no merge was necessary. 

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

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

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

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

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

> > 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).
> 
> I'd leave such an optimization as a separate topic.  This was meant
> to be a regression fix.

> Earlier, we made sure that "git pull --ff-only" (and "git -c
> pull.ff=only pull") errors out when our current HEAD is not an
> ancestor of the tip of the history we are merging, but the condition
> to trigger the error was implemented incorrectly.
> 
> Imagine you forked from a remote branch, built your history on top
> of it, and then attempted to pull from them again.  If they have not
> made any update in the meantime, our current HEAD is obviously not
> their ancestor, and this new error triggers.
> 
> Without the --ff-only option, we just report that there is no need
> to pull; we did the same historically with --ff-only, too.

> +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);
> +	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;
> +	}
> +	return 1;
> +}

> +test_expect_success 'already-up-to-date pull succeeds with "only" in pull.ff' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	git pull . c0 &&
> +	test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
> +'
> +
> +test_expect_success 'already-up-to-date pull/rebase succeeds with "only" in pull.ff' '
> +	git reset --hard c1 &&
> +	test_config pull.ff only &&
> +	git -c pull.rebase=true pull . c0 &&
> +	test "$(git rev-parse HEAD)" = "$(git rev-parse c1)"
> +'

>
> On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:
>
> > Earlier, we made sure that "git pull --ff-only" (and "git -c
> > pull.ff=only pull") errors out when our current HEAD is not an
> > ancestor of the tip of the history we are merging, but the condition
> > to trigger the error was implemented incorrectly.
> >
> > Imagine you forked from a remote branch, built your history on top
> > of it, and then attempted to pull from them again.  If they have not
> > made any update in the meantime, our current HEAD is obviously not
> > their ancestor, and this new error triggers.
> >
> > Without the --ff-only option, we just report that there is no need
> > to pull; we did the same historically with --ff-only, too.
>
> Thanks, this looks good to me overall, and I agree this is a regression
> we should try to fix promptly (so thank you for jumping on it).