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

[Jeff King <peff@peff.net>]

On Sun, Apr 19, 2026 at 10:31:37PM +0200, Mirko Faina wrote:

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

Yeah, I agree it gets weird, and I think it is OK if we don't try to
combine before/after reverses (either making it an error, or using the
usual last-one-wins to have "before" override "after" in this example).

But we should keep "--reverse --reverse" working as before, as there is
no other way to countermand a previously-given reverse option, and
because it has always worked.

Usually we'd spell the option "--no-reverse", and it probably makes
sense to add it (to override an earlier "--reverse=after"), but we'd
still want to keep "--reverse --reverse" working for historical
compatibility.

So combined with the earlier suggestions for using an enum and
disallowing the un-stuck "--reverse after" form, we probably want
something like (totally untested):

diff --git a/revision.c b/revision.c
index 599b3a66c3..89a58a65b7 100644
--- a/revision.c
+++ b/revision.c
@@ -2686,7 +2686,20 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 			git_log_output_encoding = xstrdup("");
 		return argcount;
 	} else if (!strcmp(arg, "--reverse")) {
-		revs->reverse ^= 1;
+		/*
+		 * This relies on "do not reverse" being the 0 value for our
+		 * enum, and historical "reverse after" having value 1.
+		 */
+		revs->reverse = !revs->reverse;
+	} else if (!strcmp(arg, "--no-reverse")) {
+		revs->reverse = 0;
+	} else if (skip_prefix(arg, "--reverse=", &optarg)) {
+		if (!strcmp(optarg, "after"))
+			revs->reverse = REVS_REVERSE_AFTER;
+		else if (!strcmp(optarg, "before"))
+			revs->reverse = REVS_REVERSE_BEFORE;
+		else
+			die(_("unknown value for --reverse: %s"), optarg);
 	} else if (!strcmp(arg, "--children")) {
 		revs->children.name = "children";
 		revs->limited = 1;

Note that your original also allowed --reverse-o-matic, which we
probably don't want (and is fixed here).

I _think_ the negation from using "--reverse" after "--reverse=before"
should be sensible here. And "--reverse=" with two different modes just
overrides rather than trying to be clever. But you may want to
double-check all of the combinations.

This would all be much easier if revision.c used parse-options, of
course, which has all of these sorts of rules baked-in. But that's a
much bigger conversion, and probably not something you want to make a
prerequisite for your series. ;)

-Peff