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

From: Ben Knoble <ben.knoble@gmail.com>
Date: Apr 19, 2026, 12:06 UTC
Message-ID: <C60EE993-97DA-45F7-89DE-2F97ABB0F685@gmail.com>
In-Reply-To: <20260418164736.2367523-2-mroik@delayed.space>

Show 39 quoted lines

> Le 18 avr. 2026 à 12:57, Mirko Faina <mroik@delayed.space> a écrit :
> 
> In a revision walk `--reverse` can only be applied after any commit
> limiting option. This makes getting a limited amount of commits from the
> tail impossible. E.g.
> 
>    git log --reverse --max-count=3
> 
> Some would expect this to give back the first 3 commits of the project.
> Instead it returns the last 3 but in reversed order.
> 
> Teach `get_revision()` to accpet an argument `(after|before)` from the
> CLI, and apply the reversal before or after the commit limiting options
> based on this argument. If no argument is provided default to the
> current behaviour, applying `--reverse` after the commit limiting
> options.
> 
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Documentation/rev-list-options.adoc |  6 ++--
> revision.c                          | 42 ++++++++++++++++++++++---
> revision.h                          |  7 ++++-
> t/t4202-log.sh                      | 49 +++++++++++++++++++++++++++++
> 4 files changed, 97 insertions(+), 7 deletions(-)
> 
> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..eed1813a92 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -914,10 +914,12 @@ With `--topo-order`, they would show 8 6 5 3 7 4 2 1 (or 8 7 4 2 6 5
> avoid showing the commits from two parallel development track mixed
> together.
> 
> -`--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.

Show 15 quoted lines

> +    omitted it defaults to `after`. If `before` is chosen,
> +    `--reverse` will be applied before any commit limiting options.
> endif::git-shortlog[]
> 
> ifndef::git-shortlog[]
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..8338ea7448 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2685,8 +2685,26 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>        else
>            git_log_output_encoding = xstrdup("");
>        return argcount;
> -    } else if (!strcmp(arg, "--reverse")) {
> -        revs->reverse ^= 1;

The original handles multiple reverse options inverting each other…

Show 20 quoted lines

> +    } 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 :/

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

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.

Show 16 quoted lines

> +
> +        if (revs->reverse == 2)
> +            revs->max_count = -1;
> 
> -    if (revs->reverse) {
>        reversed = NULL;
>        while ((c = get_revision_internal(revs)))
>            commit_list_insert(c, &reversed);
>        commit_list_free(revs->commits);
>        revs->commits = reversed;
> -        revs->reverse = 0;
>        revs->reverse_output_stage = 1;
> +
> +        if (revs->reverse == 2)
> +            revs->max_count = max_count;
>    }

It looks we temporarily disable reversing and then re-enable it here, which makes some sense to me as a way to do “after” mode.

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

Show 87 quoted lines

>        if (revs->track_linear)
>            revs->linear = !!(c && c->object.flags & TRACK_LINEAR);
>        return c;
> diff --git a/revision.h b/revision.h
> index 584f1338b5..5b23343f17 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -196,7 +196,12 @@ struct rev_info {
>            rewrite_parents:1,
>            print_parents:1,
>            show_decorations:1,
> -            reverse:1,
> +            /*
> +             * 0 no reverse
> +             * 1 after
> +             * 2 before
> +             */
> +            reverse:2,
>            reverse_output_stage:1,
>            cherry_pick:1,
>            cherry_mark:1,
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 05cee9e41b..21e9a61994 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1882,6 +1882,55 @@ test_expect_success 'log --graph with --name-status' '
>    test_cmp_graph --name-status tangle..reach
> '
> 
> +cat >expect <<-\EOF
> +c3f451c Merge tag 'reach'
> +046b221 to remove
> +EOF
> +
> +test_expect_success 'log --reverse --oneline --max-count=2' '
> +    test_when_finished git reset --hard HEAD~1 &&
> +    touch to_remove &&
> +    git add to_remove &&
> +    git commit -m "to remove" &&
> +    git log --reverse --oneline --max-count=2 >actual &&
> +    test_cmp expect actual
> +'
> +
> +test_expect_success 'log --reverse after --oneline --max-count=2' '
> +    test_when_finished git reset --hard HEAD~1 &&
> +    touch to_remove &&
> +    git add to_remove &&
> +    git commit -m "to remove" &&
> +    git log --reverse after --oneline --max-count=2 >actual &&
> +    test_cmp expect actual
> +'
> +
> +test_expect_success 'log --reverse=after --oneline --max-count=2' '
> +    test_when_finished git reset --hard HEAD~1 &&
> +    touch to_remove &&
> +    git add to_remove &&
> +    git commit -m "to remove" &&
> +    git log --reverse=after --oneline --max-count=2 >actual &&
> +    test_cmp expect actual
> +'
> +
> +cat >expect <<-\EOF
> +3a2fdcb initial
> +f7dab8e second
> +EOF
> +
> +test_expect_success 'log --reverse before --oneline --max-count=2' '
> +    test_when_finished rm actual &&
> +    git log --reverse before --oneline --max-count=2 >actual &&
> +    test_cmp expect actual
> +'
> +
> +test_expect_success 'log --reverse=before --oneline --max-count=2' '
> +    test_when_finished rm actual &&
> +    git log --reverse=before --oneline --max-count=2 >actual &&
> +    test_cmp expect actual
> +'
> +
> cat >expect <<-\EOF
> * reach
> |
> 
> base-commit: e8955061076952cc5eab0300424fc48b601fe12d
> --
> 2.54.0.rc2.9.ge895506107
> 
>