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

From: Mirko Faina <mroik@delayed.space>
Date: Apr 19, 2026, 18:11 UTC
Message-ID: <aeUZUqSQI8FvRUco@exploit>
In-Reply-To: <C60EE993-97DA-45F7-89DE-2F97ABB0F685@gmail.com>

On Sun, Apr 19, 2026 at 08:06:24AM -0400, Ben Knoble wrote:

Show 8 quoted lines

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

Show 26 quoted lines

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

Show 16 quoted lines

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

Show 10 quoted lines

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