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

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