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

[Mirko Faina <mroik@delayed.space>]

On Sun, Apr 19, 2026 at 03:12:27PM -0400, D. Ben Knoble wrote:
> On Sun, Apr 19, 2026 at 2:11 PM Mirko Faina <mroik@delayed.space> wrote:
> >
> > On Sun, Apr 19, 2026 at 08:06:24AM -0400, Ben Knoble wrote:
> > > The original handles multiple reverse options inverting each other…
> > >
> > > > +    } else if (starts_with(arg, "--reverse")) {
> > > > +        if (!skip_prefix(arg, "--reverse=", &optarg)) {
> > > > +            if (argc < 2) {
> > > > +                revs->reverse = 1;
> > > > +                return 1;
> > > > +            } else {
> > > > +                optarg = argv[1];
> > > > +            }
> > > > +        }
> > > > +
> > > > +        if (!strcmp(optarg, "after")) {
> > > > +            revs->reverse = 1;
> > > > +        } else if (!strcmp(optarg, "before")) {
> > > > +            revs->reverse = 2;
> > > > +        } else {
> > > > +            revs->reverse = 1;
> > > > +            return 1;
> > > > +        }
> > > > +
> > > > +        return optarg == argv[1] ? 2 : 1;
> > >
> > > …which I don’t see here.
> > >
> > > I’m not familiar with this parsing code though so I can’t add much about the test other than to say it is a bit hard to follow :/
> >
> > Given that it is no longer binary handling multiple reverse can't simply
> > be inverting bits, it wouldn't make sense. This is done before the walk
> > itself, so even from the POV of the user it wouldn't make much sense to
> > reverse multiple times as the order of the applied options before this
> > patch (commit limiting options then reverse) doesn't change.
> >
> > This doesn't break any tests so I assumed it was fine.
> 
> I think I mean that
> 
>     git log --reverse --reverse
> 
> shows commits in the same order as "git log"; what should
> 
>     git log --reverse=after --reverse
> 
> do? Or what about preserving the behavior of the original "git log
> --reverse --reverse," which I don't think is done here?

Yes, this is what I was getting at. Since it is no longer binary what
would a double reverse mean? What if "--reverse=after --reverse=before"?
How should that be handled?

> Granted, I don't see this ability documented, and I cannot tell how
> many may scream if we change this behavior, so it's a bit
> hypothetical. But there is an argument for backward compatibility as a
> default, which I think we'd need to justify changing. Perhaps in the
> proposed log message?
> 
> (The original seems nonsensical to type, but of course you can imagine
> alias.A=log --reverse <other-stuff>, and then sometimes you want to do
> "git A --reverse" to un-reverse the commits.)

Will do in v2.

> > > I haven’t looked, but it would be nice if we could use an enum instead. Unfortunately that would probably take up more space in the struct, and I suppose the bit-packing is done intentionally for performance.
> >
> > Could define new macros so that the readers don't have to mentally keep
> > track of which value rapresents what. I didn't think that was
> > necessary, should I change it?
> 
> Yeah, a few `#define`d constants would make things more readable to
> me, at least, since we can't use the enum without space concerns
> (unless there's a way to bit-pack the enum to only 2 bits?).

Will do in v2.

> > > >    if (revs->reverse_output_stage) {
> > > > +        if (revs->reverse == 2 && revs->max_count == 0)
> > > > +            return NULL;
> > > > +
> 
> PS: something I spotted on a second read. [Ignoring reverse=after
> mode] This hunk looks to me like a nice little optimization (return
> nothing if we know max_count says we yield no commits). Of course, I
> could see that being viable early in the function, right? When asking
> get_revision for commits, if max_count is 0, just return NULL.
> 
> For reverse=after mode, this condition is only true if the max_count
> was 0 in the previous conditional, also, since we use max_count=-1
> before iterating get_revision_internal. That means the original
> max_count isn't touched. At any rate, it _seems_ to me that the whole
> function could benefit from this optimization… but I wonder if it is
> _necessary_ for correctness of reverse=after in some way that I'm not
> seeing? Since the current version doesn't need the early bailout, why
> does reverse=after?

Just to clarify, "reverse = 2" is "--reverse=before" and not
"--reverse=after".

With "reverse = 2", the snippet of code you're referencing is not an
optimization but a requirement for correctness. With "reverse = 1" we
just keep the max_count as is and it's used by get_revision_internal()
to stop if that limit is reached. What we find in 'reversed' are already
just the commits we need to return.

With "reverse = 2", we first set max_count to -1 and then retrieve the
whole history, then we set max_count to its original value. Then we
return the commits on each call of get_revision(). Now, unlike with
"reverse = 1", we have the whole history in 'reversed', because of that
we need to know when to stop. That's the reason we decrement max_count
only for "reverse = 2" and why "max_count == 0" is checked only for
"reverse = 2".

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

> Of course if I'm the only one confused and others make sense of it,
> that's ok, too.

No, I completely understand. I did have to retouch the function a few
times after writing the tests :P

> [1]: I traced this to 498bcd3159 (rev-list: fix --reverse interaction
> with --parents, 2008-08-29), but I can't fathom what the pop is doing
> there.

It's pretty much doing the same thing it does now, it's returning stored
commits. In both versions, the initial setup when "revs->reverse" is
true, becomes "dead code" after the first call.

Thank you