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

[Tian Yuchen <cat@malon.dev>]

Hi Mirco,

On 4/19/26 00:47, Mirko Faina wrote:
> 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.

Reasoning looks good to me.

Nit: I think we could gather more feedback on the naming. If I were a 
user unfamiliar with how Git works, the most natural and intuitive 
operation for me would be: "Show me the three *oldest* commits" (to be 
more precise, *oldest* in the sense of topological order, rather than 
the commit date or author date order. It’s really annoying. ), rather 
than "reverse the entire list and then select the three most recent 
ones". I think the confusion arises because users do not (and should 
not) know that '--max-count' only returns the most recent commits; 
consequently, they might wonder: "Well, the 'after' and 'before' 
parameters do make a difference, but why?". I believe it is best not to 
lead users to this point.


> 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
> +	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;
> +	} 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;
>   	} 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;
> +		}
> +
> +		if (revs->reverse == 2)
> +			revs->max_count = -1;

I think the space complexity here could be reduced a little. After all, 
since we’re only retrieving a few commits, there’s no need to load the 
entire reversed commit history into memory.

Perhaps we could maintain a window (or perhaps max heap) of finite length?

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

Regards, Yuchen