In theory nobody should ever ask the low-level object code
for a null sha1. It's used as a sentinel for "no such
object" in lots of places, so leaking through to this level
is a sign that the higher-level code is not being careful
about its error-checking. In practice, though, quite a few
code paths seem to rely on the null sha1 lookup failing as a
way to quietly propagate non-existence (e.g., by feeding it
to lookup_commit_reference_gently(), which then returns
NULL).
When this happens, we do two inefficient things:
1. We actually search for the null sha1 in packs and in
the loose object directory.
2. When we fail to find it, we re-scan the pack directory
in case a simultaneous repack happened to move it from
loose to packed.
It's debatable whether (1) is a good idea or not. The
performance drop is at least linear in the number of
lookups, so it's not too bad. And if by some 2^-160th chance
somebody does have such an object, we probably ought to
access it. On the other hand, enough code paths treat the
null sha1 specially that the object probably isn't usable,
anyway.
Problem (2), on the other hand, is a pretty severe
performance issue. If you have a large number of packs,
rescanning the pack directory can be very expensive. And it
only helps in the case that you have an object with the null
sha1 _and_ somebody was simultaneously repacking it. The
tradeoff is certainly a bad one there.
In an ideal world, we'd simply fix all of the callers to
notice the null sha1 and avoid passing it to us. But a
simple experiment to catch this with a BUG() shows that
there are a large number of code paths.
In the meantime, let's address problem (2). That's the
minimal fix we can do to make the performance problems go
away. p5551 shows this off (when a fetched ref is new, the
"old" sha1 is 0{40}, which ends up being passed for
fast-forward checks, the status table abbreviations, etc):
Test HEAD^ HEAD
--------------------------------------------------------
5551.4: fetch 5.51(5.03+0.48) 0.17(0.10+0.06) -96.9%
We could address (1), too, but there's not much performance
improvement left to make.
Signed-off-by: Jeff King <peff@xxxxxxxx>
---
This is the minimal fix that addresses the performance issues.
I'd actually have no problem at all declaring that looking up a null
sha1 is insane, and having the object-lookup routines simply return "no
such object" without even doing the loose/pack lookup first.
I'm also fine with going down the BUG() route and fixing the
higher-level callers, but it's a big enough task (with little enough
real-world impact) that I think it would be worth applying this in the
meantime.
sha1_file.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/sha1_file.c b/sha1_file.c
index 8a7c6b7eba..dde0ad101d 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1180,6 +1180,9 @@ int sha1_object_info_extended(const unsigned char *sha1, struct object_info *oi,
if (!sha1_loose_object_info(real, oi, flags))
return 0;
+ if (is_null_sha1(sha1))
+ return -1;
+
/* Not a loose object; someone else may have just packed it. */
if (flags & OBJECT_INFO_QUICK) {
return -1;
--
2.15.0.494.g79a8547723
