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

[Taylor Blau <ttaylorr@openai.com>]

On Fri, Oct 23, 2026 at 07:30:58AM -0700, Junio C Hamano wrote:
> Taylor Blau <ttaylorr@openai.com> writes:
>
> > On Thu, Oct 22, 2026 at 03:34:25PM -0700, Junio C Hamano wrote:
> >> Perhaps rev_list_push() should mark the commit as 'SEEN' only after
> >> repo_parse_commit() succeeds?
> >
> > I considered this but ultimately decided against it, since the default
> > negotiator does:
> > ...
> > There are various callers of rev_list_push() in the default negotiator,
> > but all include SEEN in the provided 'mark'.
>
> It is understandable that we do not want to widen the scope of the
> patch by adding a preparatory "fix the existing ones" step, but I do
> not think "the existing ones make the same mistake" is particularly
> a good excuse to add new one.

I think that's fair. For what it's worth, when writing it my thought
process was "let's be as consistent as possible with the most
battle-tested negotiator" as opposed to "this is broken, let's repeat
the breakage elsewhere".

But looking at it more closely, I think you're right that there is a bug
here in the default negotiator. So...

> Would a hybrid approach to (1) avoid copying a mistake to new code and
> (2) document near the new code and/or in the log message that the
> difference is deliberate and fixing existing ones are #leftoverbits
> and not addressed in this commit?

I think a hybrid approach is fine, though I am happy to fix the default
negotiator alongside this series. I have a patch for it locally, but I'm
happy to send either depending on your preference.

Thanks,
Taylor