On Mon, Mar 04, 2024 at 04:57:36AM -0500, Jeff King wrote:
> 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).

Thanks for the explanation!

Patrick