Re: [PATCH] ref-filter: handle nested tags in --points-at option

From: Jeff King <peff@peff.net>
Date: Jul 2, 2023, 20:27 UTC
Message-ID: <20230702202722.GA1052071@coredump.intra.peff.net>
In-Reply-To: <df912fc4-601b-4d43-c6e4-aa5f74ec031f@web.de>

On Sun, Jul 02, 2023 at 06:25:13PM +0200, René Scharfe wrote:

Show 14 quoted lines

> >> -	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