From: Derrick Stolee Date: Wed, 07 Sep 2022 14:46:55 GMT Subject: Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set Message-ID: <60a47a3f-e4d5-2339-d79e-48e6a7d4a9f1@github.com> In-Reply-To: On 9/6/2022 7:06 PM, Jeff King wrote: > If the caller told us that they don't care about us checking the object > hash, then we're free to implement any optimizations that get us the > parsed value more quickly. An obvious one is to check the commit graph > before loading an object from disk. And in fact, both of the callers who > pass in this flag are already doing so before they call parse_object()! > > So we can simplify those callers, as well as any possible future ones, > by moving the logic into parse_object(). Nice! > There are two subtle things to note in the diff, but neither has any > impact in practice: > > - it seems least-surprising here to do the graph lookup on the > git-replace'd oid, rather than the original. This is in theory a > change of behavior from the earlier code, as neither caller did a > replace lookup itself. But in practice it doesn't matter, as we > disable the commit graph entirely if there are any replace refs. I can confirm that this is the case. > - the caller in get_reference() passes the skip_hash flag only if > revs->verify_objects isn't set, whereas it would look in the commit > graph unconditionally. In practice this should not matter as we > should disable the commit graph entirely when using verify_objects > (and that was done recently in another patch). > > So this should be a pure cleanup with no behavior change. Excellent, thanks! -Stolee