Re: [PATCH v2] pull: --ff-only should make it a noop when already-up-to-date

From: Jeff King <peff@peff.net>
Date: Oct 20, 2021, 20:45 UTC
Message-ID: <YXB/1S2fid7MUmvu@coredump.intra.peff.net>
In-Reply-To: <xmqqh7dbplji.fsf@gitster.g>

On Wed, Oct 20, 2021 at 12:02:09PM -0700, Junio C Hamano wrote:

Show 12 quoted lines

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

Show 21 quoted lines

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

Show 13 quoted lines

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