{"thread":{"id":"64435","subject":"[PATCH] ref-filter: fix stale parsed objects","startedAt":"2025-11-04T14:36:27Z","lastAt":"2025-11-06T06:04:43Z","messageCount":7,"participants":["Patrick Steinhardt","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"530192","messageId":"20251104-b4-pks-ref-filter-fixup-v1-1-2fbca52d76d9@pks.im","threadId":"64435","inReplyTo":null,"subject":"[PATCH] ref-filter: fix stale parsed objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-04T14:36:13Z","receivedAt":"2025-11-04T14:36:27Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In 054f5f457e (ref-filter: parse objects on demand, 2025-10-23) we have\nstarted to skip parsing some objects in case we don't need to access\ntheir values in the first place. This was done by introducing a new\nmember `struct expand_data::maybe_object` that gets populated on demand\nvia `get_or_parse_object()`.\n\nThis has led to a regression though where the object now gets reused\nbecause we don't reset it properly. The `oi` structure is declared in\nglobal scope, and there is no single place where we reset it before\ninvoking `get_object()`. The consequence is that the `maybe_object`\nmember doesn't get reset across calls, so subsequent calls will end up\nreusing the same object.\n\nThis is only an issue for a subset of retrieved values, as not all of\nthe infrastructure ends up calling `get_or_parse_object()`. So the\neffect is limited, which is probably why the issue wasn't detected\nearlier.\n\nFix the issue by resetting `maybe_object` in `get_object()`.\n\nReported-by: Junio C Hamano \nBased-on-patch-by: Jeff King \nSigned-off-by: Patrick Steinhardt \n---\nAs reported by Junio in . This applies\ndirectly on top of ps/ref-peeled-tags at 054f5f457e (ref-filter: parse\nobjects on demand, 2025-10-23)\n\nThanks!\n\nPatrick\n---\n ref-filter.c | 2 ++\n t/t7004-tag.sh | 20 ++++++++++++++++++++\n 2 files changed, 22 insertions(+)\n\ndiff --git a/ref-filter.c b/ref-filter.c\nindex 7cfcd5c355..d8667c569a 100644\n--- a/ref-filter.c\n+++ b/ref-filter.c\n@@ -2367,6 +2367,8 @@ static int get_object(struct ref_array_item *ref, int deref,\n \tint eaten = 0;\n \tint ret;\n \n+\toi->maybe_object = NULL;\n+\n \tif (oi->info.contentp) {\n \t\t/* We need to know that to use parse_object_buffer properly */\n \t\toi->info.sizep = &oi->size;\ndiff --git a/t/t7004-tag.sh b/t/t7004-tag.sh\nindex 10835631ca..d1388cfdf4 100755\n--- a/t/t7004-tag.sh\n+++ b/t/t7004-tag.sh\n@@ -2332,4 +2332,24 @@ test_expect_success 'If tag cannot be created then tag message file is not unlin\n \ttest_path_exists .git/TAG_EDITMSG\n '\n \n+test_expect_success 'annotated tag version sort' '\n+\tgit tag -a -m \"sample 1.0\" vsample-1.0 &&\n+\tgit tag -a -m \"sample 2.0\" vsample-2.0 &&\n+\tgit tag -a -m \"sample 10.0\" vsample-10.0 &&\n+\tcat >expect <<-EOF &&\n+\tvsample-1.0\n+\tvsample-2.0\n+\tvsample-10.0\n+\tEOF\n+\n+\tgit tag --list --sort=version:tag vsample-\\* >actual &&\n+\ttest_cmp expect actual &&\n+\n+\t# Ensure that we also handle this case alright in the case we have the\n+\t# peeled values cached e.g. via the packed-refs file.\n+\tgit pack-refs --all &&\n+\tgit tag --list --sort=version:tag vsample-\\* &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n\n---\nbase-commit: 0b2c6cbdc7ac42a10094102781112b122cbc2b88\nchange-id: 20251104-b4-pks-ref-filter-fixup-048013766635\n\n"},{"id":"530196","messageId":"xmqqikfprf8e.fsf@gitster.g","threadId":"64435","inReplyTo":"20251104-b4-pks-ref-filter-fixup-v1-1-2fbca52d76d9@pks.im","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-04T15:26:25Z","receivedAt":"2025-11-04T15:26:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> This is only an issue for a subset of retrieved values, as not all of\n> the infrastructure ends up calling `get_or_parse_object()`. So the\n> effect is limited, which is probably why the issue wasn't detected\n> earlier.\n>\n> Fix the issue by resetting `maybe_object` in `get_object()`.\n\nYup, I got lucky as --sort=version:tag is one of the things I use\nat least once a week.\n\nWill queue on top of that topic and immediately merge down to\n'next'.\n\nThanks.\n"},{"id":"530212","messageId":"xmqqpl9xps3x.fsf@gitster.g","threadId":"64435","inReplyTo":"20251104-b4-pks-ref-filter-fixup-v1-1-2fbca52d76d9@pks.im","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2025-11-04T18:31:14Z","receivedAt":"2025-11-04T18:31:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> In 054f5f457e (ref-filter: parse objects on demand, 2025-10-23) we have\n> started to skip parsing some objects in case we don't need to access\n> their values in the first place. This was done by introducing a new\n> member `struct expand_data::maybe_object` that gets populated on demand\n> via `get_or_parse_object()`.\n>\n> This has led to a regression though where the object now gets reused\n> because we don't reset it properly. The `oi` structure is declared in\n> global scope, and there is no single place where we reset it before\n> invoking `get_object()`. The consequence is that the `maybe_object`\n> member doesn't get reset across calls, so subsequent calls will end up\n> reusing the same object.\n>\n> This is only an issue for a subset of retrieved values, as not all of\n> the infrastructure ends up calling `get_or_parse_object()`. So the\n> effect is limited, which is probably why the issue wasn't detected\n> earlier.\n>\n> Fix the issue by resetting `maybe_object` in `get_object()`.\n>\n> Reported-by: Junio C Hamano \n> Based-on-patch-by: Jeff King \n> Signed-off-by: Patrick Steinhardt \n> ---\n> As reported by Junio in . This applies\n> directly on top of ps/ref-peeled-tags at 054f5f457e (ref-filter: parse\n> objects on demand, 2025-10-23)\n>\n> Thanks!\n\nThanks. As we stop reusing a stale maybe_object and instead start\nparsing the right object when we need to, I wondered if the \"on\ndemand\" commit needs a new benchmark, but the example cited in the\nmessage used %(raw) so it would not be affected, I guess.\n\nQueued. Thanks.\n"},{"id":"530218","messageId":"20251104211130.GA2618884@coredump.intra.peff.net","threadId":"64435","inReplyTo":"xmqqpl9xps3x.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-04T21:11:30Z","receivedAt":"2025-11-04T21:11:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2025 at 10:31:14AM -0800, Junio C Hamano wrote:\n\n> > As reported by Junio in . This applies\n> > directly on top of ps/ref-peeled-tags at 054f5f457e (ref-filter: parse\n> > objects on demand, 2025-10-23)\n> >\n> > Thanks!\n> \n> Thanks. As we stop reusing a stale maybe_object and instead start\n> parsing the right object when we need to, I wondered if the \"on\n> demand\" commit needs a new benchmark, but the example cited in the\n> message used %(raw) so it would not be affected, I guess.\n\nYeah. The point was to speed up stuff like %(raw) that needs the object\ncontents but doesn't want to do the parse. So it never looked at\nmaybe_object in the first place, which is why it got faster.\n\nI actually wonder if there is any other placeholder that benefits at\nall. The ref-filter code already tries to avoid doing unneeded work. The\nmost obvious there is not loading the object at all, which is why stuff\nlike \"%(refname) %(objectname)\" is faster than adding in %(raw), which\nneeds the object contents (but no parsing). And likewise stuff like\n%(tag) needs parsing, and thus also triggers loading the object.\n\nSo 054f5f457e helps formats which require the object contents but _not_\nparsing. I can't think of another placeholder besides %(raw) which would\nbenefit from that.\n\nWhat _would_ help more formats is doing the parsing more progressively,\nskipping parts that aren't needed. If you just want %(tree), for\nexample, then:\n\n 1. You don't need to look at non-commit objects.\n\n 2. You can stop parsing the commit objects after the \"tree\" line.\n\nBut doing that is more involved. So not an argument against the patch,\nbut just noting its limitations. ;)\n\nI think the parser in pretty.c tries to be a bit more careful here. It\nwould be nice if we could unify these.\n\n-Peff\n"},{"id":"530220","messageId":"20251104213332.GB2618884@coredump.intra.peff.net","threadId":"64435","inReplyTo":"20251104-b4-pks-ref-filter-fixup-v1-1-2fbca52d76d9@pks.im","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-04T21:33:32Z","receivedAt":"2025-11-04T21:33:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2025 at 03:36:13PM +0100, Patrick Steinhardt wrote:\n\n> Fix the issue by resetting `maybe_object` in `get_object()`.\n\nThanks, this spot makes sense looking at the context.\n\n> +test_expect_success 'annotated tag version sort' '\n> +\tgit tag -a -m \"sample 1.0\" vsample-1.0 &&\n> +\tgit tag -a -m \"sample 2.0\" vsample-2.0 &&\n> +\tgit tag -a -m \"sample 10.0\" vsample-10.0 &&\n> +\tcat >expect <<-EOF &&\n> +\tvsample-1.0\n> +\tvsample-2.0\n> +\tvsample-10.0\n> +\tEOF\n> +\n> +\tgit tag --list --sort=version:tag vsample-\\* >actual &&\n> +\ttest_cmp expect actual &&\n> +\n> +\t# Ensure that we also handle this case alright in the case we have the\n> +\t# peeled values cached e.g. via the packed-refs file.\n> +\tgit pack-refs --all &&\n> +\tgit tag --list --sort=version:tag vsample-\\* &&\n> +\ttest_cmp expect actual\n> +'\n\nThis test seems fine, though I think you can see the same thing even\nmore easily with just:\n\n git for-each-ref --format='%(refname) %(tag)' refs/tags/vsample-\\*\n\nwhich shows each tag after the first with the same (wrong) tag.\n\nCuriously if I run something similar in git.git like:\n\n git for-each-ref --format='%(refname) %(tag)'\n\nI get garbage uninitialized data on each of the tag lines. The\ndifference is that the first parsed object is a non-tag, so our stale\nparsed state didn't actually fill in the tag values. Surprisingly ASan\ndoesn't complain, but it may be because we end up looking at memory with\na bogus type-cast (get_or_parse returns the stale \"struct commit\", but\nwe cast it to a \"struct tag\").\n\nI didn't dig too deeply there since the fix here should make it all go\naway (and seems to in my testing). I wondered if there was a way for a\nbroken repo to fool the code here (we think something is a tag, but in\nthe odb it's really a commit or something), but I don't think so. We\nenter grab_tag_values() based on data->type being OBJ_TAG, and then we\nfeed that same value to parse_object_buffer(). So it will be the same\neverywhere, and our cast can never do the wrong thing. And if we see\ncorruption (e.g., a tag refers to object X as a blob, and then another\ntag refers to it as a commit), then lookup_commit() should return NULL\nfor us.\n\nSo I think this fix should be sufficient.\n\n-Peff\n"},{"id":"530221","messageId":"20251104220454.GC2618884@coredump.intra.peff.net","threadId":"64435","inReplyTo":"20251104211130.GA2618884@coredump.intra.peff.net","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2025-11-04T22:04:54Z","receivedAt":"2025-11-04T22:04:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Nov 04, 2025 at 04:11:30PM -0500, Jeff King wrote:\n\n> I actually wonder if there is any other placeholder that benefits at\n> all. The ref-filter code already tries to avoid doing unneeded work. The\n> most obvious there is not loading the object at all, which is why stuff\n> like \"%(refname) %(objectname)\" is faster than adding in %(raw), which\n> needs the object contents (but no parsing). And likewise stuff like\n> %(tag) needs parsing, and thus also triggers loading the object.\n> \n> So 054f5f457e helps formats which require the object contents but _not_\n> parsing. I can't think of another placeholder besides %(raw) which would\n> benefit from that.\n\nBTW, the one other oddity I aw while looking at this is %(describe). It\nasks for SOURCE_OBJ, which causes ref-filter to load the object. But we\ndon't need it! We're just going to call out to git-describe.\n\nBut just changing that SOURCE_ flag is not enough, since we call\ngrab_describe_values() from deep inside grab_values(), which we do after\ngetting the object content. We'd have to move that call further up, but\ntaking care to handle both deref=0 and deref=1 cases. Or maybe not? Does\ngit-describe always return the same answer when describing a tag versus\nwhat it points to? In which case it should be more like how the\nAHEAD_BEHIND atom works.\n\nAt any rate, I was sufficiently grossed out by the ref-filter code that\nI stopped poking at it. Loading the objects is an unnecessary\ninefficiency, but compared to spawning a separate git-describe process,\nit is probably peanuts. So somebody who cares more can dig further if\nthey want.\n\n-Peff\n"},{"id":"530288","messageId":"aQw6dM2O5wSoLd9E@pks.im","threadId":"64435","inReplyTo":"xmqqpl9xps3x.fsf@gitster.g","subject":"Re: [PATCH] ref-filter: fix stale parsed objects","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2025-11-06T06:04:36Z","receivedAt":"2025-11-06T06:04:43Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Tue, Nov 04, 2025 at 10:31:14AM -0800, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n> > In 054f5f457e (ref-filter: parse objects on demand, 2025-10-23) we have\n> > started to skip parsing some objects in case we don't need to access\n> > their values in the first place. This was done by introducing a new\n> > member `struct expand_data::maybe_object` that gets populated on demand\n> > via `get_or_parse_object()`.\n> >\n> > This has led to a regression though where the object now gets reused\n> > because we don't reset it properly. The `oi` structure is declared in\n> > global scope, and there is no single place where we reset it before\n> > invoking `get_object()`. The consequence is that the `maybe_object`\n> > member doesn't get reset across calls, so subsequent calls will end up\n> > reusing the same object.\n> >\n> > This is only an issue for a subset of retrieved values, as not all of\n> > the infrastructure ends up calling `get_or_parse_object()`. So the\n> > effect is limited, which is probably why the issue wasn't detected\n> > earlier.\n> >\n> > Fix the issue by resetting `maybe_object` in `get_object()`.\n> >\n> > Reported-by: Junio C Hamano \n> > Based-on-patch-by: Jeff King \n> > Signed-off-by: Patrick Steinhardt \n> > ---\n> > As reported by Junio in . This applies\n> > directly on top of ps/ref-peeled-tags at 054f5f457e (ref-filter: parse\n> > objects on demand, 2025-10-23)\n> >\n> > Thanks!\n> \n> Thanks. As we stop reusing a stale maybe_object and instead start\n> parsing the right object when we need to, I wondered if the \"on\n> demand\" commit needs a new benchmark, but the example cited in the\n> message used %(raw) so it would not be affected, I guess.\n\nI just did another benchmark, and relative numbers still look the same\nas in the original one:\n\n Benchmark 1: for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe~)\n Time (mean ± σ): 369.6 ms ± 0.5 ms [User: 311.9 ms, System: 56.3 ms]\n Range (min … max): 368.7 ms … 370.1 ms 10 runs\n\n Benchmark 2: for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe)\n Time (mean ± σ): 327.9 ms ± 0.5 ms [User: 279.9 ms, System: 46.6 ms]\n Range (min … max): 327.3 ms … 328.8 ms 10 runs\n\n Summary\n for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe) ran\n 1.13 ± 0.00 times faster than for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe~)\n\nThanks!\n\nPatrick\n"}]}