From: Patrick Steinhardt <ps@pks.im>
Date: Thu, 06 Nov 2025 06:04:36 GMT
Subject: Re: [PATCH] ref-filter: fix stale parsed objects
Message-ID: <aQw6dM2O5wSoLd9E@pks.im>
In-Reply-To: <xmqqpl9xps3x.fsf@gitster.g>

On Tue, Nov 04, 2025 at 10:31:14AM -0800, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > In 054f5f457e (ref-filter: parse objects on demand, 2025-10-23) we have
> > started to skip parsing some objects in case we don't need to access
> > their values in the first place. This was done by introducing a new
> > member `struct expand_data::maybe_object` that gets populated on demand
> > via `get_or_parse_object()`.
> >
> > This has led to a regression though where the object now gets reused
> > because we don't reset it properly. The `oi` structure is declared in
> > global scope, and there is no single place where we reset it before
> > invoking `get_object()`. The consequence is that the `maybe_object`
> > member doesn't get reset across calls, so subsequent calls will end up
> > reusing the same object.
> >
> > This is only an issue for a subset of retrieved values, as not all of
> > the infrastructure ends up calling `get_or_parse_object()`. So the
> > effect is limited, which is probably why the issue wasn't detected
> > earlier.
> >
> > Fix the issue by resetting `maybe_object` in `get_object()`.
> >
> > Reported-by: Junio C Hamano <gitster@pobox.com>
> > Based-on-patch-by: Jeff King <peff@peff.net>
> > Signed-off-by: Patrick Steinhardt <ps@pks.im>
> > ---
> > As reported by Junio in <xmqqo6pjt2wn.fsf@gitster.g>. This applies
> > directly on top of ps/ref-peeled-tags at 054f5f457e (ref-filter: parse
> > objects on demand, 2025-10-23)
> >
> > Thanks!
> 
> Thanks.  As we stop reusing a stale maybe_object and instead start
> parsing the right object when we need to, I wondered if the "on
> demand" commit needs a new benchmark, but the example cited in the
> message used %(raw) so it would not be affected, I guess.

I just did another benchmark, and relative numbers still look the same
as in the original one:

    Benchmark 1: for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe~)
      Time (mean ± σ):     369.6 ms ±   0.5 ms    [User: 311.9 ms, System: 56.3 ms]
      Range (min … max):   368.7 ms … 370.1 ms    10 runs

    Benchmark 2: for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe)
      Time (mean ± σ):     327.9 ms ±   0.5 ms    [User: 279.9 ms, System: 46.6 ms]
      Range (min … max):   327.3 ms … 328.8 ms    10 runs

    Summary
      for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe) ran
        1.13 ± 0.00 times faster than for-each-ref (revision = a29e2e8fe7e3935e23d2a03dc429cc9c2e68bfbe~)

Thanks!

Patrick