From: D. Ben Knoble Date: Wed, 22 Apr 2026 18:24:48 GMT Subject: Re: [PATCH] revision.c: implement --reverse=before for walks Message-ID: In-Reply-To: On Sun, Apr 19, 2026 at 4:31 PM Mirko Faina wrote: > > > > > 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". Oh golly, sorry about that! > 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". Ok, this explanation hasn't yet clicked… > > > > > 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. > > 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. And this pop makes more sense now, too. Phew! -- D. Ben Knoble