Re: [PATCH v2 5/7] commit-reach: introduce struct paint_state with per-side counters
- From
Kristofer Karlsson <krka@spotify.com>
- Date
- Jun 24, 2026, 14:38 UTC
- 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:
Show 5 quoted lines
> > 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.
Show 9 quoted lines
> > +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.
Show 10 quoted lines
> 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