Re: [PATCH/RFC 4/6] t6600: add test cases for side-exhaustion edge cases

From: Kristofer Karlsson <krka@spotify.com>
Date: Jun 22, 2026, 19:25 UTC
Message-ID: <CAL71e4M0T4fFG4JuYTp_ZPHzNcHXf342Xkh0n0dt4LVKsuSu2Q@mail.gmail.com>
In-Reply-To: <1588b53d-9576-4752-9459-da48276e4b2a@gmail.com>

On Mon, 22 Jun 2026 at 20:15, Derrick Stolee <stolee@gmail.com> wrote:

Show 6 quoted lines

> It's usually my preference to see these tests show up before the
> new code arrives, that way we can see that they already work with
> the old logic and continue to work with the new logic.
>
> It's minor, but putting them after your code change may be adding
> enforcement of a change of behavior.

Agreed, I actually also prefer that in practice so I am not sure why I ordered them this way - perhaps some attempt at making it easier to review (show the idea and change before the verification). I will reorder to put all new tests as the first commit (or second, if I will also introduce a status-quo technical first).

Show 5 quoted lines

>
> One thing that could be helpful here is to consider tracing a
> count of "commits walked" in the merge-base code, then you could
> have these tests demonstrate the performance benefit by checking
> for that number changing.

Good idea, I actually had some of that locally when developing it, but I removed the ugly traces before submitting this. I will try to re-introduce that in a nice way. It would be neat to let tests inspect that side effect, though in the worst case that could make it fragile. At the very least it's good for human debugging though.

> In t6600, that tracing number would not be the same across the
> three different data shapes (full graph, half graph, no graph) and
> that could be valuable to demonstrate in tests.

Agreed, the number of commits visited would be more interesting than the relative performance numbers since it's an algorithmic change rather than a micro-optimization.

Thanks, Kristofer