Re: [PATCH v7 05/10] commit-reach: add trace2 instrumentation to paint_down_to_common()

On Fri, Aug 7, 2026 at 5:34 AM Kristofer Karlsson <[email protected]> wrote:
>
> On Fri, 7 Aug 2026 at 05:03, Elijah Newren <[email protected]> wrote:
> >
> > > Add a step counter and trace2_data_intmax() call so that the number
> > > of commits visited during the paint walk is observable via
> > > GIT_TRACE2_EVENT. This provides a way to measure the impact of
> > > future optimizations without relying on wall-clock benchmarks alone.
> >
> > Ooh, I like it.
>
> I will need to credit Stolee for this idea to count steps instead
> of measuring wall clock -- but I agree, it comes in very handy
> here.
>
> > > -       test_all_modes in_merge_bases_many
> > > +       test_all_modes in_merge_bases_many &&
> > > +       test_paint_down_steps 45 2 25 3
> > >  '
> >
> > Whoa, what?  <Digs around for a while.>  So, this is really confusing
> > at first to a reviewer; it makes me think you are testing that you've
> > already written the optimization and that some forms of commit-graphs
> > provide a speedup from your work that doesn't land until later in the
> > series.  It might help if you point out either in the commit message
> > or a comment here that this code is just relying on pre-existing
> > optimization where a min_generation is passed and --all is not passed.
> > (In contrast to below where --all is passed, so it has to dig deeper
> > with or without the commit graph).
>
> Yeah, the numbers are a bit hard to understand here -- I could
> add a comment saying that the min_generation floor optimization
> kicks in here and this is how it behaves for:
> no graph, full v2 graph, partial v2 graph, v1 graph
> (in that order)
>
> So it's not about the new optimization, it's adding these
> counters to existing graph tests.
>
> I am not sure what the best approach is here:
> skip these step-asserts for graphs that already use some other
> optimization (min_generation floor), add a test comment,
> or leave it as it is (confusing for reviewing now, but perhaps
> not as confusing long term?)

I think a short comment in the commit message about the new tests in
this commit triggering the existing min_generation optimization would
have dispelled my original misunderstanding and made the review
easier, and would thus be worthwhile to add for other reviewers or
future folks running across the commit.