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

[Taylor Blau <ttaylorr@openai.com>]

On Wed, Oct 21, 2026 at 08:49:27AM +0200, Patrick Steinhardt wrote:
> 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.

I originally wrote it this way because the skipping negotiator's version
of `rev_list_push()` returns a pointer to an 'entry' struct, whereas the
default negotiator's simply returns void. But the only caller that needs
to learn to handle a NULL return value from that function is simply
`push_parent()` itself, since the other two callers already disregard
the return value.

I agree that performing the parse in `rev_list_push()` makes more sense
from both an ergonomic and consistency perspective, so I have moved it
there for the next round.

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

That makes sense. The commit struct comes from the queue, and we know
that the queue always has fully-parsed commits, so there is no need to
re-parse it (even though the second and subsequent calls are noops).
I've removed it.

> > +	sed -n "s/.*fetch> have //p" trace >haves &&
> > +	sed -n "1,2p" haves >actual &&
>
> Nit: `head -n2 haves >actual`

Thanks, I realized when responding here that we don't even need the
'haves' intermediate file, so we can replace this and the above line
with:

    sed -n "s/.*fetch> have //p" trace | head -n 2 >actual &&

Thanks for reviewing!

Thanks,
Taylor