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

[Derrick Stolee <stolee@gmail.com>]

On 6/26/2026 10:53 AM, Kristofer Karlsson wrote:
> 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.

And the remaining condition exposed in this diff isn't included,
either:
 
>>>               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.

I'm happy to keep this one out of the series. Perhaps it would
be good for you to finish this series with the current scope
and then for another contributor (me, probably) to do another
round of cleanup/reaction on top. I only say "another
contributor" because new eyes can help to see new things, outside
of a patch diff.

>> 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.
Sounds good.
-Stolee