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

[Junio C Hamano <gitster@pobox.com>]

Patrick Steinhardt <ps@pks.im> writes:

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

Thanks for noticing this.