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

[Mirko Faina <mroik@delayed.space>]

On Sun, Apr 19, 2026 at 08:06:24AM -0400, Ben Knoble wrote:
> > -`--reverse`::
> > +`--reverse[=(after|before)]`::
> >    Output the commits chosen to be shown (see 'Commit Limiting'
> >    section above) in reverse order. Cannot be combined with
> > -    `--walk-reflogs`.
> > +    `--walk-reflogs`. `when` can either be `after` or `before`, if
> 
> “When” is not mentioned prior to here, so it’s explanation leaves the reader wondering what it refers to.

Yes, when I first wrote it it said `--reverse[=when]` but forgot to
change the text after deciding to give the valid inputs instead. Will
fix in v2.

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

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

> > 
> >    if (revs->reverse_output_stage) {
> > +        if (revs->reverse == 2 && revs->max_count == 0)
> > +            return NULL;
> > +
> >        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.

Thank you