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()` directly, call `oid_object_info()`
and then either return early or go on to call `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. As an example, on git.git,
git log --oneline --decorate origin/master | grep '(.*tag:.*)'
returns zero hits after 88473c8bae. Before it, we have around 800 hits.
What happens is, all annotated tags are lost from the decoration.
Let's do a partial revert of 88473c8bae: Keep doing that early
`oid_object_info()`, but after that, go on with the full
`parse_object()`. This restores the pre-88473c8bae behavior. We clearly
have lacking test coverage here, so make sure to add a test. Without
this fix to log-tree.c, the git-log invocation in this new test does
pick up the lightweight tags involved, but misses out on the annotated
tag.
Signed-off-by: Martin Ågren <redacted>
---
Cc-ing Peff, the author of 88473c8bae, and Taylor, who made a comment
regarding the peeling of tags [1], which might be related.
I'm genuinely unconvinced that my proposed fix is the best possible one.
Or maybe trying a more surgical fix around annotated tags misses a
whole bunch of *other* special cases and we really should go for the
full `parse_object()` to cover all possibilities.
In my brief testing (time git log -1 --decorate on git.git), the time
savings from 88473c8bae seem to be gone. So maybe this should be a full
revert, rather than a partial one. (Plus the test.) Let's hope that
won't be necessary.
Also, I'm not sure whether the test really needs to worry about the
order of the decorations suddenly changing -- maybe it's supposed to be
stable.
[1] https://lore.kernel.org/git/YNKgkGkPiMgNubNE@nand.local/
log-tree.c | 6 ++----
t/t4202-log.sh | 14 ++++++++++++++
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index 4f69ed176d..0b638d2e3c 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -134,7 +134,6 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
int flags, void *cb_data)
{
struct object *obj;
- enum object_type objtype;
enum decoration_type deco_type = DECORATION_NONE;
struct decoration_filter *filter = (struct decoration_filter *)cb_data;
@@ -156,10 +155,9 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,
return 0;
}
- objtype = oid_object_info(the_repository, oid, NULL);
- if (objtype < 0)
+ if (oid_object_info(the_repository, oid, NULL) < 0)
return 0;
- obj = lookup_object_by_type(the_repository, oid, objtype);
+ obj = parse_object(the_repository, oid);
if (starts_with(refname, "refs/heads/"))
deco_type = DECORATION_REF_LOCAL;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 350cfa3593..3aa5451913 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, e.g., all kinds of tags' '
+ git log --oneline --decorate >log &&
+ test_line_count = 2 log &&
+ grep "^1ac6c77 (tag: one) one$" log &&
+ grep HEAD log | sed -e "s/^.*(\(.*\)).*$/\1/" -e "s/, /\n/g" |
+ sort >actual &&
+ cat >expect <<-\EOF &&
+ HEAD -> source-b
+ tag: source-tag
+ tag: three
+ EOF
+ 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
On Tue, Jul 13, 2021 at 12:41:52AM +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()` directly, call `oid_object_info()`
and then either return early or go on to call `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. As an example, on git.git,
git log --oneline --decorate origin/master | grep '(.*tag:.*)'
returns zero hits after 88473c8bae. Before it, we have around 800 hits.
What happens is, all annotated tags are lost from the decoration.
Eek. Thanks for catching this.
I wondered how I could have missed this, but it does work if somebody
else happens to have parsed it. For example:
$ git log -1 --oneline --decorate v5.11
f40ddce88593 (tag: v5.11) Linux 5.11
works because we'll already have parsed it as a traversal tip.
Let's do a partial revert of 88473c8bae: Keep doing that early
`oid_object_info()`, but after that, go on with the full
`parse_object()`. This restores the pre-88473c8bae behavior.
Your fix is _almost_ there. There's not much point in doing
oid_object_info() if we're just going to parse unconditionally. But we
would want to parse only tags.
And we even do parse tags recursively in the peeling loop. The trouble
is that we do so after realizing we need to recurse. We just need to
bump it up in the loop, like:
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;
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).
Do you want to incorporate the fix above, or would you prefer me to pick
it up from here?
We clearly
have lacking test coverage here, so make sure to add a test. Without
this fix to log-tree.c, the git-log invocation in this new test does
pick up the lightweight tags involved, but misses out on the annotated
tag.
Yeah, definitely.
In my brief testing (time git log -1 --decorate on git.git), the time
savings from 88473c8bae seem to be gone. So maybe this should be a full
revert, rather than a partial one. (Plus the test.) Let's hope that
won't be necessary.
Right. After your patch, the oid_object_info() is pointless, because
we're still parsing everything (the "< 0" error check would only trigger
in a corrupted repo). And it adds some overhead, so it may even be
slightly slower than the original code. :)
The timings I get for "git log -1 --decorate" on git.git are:
- before either patch: 27.5ms
- with my broken patch: 5.9ms
- with the patch above: 11.3ms
which makes sense. I have a bunch of branches, and now we don't parse
them. We do still have to parse tags. On linux.git, where it's almost
entirely tags, most of the advantage dries up (but it would probably be
helped a bit by the further suggestion I gave above to avoid parsing
tagged commits).
On my big ~220k ref test case (where it's mostly non-tags), the timings
are:
- before either patch: 2.945s
- with my broken patch: 0.707s
- with the patch above: 0.788s
so the savings are retained.
Also, I'm not sure whether the test really needs to worry about the
order of the decorations suddenly changing -- maybe it's supposed to be
stable.
I think it's probably OK to count on it being stable. We iterate the
refs in a stable order to insert them, and then store the result as a
linked list. If that strategy ever changed, I think we'd end up doing a
manual sort on them to get a stable order anyway.
quoted hunk
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 350cfa3593..3aa5451913 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, e.g., all kinds of tags' '
+ git log --oneline --decorate >log &&
+ test_line_count = 2 log &&
+ grep "^1ac6c77 (tag: one) one$" log &&
This presumably breaks when the tests are run in sha256 mode. Coupled
with the ordering simplification, maybe:
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
(or maybe %d is more readable in the output; it doesn't matter much if
we're matching it verbatim).
Thanks again for noticing this.
-Peff
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.788s
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.
Helped-by: Jeff King [off-list ref]
Signed-off-by: Martin Ågren <redacted>
---
On Tue, 13 Jul 2021 at 02:06, Jeff King [off-list ref] 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
[+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 [off-list ref] 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
On Tue, 13 Jul 2021 at 09:52, Jeff King [off-list ref] wrote:
[+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]
Thanks for calling out the branch name when I failed to. That's indeed
where my copy of the patch sits and where I've tested this.
quoted
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.
That's the best kind of hindsight: the sudden realization that you
actually got it right.
Patch looks good. Thanks for noticing the problem and cleaning up my
mess.
Thanks a lot for your comments and all the help. I'm glad you liked my
explanation in the posted patch.
Martin