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

From: Jeff King <peff@peff.net>
Date: Jul 13, 2021, 07:52 UTC
Message-ID: <YO1GNWjMol8JV8MR@coredump.intra.peff.net>
In-Reply-To: <20210713074018.232372-1-martin.agren@gmail.com>

[+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:

Show 13 quoted lines

> 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.

Show 7 quoted lines

>  On Tue, 13 Jul 2021 at 02:06, Jeff King <peff@peff.net> 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. :)

Show 8 quoted lines

>  > 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