Re: [PATCH] negotiator/skipping.c: parse parents before queueing

[Patrick Steinhardt <ps@pks.im>]

On Mon, Oct 19, 2026 at 03:17:07PM -0500, Taylor Blau wrote:
> diff --git a/negotiator/skipping.c b/negotiator/skipping.c
> index db90fa77b53..ed6e88b59b4 100644
> --- a/negotiator/skipping.c
> +++ b/negotiator/skipping.c
> @@ -151,6 +151,9 @@ static int push_parent(struct data *data, struct entry *entry,
>  parent_found:
>  		;
>  	} else {
> +		/* Parse the parent before using its date to order the queue. */
> +		if (repo_parse_commit(the_repository, to_push))
> +			return 0;
>  		parent_entry = rev_list_push(data, to_push, 0);
>  	}

Why do you parse the commit in `push_parent()` instead of parsing it in
`rev_list_push()`? The latter would also cover the other two callsites
that call it, and to me it's not clear why they wouldn't want to parse
the commits before adding it to the queue.

Doing so in `rev_list_push()` would also bring it in line with the
default negotiator.

There's also another call to `repo_parse_commit()` in `get_rev()`. Once
we start parsing the commit in `rev_list_push()` we could drop that call
as we'd know that every commit in the queue is parsed already.

> diff --git a/t/t5552-skipping-fetch-negotiator.sh b/t/t5552-skipping-fetch-negotiator.sh
> index 96e224168eb..7a0b9b32474 100755
> --- a/t/t5552-skipping-fetch-negotiator.sh
> +++ b/t/t5552-skipping-fetch-negotiator.sh
> @@ -223,4 +223,37 @@ test_expect_success 'do not send "have" with ancestors of commits that server AC
>  	have_sent b2.c1 b2.c0
>  '
>  
> +test_expect_success 'recent ancestors are sent before old ref tips' '
> +	rm -rf server client trace &&
> +	git init server &&
> +	test_tick=2000000000 &&
> +	test_commit -C server --no-tag base &&
> +	test_commit -C server --no-tag parent &&
> +	test_commit -C server --no-tag main &&
> +	git clone server client &&
> +	test_commit -C server --no-tag to_fetch &&
> +
> +	git -C client checkout -b topic &&
> +	test_commit -C client --no-tag topic &&
> +
> +	# These old tips have known dates, but the recent parent has no ref.
> +	# Without parsing parents before queueing them, its date is zero and
> +	# all of these tips are sent first, delaying it past the first batch.
> +	for i in $(test_seq 32)
> +	do
> +		old=$(echo "old $i" |
> +			GIT_AUTHOR_DATE="1000000000 +0000" \
> +			GIT_COMMITTER_DATE="1000000000 +0000" \
> +			git -C client commit-tree HEAD^{tree}) &&
> +		git -C client update-ref refs/heads/old-$i "$old" || return 1
> +	done &&
> +
> +	test_config -C client fetch.negotiationalgorithm skipping &&
> +	trace_fetch client origin &&
> +	git -C client rev-parse HEAD HEAD~2 >expect &&
> +	sed -n "s/.*fetch> have //p" trace >haves &&
> +	sed -n "1,2p" haves >actual &&

Nit: `head -n2 haves >actual`

Thanks!

Patrick