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

[Mirko Faina <mroik@delayed.space>]

On Sun, Apr 19, 2026 at 08:21:18PM -0400, Jeff King wrote:
> 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.

What about a triple reverse? That would mean the original reverse choice
is lost and it defaults to the historical "after", which I'm fine with,
but this will need some extra caveat in the documentation :')

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

Yes, I will add a negated form as well.

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

This unfortunately wouldn't work as the first condition is a prefix of
the third, so no free copy-paste for me.

Will have separate parsing for omitted and explicit forms in v2.

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

I'm sure someone will be fed up enough to bring in parse-options at some
point.

> -Peff

Thank you