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

Jeff King [off-list ref] writes:

quoted

Puzzled.

...and the answer is that we don't need to parse it. The tag object
mentions the type of what it points to, and we use lookup_commit(), etc,
to create the object pointed to by its "tagged" field.

Ahh, parse_object() on the outer tag, when instantiating the in-core
obj, allocated an in-core object and that instance is already given
a type from the tag object and .taggeed member points at that
object, so it is not an "unknown" object (tag.c::parse_tag_buffer()).

Totally forgot about that one; thanks.

Earlier, Junio C Hamano [off-list ref] wrote:

quoted

quoted

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.

It is too subtle that I am not sure what the paragraph wants to say.

Then:

Jeff King [off-list ref] writes:

quoted

quoted

Puzzled.

...and the answer is that we don't need to parse it. The tag object
mentions the type of what it points to, and we use lookup_commit(), etc,
to create the object pointed to by its "tagged" field.

Ahh, parse_object() on the outer tag, when instantiating the in-core
obj, allocated an in-core object and that instance is already given
a type from the tag object and .taggeed member points at that
object, so it is not an "unknown" object (tag.c::parse_tag_buffer()).

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.

Martin

On Tue, Jul 13, 2021 at 11:52:53PM +0200, Martin Ågren wrote:

quoted

quoted

quoted

Puzzled.

...and the answer is that we don't need to parse it. The tag object
mentions the type of what it points to, and we use lookup_commit(), etc,
to create the object pointed to by its "tagged" field.

Ahh, parse_object() on the outer tag, when instantiating the in-core
obj, allocated an in-core object and that instance is already given
a type from the tag object and .taggeed member points at that
object, so it is not an "unknown" object (tag.c::parse_tag_buffer()).

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.

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.

Here it is.

-- >8 --
From: Martin Ågren <redacted>
Subject: load_ref_decorations(): fix decoration with tags

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

Helped-by: Jeff King [off-list ref]
Signed-off-by: Martin Ågren <redacted>
Signed-off-by: Jeff King <redacted>
---
 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 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..fe8f5e2067 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 &&

-- 
2.32.0.663.g932e3f012f

On Wed, 14 Jul 2021 at 00:22, Jeff King [off-list ref] wrote:

On Tue, Jul 13, 2021 at 11:52:53PM +0200, Martin Ågren wrote:

quoted

quoted

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

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 [off-list ref]
Signed-off-by: Martin Ågren <redacted>
Signed-off-by: Jeff King <redacted>

If you take authorship of this, I think this could be something like

Reported-by: Martin Ågren <redacted>
Signed-off-by: Martin Ågren <redacted>
Signed-off-by: Jeff King <redacted>
Reviewed-by: Martin Ågren <redacted>

Martin

On Wed, Jul 14, 2021 at 10:13:38AM +0200, Martin Ågren wrote:

quoted

From: Martin Ågren <redacted>

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

Yeah, that occurred to me, too. Let's swap it, then. When the fix
inevitably turns out to be wrong, I can take the blame. ;)

Here's an updated patch for Junio's convenience (same as what I posted
before, but with author/trailers tweaked). Thanks again for finding and
working on this!

-- >8 --
Subject: [PATCH] load_ref_decorations(): fix decoration with tags

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 <redacted>
Helped-by: Martin Ågren [off-list ref]
Signed-off-by: Martin Ågren <redacted>
Signed-off-by: Jeff King <redacted>
Reviewed-by: Martin Ågren <redacted>
---
 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 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 39e746fbcb..9dfead936b 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh

@@ -1915,6 +1915,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 &&

-- 
2.32.0.689.gbb74d99cdd