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

["René Scharfe" <l.s.r@web.de>]

Am 02.07.23 um 14:56 schrieb Jeff King:
> On Sat, Jul 01, 2023 at 10:57:02PM +0200, Jan Klötzke wrote:
>
>> @@ -2222,18 +2219,19 @@ static const struct object_id *match_points_at(struct oid_array *points_at,
>>  					       const struct object_id *oid,
>>  					       const char *refname)
>>  {
>> -	const struct object_id *tagged_oid = NULL;
>>  	struct object *obj;
>>
>>  	if (oid_array_lookup(points_at, oid) >= 0)
>>  		return oid;
>>  	obj = parse_object(the_repository, oid);
>> +	while (obj && obj->type == OBJ_TAG) {
>> +		oid = get_tagged_oid((struct tag *)obj);
>> +		if (oid_array_lookup(points_at, oid) >= 0)
>> +			return oid;
>> +		obj = parse_object(the_repository, oid);
>> +	}
>
> OK, so we are doing the usual peeling loop here. I wondered if we might
> be able to use peel_object(), but it again suffers from the "peel all
> the way" syndrome. So we have to loop ourselves so that we can check at
> each level. Good.
>
>>  	if (!obj)
>>  		die(_("malformed object at '%s'"), refname);
>
> This will now trigger if refname points to a broken object, or if its
> tag does. I think the resulting message is OK in either case (and
> presumably lower level code would produce extra error messages, too).
>
>> -	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.

> So the code looks good.

I agree.

René