On Wed, Oct 20, 2021 at 09:28:08AM -0700, Junio C Hamano wrote:
> Jeff King <peff@xxxxxxxx> 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...
> +/*
> + * 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).
> + 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
