Taylor Blau [off-list ref] writes:

On Thu, Oct 31, 2024 at 02:43:19PM -0700, Jonathan Tan wrote:

quoted

quoted

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

deref_without_lazy_fetch() is used in these situations:
 (1) to mark things COMPLETE (the 2nd argument is set to 1)
 (2) all other situations (the 2nd argument is set to 0)

62b5a35a33 teaches deref_without_lazy_fetch() to use the commit-graph in
all situations.

The change I have presented in this patch set teaches
deref_without_lazy_fetch() to read both the commit-graph and the object
DB in (1) but not (2). So I'm undoing the optimization, but not for
all situations.

My understanding of your suggestion was to undo the optimization in
all situations.