[+cc Junio; this patch looks good to me, and should go on top of
jk/log-decorate-optim, which is in 'next' and has a pretty
ugly regression]
On Tue, Jul 13, 2021 at 09:40:18AM +0200, Martin Ågren wrote:
> Commit 88473c8bae ("load_ref_decorations(): avoid parsing non-tag
> objects", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:
> Rather than calling `parse_object()`, we go for `oid_object_info()` and
> then `lookup_object_by_type()` using the type just discovered. As
> detailed in the commit message, this provides a significant time saving.
>
> Unfortunately, it also changes the behavior: We lose all annotated tags
> from the decoration.
>
> 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.
Thanks, nicely explained.
> Note how this commit could have been done as an optimization before
> 88473c8bae: When our peeling hits a non-tag, we won't parse that tagged
> object only to immediately end the loop.
Yep, thanks for mentioning this, as it's somewhat subtle.
> On Tue, 13 Jul 2021 at 02:06, Jeff King <peff@xxxxxxxx> wrote:
> >
> > Your fix is _almost_ there.
>
> It's very kind of you to put it like that. I've picked up your
> suggestions and have tried to summarize my understanding of the issue
> and the fix in the commit message.
When I wrote that, I thought the fix would just be:
if (obj_type == OBJ_TAG)
parse_object(...);
which really would put it only one line off of your fix. :)
> > That's the minimum needed to unbreak things. I think we could do even
> > better, though. There is no need for us to parse a commit object pointed
> > to by a tag here. We should only be parsing tags we see (whether at the
> > top-level or recursively).
>
> Maybe you wrote this before circling back and actually writing that
> "even better" thing? Because it seems to me like that's what you did.
> Maybe I'm still missing something.
Nope, I'm just dumb. I wrote what I sent in the other email (rather than
just adding the "if" as above) because it only involved having a single
parse_object() call in the function. To my credit, I did realize about
an hour after sending the other email that I had in fact done the
"better thing" quite accidentally. But I really like how you explained
it in the commit message here, which I had not quite thought through.
> log-tree.c | 4 ++--
> t/t4202-log.sh | 9 +++++++++
Patch looks good. Thanks for noticing the problem and cleaning up my
mess.
-Peff
