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).
> Make sure we do not fail with the recently added check to restore
> the historycal behaviour.
Not sure if "historycal" is a typo or some clever pun. :)
> +/*
> + * Is orig_head is a descendant of _all_ merge_heads?
s/is a/a/
> +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;
> +}
You answered all of my "what about..." questions from before elsewhere
in the thread, so this looks correct.
> +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)"
> +'
And these tests cover the cases I'd expect. The use of "test" with
process substitution looks a bit funny to me these days, but it does
match the surrounding code (and losing the exit codes isn't a big deal
here, as we are not testing rev-parse).
The combo of "test_config" and "git -c" is unusual, but I don't see
anything wrong with it.
-Peff
