On Sun, Jul 02, 2023 at 06:25:13PM +0200, René Scharfe wrote:
> >> - if (obj->type == OBJ_TAG)
> >> - tagged_oid = get_tagged_oid((struct tag *)obj);
> >> - if (tagged_oid && oid_array_lookup(points_at, tagged_oid) >= 0)
> >> - return tagged_oid;
> >
> > This code is moved into the loop body, but your version there drops the
> > "if (tagged_oid)" check. I think that is OK (and even preferable),
> > though. In get_tagged_oid() we will die() if the tagged object is NULL
> > (though even before switching to that function this check was
> > questionable, because it is "tag->tagged" that may be NULL, and we were
> > dereferencing that unconditionally).
>
> The check is necessary in the current code because tagged_oid is NULL if
> obj is not a tag. The new code no longer needs it.
Oh, right. Probably:
if (obj->type == OBJ_TAG) {
const struct object_id *tagged_oid = get_tagged_oid((struct tag *)obj);
if (oid_array_lookup(points_at, tagged_oid) >= 0)
return tagged_oid;
}
would have been a better way to write the original, but the new one with
the loop is better still. ;)
I also notice that the function returns a pointer to an oid, even though
the sole caller only cares about a boolean result. Not that big a deal,
though the memory lifetime of the return value is confusing. We might
return "tagged_oid" which points to a struct that will live forever, but
we might also return "oid" directly, which points to memory passed in
from the caller with a limited lifetime.
-Peff
