Re: [PATCH v3 8/8] commit-reach: move min_generation check into paint_queue_get()

> On Fri, 26 Jun 2026 at 16:42, Derrick Stolee <stolee@gmail.com> wrote:
>>
>>> +  4. Generation cutoff: the dequeued commit's generation is below
>>> +     a caller-supplied `min_generation` threshold.
>>
>> Technically, this was always a termination condition of the walk,
>> but now we are correcting the documentation to match. It was just
>> not part of the termination in the dequeue method until now.
> 
> You're right, I should perhaps fold it into the first patch instead,
> which would be logically more accurate. Would be an easy thing
> to fix for a v4.

>>>               flags = commit->object.flags & (PARENT1 | PARENT2 | STALE);
>>>               if (flags == (PARENT1 | PARENT2)) {
>>>                       if (!(commit->object.flags & RESULT)) {
>>> @@ -219,7 +224,7 @@ static int paint_down_to_common(struct repository *r,
>>>                                * descendant of this one.
>>>                                */
>>>                               if (!(mb_flags & MERGE_BASE_FIND_ALL) &&
>>> -                                 generation < GENERATION_NUMBER_INFINITY)
>>> +                                 state.last_gen < GENERATION_NUMBER_INFINITY)
>>>                                       break;
>>>                       }
>>>                       /* Mark parents of a found merge stale */
>>
>> And here's another termination condition. We are now leaking the
>> abstraction of the 'state.last_gen' which give me some bad feelings.
> 
> Yes, this is one of the minor annoyances I also noticed,
> but it's not too bad. I think a followup could be to either:
> 
> 1. remove this optimization entirely (though I will have to spend
> some time reasoning if there are realistic use cases where this
> would trigger much earlier than side exhaustion.
> 
> 2. tweak the logic to instead halting on exactly this commit,
> instead halt inside paint_queue_get if:
>    generation < INFINITY && !FIND_ALL && num_results >= 1
> This would change the semantics slightly (but for the better?)
> in the the found merge-base could be in the infinite region but
> near the finite region and thus would unlock the optimization
> as soon as we pass that boundary. But I did not want to include
> that change in this series, which is perhaps already getting
> too complex.

>> We are getting to the point where I'd leave such a thing for a
>> follow-up, but since you are needing to re-roll, then this is
>> another case where we can move this into the paint_queue_get(). I
>> don't think this is me "raising the bar" from earlier recommendations,
>> because I was asking for all loop termination to be in the get()
>> method, if possible.
>>
>> But also: I'm not looking at the full method right now to see if
>> terminating _at this location in the loop_ is critical. So it may
>> very well be impossible to move this into the get() call, in which
>> case please ignore this suggestion and use state.last_gen.
> 
> I think it's not critical (as I mentioned above) and I think I will
> need to follow up on this later.