On Fri, 26 Jun 2026 at 16:42, Derrick Stolee [off-list ref] wrote:

quoted

+  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.

quoted

              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.

Thanks,
Kristofer