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é
