Re: [PATCH v2] packfile: skip decompressing and hashing blobs in add_promisor_object()

From: Jeff King <peff@peff.net>
Date: Dec 8, 2025, 20:28 UTC
Message-ID: <20251208202812.GC216526@coredump.intra.peff.net>
In-Reply-To: <51c866cb-9a7a-4c59-834a-2f710f34f3a1@nvidia.com>

On Sat, Dec 06, 2025 at 11:40:01AM -0800, Aaron Plattner wrote:

Show 12 quoted lines

> I was rewriting the commit message for that part to justify why it's safe to
> use PARSE_OBJECT_SKIP_HASH_CHECK, and now I'm questioning it. :)
> 
> It definitely seems fine for blobs but if what we're trying to check for is
> on-disk corruption, maybe it's not a good idea to skip it for other objects
> since we're actually using their contents here.
> 
> I still think the OBJ_NONE fix is worthwhile and I'll send that out
> separately, but maybe it would be a good idea to split
> PARSE_OBJECT_SKIP_HASH_CHECK into separate flags for blobs vs. all objects?
> Or just go back to v1 of the add_promisor_object() patch? Or do you think
> this version is okay despite that concern?

I think it's OK to skip the hashes here. In some code paths we really care about checking the consistency of the objects (like "rev-list --verify", or fsck). But if the caller knows that we are not trying to do that, then that opens the doors to other optimizations like avoiding object loads entirely.

You could argue that if we _do_ load an object, we might as well hash it to check its consistency. IMHO there is not much value in that, as the cost is not totally trivial, it's not a thorough validation of what's on disk (because we are skipping some objects), and this sort of bit-flipping corruption is pretty rare in the first place.

So to my mind, there are really two types of callers that want to parse an object: ones that are verifying database consistency and want to be thorough, and ones that want things to be as fast as possible. And I think the caller here (traversing the promisor objects) is in the latter camp. E.g., if we ever added a secondary index of "these are all the promised objects we don't have" we would just use that!

So really, I think all I was suggesting for the commit message is to say "this is the kind of caller that wants things to be as fast as possible". ;)

The ideal series to me is probably:

  - patch 1 fixes the OBJ_NONE issues. It would be great if there was
    some way to show the impact of this bug on an existing case, but I
    imagine it would be quite hard. We'd never produce the wrong answer
    but just do things slowly. So you'd need a case where we end up with
    OBJ_NONE, and then a SKIP_HASH code path that loads a big blob. It's
    easy to find the latter (just hand rev-list a blob on the command
    line), but the former is harder. I doubt it's worth the effort to
    dig for one, since we know your case in patch 2 will demonstrate it.

  - patch 2 uses SKIP_HASH

-Peff