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
