Taylor Blau <me@xxxxxxxxxxxx> writes: > 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 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.
