{"thread":{"id":"56097","subject":"[PATCH v1] load_ref_decorations(): fix decoration with tags","startedAt":"2021-07-12T22:42:40Z","lastAt":"2021-07-14T16:31:38Z","messageCount":12,"participants":["Martin Ågren","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"429848","messageId":"20210712224152.2698500-1-martin.agren@gmail.com","threadId":"56097","inReplyTo":null,"subject":"[PATCH v1] load_ref_decorations(): fix decoration with tags","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-12T22:41:52Z","receivedAt":"2021-07-12T22:42:40Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\nobjects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\nRather than calling `parse_object()` directly, call `oid_object_info()`\nand then either return early or go on to call `lookup_object_by_type()`\nusing the type just discovered. As detailed in the commit message, this\nprovides a significant time saving.\n\nUnfortunately, it also changes the behavior. As an example, on git.git,\n\n  git log --oneline --decorate origin/master | grep '(.*tag:.*)'\n\nreturns zero hits after 88473c8bae. Before it, we have around 800 hits.\nWhat happens is, all annotated tags are lost from the decoration.\n\nLet's do a partial revert of 88473c8bae: Keep doing that early\n`oid_object_info()`, but after that, go on with the full\n`parse_object()`. This restores the pre-88473c8bae behavior. We clearly\nhave lacking test coverage here, so make sure to add a test. Without\nthis fix to log-tree.c, the git-log invocation in this new test does\npick up the lightweight tags involved, but misses out on the annotated\ntag.\n\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n Cc-ing Peff, the author of 88473c8bae, and Taylor, who made a comment\n regarding the peeling of tags [1], which might be related.\n\n I'm genuinely unconvinced that my proposed fix is the best possible one.\n Or maybe trying a more surgical fix around annotated tags misses a\n whole bunch of *other* special cases and we really should go for the\n full `parse_object()` to cover all possibilities.\n\n In my brief testing (time git log -1 --decorate on git.git), the time\n savings from 88473c8bae seem to be gone. So maybe this should be a full\n revert, rather than a partial one. (Plus the test.) Let's hope that\n won't be necessary.\n\n Also, I'm not sure whether the test really needs to worry about the\n order of the decorations suddenly changing -- maybe it's supposed to be\n stable.\n\n [1] https://lore.kernel.org/git/YNKgkGkPiMgNubNE@nand.local/\n\n log-tree.c     |  6 ++----\n t/t4202-log.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 4 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f69ed176d..0b638d2e3c 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -134,7 +134,6 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\t\t      int flags, void *cb_data)\n {\n \tstruct object *obj;\n-\tenum object_type objtype;\n \tenum decoration_type deco_type = DECORATION_NONE;\n \tstruct decoration_filter *filter = (struct decoration_filter *)cb_data;\n \n@@ -156,10 +155,9 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \t\treturn 0;\n \t}\n \n-\tobjtype = oid_object_info(the_repository, oid, NULL);\n-\tif (objtype < 0)\n+\tif (oid_object_info(the_repository, oid, NULL) < 0)\n \t\treturn 0;\n-\tobj = lookup_object_by_type(the_repository, oid, objtype);\n+\tobj = parse_object(the_repository, oid);\n \n \tif (starts_with(refname, \"refs/heads/\"))\n \t\tdeco_type = DECORATION_REF_LOCAL;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 350cfa3593..3aa5451913 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1905,6 +1905,20 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git log --exclude-promisor-objects source-a\n '\n \n+test_expect_success 'log --decorate includes, e.g., all kinds of tags' '\n+\tgit log --oneline --decorate >log &&\n+\ttest_line_count = 2 log &&\n+\tgrep \"^1ac6c77 (tag: one) one$\" log &&\n+\tgrep HEAD log | sed -e \"s/^.*(\\(.*\\)).*$/\\1/\" -e \"s/, /\\n/g\" |\n+\t\tsort >actual &&\n+\tcat >expect <<-\\EOF &&\n+\t\tHEAD -> source-b\n+\t\ttag: source-tag\n+\t\ttag: three\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --end-of-options' '\n        git update-ref refs/heads/--source HEAD &&\n        git log --end-of-options --source >actual &&\n-- \n2.32.0.402.g57bb445576\n\n"},{"id":"429854","messageId":"YOzY+qNFM2GsgKMO@coredump.intra.peff.net","threadId":"56097","inReplyTo":"20210712224152.2698500-1-martin.agren@gmail.com","subject":"Re: [PATCH v1] load_ref_decorations(): fix decoration with tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T00:06:18Z","receivedAt":"2021-07-13T00:06:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 13, 2021 at 12:41:52AM +0200, Martin Ågren wrote:\n\n> Commit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\n> objects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\n> Rather than calling `parse_object()` directly, call `oid_object_info()`\n> and then either return early or go on to call `lookup_object_by_type()`\n> using the type just discovered. As detailed in the commit message, this\n> provides a significant time saving.\n> \n> Unfortunately, it also changes the behavior. As an example, on git.git,\n> \n>   git log --oneline --decorate origin/master | grep '(.*tag:.*)'\n> \n> returns zero hits after 88473c8bae. Before it, we have around 800 hits.\n> What happens is, all annotated tags are lost from the decoration.\n\nEek. Thanks for catching this.\n\nI wondered how I could have missed this, but it does work if somebody\nelse happens to have parsed it. For example:\n\n  $ git log -1 --oneline --decorate v5.11\n  f40ddce88593 (tag: v5.11) Linux 5.11\n\nworks because we'll already have parsed it as a traversal tip.\n\n> Let's do a partial revert of 88473c8bae: Keep doing that early\n> `oid_object_info()`, but after that, go on with the full\n> `parse_object()`. This restores the pre-88473c8bae behavior.\n\nYour fix is _almost_ there. There's not much point in doing\noid_object_info() if we're just going to parse unconditionally. But we\nwould want to parse only tags.\n\nAnd we even do parse tags recursively in the peeling loop. The trouble\nis that we do so after realizing we need to recurse. We just need to\nbump it up in the loop, like:\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f69ed176d..6dc4412268 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -174,11 +174,11 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \n \tadd_name_decoration(deco_type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n+\t\tif (!obj->parsed)\n+\t\t\tparse_object(the_repository, &obj->oid);\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n \t\t\tbreak;\n-\t\tif (!obj->parsed)\n-\t\t\tparse_object(the_repository, &obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n \treturn 0;\n\nThat's the minimum needed to unbreak things. I think we could do even\nbetter, though. There is no need for us to parse a commit object pointed\nto by a tag here. We should only be parsing tags we see (whether at the\ntop-level or recursively).\n\nDo you want to incorporate the fix above, or would you prefer me to pick\nit up from here?\n\n> We clearly\n> have lacking test coverage here, so make sure to add a test. Without\n> this fix to log-tree.c, the git-log invocation in this new test does\n> pick up the lightweight tags involved, but misses out on the annotated\n> tag.\n\nYeah, definitely.\n\n>  In my brief testing (time git log -1 --decorate on git.git), the time\n>  savings from 88473c8bae seem to be gone. So maybe this should be a full\n>  revert, rather than a partial one. (Plus the test.) Let's hope that\n>  won't be necessary.\n\nRight. After your patch, the oid_object_info() is pointless, because\nwe're still parsing everything (the \"< 0\" error check would only trigger\nin a corrupted repo). And it adds some overhead, so it may even be\nslightly slower than the original code. :)\n\nThe timings I get for \"git log -1 --decorate\" on git.git are:\n\n  -  before either patch: 27.5ms\n  - with my broken patch:  5.9ms\n  - with the patch above: 11.3ms\n\nwhich makes sense. I have a bunch of branches, and now we don't parse\nthem. We do still have to parse tags. On linux.git, where it's almost\nentirely tags, most of the advantage dries up (but it would probably be\nhelped a bit by the further suggestion I gave above to avoid parsing\ntagged commits).\n\nOn my big ~220k ref test case (where it's mostly non-tags), the timings\nare:\n\n  -  before either patch: 2.945s\n  - with my broken patch: 0.707s\n  - with the patch above: 0.788s\n\nso the savings are retained.\n\n>  Also, I'm not sure whether the test really needs to worry about the\n>  order of the decorations suddenly changing -- maybe it's supposed to be\n>  stable.\n\nI think it's probably OK to count on it being stable. We iterate the\nrefs in a stable order to insert them, and then store the result as a\nlinked list. If that strategy ever changed, I think we'd end up doing a\nmanual sort on them to get a stable order anyway.\n\n> diff --git a/t/t4202-log.sh b/t/t4202-log.sh\n> index 350cfa3593..3aa5451913 100755\n> --- a/t/t4202-log.sh\n> +++ b/t/t4202-log.sh\n> @@ -1905,6 +1905,20 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n>  \ttest_must_fail git log --exclude-promisor-objects source-a\n>  '\n>  \n> +test_expect_success 'log --decorate includes, e.g., all kinds of tags' '\n> +\tgit log --oneline --decorate >log &&\n> +\ttest_line_count = 2 log &&\n> +\tgrep \"^1ac6c77 (tag: one) one$\" log &&\n\nThis presumably breaks when the tests are run in sha256 mode. Coupled\nwith the ordering simplification, maybe:\n\n        cat >expect <<-\\EOF &&\n        three HEAD -> source-b, tag: three, tag: source-tag\n        one tag: one\n        EOF\n        git log --format=\"%s %D\" >actual &&\n        test_cmp expect actual\n\n(or maybe %d is more readable in the output; it doesn't matter much if\nwe're matching it verbatim).\n\nThanks again for noticing this.\n\n-Peff\n"},{"id":"429867","messageId":"20210713074018.232372-1-martin.agren@gmail.com","threadId":"56097","inReplyTo":"YOzY+qNFM2GsgKMO@coredump.intra.peff.net","subject":"[PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-13T07:40:18Z","receivedAt":"2021-07-13T07:40:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Commit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\nobjects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\nRather than calling `parse_object()`, we go for `oid_object_info()` and\nthen `lookup_object_by_type()` using the type just discovered. As\ndetailed in the commit message, this provides a significant time saving.\n\nUnfortunately, it also changes the behavior: We lose all annotated tags\nfrom the decoration.\n\nThe reason this happens is in the loop where we try to peel the tags, we\nwon't necessarily have parsed that first object. If we haven't, its\n`tag` will be NULL, so nothing will be displayed, and its `tagged` will\nalso be NULL, so we won't peel any further.\n\nMake sure to parse the tag object at the top of the peeling loop. This\neffectively restores the pre-88473c8bae parsing -- but only of tags,\nallowing us to keep most of the possible speedup from 88473c8bae. Jeff\nKing reports:\n\n  On my big ~220k ref test case (where it's mostly non-tags), the\n  timings [using \"git log -1 --decorate\"] are:\n\n    - before either patch: 2.945s\n    - with my broken patch: 0.707s\n    - with [this patch]: 0.788s\n\nNote how this commit could have been done as an optimization before\n88473c8bae: When our peeling hits a non-tag, we won't parse that tagged\nobject only to immediately end the loop.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\n---\n On Tue, 13 Jul 2021 at 02:06, Jeff King <peff@peff.net> wrote:\n >\n > Your fix is _almost_ there.\n\n It's very kind of you to put it like that. I've picked up your\n suggestions and have tried to summarize my understanding of the issue\n and the fix in the commit message.\n\n > That's the minimum needed to unbreak things. I think we could do even\n > better, though. There is no need for us to parse a commit object pointed\n > to by a tag here. We should only be parsing tags we see (whether at the\n > top-level or recursively).\n\n Maybe you wrote this before circling back and actually writing that\n \"even better\" thing? Because it seems to me like that's what you did.\n Maybe I'm still missing something.\n\n Thank you for your insightful and helpful comments.\n\n log-tree.c     | 4 ++--\n t/t4202-log.sh | 9 +++++++++\n 2 files changed, 11 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f69ed176d..6dc4412268 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -174,11 +174,11 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \n \tadd_name_decoration(deco_type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n+\t\tif (!obj->parsed)\n+\t\t\tparse_object(the_repository, &obj->oid);\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n \t\t\tbreak;\n-\t\tif (!obj->parsed)\n-\t\t\tparse_object(the_repository, &obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n \treturn 0;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 350cfa3593..536b1eef42 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1905,6 +1905,15 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git log --exclude-promisor-objects source-a\n '\n \n+test_expect_success 'log --decorate includes lightweight and annotated tags' '\n+\tcat >expect <<-\\EOF &&\n+\tthree HEAD -> source-b, tag: three, tag: source-tag\n+\tone tag: one\n+\tEOF\n+\tgit log --format=\"%s %D\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --end-of-options' '\n        git update-ref refs/heads/--source HEAD &&\n        git log --end-of-options --source >actual &&\n-- \n2.32.0.402.g57bb445576\n\n"},{"id":"429868","messageId":"YO1GNWjMol8JV8MR@coredump.intra.peff.net","threadId":"56097","inReplyTo":"20210713074018.232372-1-martin.agren@gmail.com","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T07:52:21Z","receivedAt":"2021-07-13T07:52:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"[+cc Junio; this patch looks good to me, and should go on top of\n\t    jk/log-decorate-optim, which is in 'next' and has a pretty\n\t    ugly regression]\n\nOn Tue, Jul 13, 2021 at 09:40:18AM +0200, Martin Ågren wrote:\n\n> Commit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\n> objects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\n> Rather than calling `parse_object()`, we go for `oid_object_info()` and\n> then `lookup_object_by_type()` using the type just discovered. As\n> detailed in the commit message, this provides a significant time saving.\n> \n> Unfortunately, it also changes the behavior: We lose all annotated tags\n> from the decoration.\n> \n> The reason this happens is in the loop where we try to peel the tags, we\n> won't necessarily have parsed that first object. If we haven't, its\n> `tag` will be NULL, so nothing will be displayed, and its `tagged` will\n> also be NULL, so we won't peel any further.\n\nThanks, nicely explained.\n\n> Note how this commit could have been done as an optimization before\n> 88473c8bae: When our peeling hits a non-tag, we won't parse that tagged\n> object only to immediately end the loop.\n\nYep, thanks for mentioning this, as it's somewhat subtle.\n\n>  On Tue, 13 Jul 2021 at 02:06, Jeff King <peff@peff.net> wrote:\n>  >\n>  > Your fix is _almost_ there.\n> \n>  It's very kind of you to put it like that. I've picked up your\n>  suggestions and have tried to summarize my understanding of the issue\n>  and the fix in the commit message.\n\nWhen I wrote that, I thought the fix would just be:\n\n  if (obj_type == OBJ_TAG)\n\tparse_object(...);\n\nwhich really would put it only one line off of your fix. :)\n\n>  > That's the minimum needed to unbreak things. I think we could do even\n>  > better, though. There is no need for us to parse a commit object pointed\n>  > to by a tag here. We should only be parsing tags we see (whether at the\n>  > top-level or recursively).\n> \n>  Maybe you wrote this before circling back and actually writing that\n>  \"even better\" thing? Because it seems to me like that's what you did.\n>  Maybe I'm still missing something.\n\nNope, I'm just dumb. I wrote what I sent in the other email (rather than\njust adding the \"if\" as above) because it only involved having a single\nparse_object() call in the function. To my credit, I did realize about\nan hour after sending the other email that I had in fact done the\n\"better thing\" quite accidentally. But I really like how you explained\nit in the commit message here, which I had not quite thought through.\n\n>  log-tree.c     | 4 ++--\n>  t/t4202-log.sh | 9 +++++++++\n\nPatch looks good. Thanks for noticing the problem and cleaning up my\nmess.\n\n-Peff\n"},{"id":"429883","messageId":"CAN0heSpFDC2c=S6RZo=mrM3n09WL1jFsv-Px=8DpYP=ZYKHjDA@mail.gmail.com","threadId":"56097","inReplyTo":"YO1GNWjMol8JV8MR@coredump.intra.peff.net","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-13T08:47:16Z","receivedAt":"2021-07-13T08:47:34Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Tue, 13 Jul 2021 at 09:52, Jeff King <peff@peff.net> wrote:\n>\n> [+cc Junio; this patch looks good to me, and should go on top of\n>             jk/log-decorate-optim, which is in 'next' and has a pretty\n>             ugly regression]\n\nThanks for calling out the branch name when I failed to. That's indeed\nwhere my copy of the patch sits and where I've tested this.\n\n> >  Maybe you wrote this before circling back and actually writing that\n> >  \"even better\" thing? Because it seems to me like that's what you did.\n> >  Maybe I'm still missing something.\n>\n> Nope, I'm just dumb. I wrote what I sent in the other email (rather than\n> just adding the \"if\" as above) because it only involved having a single\n> parse_object() call in the function. To my credit, I did realize about\n> an hour after sending the other email that I had in fact done the\n> \"better thing\" quite accidentally. But I really like how you explained\n> it in the commit message here, which I had not quite thought through.\n\nThat's the best kind of hindsight: the sudden realization that you\nactually got it right.\n\n> Patch looks good. Thanks for noticing the problem and cleaning up my\n> mess.\n\nThanks a lot for your comments and all the help. I'm glad you liked my\nexplanation in the posted patch.\n\nMartin\n"},{"id":"429937","messageId":"xmqqpmvl29ry.fsf@gitster.g","threadId":"56097","inReplyTo":"YO1GNWjMol8JV8MR@coredump.intra.peff.net","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-13T21:17:53Z","receivedAt":"2021-07-13T21:17:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> The reason this happens is in the loop where we try to peel the tags, we\n>> won't necessarily have parsed that first object. If we haven't, its\n>> `tag` will be NULL, so nothing will be displayed, and its `tagged` will\n>> also be NULL, so we won't peel any further.\n>\n> Thanks, nicely explained.\n\nYup, nicely explained indeed.\n\n>> Note how this commit could have been done as an optimization before\n>> 88473c8bae: When our peeling hits a non-tag, we won't parse that tagged\n>> object only to immediately end the loop.\n>\n> Yep, thanks for mentioning this, as it's somewhat subtle.\n\nIt is too subtle that I am not sure what the paragraph wants to say.\n\nBefore 88473c8b, we had a fully parsed object in obj and entered the\nwhile() loop iff the outermost object is a tag, then we find the\nunderlying object via obj->tagged.  We parse that underlying object\nto find if it is a tag, and break out if it is not.\n\nBy \"this commit\", I assume that the above mean the change in this\nfix, i.e. parse 'obj' if it has not been parsed before looking at\nits tagged field.  But I am not sure how that would have been an\noptimization before 88473c8b that gave a parsed tag object 'obj'\nupon entry to the loop.\n\nPuzzled.\n\nIn any case, let's talk about this patch in the context to which it\nis designed to be applied, i.e. post 88473c8b3c8b.\n\nWhen we come here, we have done oid_object_info() plus\nlookup_object_by_type() to obtain 'obj' and we know its type.\nThen we enter the loop.\n\n \twhile (obj->type == OBJ_TAG) {\n+\t\tif (!obj->parsed)\n+\t\t\tparse_object(the_repository, &obj->oid);\n\nAnd we parse if it hasn't been parsed.  THat is why we can ...\n\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n \t\t\tbreak;\n\n... look at its tagged member.\n\n-\t\tif (!obj->parsed)\n-\t\t\tparse_object(the_repository, &obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n\nAnd the updated 'obj' (i.e. direct referent of the tag object) is\nfed to add_name_decoration().  And then we move to the next\niteration.\n\nNow, do we know what type of object 'obj' is at this point?  We\ndid parse the outermost tag upon entry to this loop, we replaced\n'obj' variable with the referent by following the .tagged member,\nbut we haven't parsed that object or ran oid_object_info() on it.\n\nPuzzled.\n\n \t}\n"},{"id":"429938","messageId":"YO4FObgRvpt1nVr0@coredump.intra.peff.net","threadId":"56097","inReplyTo":"xmqqpmvl29ry.fsf@gitster.g","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T21:27:21Z","receivedAt":"2021-07-13T21:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 13, 2021 at 02:17:53PM -0700, Junio C Hamano wrote:\n\n> >> Note how this commit could have been done as an optimization before\n> >> 88473c8bae: When our peeling hits a non-tag, we won't parse that tagged\n> >> object only to immediately end the loop.\n> >\n> > Yep, thanks for mentioning this, as it's somewhat subtle.\n> \n> It is too subtle that I am not sure what the paragraph wants to say.\n> \n> Before 88473c8b, we had a fully parsed object in obj and entered the\n> while() loop iff the outermost object is a tag, then we find the\n> underlying object via obj->tagged.  We parse that underlying object\n> to find if it is a tag, and break out if it is not.\n> \n> By \"this commit\", I assume that the above mean the change in this\n> fix, i.e. parse 'obj' if it has not been parsed before looking at\n> its tagged field.  But I am not sure how that would have been an\n> optimization before 88473c8b that gave a parsed tag object 'obj'\n> upon entry to the loop.\n> \n> Puzzled.\n\nThe optimization is that we are parsing tags before looking at their\nstructs, instead of always parsing the thing that the tag points to.\n\nSo in the old loop (pseudo-code for clarity):\n\n  parse_object(obj);\n  while (obj->type == OBJ_TAG) {\n          obj = obj->tagged;\n\t  parse_object(obj);\n  }\n\nif we had a tag pointing to a commit, we'd parse the commit. But we\ndon't need to. We just need to know that it exists and is a commit.\n\nIn the new code, we parse only when we need to look at obj->tagged:\n\n  while (obj->type == OBJ_TAG) {\n          parse_object(obj);\n\t  obj = obj->tagged;\n  }\n\nSo we must \"somehow\" know the type of \"obj\" in the first place, as well\nas the type of every obj->tagged we look at. And that leads into your\nquestion here:\n\n> In any case, let's talk about this patch in the context to which it\n> is designed to be applied, i.e. post 88473c8b3c8b.\n> \n> When we come here, we have done oid_object_info() plus\n> lookup_object_by_type() to obtain 'obj' and we know its type.\n> Then we enter the loop.\n> \n>  \twhile (obj->type == OBJ_TAG) {\n> +\t\tif (!obj->parsed)\n> +\t\t\tparse_object(the_repository, &obj->oid);\n> \n> And we parse if it hasn't been parsed.  THat is why we can ...\n> \n>  \t\tobj = ((struct tag *)obj)->tagged;\n>  \t\tif (!obj)\n>  \t\t\tbreak;\n> \n> ... look at its tagged member.\n> \n> -\t\tif (!obj->parsed)\n> -\t\t\tparse_object(the_repository, &obj->oid);\n>  \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n> \n> And the updated 'obj' (i.e. direct referent of the tag object) is\n> fed to add_name_decoration().  And then we move to the next\n> iteration.\n> \n> Now, do we know what type of object 'obj' is at this point?  We\n> did parse the outermost tag upon entry to this loop, we replaced\n> 'obj' variable with the referent by following the .tagged member,\n> but we haven't parsed that object or ran oid_object_info() on it.\n> \n> Puzzled.\n\n...and the answer is that we don't need to parse it. The tag object\nmentions the type of what it points to, and we use lookup_commit(), etc,\nto create the object pointed to by its \"tagged\" field. If we can't do\nthat (say, because the tag is missing the type field, or we previously\nsaw the object as a different type, etc), then obj->tagged would be NULL\nand we'd break out of the loop.\n\n-Peff\n"},{"id":"429939","messageId":"xmqqlf6928qv.fsf@gitster.g","threadId":"56097","inReplyTo":"YO4FObgRvpt1nVr0@coredump.intra.peff.net","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-13T21:40:08Z","receivedAt":"2021-07-13T21:40:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> Puzzled.\n>\n> ...and the answer is that we don't need to parse it. The tag object\n> mentions the type of what it points to, and we use lookup_commit(), etc,\n> to create the object pointed to by its \"tagged\" field.\n\nAhh, parse_object() on the outer tag, when instantiating the in-core\nobj, allocated an in-core object and that instance is already given\na type from the tag object and .taggeed member points at that\nobject, so it is not an \"unknown\" object (tag.c::parse_tag_buffer()).\n\nTotally forgot about that one; thanks.\n\n\n"},{"id":"429940","messageId":"CAN0heSqCFVqC9Ncn5r3b4dKOE80byDt_XRM3pnswixX4jFcLFA@mail.gmail.com","threadId":"56097","inReplyTo":"xmqqlf6928qv.fsf@gitster.g","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-13T21:52:53Z","receivedAt":"2021-07-13T21:53:06Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"Earlier, Junio C Hamano <gitster@pobox.com> wrote:\n\n>>> Note how this commit could have been done as an optimization before\n>>> 88473c8bae: When our peeling hits a non-tag, we won't parse that tagged\n>>> object only to immediately end the loop.\n>>\n>> Yep, thanks for mentioning this, as it's somewhat subtle.\n>\n> It is too subtle that I am not sure what the paragraph wants to say.\n\nThen:\n\n> Jeff King <peff@peff.net> writes:\n>\n> >> Puzzled.\n> >\n> > ...and the answer is that we don't need to parse it. The tag object\n> > mentions the type of what it points to, and we use lookup_commit(), etc,\n> > to create the object pointed to by its \"tagged\" field.\n>\n> Ahh, parse_object() on the outer tag, when instantiating the in-core\n> obj, allocated an in-core object and that instance is already given\n> a type from the tag object and .taggeed member points at that\n> object, so it is not an \"unknown\" object (tag.c::parse_tag_buffer()).\n>\n> Totally forgot about that one; thanks.\n\nDo you have any suggestions for how this could be explained better? I\nwaffled on whether to add that paragraph to the commit message and when\nI finally did, it seems it got a little bit too succinct.\n\nI'm about to check out for today. Maybe in the morning I can think of\nsome clarification.\n\nMartin\n"},{"id":"429941","messageId":"YO4SEp/B4826sc+j@coredump.intra.peff.net","threadId":"56097","inReplyTo":"CAN0heSqCFVqC9Ncn5r3b4dKOE80byDt_XRM3pnswixX4jFcLFA@mail.gmail.com","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-13T22:22:10Z","receivedAt":"2021-07-13T22:22:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 13, 2021 at 11:52:53PM +0200, Martin Ågren wrote:\n\n> > >> Puzzled.\n> > >\n> > > ...and the answer is that we don't need to parse it. The tag object\n> > > mentions the type of what it points to, and we use lookup_commit(), etc,\n> > > to create the object pointed to by its \"tagged\" field.\n> >\n> > Ahh, parse_object() on the outer tag, when instantiating the in-core\n> > obj, allocated an in-core object and that instance is already given\n> > a type from the tag object and .taggeed member points at that\n> > object, so it is not an \"unknown\" object (tag.c::parse_tag_buffer()).\n> >\n> > Totally forgot about that one; thanks.\n> \n> Do you have any suggestions for how this could be explained better? I\n> waffled on whether to add that paragraph to the commit message and when\n> I finally did, it seems it got a little bit too succinct.\n> \n> I'm about to check out for today. Maybe in the morning I can think of\n> some clarification.\n\nMy attempt is below. Most of the new explanation is near the end, but I\ntweaked a few other things.\n\nYour original said:\n\n  The reason this happens is in the loop where we try to peel the tags,\n  we won't necessarily have parsed that first object. If we haven't, its\n  `tag` will be NULL, so nothing will be displayed, and its `tagged`\n  will also be NULL, so we won't peel any further.\n\nand my earlier explanations were not thinking of the \"tag\" field at all,\nwhich made me worried there was another subtle bug in not parsing the\ntag earlier. But I don't think so. We don't look at the \"tag\" field for\nsetting the annotation; it always comes from the refname. So the\nparagraph above should not mention \"tag\" at all.\n\nI also beefed up the test a bit. All this talk of parsing made me want\nto make sure we were covering tags-of-tags correctly (which I think we\nare both before and after the patch). After adding that, the expected\ndecoration output was getting quite cluttered. So I tweaked the test to\nmake a new commit, give the tags sensible names, and just look at that\none commit.\n\nHere it is.\n\n-- >8 --\nFrom: Martin Ågren <martin.agren@gmail.com>\nSubject: load_ref_decorations(): fix decoration with tags\n\nCommit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\nobjects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\nRather than calling `parse_object()`, we go for `oid_object_info()` and\nthen `lookup_object_by_type()` using the type just discovered. As\ndetailed in the commit message, this provides a significant time saving.\n\nUnfortunately, it also changes the behavior: We lose all annotated tags\nfrom the decoration.\n\nThe reason this happens is in the loop where we try to peel the tags, we\nwon't necessarily have parsed that first object. If we haven't, its\n`tagged` field will be NULL, so we won't actually add a decoration for\nthe pointed-to object.\n\nMake sure to parse the tag object at the top of the peeling loop. This\neffectively restores the pre-88473c8bae parsing -- but only of tags,\nallowing us to keep most of the possible speedup from 88473c8bae. Jeff\nKing reports:\n\n  On my big ~220k ref test case (where it's mostly non-tags), the\n  timings [using \"git log -1 --decorate\"] are:\n\n    - before either patch: 2.945s\n    - with my broken patch: 0.707s\n    - with [this patch]: 0.788s\n\nThe simplest way to do this is to just conditionally parse before the\nloop:\n\n  if (obj->type == OBJ_TAG)\n          parse_object(&obj->oid);\n\nBut we can observe that our tag-peeling loop needs to peel already, to\nexamine recursive tags-of-tags. So instead of introducing a new call to\nparse_object(), we can simply move the parsing higher in the loop:\ninstead of parsing the new object before we loop, parse each tag object\nbefore we look at its \"tagged\" field.\n\nThis has another beneficial side effect: if a tag points at a commit (or\nother non-tag type), we do not bother to parse the commit at all now.\nAnd we know it is a commit without calling oid_object_info(), because\nparsing the surrounding tag object will have created the correct in-core\nobject based on the \"type\" field of the tag.\n\nOur test coverage for --decorate was obviously not good, since we missed\nthis quite-basic regression. The new tests covers an annotated tag\n(showing the fix), but also that we correctly show annotations for\nlightweight tags and double-annotated tag-of-tags.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n log-tree.c     |  4 ++--\n t/t4202-log.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f69ed176d..6dc4412268 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -174,11 +174,11 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \n \tadd_name_decoration(deco_type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n+\t\tif (!obj->parsed)\n+\t\t\tparse_object(the_repository, &obj->oid);\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n \t\t\tbreak;\n-\t\tif (!obj->parsed)\n-\t\t\tparse_object(the_repository, &obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n \treturn 0;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 350cfa3593..fe8f5e2067 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1905,6 +1905,20 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git log --exclude-promisor-objects source-a\n '\n \n+test_expect_success 'log --decorate includes all levels of tag annotated tags' '\n+\tgit checkout -b branch &&\n+\tgit commit --allow-empty -m \"new commit\" &&\n+\tgit tag lightweight HEAD &&\n+\tgit tag -m annotated annotated HEAD &&\n+\tgit tag -m double-0 double-0 HEAD &&\n+\tgit tag -m double-1 double-1 double-0 &&\n+\tcat >expect <<-\\EOF &&\n+\tHEAD -> branch, tag: lightweight, tag: double-1, tag: double-0, tag: annotated\n+\tEOF\n+\tgit log -1 --format=\"%D\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --end-of-options' '\n        git update-ref refs/heads/--source HEAD &&\n        git log --end-of-options --source >actual &&\n-- \n2.32.0.663.g932e3f012f\n\n"},{"id":"429998","messageId":"CAN0heSp1eJmeMTq-ECMUVh=hBPRTEkQeU0-3YXvtaDdZtU1sSA@mail.gmail.com","threadId":"56097","inReplyTo":"YO4SEp/B4826sc+j@coredump.intra.peff.net","subject":"Re: [PATCH v2] load_ref_decorations(): fix decoration with tags","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2021-07-14T08:13:38Z","receivedAt":"2021-07-14T08:13:51Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On Wed, 14 Jul 2021 at 00:22, Jeff King <peff@peff.net> wrote:\n>\n> On Tue, Jul 13, 2021 at 11:52:53PM +0200, Martin Ågren wrote:\n>\n> > > Totally forgot about that one; thanks.\n> >\n> > Do you have any suggestions for how this could be explained better? I\n> > waffled on whether to add that paragraph to the commit message and when\n> > I finally did, it seems it got a little bit too succinct.\n> >\n> > I'm about to check out for today. Maybe in the morning I can think of\n> > some clarification.\n>\n> My attempt is below. Most of the new explanation is near the end, but I\n> tweaked a few other things.\n>\n> Your original said:\n>\n>   The reason this happens is in the loop where we try to peel the tags,\n>   we won't necessarily have parsed that first object. If we haven't, its\n>   `tag` will be NULL, so nothing will be displayed, and its `tagged`\n>   will also be NULL, so we won't peel any further.\n>\n> and my earlier explanations were not thinking of the \"tag\" field at all,\n> which made me worried there was another subtle bug in not parsing the\n> tag earlier. But I don't think so. We don't look at the \"tag\" field for\n> setting the annotation; it always comes from the refname. So the\n> paragraph above should not mention \"tag\" at all.\n\nThanks for correcting that. The parsed-ness of \"obj\" affects whether the\ndecoration is shown at all. I originally concluded that when the\ndecorations are eventually displayed, it was something like \"if ->tag is\nnon-NULL, display it\". But that's obviously not the case. It's more like\n\"->tagged is NULL, so I have no idea where to place this decoration\".\n\n> I also beefed up the test a bit. All this talk of parsing made me want\n> to make sure we were covering tags-of-tags correctly (which I think we\n> are both before and after the patch). After adding that, the expected\n> decoration output was getting quite cluttered. So I tweaked the test to\n> make a new commit, give the tags sensible names, and just look at that\n> one commit.\n\n> From: Martin Ågren <martin.agren@gmail.com>\n\nAt this point, I think it's fair to say that you've done most of the\nauthoring here. I wouldn't be at all offended if you took the credit for\nthis patch. It's your code diff, it's your test, and now it's even your\n*updated* test, plus half the commit message. :)\n\nHere's that added half of the message:\n\n> The simplest way to do this is to just conditionally parse before the\n> loop:\n>\n>   if (obj->type == OBJ_TAG)\n>           parse_object(&obj->oid);\n>\n> But we can observe that our tag-peeling loop needs to peel already, to\n> examine recursive tags-of-tags. So instead of introducing a new call to\n> parse_object(), we can simply move the parsing higher in the loop:\n> instead of parsing the new object before we loop, parse each tag object\n> before we look at its \"tagged\" field.\n>\n> This has another beneficial side effect: if a tag points at a commit (or\n> other non-tag type), we do not bother to parse the commit at all now.\n> And we know it is a commit without calling oid_object_info(), because\n> parsing the surrounding tag object will have created the correct in-core\n> object based on the \"type\" field of the tag.\n>\n> Our test coverage for --decorate was obviously not good, since we missed\n> this quite-basic regression. The new tests covers an annotated tag\n> (showing the fix), but also that we correctly show annotations for\n> lightweight tags and double-annotated tag-of-tags.\n\nVery well described.\n\n> Helped-by: Jeff King <peff@peff.net>\n> Signed-off-by: Martin Ågren <martin.agren@gmail.com>\n> Signed-off-by: Jeff King <peff@peff.net>\n\nIf you take authorship of this, I think this could be something like\n\nReported-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\nReviewed-by: Martin Ågren <martin.agren@gmail.com>\n\nMartin\n"},{"id":"430071","messageId":"YO8RaMj7sN4ABe5d@coredump.intra.peff.net","threadId":"56097","inReplyTo":"CAN0heSp1eJmeMTq-ECMUVh=hBPRTEkQeU0-3YXvtaDdZtU1sSA@mail.gmail.com","subject":"[PATCH v3] load_ref_decorations(): fix decoration with tags","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-07-14T16:31:36Z","receivedAt":"2021-07-14T16:31:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 14, 2021 at 10:13:38AM +0200, Martin Ågren wrote:\n\n> > From: Martin Ågren <martin.agren@gmail.com>\n> \n> At this point, I think it's fair to say that you've done most of the\n> authoring here. I wouldn't be at all offended if you took the credit for\n> this patch. It's your code diff, it's your test, and now it's even your\n> *updated* test, plus half the commit message. :)\n\nYeah, that occurred to me, too. Let's swap it, then. When the fix\ninevitably turns out to be wrong, I can take the blame. ;)\n\nHere's an updated patch for Junio's convenience (same as what I posted\nbefore, but with author/trailers tweaked). Thanks again for finding and\nworking on this!\n\n-- >8 --\nSubject: [PATCH] load_ref_decorations(): fix decoration with tags\n\nCommit 88473c8bae (\"load_ref_decorations(): avoid parsing non-tag\nobjects\", 2021-06-22) introduced a shortcut to `add_ref_decoration()`:\nRather than calling `parse_object()`, we go for `oid_object_info()` and\nthen `lookup_object_by_type()` using the type just discovered. As\ndetailed in the commit message, this provides a significant time saving.\n\nUnfortunately, it also changes the behavior: We lose all annotated tags\nfrom the decoration.\n\nThe reason this happens is in the loop where we try to peel the tags, we\nwon't necessarily have parsed that first object. If we haven't, its\n`tagged` field will be NULL, so we won't actually add a decoration for\nthe pointed-to object.\n\nMake sure to parse the tag object at the top of the peeling loop. This\neffectively restores the pre-88473c8bae parsing -- but only of tags,\nallowing us to keep most of the possible speedup from 88473c8bae.\n\nOn my big ~220k ref test case (where it's mostly non-tags), the\ntimings [using \"git log -1 --decorate\"] are:\n\n  - before either patch: 2.945s\n  - with my broken patch: 0.707s\n  - with [this patch]: 0.788s\n\nThe simplest way to do this is to just conditionally parse before the\nloop:\n\n  if (obj->type == OBJ_TAG)\n          parse_object(&obj->oid);\n\nBut we can observe that our tag-peeling loop needs to peel already, to\nexamine recursive tags-of-tags. So instead of introducing a new call to\nparse_object(), we can simply move the parsing higher in the loop:\ninstead of parsing the new object before we loop, parse each tag object\nbefore we look at its \"tagged\" field.\n\nThis has another beneficial side effect: if a tag points at a commit (or\nother non-tag type), we do not bother to parse the commit at all now.\nAnd we know it is a commit without calling oid_object_info(), because\nparsing the surrounding tag object will have created the correct in-core\nobject based on the \"type\" field of the tag.\n\nOur test coverage for --decorate was obviously not good, since we missed\nthis quite-basic regression. The new tests covers an annotated tag\n(showing the fix), but also that we correctly show annotations for\nlightweight tags and double-annotated tag-of-tags.\n\nReported-by: Martin Ågren <martin.agren@gmail.com>\nHelped-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Martin Ågren <martin.agren@gmail.com>\nSigned-off-by: Jeff King <peff@peff.net>\nReviewed-by: Martin Ågren <martin.agren@gmail.com>\n---\n log-tree.c     |  4 ++--\n t/t4202-log.sh | 14 ++++++++++++++\n 2 files changed, 16 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f69ed176d..6dc4412268 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -174,11 +174,11 @@ static int add_ref_decoration(const char *refname, const struct object_id *oid,\n \n \tadd_name_decoration(deco_type, refname, obj);\n \twhile (obj->type == OBJ_TAG) {\n+\t\tif (!obj->parsed)\n+\t\t\tparse_object(the_repository, &obj->oid);\n \t\tobj = ((struct tag *)obj)->tagged;\n \t\tif (!obj)\n \t\t\tbreak;\n-\t\tif (!obj->parsed)\n-\t\t\tparse_object(the_repository, &obj->oid);\n \t\tadd_name_decoration(DECORATION_REF_TAG, refname, obj);\n \t}\n \treturn 0;\ndiff --git a/t/t4202-log.sh b/t/t4202-log.sh\nindex 39e746fbcb..9dfead936b 100755\n--- a/t/t4202-log.sh\n+++ b/t/t4202-log.sh\n@@ -1915,6 +1915,20 @@ test_expect_success '--exclude-promisor-objects does not BUG-crash' '\n \ttest_must_fail git log --exclude-promisor-objects source-a\n '\n \n+test_expect_success 'log --decorate includes all levels of tag annotated tags' '\n+\tgit checkout -b branch &&\n+\tgit commit --allow-empty -m \"new commit\" &&\n+\tgit tag lightweight HEAD &&\n+\tgit tag -m annotated annotated HEAD &&\n+\tgit tag -m double-0 double-0 HEAD &&\n+\tgit tag -m double-1 double-1 double-0 &&\n+\tcat >expect <<-\\EOF &&\n+\tHEAD -> branch, tag: lightweight, tag: double-1, tag: double-0, tag: annotated\n+\tEOF\n+\tgit log -1 --format=\"%D\" >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'log --end-of-options' '\n        git update-ref refs/heads/--source HEAD &&\n        git log --end-of-options --source >actual &&\n-- \n2.32.0.689.gbb74d99cdd\n\n"}]}