Re: [PATCH 3/3] parse_object(): check commit-graph when skip_hash set

[Derrick Stolee <derrickstolee@github.com>]

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