From d1ed8d6cee57c91ec770a8a183ed40c3ec867ac1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Wed, 14 Jul 2021 12:31:36 -0400 Subject: [PATCH] load_ref_decorations(): fix decoration with tags MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 `tagged` field will be NULL, so we won't actually add a decoration for the pointed-to object. 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. 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.788s 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. Reported-by: Martin Ågren Helped-by: Martin Ågren Signed-off-by: Martin Ågren Signed-off-by: Jeff King Reviewed-by: Martin Ågren Signed-off-by: Junio C Hamano --- log-tree.c | 4 ++-- t/t4202-log.sh | 14 ++++++++++++++ 2 files changed, 16 insertions(+), 2 deletions(-) diff --git a/log-tree.c b/log-tree.c index 4f69ed176db..6dc4412268b 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 350cfa35936..fe8f5e20676 100755 --- a/t/t4202-log.sh +++ b/t/t4202-log.sh @@ -1905,6 +1905,20 @@ 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 all levels of tag annotated tags' ' + git checkout -b branch && + git commit --allow-empty -m "new commit" && + git tag lightweight HEAD && + git tag -m annotated annotated HEAD && + git tag -m double-0 double-0 HEAD && + git tag -m double-1 double-1 double-0 && + cat >expect <<-\EOF && + HEAD -> branch, tag: lightweight, tag: double-1, tag: double-0, tag: annotated + EOF + git log -1 --format="%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 &&