Re: [PATCH 2/2] fetch-pack: warn if in commit graph but not obj db

[Taylor Blau <me@ttaylorr.com>]

On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:
> > Another thought about this whole thing is that we essentially have a
> > code path that says: "I found this object from the commit-graph, but
> > don't know if I actually have it on disk, so mark it to be checked later
> > via has_object()".
> >
> > I wonder if it would be more straightforward to replace the call to
> > lookup_commit_in_graph() with a direct call to has_object() in the
> > deref_without_lazy_fetch() function, which I think would both (a)
> > eliminate the need for a new flag bit to be allocated, and (b) prevent
> > looking up the object twice.
> >
> > Thoughts?
>
> This would undo the optimization in 62b5a35a33 (fetch-pack: optimize
> loading of refs via commit graph, 2021-09-01), and also would not work
> without changes to the fetch negotiation code - I tried to describe it
> in the commit message, perhaps not very clearly, but the issue is that
> even if we emit "want X", the fetch negotiation code would emit "have
> X" (the X is the same in both), and at least for our JGit server at
> $DAYJOB, the combination of "want X" and "have X" results in the server
> sending an empty packfile (reasonable behavior, I think). (And I don't
> think the changes to the fetch negotiation code are worth it.)

Thanks for the clarifications above. What I was trying to poke at here
was... doesn't the change as presented undo that optimization, just in a
different way?

In 62b5a35a33 we taught deref_without_lazy_fetch() to lookup commits
through the commit-graph. But in this patch, we now call has_object()
on top of that existing check. Am I missing something obvious?

Thanks,
Taylor