Re: [PATCH] revision.c: implement --reverse=before for walks

[Mirko Faina <mroik@delayed.space>]

On Wed, Apr 22, 2026 at 02:24:48PM -0400, D. Ben Knoble wrote:
> > > > > >        c = pop_commit(&revs->commits);
> > > > > > +        if (revs->reverse == 2)
> > > > > > +            revs->max_count--;
> > > > >
> > > > > Hm. Why do we decrement here? Again, not an area I’m familiar with, but a bit surprising.
> > > >
> > > > get_revision() (in revision.c) handles the reverse option and updates
> > > > the "struct git_graph". get_revision() then calls
> > > > get_revision_internal(), which handles commit boundaries and max_count,
> > > > here is where it gets decreased. Since max_count gets decreased
> > > > everytime get_revision_internal() is called, if we were to leave
> > > > max_count as is before the walk (in get_revision() at line 4558), the
> > > > walk would stop before reaching the root commit. This is why the current
> > > > --reverse option is applied only after commit limiting options. So
> > > > instead we set max_count at -1 walking the whole history and storing it
> > > > in 'reversed'. Now we're in "reverse_output_stage = 1", and in this
> > > > state we never call get_revision_internal() again, instead we pop
> > > > commits from 'reversed'. Because of this we have to handle max_count
> > > > outside get_revision_internal(), so we decrement it in the snippet of
> > > > code you referenced.
> > > >
> > > > A bit verbose but hopefully it'll get my point across.
> > >
> > > I don't 100% follow, but I'm out of my depth :)
> > >
> > > I think I see that get_revision() effectively has 2 modes pertaining
> > > to reverse: reverse and reverse output stage (the former falls
> > > directly into the latter, though).
> > >
> > > After some setup, the reverse mode calls get_revision_internal() as
> > > you said. That decrements max_count as a way of counting how many
> > > commits we've seen through the loop, so if we asked for 5 we'd only
> > > process 5 commits.
> > >
> > > Then we fall into the output stage mode, which pops a commit [1].
> > >
> > > With this patch, in reverse=after we disable max_count in the first
> > > (reverse) mode, as you said. Ok: we get the whole (filtered) history
> > > then, at which point we can now shrink. That makes sense.
> > >
> > > Then in the reverse output stage mode, we pretend to have one less
> > > max_count. That's what I can't figure out. Is it because of the
> > > pop_commit()? I guess I'm not totally seeing how that interacted with
> > > the max_count in the original code: does the current code yield one
> > > extra commit in get_revision_internal() ?
> >
> > I'm not sure I understand what you're referencing with "Then in the
> > reverse output stage mode, we pretend to have one less max_count".
> >
> > If you're referring to line 4573, then...
> >
> > > You wrote that "we never call get_revision_internal() again," but I
> > > don't see why that's true with this patch and not true before it.
> > >
> > > I do agree that _somebody_ has to handle max_count after
> > > get_revision() returns with reverse=after. I'm just not sure what
> > >
> > >     if (revs->reverse == 2)
> > >         revs->max_count--;
> > >
> > > is doing.
> >
> > ...we're not pretending we have fewer commits. Every subsequent call to
> > get_revision() after the first call will never enter the branch at line
> > 4548 and will only enter the branch at 4568. Everytime we pop a commit
> > from 'reversed' we decrease max_count so we can limit only to the amount
> > of commits the user wants.
> >
> > So, to recap, with "reverse = 2", on the first call to get_revision() we
> > walk the whole history and store it in 'reversed' in reversed order and
> > return the first commit.
> > On subsequent calls to get_revision() we do not walk the history again,
> > we simply return the commits that have been stored in 'reversed'.
> > Everytime we pop a commit we have to decrease max_count, and we check
> > againts max_count to know if we shouldn't return anymore commits (by
> > returning NULL).
> 
> …but I think this one does. I think what I missed is that in all
> "reverse" modes, get_revision() does some pre-computation and then
> yields one at a time the commits. In traditional "after" mode, the
> counting is done by get_revision_internal() [before reversal]. In the
> new mode, get_revision takes on that responsibility of
> get_revision_internal instead.
> 
> Hm. That suggests to me that get_revision's responsibilities are
> becoming complex. Might be worth some version of a refactor, but idk
> which.

If the refactor is to yield the responsibility of max_count to
get_revision_internal() for the new reverse mode too, then I'm not sure
if we want that. To maintain state across calls to
get_revision_internal() we'd have to store the temporary max_count that
currently lives in get_revision() in rev_info. I don't want to add
unnecessary new fields to the struct when its space is so valuable
(afterall we're using bitpacking).

In the current implementation all of the reversal happens in the same
call to get_revision(), so state is preserved across multiple
get_revision_internal() calls through the temporary int max_count.