Re: [PATCH 9/9] upload-pack: free tree buffers after parsing
- From
Jeff King <peff@peff.net>
- Date
- Mar 4, 2024, 09:57 UTC
- Message-ID
- <20240304095736.GA3723539@coredump.intra.peff.net>
- In-Reply-To
- <ZeWHdaZnhOHKs5QP@tanuki>
On Mon, Mar 04, 2024 at 09:33:57AM +0100, Patrick Steinhardt wrote:
Show 20 quoted lines
> > + 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