From: Kristofer Karlsson <krka@spotify.com>
Date: Wed, 24 Jun 2026 14:31:26 GMT
Subject: Re: [PATCH v2 4/7] commit-reach: add trace2 instrumentation to paint_down_to_common()
Message-ID: <CAL71e4O7s7y+SJRp3GZB+j9SLB_q=kK8ysCKqH9Mp2VDn0sT=Q@mail.gmail.com>
In-Reply-To: <560c91df-3c07-4c8f-9924-ef0cc7646e08@gmail.com>

On Wed, 24 Jun 2026 at 15:41, Derrick Stolee <stolee@gmail.com> wrote:
>
> (highlighting this chunk)
>
> > +     rm -rf .git/objects/info/commit-graph \
> > +             .git/objects/info/commit-graphs &&
> > +
> > +     GIT_TRACE2_EVENT="$(pwd)/trace-none.txt" \
> > +             git merge-base --all commit-9-9 commit-9-1 >actual &&
> > +     test_trace2_data paint_down_to_common steps 81 <trace-none.txt &&
>
> I'd rather see the whitespace line before the `rm` to make it
> more obvious that it's setting up the "none" case.

Ah yes, good point, will fix.

> > +     cp commit-graph-full .git/objects/info/commit-graph &&
> > +     GIT_TRACE2_EVENT="$(pwd)/trace-full.txt" \
> > +             git merge-base --all commit-9-9 commit-9-1 >actual &&
> > +     test_trace2_data paint_down_to_common steps 80 <trace-full.txt &&
> > +
> > +     cp commit-graph-half .git/objects/info/commit-graph &&
> > +     GIT_TRACE2_EVENT="$(pwd)/trace-half.txt" \
> > +             git merge-base --all commit-9-9 commit-9-1 >actual &&
> > +     test_trace2_data paint_down_to_common steps 81 <trace-half.txt
> > +'
> > +
>
> This test is a great example. I look forward to seeing that it
> updates in the future.
>
> One thing I was hoping to see was that your side-exhaustion tests
> (from patch v2 2/7) would also include these checks so they are
> more obviously updating when the implementation updates later.

I was internally contemplating how much I should introduce the steps
validation to existing tests. My worry was that it might make tests
fragile - for example I repeatedly got some off-by-one changes
after refactoring the halt condition slightly (differs depending on
adding the halts solely within paint_queue_get or having it at the end
of the loop) and I think potentially other future work could affect it.

But I'm happy to attach the steps checks for more relevant tests,
it's not much work to change.

> One way to accomplish that is to reorder this patch before adding
> those tests so their first version includes these checks and then
> the values update when changing the implementation.

I was thinking I could keep the same order, but the patch to introduce
the trace could also modify the tests at the same time - that would
perhaps make it even more clear. Also this means I could avoid
making changes to Elijah's commit that I already _partly_ butchered
(extracted the test change as-is, but dropped the other file changes)
and I don't want to make that one more unclean.

Thanks,
Kristofer