Re: [PATCH 1/3] parse_object(): allow skipping hash check

From: Derrick Stolee <derrickstolee@github.com>
Date: Sep 7, 2022, 14:15 UTC
Message-ID: <f79b0ccd-3e36-f447-0dbb-6e40ad547c8d@github.com>
In-Reply-To: <YxfRTubqh7aFvNJs@coredump.intra.peff.net>

On 9/6/2022 7:01 PM, Jeff King wrote:

> I'm sorry, I know the argument here is really hand-wavy. But I really
> think this isn't making anything much less safe.

I agree with you that this is the safest way to move forward here.

Show 5 quoted lines

> I was actually tempted to rip out the blob hash-check entirely by
> default!  Anybody who really cares about checking the bits can do so
> with read_object_file(). That's what fsck does, and we could pretty
> easily convert "rev-list --verify-objects" to do so, too. So this is the
> less extreme version of the patch. ;)

A quick search shows many uses of parse_object() across the codebase. It would certainly be nice if they all suddenly got faster by avoiding this hashing, but I also suppose that most of the calls are using parse_object() only because they are unsure if they are parsing a commit or a tag and would never parse a large blob.

I think this approach of making parse_object_with_flags() is the best way to incrementally approach things here. If we decide that we need the _with_flags() version specifically to avoid this hash check, then we could probably take the second approach: remove the hash check from parse_object() and swap the places that care to use read_object_file() instead. My guess is that in the long term there will be fewer swaps to read_object_file() than to parse_object_with_flags().

However, this is a good first step to make progress without doing the time-consuming audit of every caller to parse_object().

Thanks, -Stolee