Re: [PATCH] revision.c: implement --reverse=before for walks
- From
D. Ben Knoble <ben.knoble@gmail.com>
- Date
- Apr 19, 2026, 19:12 UTC
- Message-ID
- <CALnO6CACfSyzyguX4623Dk3y+QEM_Dbmfko8dTyM1p3JxBjZFg@mail.gmail.com>
- In-Reply-To
- <aeUZUqSQI8FvRUco@exploit>
On Sun, Apr 19, 2026 at 2:11 PM Mirko Faina <mroik@delayed.space> wrote:
Show 36 quoted lines
>
> 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?
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.)
Show 23 quoted lines
> > > } else if (!strcmp(arg, "--children")) {
> > > revs->children.name = "children";
> > > revs->limited = 1;
> > > @@ -4525,19 +4543,35 @@ struct commit *get_revision(struct rev_info *revs)
> > > {
> > > struct commit *c;
> > > struct commit_list *reversed;
> > > + int max_count = revs->max_count;
> > > +
> > > + if (revs->reverse && !revs->reverse_output_stage) {
> > > + if (revs->reverse == 3) {
> > > + BUG("allowed values for reverse are 0, 1 and 2");
> > > + revs->reverse = 1;
> > > + }
> >
> > Is this possible? I guess I can see from the expanded bit width that it’s a valid input, and there’s no protection stopping other callers accidentally adding this.
>
> Current code should never generate a 3, but in case it happens I assume
> the user wants to use the original behaviour of reverse, so I set the
> value accordingly instead of stopping the program and notify that
> there's a bug.
>
> Should this be changed?I don't have any strong opinions on this.
Show 5 quoted lines
> > 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?).
> > > 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?
Show 22 quoted lines
> > > 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() ?
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.
Of course if I'm the only one confused and others make sense of it, that's ok, too.
> Thank you
Thanks!
-- D. Ben Knoble [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.