On Wed, 14 Jul 2021 at 00:22, Jeff King <peff@xxxxxxxx> wrote: > > 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. Thanks for correcting that. The parsed-ness of "obj" affects whether the decoration is shown at all. I originally concluded that when the decorations are eventually displayed, it was something like "if ->tag is non-NULL, display it". But that's obviously not the case. It's more like "->tagged is NULL, so I have no idea where to place this decoration". > 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. > From: Martin Ågren <martin.agren@xxxxxxxxx> At this point, I think it's fair to say that you've done most of the authoring here. I wouldn't be at all offended if you took the credit for this patch. It's your code diff, it's your test, and now it's even your *updated* test, plus half the commit message. :) Here's that added half of the message: > 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. Very well described. > Helped-by: Jeff King <peff@xxxxxxxx> > Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx> > Signed-off-by: Jeff King <peff@xxxxxxxx> If you take authorship of this, I think this could be something like Reported-by: Martin Ågren <martin.agren@xxxxxxxxx> Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx> Signed-off-by: Jeff King <peff@xxxxxxxx> Reviewed-by: Martin Ågren <martin.agren@xxxxxxxxx> Martin
