Re: [PATCH v2] load_ref_decorations(): fix decoration with tags

>
> On Tue, Jul 13, 2021 at 11:52:53PM +0200, Martin Ågren wrote:
>
> > > Totally forgot about that one; thanks.
> >
> > Do you have any suggestions for how this could be explained better? I
> > waffled on whether to add that paragraph to the commit message and when
> > I finally did, it seems it got a little bit too succinct.
> >
> > I'm about to check out for today. Maybe in the morning I can think of
> > some clarification.
>
> My attempt is below. Most of the new explanation is near the end, but I
> tweaked a few other things.
>
> Your original said:
>
>   The reason this happens is in the loop where we try to peel the tags,
>   we won't necessarily have parsed that first object. If we haven't, its
>   `tag` will be NULL, so nothing will be displayed, and its `tagged`
>   will also be NULL, so we won't peel any further.
>
> and my earlier explanations were not thinking of the "tag" field at all,
> which made me worried there was another subtle bug in not parsing the
> tag earlier. But I don't think so. We don't look at the "tag" field for
> setting the annotation; it always comes from the refname. So the
> paragraph above should not mention "tag" at all.

> I also beefed up the test a bit. All this talk of parsing made me want
> to make sure we were covering tags-of-tags correctly (which I think we
> are both before and after the patch). After adding that, the expected
> decoration output was getting quite cluttered. So I tweaked the test to
> make a new commit, give the tags sensible names, and just look at that
> one commit.

> The simplest way to do this is to just conditionally parse before the
> loop:
>
>   if (obj->type == OBJ_TAG)
>           parse_object(&obj->oid);
>
> But we can observe that our tag-peeling loop needs to peel already, to
> examine recursive tags-of-tags. So instead of introducing a new call to
> parse_object(), we can simply move the parsing higher in the loop:
> instead of parsing the new object before we loop, parse each tag object
> before we look at its "tagged" field.
>
> This has another beneficial side effect: if a tag points at a commit (or
> other non-tag type), we do not bother to parse the commit at all now.
> And we know it is a commit without calling oid_object_info(), because
> parsing the surrounding tag object will have created the correct in-core
> object based on the "type" field of the tag.
>
> Our test coverage for --decorate was obviously not good, since we missed
> this quite-basic regression. The new tests covers an annotated tag
> (showing the fix), but also that we correctly show annotations for
> lightweight tags and double-annotated tag-of-tags.