On Wed, Jul 15, 2020 at 03:55:18PM -0700, Junio C Hamano wrote:

Fixing issues hit by real users reactively is a necessary and good
thing, but this is not the first time we patch callers of
has_object_file() for this kind of "we are merely trying to
determine the boundary of what we have, so that we know what we need
to add to this repository" queries, I am afraid.

Perhaps it is a good idea to sweep all the hits from "git grep -e
has_object_file \*.c" and audit the codebase to see if there are
other problematic ones?

Interestingly, the case fixed here was mentioned after the three-dashes
in this patch:

  https://lore.kernel.org/git/20191127123211.GG22221@sigill.intra.peff.net/ (local)

That thread was about OBJECT_INFO_QUICK, but I think any "we are finding
the boundary" hunt should consider whether that's appropriate, too. In
that case they are really the same issue: we do not want to expend
effort A on the unlikely chance P that we will find the object and save
ourselves effort B. The magnitude of A and B, and the probability P may
be different for the two optimizations, though.

I think the instance fixed here _probably_ could be converted to QUICK
as well. It saves extra refreshes of the pack directory when a client
asks for an object we don't have. That's usually not too expensive, but
can be in some cases (many alternates, slow NFS filesystems, client has
many objects we don't). In the worst case, using QUICK would mean we can
get fooled by a simultaneous repack into thinking we don't have an
object we _do_ have, and end up negotiating a worse pack for the client.

Thanks.  Will queue.

That said, yeah, I think Jonathan's patch is a strict improvement by
itself.

-Peff