From: Kristofer Karlsson <krka@spotify.com>
Date: Wed, 24 Jun 2026 14:38:50 GMT
Subject: Re: [PATCH v2 5/7] commit-reach: introduce struct paint_state with per-side counters
Message-ID: <CAL71e4N88H_VLd8nNfEVGqegbjT0bjQBgRdBN-kp1Y_U8ejJYg@mail.gmail.com>
In-Reply-To: <19639ad3-2d16-4f3b-be79-138e00144ea3@gmail.com>

On Wed, 24 Jun 2026 at 15:54, Derrick Stolee <stolee@gmail.com> wrote:
>
> I'm grateful to see these changes happening to the doc in real-
> time. I know it was extra work, but I'm grateful right now.
>
> Hopefully future historians will also benefit from this effort.

It was honestly not bad at all, and I agree it felt quite nice to
see how the doc naturally changed along with the implementation.

> > +static struct commit *paint_queue_get(struct paint_state *state)
> > +{
>
> Since we are going to make this a more complete termination
> condition, we may want to make that very explicit with a doc-
> comment. Something along the lines of "dequeue a commit when
> possible, but also signal termination of the walk when we
> conclude that no more merge bases will be discovered due to
> internal state."

Yes, I'll make sure to clean that part up more, maybe also
rename the function to be more descriptive.

> You mentioned in your cover letter how the min_generation value
> can add extra termination conditions. It may be a good idea to
> insert min_generation into the paint_queue struct and make it a
> termination condition for paint_queue_get(). If you consider this
> direction, then I'd make it a separate patch on top of this one
> _before_ adding the one-sided change. The extra tests that cover
> the exact number of walked commits can help to guarantee the same
> behavior, assuming that some of those tests check a non-zero
> min_generation input. (It may be good to add such trace tests in
> an earlier patch to help confidence in this case.)

I think I might wait with this - the patch series already feels
quite big, and I think it has a natural progression and finish now.
But I can definitely commit to following up later -- it would be a
smaller series that is easier to reason about, likely a single commit.

Thanks,
Kristofer