Re: [PATCH/RFC 2/6] commit-reach: introduce struct paint_queue with per-side counters

[Derrick Stolee <stolee@gmail.com>]

On 6/23/2026 10:09 AM, Kristofer Karlsson wrote:
> On Tue, 23 Jun 2026 at 15:50, Derrick Stolee <stolee@gmail.com> wrote:

>> How much of this data that you are passing into the method could be
>> state in the paint_queue struct? Could we have the paint_queue manage
>> all of the state necessary to make decisions around the walk
>> termination?
> 
> Good idea, I think adding last_gen to the struct is doable and makes it cleaner.
> If needed we could also add the mb_flags there (but would be a followup patch)
> Minor note: I renamed the struct to paint_state so that I could rename
> the prio_queue to queue and not have "queue.queue" which felt
> confusing in the code.
> 
>> Or, could we do a peek into the queue to see the "top" commit, and
>> check if it is a finite commit or not? I know that 'last_gen' is
>> supposed to be the commit walked in the previous cycle, but it seems
>> that we only care about "the remaining commits are finite" as our
>> condition.
> 
> Yes, peeking into the queue would work too, but it would feel awkward,
> 
>   commit = prio_queue_peek();
>   if (halt conditions) return NULL;
>   prio_queue_get();

Good instinct to notice that peeking and getting from the same
method is awkward.
> And if we get first, the condition is not valid - that said, it would be doable
> to instead put the halt conditions _between_ popping the commit and
> updating the counters. I am not sure how ugly or confusing it would be,
> but I could add a comment to explain why that sequencing is important.
> (Popping the commit and updating the counters may lead to temporary
> 0 counts, but then when we enqueue parents of the commits they
> move away from the 0 anyway). It would become something like:
> 
> // dry-/pseudo-coded
>   commit *paint_queue_pop() {
>     commit = prio_queue_pop();
>     if (!commit) return NULL;
>     if (halt_condition(state, commit.generation)) return NULL;
>     // important: don't decrement counters before checking the halt condition
>     paint_count_update(state, commit->object.flags, -1);
>     return commit;
>   }
I think this would be an appropriate way to handle this. If we
pop and return NULL then it's ok that we removed data from the
queue because it shouldn't be reused.

Thanks,
-Stolee