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

quoted

+	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