Re: [PATCH] revision.c: implement --reverse=before for walks
- From
Jeff King <peff@peff.net>
- Date
- Apr 20, 2026, 00:21 UTC
- Message-ID
- <20260420002118.GB1238475@coredump.intra.peff.net>
- In-Reply-To
- <aeUqSltEWIWaPDh3@exploit>
On Sun, Apr 19, 2026 at 10:31:37PM +0200, Mirko Faina wrote:
Show 14 quoted lines
> > 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