[PATCH v2] load_ref_decorations(): fix decoration with tags
- From
- Martin Ågren <martin.agren@gmail.com>
- Date
- Jul 13, 2021, 07:40 UTC
- Message-ID
- <20210713074018.232372-1-martin.agren@gmail.com>
- In-Reply-To
- <YOzY+qNFM2GsgKMO@coredump.intra.peff.net>
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.
Make sure to parse the tag object at the top of the peeling loop. This effectively restores the pre-88473c8bae parsing -- but only of tags, allowing us to keep most of the possible speedup from 88473c8bae. Jeff King reports:
On my big ~220k ref test case (where it's mostly non-tags), the timings [using "git log -1 --decorate"] are:
- before either patch: 2.945s
- with my broken patch: 0.707s
- with [this patch]: 0.788sNote 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.
Helped-by: Jeff King <peff@peff.net> Signed-off-by: Martin Ågren <martin.agren@gmail.com> --- 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.
> 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.
Thank you for your insightful and helpful comments.
log-tree.c | 4 ++-- t/t4202-log.sh | 9 +++++++++ 2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/log-tree.c b/log-tree.c index 4f69ed176d..6dc4412268 100644 --- a/log-tree.c +++ b/log-tree.c @@ -174,11 +174,11 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid, add_name_decoration(deco_type, refname, obj); while (obj->type == OBJ_TAG) { + if (!obj->parsed) + parse_object(the_repository, &obj->oid); obj = ((struct tag *)obj)->tagged; if (!obj) break; - if (!obj->parsed) - parse_object(the_repository, &obj->oid); add_name_decoration(DECORATION_REF_TAG, refname, obj); } return 0; diff --git a/t/t4202-log.sh b/t/t4202-log.sh index 350cfa3593..536b1eef42 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1905,6 +1905,15 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' ' test_must_fail git log --exclude-promisor-objects source-a ' +test_expect_success 'log --decorate includes lightweight and annotated tags' ' + cat >expect <<-\EOF && + three HEAD -> source-b, tag: three, tag: source-tag + one tag: one + EOF + git log --format="%s %D" >actual && + test_cmp expect actual +' + test_expect_success 'log --end-of-options' ' git update-ref refs/heads/--source HEAD && git log --end-of-options --source >actual &&
-- 2.32.0.402.g57bb445576