Re: [PATCH 9/9] upload-pack: free tree buffers after parsing

[Jeff King <peff@peff.net>]

On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:

> > +	if (skip_hash && discard_tree &&
> > +	    (!obj || obj->type == OBJ_TREE) &&
> > +	    oid_object_info(r, oid, NULL) == OBJ_TREE) {
> > +		return &lookup_tree(r, oid)->object;
> > +	}
> 
> The other condition for blobs does the same, but the condition here
> confuses me. Why do we call `oid_object_info()` if we have already
> figured out that `obj->type == OBJ_TREE`? Feels like wasted effort if
> the in-memory object has been determined to be a tree already anyway.
> 
> I'd rather have expected it to look like the following:
> 
> if (skip_hash && discard_tree &&
>     ((obj && obj->type == OBJ_TREE) ||
>      (!obj && oid_object_info(r, oid, NULL)) == OBJ_TREE)) {
> 		return &lookup_tree(r, oid)->object;
> }
> 
> Am I missing some side effect that `oid_object_info()` provides?

Calling oid_object_info() will make sure the on-disk object exists and
has the expected type. Keep in mind that an in-memory "struct object"
may have a type that was just implied by another reference. E.g., if a
commit references some object X in its tree field, then we'll call
lookup_tree(X) to get a "struct tree" without actually touching the odb
at all. When it comes time to parse that object, that's when we'll see
if we really have it and if it's a tree.

In the case of skip_hash (and discard_tree) it might be OK to skip both
of those checks. If we do, I think we should probably do the same for
blobs (in the skip_hash case, we could just return the object we found
already).

But I'd definitely prefer to do that as a separate step (if at all).

-Peff