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

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

[Mirko Faina]

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
+	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;
 
-	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
-- 
2.54.0.rc2.9.ge895506107

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

[Tian Yuchen]

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

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

[Mirko Faina]

On Sun, Apr 19, 2026 at 02:20:41AM +0800, Tian Yuchen wrote:
> Hi Mirco,

Unfortunate miss spell T_T

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

The rest of the flags do apply during traversal, and since git goes
through the whole history the relative order should be preserved, so
depending on the flags that are passed the user can order topologically
or chronologically (tho some flags are forbidden together).

I'm up to change the naming tho (and if someone has better phrasing for
the docs please tell).

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

Unfortunately since the underlying data structure is a linked list we
have to traverse the whole tree to get the first one from the tail. The
way get_revision() loads the next commit is through process_parents().
Even if we were able to start from the tail we wouldn't have any
reference to the children.

I suspect reducing space complexity would require to change a lot of
inner workings of git to make the history traversable both ways.

Thank you

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

[Mirko Faina]

On Sat, Apr 18, 2026 at 08:42:15PM +0200, Mirko Faina wrote:
> > 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?
> 
> Unfortunately since the underlying data structure is a linked list we
> have to traverse the whole tree to get the first one from the tail. The
> way get_revision() loads the next commit is through process_parents().
> Even if we were able to start from the tail we wouldn't have any
> reference to the children.
> 
> I suspect reducing space complexity would require to change a lot of
> inner workings of git to make the history traversable both ways.

Oh, I missunderstood what you meant, sorry. Yes, a window should allow
us to reduce the amount of the memory used. Will do in v2.

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

[Junio C Hamano]

Tian Yuchen <cat@malon.dev> writes:

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

Does "we're only retrieving a few commits" come from the fact that
the command example is "log --reverse -3"?  

 - What should happen when you give "git log --reverse=before"
   without "--max-count=3"?

 - What should happen without "--max-count" but other limiting
   options, like "--author=Tian" or "--min-parents=2"?

It might be that the right way to look at this new feature is not
that "we are changing where reverse is applied", but "count limit is
applied much later than usual", which may mean at the UI level, it
may not be good at the conceptual level to sell this as an extension
to the "--reverse" option?  I dunno.

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

[Tian Yuchen]

On 4/21/26 00:06, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
>> 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.
> 
> Does "we're only retrieving a few commits" come from the fact that
> the command example is "log --reverse -3"?
> 
>   - What should happen when you give "git log --reverse=before"
>     without "--max-count=3"?

I still wanna talk my way out of it ;)

If we maintain a window of length K out of N nodes:

   - If K is small (e.g. 3), The space complexity is strictly O(K), 
which saves a considerable amount compared to the unoptimised one;

   - If K is huge, reduce to traversing all N nodes. At this point, the 
space complexity is the same as when unoptimised (O(N)). At most, there 
is an additional time cost of O(log N) for stack operation, which can be 
disregarded.

It seems like a strategy that could be applied consistently, but...

>   - What should happen without "--max-count" but other limiting
>     options, like "--author=Tian" or "--min-parents=2"?

I must admit you’re absolutely right on this point.

If we were to introduce a new state machine and data structure for every 
type of parameter, it would indeed be rather cumbersome to maintain and 
not particularly cost-effective.

> It might be that the right way to look at this new feature is not
> that "we are changing where reverse is applied", but "count limit is
> applied much later than usual", which may mean at the UI level, it
> may not be good at the conceptual level to sell this as an extension
> to the "--reverse" option?  I dunno.

That's what I meant to say earlier in the 'nit' section. I couldn’t 
quite put it into words at that time :((

Thanks, Yuchen

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

[Mirko Faina]

On Mon, Apr 20, 2026 at 09:06:44AM -0700, Junio C Hamano wrote:
> Tian Yuchen <cat@malon.dev> writes:
> 
> > 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.
> 
> Does "we're only retrieving a few commits" come from the fact that
> the command example is "log --reverse -3"?  
> 
>  - What should happen when you give "git log --reverse=before"
>    without "--max-count=3"?
> 
>  - What should happen without "--max-count" but other limiting
>    options, like "--author=Tian" or "--min-parents=2"?
> 
> It might be that the right way to look at this new feature is not
> that "we are changing where reverse is applied", but "count limit is
> applied much later than usual", which may mean at the UI level, it
> may not be good at the conceptual level to sell this as an extension
> to the "--reverse" option?  I dunno.

I'm not sure about that. The way max_count actually interacts with
--reverse in the code is an implementation detail that the user doesn't
need to worry about. It should be fine to tell the user that this
feature is an extention of how --reverse behaves. Regarding "Commit
Limiting options", we already tell the user

	Note that these are applied before commit ordering and formatting
	options, such as --reverse.

so explaining to the user that this new feature acts on reverse's
behaviour might be easier (and not necessarily wrong on a conceptual
level). I find "you can choose to apply reverse before any commit
limiting option" easier to understand than "--max-count can be applied
last, or before reverse but after all other Commit Limiting options".

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

[Ben Knoble]

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

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

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

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

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

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

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

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

[Mirko Faina]

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

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

[D. Ben Knoble]

On Sun, Apr 19, 2026 at 2:11 PM Mirko Faina <mroik@delayed.space> wrote:
>
> On Sun, Apr 19, 2026 at 08:06:24AM -0400, Ben Knoble wrote:
> > 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.

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?

Granted, I don't see this ability documented, and I cannot tell how
many may scream if we change this behavior, so it's a bit
hypothetical. But there is an argument for backward compatibility as a
default, which I think we'd need to justify changing. Perhaps in the
proposed log message?

(The original seems nonsensical to type, but of course you can imagine
alias.A=log --reverse <other-stuff>, and then sometimes you want to do
"git A --reverse" to un-reverse the commits.)

> > >    } 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 don't have any strong opinions on 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.
>
> 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?

Yeah, a few `#define`d constants would make things more readable to
me, at least, since we can't use the enum without space concerns
(unless there's a way to bit-pack the enum to only 2 bits?).

> > >    if (revs->reverse_output_stage) {
> > > +        if (revs->reverse == 2 && revs->max_count == 0)
> > > +            return NULL;
> > > +

PS: something I spotted on a second read. [Ignoring reverse=after
mode] This hunk looks to me like a nice little optimization (return
nothing if we know max_count says we yield no commits). Of course, I
could see that being viable early in the function, right? When asking
get_revision for commits, if max_count is 0, just return NULL.

For reverse=after mode, this condition is only true if the max_count
was 0 in the previous conditional, also, since we use max_count=-1
before iterating get_revision_internal. That means the original
max_count isn't touched. At any rate, it _seems_ to me that the whole
function could benefit from this optimization… but I wonder if it is
_necessary_ for correctness of reverse=after in some way that I'm not
seeing? Since the current version doesn't need the early bailout, why
does reverse=after?

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

I don't 100% follow, but I'm out of my depth :)

I think I see that get_revision() effectively has 2 modes pertaining
to reverse: reverse and reverse output stage (the former falls
directly into the latter, though).

After some setup, the reverse mode calls get_revision_internal() as
you said. That decrements max_count as a way of counting how many
commits we've seen through the loop, so if we asked for 5 we'd only
process 5 commits.

Then we fall into the output stage mode, which pops a commit [1].

With this patch, in reverse=after we disable max_count in the first
(reverse) mode, as you said. Ok: we get the whole (filtered) history
then, at which point we can now shrink. That makes sense.

Then in the reverse output stage mode, we pretend to have one less
max_count. That's what I can't figure out. Is it because of the
pop_commit()? I guess I'm not totally seeing how that interacted with
the max_count in the original code: does the current code yield one
extra commit in get_revision_internal() ?

You wrote that "we never call get_revision_internal() again," but I
don't see why that's true with this patch and not true before it.

I do agree that _somebody_ has to handle max_count after
get_revision() returns with reverse=after. I'm just not sure what

    if (revs->reverse == 2)
        revs->max_count--;

is doing.

Of course if I'm the only one confused and others make sense of it,
that's ok, too.

> Thank you

Thanks!

-- 
D. Ben Knoble

[1]: I traced this to 498bcd3159 (rev-list: fix --reverse interaction
with --parents, 2008-08-29), but I can't fathom what the pop is doing
there.

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

[Mirko Faina]

On Sun, Apr 19, 2026 at 03:12:27PM -0400, D. Ben Knoble wrote:
> On Sun, Apr 19, 2026 at 2:11 PM Mirko Faina <mroik@delayed.space> wrote:
> >
> > On Sun, Apr 19, 2026 at 08:06:24AM -0400, Ben Knoble wrote:
> > > 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.
> 
> 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?

> Granted, I don't see this ability documented, and I cannot tell how
> many may scream if we change this behavior, so it's a bit
> hypothetical. But there is an argument for backward compatibility as a
> default, which I think we'd need to justify changing. Perhaps in the
> proposed log message?
> 
> (The original seems nonsensical to type, but of course you can imagine
> alias.A=log --reverse <other-stuff>, and then sometimes you want to do
> "git A --reverse" to un-reverse the commits.)

Will do in v2.

> > > 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?
> 
> Yeah, a few `#define`d constants would make things more readable to
> me, at least, since we can't use the enum without space concerns
> (unless there's a way to bit-pack the enum to only 2 bits?).

Will do in v2.

> > > >    if (revs->reverse_output_stage) {
> > > > +        if (revs->reverse == 2 && revs->max_count == 0)
> > > > +            return NULL;
> > > > +
> 
> PS: something I spotted on a second read. [Ignoring reverse=after
> mode] This hunk looks to me like a nice little optimization (return
> nothing if we know max_count says we yield no commits). Of course, I
> could see that being viable early in the function, right? When asking
> get_revision for commits, if max_count is 0, just return NULL.
> 
> For reverse=after mode, this condition is only true if the max_count
> was 0 in the previous conditional, also, since we use max_count=-1
> before iterating get_revision_internal. That means the original
> max_count isn't touched. At any rate, it _seems_ to me that the whole
> function could benefit from this optimization… but I wonder if it is
> _necessary_ for correctness of reverse=after in some way that I'm not
> seeing? Since the current version doesn't need the early bailout, why
> does reverse=after?

Just to clarify, "reverse = 2" is "--reverse=before" and not
"--reverse=after".

With "reverse = 2", the snippet of code you're referencing is not an
optimization but a requirement for correctness. With "reverse = 1" we
just keep the max_count as is and it's used by get_revision_internal()
to stop if that limit is reached. What we find in 'reversed' are already
just the commits we need to return.

With "reverse = 2", we first set max_count to -1 and then retrieve the
whole history, then we set max_count to its original value. Then we
return the commits on each call of get_revision(). Now, unlike with
"reverse = 1", we have the whole history in 'reversed', because of that
we need to know when to stop. That's the reason we decrement max_count
only for "reverse = 2" and why "max_count == 0" is checked only for
"reverse = 2".

> > > >        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.
> 
> I don't 100% follow, but I'm out of my depth :)
> 
> I think I see that get_revision() effectively has 2 modes pertaining
> to reverse: reverse and reverse output stage (the former falls
> directly into the latter, though).
> 
> After some setup, the reverse mode calls get_revision_internal() as
> you said. That decrements max_count as a way of counting how many
> commits we've seen through the loop, so if we asked for 5 we'd only
> process 5 commits.
> 
> Then we fall into the output stage mode, which pops a commit [1].
> 
> With this patch, in reverse=after we disable max_count in the first
> (reverse) mode, as you said. Ok: we get the whole (filtered) history
> then, at which point we can now shrink. That makes sense.
> 
> Then in the reverse output stage mode, we pretend to have one less
> max_count. That's what I can't figure out. Is it because of the
> pop_commit()? I guess I'm not totally seeing how that interacted with
> the max_count in the original code: does the current code yield one
> extra commit in get_revision_internal() ?

I'm not sure I understand what you're referencing with "Then in the
reverse output stage mode, we pretend to have one less max_count".

If you're referring to line 4573, then...

> You wrote that "we never call get_revision_internal() again," but I
> don't see why that's true with this patch and not true before it.
> 
> I do agree that _somebody_ has to handle max_count after
> get_revision() returns with reverse=after. I'm just not sure what
> 
>     if (revs->reverse == 2)
>         revs->max_count--;
> 
> is doing.

...we're not pretending we have fewer commits. Every subsequent call to
get_revision() after the first call will never enter the branch at line
4548 and will only enter the branch at 4568. Everytime we pop a commit
from 'reversed' we decrease max_count so we can limit only to the amount
of commits the user wants.

So, to recap, with "reverse = 2", on the first call to get_revision() we
walk the whole history and store it in 'reversed' in reversed order and
return the first commit.
On subsequent calls to get_revision() we do not walk the history again,
we simply return the commits that have been stored in 'reversed'.
Everytime we pop a commit we have to decrease max_count, and we check
againts max_count to know if we shouldn't return anymore commits (by
returning NULL).

> Of course if I'm the only one confused and others make sense of it,
> that's ok, too.

No, I completely understand. I did have to retouch the function a few
times after writing the tests :P

> [1]: I traced this to 498bcd3159 (rev-list: fix --reverse interaction
> with --parents, 2008-08-29), but I can't fathom what the pop is doing
> there.

It's pretty much doing the same thing it does now, it's returning stored
commits. In both versions, the initial setup when "revs->reverse" is
true, becomes "dead code" after the first call.

Thank you

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

[Jeff King]

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.

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.

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;

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

-Peff

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

[Mirko Faina]

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

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

[Mirko Faina]

On Mon, Apr 20, 2026 at 11:33:25AM +0200, Mirko Faina wrote:
> > 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.

Just realized it's a strcmp and not start_with, so this should work
fine.

Thank you

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

[Jeff King]

On Mon, Apr 20, 2026 at 11:33:25AM +0200, Mirko Faina wrote:

> > 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 :')

If "--reverse" means "reverse after", then:

  --reverse=before --reverse --reverse

is back to reversing after. You could also make it retain before/after
if you stored that as a separate bit. I.e.,:

  reverse=before:
    revs->reverse = 1;
    revs->reverse_when = REVERSE_BEFORE;
  reverse=after:
    revs->reverse = 1;
    revs->reverse_when = REVERSE_AFTER;
  reverse:
    revs->reverse ^= 1; /* flip reversing */
    /* do not touch reverse_when! */

And then the triple-reverse takes you back to reverse=before. I'm not
sure if that is more or less confusing, though. ;)

At any rate, I agree that the behavior should be mentioned in the docs,
especially since "--reverse" is not a true synonym for "--reverse=after"
because of the override vs negation behavior.

-Peff

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

[D. Ben Knoble]

On Sun, Apr 19, 2026 at 4:31 PM Mirko Faina <mroik@delayed.space> wrote:
> > > > >    if (revs->reverse_output_stage) {
> > > > > +        if (revs->reverse == 2 && revs->max_count == 0)
> > > > > +            return NULL;
> > > > > +
> >
> > PS: something I spotted on a second read. [Ignoring reverse=after
> > mode] This hunk looks to me like a nice little optimization (return
> > nothing if we know max_count says we yield no commits). Of course, I
> > could see that being viable early in the function, right? When asking
> > get_revision for commits, if max_count is 0, just return NULL.
> >
> > For reverse=after mode, this condition is only true if the max_count
> > was 0 in the previous conditional, also, since we use max_count=-1
> > before iterating get_revision_internal. That means the original
> > max_count isn't touched. At any rate, it _seems_ to me that the whole
> > function could benefit from this optimization… but I wonder if it is
> > _necessary_ for correctness of reverse=after in some way that I'm not
> > seeing? Since the current version doesn't need the early bailout, why
> > does reverse=after?
>
> Just to clarify, "reverse = 2" is "--reverse=before" and not
> "--reverse=after".

Oh golly, sorry about that!

> With "reverse = 2", the snippet of code you're referencing is not an
> optimization but a requirement for correctness. With "reverse = 1" we
> just keep the max_count as is and it's used by get_revision_internal()
> to stop if that limit is reached. What we find in 'reversed' are already
> just the commits we need to return.
>
> With "reverse = 2", we first set max_count to -1 and then retrieve the
> whole history, then we set max_count to its original value. Then we
> return the commits on each call of get_revision(). Now, unlike with
> "reverse = 1", we have the whole history in 'reversed', because of that
> we need to know when to stop. That's the reason we decrement max_count
> only for "reverse = 2" and why "max_count == 0" is checked only for
> "reverse = 2".

Ok, this explanation hasn't yet clicked…

> > > > >        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.
> >
> > I don't 100% follow, but I'm out of my depth :)
> >
> > I think I see that get_revision() effectively has 2 modes pertaining
> > to reverse: reverse and reverse output stage (the former falls
> > directly into the latter, though).
> >
> > After some setup, the reverse mode calls get_revision_internal() as
> > you said. That decrements max_count as a way of counting how many
> > commits we've seen through the loop, so if we asked for 5 we'd only
> > process 5 commits.
> >
> > Then we fall into the output stage mode, which pops a commit [1].
> >
> > With this patch, in reverse=after we disable max_count in the first
> > (reverse) mode, as you said. Ok: we get the whole (filtered) history
> > then, at which point we can now shrink. That makes sense.
> >
> > Then in the reverse output stage mode, we pretend to have one less
> > max_count. That's what I can't figure out. Is it because of the
> > pop_commit()? I guess I'm not totally seeing how that interacted with
> > the max_count in the original code: does the current code yield one
> > extra commit in get_revision_internal() ?
>
> I'm not sure I understand what you're referencing with "Then in the
> reverse output stage mode, we pretend to have one less max_count".
>
> If you're referring to line 4573, then...
>
> > You wrote that "we never call get_revision_internal() again," but I
> > don't see why that's true with this patch and not true before it.
> >
> > I do agree that _somebody_ has to handle max_count after
> > get_revision() returns with reverse=after. I'm just not sure what
> >
> >     if (revs->reverse == 2)
> >         revs->max_count--;
> >
> > is doing.
>
> ...we're not pretending we have fewer commits. Every subsequent call to
> get_revision() after the first call will never enter the branch at line
> 4548 and will only enter the branch at 4568. Everytime we pop a commit
> from 'reversed' we decrease max_count so we can limit only to the amount
> of commits the user wants.
>
> So, to recap, with "reverse = 2", on the first call to get_revision() we
> walk the whole history and store it in 'reversed' in reversed order and
> return the first commit.
> On subsequent calls to get_revision() we do not walk the history again,
> we simply return the commits that have been stored in 'reversed'.
> Everytime we pop a commit we have to decrease max_count, and we check
> againts max_count to know if we shouldn't return anymore commits (by
> returning NULL).

…but I think this one does. I think what I missed is that in all
"reverse" modes, get_revision() does some pre-computation and then
yields one at a time the commits. In traditional "after" mode, the
counting is done by get_revision_internal() [before reversal]. In the
new mode, get_revision takes on that responsibility of
get_revision_internal instead.

Hm. That suggests to me that get_revision's responsibilities are
becoming complex. Might be worth some version of a refactor, but idk
which.

> > Of course if I'm the only one confused and others make sense of it,
> > that's ok, too.
>
> No, I completely understand. I did have to retouch the function a few
> times after writing the tests :P
>
> > [1]: I traced this to 498bcd3159 (rev-list: fix --reverse interaction
> > with --parents, 2008-08-29), but I can't fathom what the pop is doing
> > there.
>
> It's pretty much doing the same thing it does now, it's returning stored
> commits. In both versions, the initial setup when "revs->reverse" is
> true, becomes "dead code" after the first call.

And this pop makes more sense now, too. Phew!
-- 
D. Ben Knoble

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

[Mirko Faina]

On Wed, Apr 22, 2026 at 02:24:48PM -0400, D. Ben Knoble wrote:
> > > > > >        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.
> > >
> > > I don't 100% follow, but I'm out of my depth :)
> > >
> > > I think I see that get_revision() effectively has 2 modes pertaining
> > > to reverse: reverse and reverse output stage (the former falls
> > > directly into the latter, though).
> > >
> > > After some setup, the reverse mode calls get_revision_internal() as
> > > you said. That decrements max_count as a way of counting how many
> > > commits we've seen through the loop, so if we asked for 5 we'd only
> > > process 5 commits.
> > >
> > > Then we fall into the output stage mode, which pops a commit [1].
> > >
> > > With this patch, in reverse=after we disable max_count in the first
> > > (reverse) mode, as you said. Ok: we get the whole (filtered) history
> > > then, at which point we can now shrink. That makes sense.
> > >
> > > Then in the reverse output stage mode, we pretend to have one less
> > > max_count. That's what I can't figure out. Is it because of the
> > > pop_commit()? I guess I'm not totally seeing how that interacted with
> > > the max_count in the original code: does the current code yield one
> > > extra commit in get_revision_internal() ?
> >
> > I'm not sure I understand what you're referencing with "Then in the
> > reverse output stage mode, we pretend to have one less max_count".
> >
> > If you're referring to line 4573, then...
> >
> > > You wrote that "we never call get_revision_internal() again," but I
> > > don't see why that's true with this patch and not true before it.
> > >
> > > I do agree that _somebody_ has to handle max_count after
> > > get_revision() returns with reverse=after. I'm just not sure what
> > >
> > >     if (revs->reverse == 2)
> > >         revs->max_count--;
> > >
> > > is doing.
> >
> > ...we're not pretending we have fewer commits. Every subsequent call to
> > get_revision() after the first call will never enter the branch at line
> > 4548 and will only enter the branch at 4568. Everytime we pop a commit
> > from 'reversed' we decrease max_count so we can limit only to the amount
> > of commits the user wants.
> >
> > So, to recap, with "reverse = 2", on the first call to get_revision() we
> > walk the whole history and store it in 'reversed' in reversed order and
> > return the first commit.
> > On subsequent calls to get_revision() we do not walk the history again,
> > we simply return the commits that have been stored in 'reversed'.
> > Everytime we pop a commit we have to decrease max_count, and we check
> > againts max_count to know if we shouldn't return anymore commits (by
> > returning NULL).
> 
> …but I think this one does. I think what I missed is that in all
> "reverse" modes, get_revision() does some pre-computation and then
> yields one at a time the commits. In traditional "after" mode, the
> counting is done by get_revision_internal() [before reversal]. In the
> new mode, get_revision takes on that responsibility of
> get_revision_internal instead.
> 
> Hm. That suggests to me that get_revision's responsibilities are
> becoming complex. Might be worth some version of a refactor, but idk
> which.

If the refactor is to yield the responsibility of max_count to
get_revision_internal() for the new reverse mode too, then I'm not sure
if we want that. To maintain state across calls to
get_revision_internal() we'd have to store the temporary max_count that
currently lives in get_revision() in rev_info. I don't want to add
unnecessary new fields to the struct when its space is so valuable
(afterall we're using bitpacking).

In the current implementation all of the reversal happens in the same
call to get_revision(), so state is preserved across multiple
get_revision_internal() calls through the temporary int max_count.

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

[Jeff King]

On Sat, Apr 18, 2026 at 06:47:35PM +0200, Mirko Faina wrote:

> @@ -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];
> +			}
> +		}

It looks like you're trying to support "--reverse after" here, but don't
do that. Flags with optional arguments must use the "stuck" form,
"--reverse=after", which is covered in the "gitcli" manpage.

That's to prevent "--reverse --foo" from being ambiguous. It looks like
you try to limit that with the final "else" here:

> +
> +		if (!strcmp(optarg, "after")) {
> +			revs->reverse = 1;
> +		} else if (!strcmp(optarg, "before")) {
> +			revs->reverse = 2;
> +		} else {
> +			revs->reverse = 1;
> +			return 1;
> +		}

but that just makes things more complicated:

  - doing "git log --reverse=bogus" is silently accepted

  - trying to show a branch named "after" with "git log --reverse after"
    has changed meanings

So I think you really just want to handle "--reverse=" separately from
"--reverse", and the latter should behave as it always has.

-Peff

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

[Mirko Faina]

On Sun, Apr 19, 2026 at 08:04:40PM -0400, Jeff King wrote:
> On Sat, Apr 18, 2026 at 06:47:35PM +0200, Mirko Faina wrote:
> 
> > @@ -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];
> > +			}
> > +		}
> 
> It looks like you're trying to support "--reverse after" here, but don't
> do that. Flags with optional arguments must use the "stuck" form,
> "--reverse=after", which is covered in the "gitcli" manpage.

Oh I see, I thought I had to parse both ways. If I just have to allow
for the stuck form then it becomes way easier to deal with.

> That's to prevent "--reverse --foo" from being ambiguous. It looks like
> you try to limit that with the final "else" here:
> 
> > +
> > +		if (!strcmp(optarg, "after")) {
> > +			revs->reverse = 1;
> > +		} else if (!strcmp(optarg, "before")) {
> > +			revs->reverse = 2;
> > +		} else {
> > +			revs->reverse = 1;
> > +			return 1;
> > +		}
> 
> but that just makes things more complicated:
> 
>   - doing "git log --reverse=bogus" is silently accepted
> 
>   - trying to show a branch named "after" with "git log --reverse after"
>     has changed meanings
> 
> So I think you really just want to handle "--reverse=" separately from
> "--reverse", and the latter should behave as it always has.
> 
> -Peff

Thanks you

[PATCH v2 0/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

Since v1 I've:
	* removed the non stuck form for the --reverse option
	* --reverse with no argument now flips "no reverse -> reverse
	  after", "reverse after -> no reverse" and "reverse before ->
	  no reverse"
	* implemented a window to reduce memory usage when
	  --reverse=before with --max-count=<n>
	* updated the docs to highlight the peculiarities of --reverse
	  when it's specified multiple times

[1/2] revision.c: implement --reverse=before for walks (Mirko Faina)
[2/2] revision.c: reduce memory usage on reverse before (Mirko Faina)

 Documentation/rev-list-options.adoc | 14 +++--
 revision.c                          | 85 +++++++++++++++++++++++++++--
 revision.h                          |  8 ++-
 t/t4202-log.sh                      | 66 ++++++++++++++++++++++
 4 files changed, 163 insertions(+), 10 deletions(-)


base-commit: e8955061076952cc5eab0300424fc48b601fe12d
-- 
2.54.0

Re: [PATCH v2 0/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

Sorry, I forgot to thread it

[PATCH v3 0/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

I've fixed the docs with the suggested changes by Jeff and applied some
styling fixes.

[1/2] revision.c: implement --reverse=before for walks (Mirko Faina)
[2/2] revision.c: reduce memory usage on reverse before (Mirko Faina)

 Documentation/rev-list-options.adoc | 16 +++++--
 revision.c                          | 73 +++++++++++++++++++++++++++--
 revision.h                          |  8 +++-
 t/t4202-log.sh                      | 66 ++++++++++++++++++++++++++
 4 files changed, 153 insertions(+), 10 deletions(-)

Range-diff against v2:
1:  599a247d82 ! 1:  4864ac46dd revision.c: implement --reverse=before for walks
    @@ Documentation/rev-list-options.adoc: With `--topo-order`, they would show 8 6 5
     +	`--walk-reflogs`. If `after`, output the commits chosen to be
     +	shown (see 'Commit Limiting' section above) in reverse order. If
     +	`before`, reverse the commits before filtering with `Commit
    -+	Limiting` options. This option can be used multiple times, last
    -+	one is applied. When the argument for `--reverse` is omitted, if
    -+	the current state is in no reverse, it defaults to `after`. If
    -+	it is in any reversed state, it restores the original ordering
    -+	by removing the reverse state.
    ++	Limiting` options. When multiple `--reverse=` options are given,
    ++	the final option overrides any previous options. The `--reverse`
    ++	option (with no specifier) behaves as `--reverse=after`, except
    ++	that, for historical reasons, it negates any previous reversed
    ++	state (so `--reverse --reverse` does nothing, nor does
    ++	`--reverse=before --reverse`. Note that `--reverse=before
    ++	--reverse --reverse` is the same as `--reverse=after`).
      endif::git-shortlog[]
      
      ifndef::git-shortlog[]
2:  480b322cf8 ! 2:  00489b0e52 revision.c: reduce memory usage on reverse before
    @@ revision.c: static struct commit *get_revision_internal(struct rev_info *revs)
      }
      
     +static void retrieve_with_window(struct rev_info *revs, int max_count,
    -+			  struct commit_list **reversed)
    ++			  	 struct commit_list **reversed)
     +{
     +	struct commit *c;
     +	struct commit_list *into_queue = NULL;
    @@ revision.c: static struct commit *get_revision_internal(struct rev_info *revs)
     +		}
     +	}
     +
    -+	while (outo_count) {
    -+		c = pop_commit(&outo_queue);
    -+		outo_count--;
    ++	while ((c = pop_commit(&outo_queue)))
     +		commit_list_insert(c, reversed);
    -+	}
    -+
    -+	while (into_count) {
    -+		c = pop_commit(&into_queue);
    -+		into_count--;
    ++	while ((c = pop_commit(&into_queue)))
     +		commit_list_insert(c, &outo_queue);
    -+		outo_count++;
    -+	}
    -+
    -+	while (outo_count) {
    -+		c = pop_commit(&outo_queue);
    -+		outo_count--;
    ++	while ((c = pop_commit(&outo_queue)))
     +		commit_list_insert(c, reversed);
    -+	}
     +}
     +
      struct commit *get_revision(struct rev_info *revs)
-- 
2.54.0

[PATCH v3 1/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

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 | 16 +++++--
 revision.c                          | 31 ++++++++++++--
 revision.h                          |  8 +++-
 t/t4202-log.sh                      | 66 +++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..e97f6f2aff 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -914,10 +914,18 @@ 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`::
-	Output the commits chosen to be shown (see 'Commit Limiting'
-	section above) in reverse order. Cannot be combined with
-	`--walk-reflogs`.
+`--[no-]reverse[=(after|before)]`::
+	Accepts `after` or `before`. Cannot be combined with
+	`--walk-reflogs`. If `after`, output the commits chosen to be
+	shown (see 'Commit Limiting' section above) in reverse order. If
+	`before`, reverse the commits before filtering with `Commit
+	Limiting` options. When multiple `--reverse=` options are given,
+	the final option overrides any previous options. The `--reverse`
+	option (with no specifier) behaves as `--reverse=after`, except
+	that, for historical reasons, it negates any previous reversed
+	state (so `--reverse --reverse` does nothing, nor does
+	`--reverse=before --reverse`. Note that `--reverse=before
+	--reverse --reverse` is the same as `--reverse=after`).
 endif::git-shortlog[]
 
 ifndef::git-shortlog[]
diff --git a/revision.c b/revision.c
index 599b3a66c3..d581f5e38e 100644
--- a/revision.c
+++ b/revision.c
@@ -2686,7 +2686,16 @@ 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;
+		revs->reverse = !revs->reverse;
+	} else if (skip_prefix(arg, "--reverse=", &optarg)) {
+		if (!strcmp(optarg, "after"))
+			revs->reverse = REVERSE_AFTER;
+		else if(!strcmp(optarg, "before"))
+			revs->reverse = REVERSE_BEFORE;
+		else
+			die(_("unknown value for --reverse: %s"), optarg);
+	} else if (!strcmp(arg, "--no-reverse")) {
+		revs->reverse = NO_REVERSE;
 	} else if (!strcmp(arg, "--children")) {
 		revs->children.name = "children";
 		revs->limited = 1;
@@ -4525,19 +4534,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 == REVERSE_BEFORE)
+			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 == REVERSE_BEFORE)
+			revs->max_count = max_count;
 	}
 
 	if (revs->reverse_output_stage) {
+		if (revs->reverse == REVERSE_BEFORE && revs->max_count == 0)
+			return NULL;
+
 		c = pop_commit(&revs->commits);
+		if (revs->reverse == REVERSE_BEFORE)
+			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..02881577dc 100644
--- a/revision.h
+++ b/revision.h
@@ -121,6 +121,12 @@ struct ref_exclusions {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_reverse {
+	NO_REVERSE = 0,
+	REVERSE_AFTER = 1,
+	REVERSE_BEFORE = 2,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -167,6 +173,7 @@ struct rev_info {
 			ignore_missing_links:1;
 
 	/* Traversal flags */
+	enum rev_reverse reverse:2;
 	unsigned int	dense:1,
 			prune:1,
 			no_walk:1,
@@ -196,7 +203,6 @@ struct rev_info {
 			rewrite_parents:1,
 			print_parents:1,
 			show_decorations:1,
-			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
 			cherry_mark:1,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..3bfe2c99b8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,72 @@ 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 --reverse --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 --reverse --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
+'
+
+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
+'
+
+cat >expect <<-\EOF
+046b221 to remove
+c3f451c Merge tag 'reach'
+EOF
+
+test_expect_success 'log --reverse --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 --reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --reverse --no-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 --no-reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0

Re: [PATCH v3 1/2] revision.c: implement --reverse=before for walks

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 05cee9e41b..3bfe2c99b8 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh

The hardcoded short object names are setting up traps to fail when 

    $ GIT_TEST_DEFAULT_HASH=sha256 make test

is run.  It also may break when the default abbreviation length
and other things change.

> @@ -1882,6 +1882,72 @@ 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 --reverse --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 --reverse --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
> +'
> +
> +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
> +'
> +
> +cat >expect <<-\EOF
> +046b221 to remove
> +c3f451c Merge tag 'reach'
> +EOF
> +
> +test_expect_success 'log --reverse --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 --reverse --oneline --max-count=2 >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --reverse --no-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 --no-reverse --oneline --max-count=2 >actual &&
> +	test_cmp expect actual
> +'
> +
>  cat >expect <<-\EOF
>  * reach
>  |

[PATCH v3 2/2] revision.c: reduce memory usage on reverse before

[Mirko Faina]

Due to the nature of --reverse=before we have to walk all of the history
and store each non-filtered processed commit, this can be expensive on
memory for projects with a long history. When --max-count is being used
we don't really have to keep every processed commit, we can discard
older commits (as in have been processed before than the ones we're now
considering, from a chronological commit order they are the newer
commits) as we surpass the --max-count limit.

Teach get_revision() to keep only the newer commits as we walk a
revision with --reverse=before and --max-count=<k>. We do this through a
simple queue. With N nodes and K as the --max-count argument, assuming K
< N, we go from a space complexity of O(N) to O(K). When it comes down
to time complexity, the queue has an ammortized time of O(1) for pops,
so the complexity remains O(N).

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 revision.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index d581f5e38e..03acff9bac 100644
--- a/revision.c
+++ b/revision.c
@@ -4530,6 +4530,40 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	return c;
 }
 
+static void retrieve_with_window(struct rev_info *revs, int max_count,
+			  	 struct commit_list **reversed)
+{
+	struct commit *c;
+	struct commit_list *into_queue = NULL;
+	struct commit_list *outo_queue = NULL;
+	int into_count = 0;
+	int outo_count = 0;
+
+	while ((c = get_revision_internal(revs))) {
+		commit_list_insert(c, &into_queue);
+		into_count++;
+		if (into_count + outo_count > max_count) {
+			if (!outo_count) {
+				while (into_count) {
+					c = pop_commit(&into_queue);
+					into_count--;
+					commit_list_insert(c, &outo_queue);
+					outo_count++;
+				}
+			}
+			pop_commit(&outo_queue);
+			outo_count--;
+		}
+	}
+
+	while ((c = pop_commit(&outo_queue)))
+		commit_list_insert(c, reversed);
+	while ((c = pop_commit(&into_queue)))
+		commit_list_insert(c, &outo_queue);
+	while ((c = pop_commit(&outo_queue)))
+		commit_list_insert(c, reversed);
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c;
@@ -4546,8 +4580,12 @@ struct commit *get_revision(struct rev_info *revs)
 			revs->max_count = -1;
 
 		reversed = NULL;
-		while ((c = get_revision_internal(revs)))
-			commit_list_insert(c, &reversed);
+		if (revs->reverse == REVERSE_BEFORE && max_count != -1) {
+			retrieve_with_window(revs, max_count, &reversed);
+		} else {
+			while ((c = get_revision_internal(revs)))
+				commit_list_insert(c, &reversed);
+		}
 		commit_list_free(revs->commits);
 		revs->commits = reversed;
 		revs->reverse_output_stage = 1;
-- 
2.54.0

[PATCH v4 0/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

v4 aligns the behaviour for --reverse=before and --reverse=after on
negative numbers that are less than -1 to treat them the same as -1,
which is current behaviour (before this series).

[1/2] revision.c: implement --reverse=before for walks (Mirko Faina)
[2/2] revision.c: reduce memory usage on reverse before (Mirko Faina)

 Documentation/rev-list-options.adoc | 16 +++++--
 revision.c                          | 73 +++++++++++++++++++++++++++--
 revision.h                          |  8 +++-
 t/t4202-log.sh                      | 66 ++++++++++++++++++++++++++
 4 files changed, 153 insertions(+), 10 deletions(-)

Range-diff against v3:
1:  4864ac46dd = 1:  4864ac46dd revision.c: implement --reverse=before for walks
2:  00489b0e52 ! 2:  7c0bab5d14 revision.c: reduce memory usage on reverse before
    @@ Commit message
         revision with --reverse=before and --max-count=<k>. We do this through a
         simple queue. With N nodes and K as the --max-count argument, assuming K
         < N, we go from a space complexity of O(N) to O(K). When it comes down
    -    to time complexity, the queue has an ammortized time of O(1) for pops,
    -    so the complexity remains O(N).
    +    to time complexity, the queue has an amortized time of O(1) for pops, so
    +    the complexity remains O(N).
     
         Signed-off-by: Mirko Faina <mroik@delayed.space>
     
    @@ revision.c: struct commit *get_revision(struct rev_info *revs)
      		reversed = NULL;
     -		while ((c = get_revision_internal(revs)))
     -			commit_list_insert(c, &reversed);
    -+		if (revs->reverse == REVERSE_BEFORE && max_count != -1) {
    ++		if (revs->reverse == REVERSE_BEFORE && max_count >= 0) {
     +			retrieve_with_window(revs, max_count, &reversed);
     +		} else {
     +			while ((c = get_revision_internal(revs)))

base-commit: e8955061076952cc5eab0300424fc48b601fe12d
-- 
2.54.0

[PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

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 | 16 +++++--
 revision.c                          | 31 ++++++++++++--
 revision.h                          |  8 +++-
 t/t4202-log.sh                      | 66 +++++++++++++++++++++++++++++
 4 files changed, 113 insertions(+), 8 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..e97f6f2aff 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -914,10 +914,18 @@ 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`::
-	Output the commits chosen to be shown (see 'Commit Limiting'
-	section above) in reverse order. Cannot be combined with
-	`--walk-reflogs`.
+`--[no-]reverse[=(after|before)]`::
+	Accepts `after` or `before`. Cannot be combined with
+	`--walk-reflogs`. If `after`, output the commits chosen to be
+	shown (see 'Commit Limiting' section above) in reverse order. If
+	`before`, reverse the commits before filtering with `Commit
+	Limiting` options. When multiple `--reverse=` options are given,
+	the final option overrides any previous options. The `--reverse`
+	option (with no specifier) behaves as `--reverse=after`, except
+	that, for historical reasons, it negates any previous reversed
+	state (so `--reverse --reverse` does nothing, nor does
+	`--reverse=before --reverse`. Note that `--reverse=before
+	--reverse --reverse` is the same as `--reverse=after`).
 endif::git-shortlog[]
 
 ifndef::git-shortlog[]
diff --git a/revision.c b/revision.c
index 599b3a66c3..d581f5e38e 100644
--- a/revision.c
+++ b/revision.c
@@ -2686,7 +2686,16 @@ 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;
+		revs->reverse = !revs->reverse;
+	} else if (skip_prefix(arg, "--reverse=", &optarg)) {
+		if (!strcmp(optarg, "after"))
+			revs->reverse = REVERSE_AFTER;
+		else if(!strcmp(optarg, "before"))
+			revs->reverse = REVERSE_BEFORE;
+		else
+			die(_("unknown value for --reverse: %s"), optarg);
+	} else if (!strcmp(arg, "--no-reverse")) {
+		revs->reverse = NO_REVERSE;
 	} else if (!strcmp(arg, "--children")) {
 		revs->children.name = "children";
 		revs->limited = 1;
@@ -4525,19 +4534,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 == REVERSE_BEFORE)
+			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 == REVERSE_BEFORE)
+			revs->max_count = max_count;
 	}
 
 	if (revs->reverse_output_stage) {
+		if (revs->reverse == REVERSE_BEFORE && revs->max_count == 0)
+			return NULL;
+
 		c = pop_commit(&revs->commits);
+		if (revs->reverse == REVERSE_BEFORE)
+			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..02881577dc 100644
--- a/revision.h
+++ b/revision.h
@@ -121,6 +121,12 @@ struct ref_exclusions {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_reverse {
+	NO_REVERSE = 0,
+	REVERSE_AFTER = 1,
+	REVERSE_BEFORE = 2,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -167,6 +173,7 @@ struct rev_info {
 			ignore_missing_links:1;
 
 	/* Traversal flags */
+	enum rev_reverse reverse:2;
 	unsigned int	dense:1,
 			prune:1,
 			no_walk:1,
@@ -196,7 +203,6 @@ struct rev_info {
 			rewrite_parents:1,
 			print_parents:1,
 			show_decorations:1,
-			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
 			cherry_mark:1,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..3bfe2c99b8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,72 @@ 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 --reverse --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 --reverse --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
+'
+
+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
+'
+
+cat >expect <<-\EOF
+046b221 to remove
+c3f451c Merge tag 'reach'
+EOF
+
+test_expect_success 'log --reverse --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 --reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --reverse --no-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 --no-reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0

Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> 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

Can we rephrase "from the tail" somehow to reduce ambiguity?

Normally we generate a list of commits from newer to older, and you
are saying that it is not possible to take the oldest three commits
and show them from older to newer (i.e., in reverse).  But that, to
some readers, is showing commits from the beginning end, not from
the tail end.

Perhaps "... limited number of oldest commits impossible"?

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

I think "after" and "before" comes from "Do other things (including
count limiting) and then apply reverse after all that" and would be
very much understandable to those who know how the machinery works,
but should mere mortals need to know the machinery only to use "git
log"?

To put it another way, do you tnink experienced Git users who
haven't seen the actual implementation of revision traversal can
immediately answer this question:

    Now we have --reverse=after and --reverse=before to let you take
    a limited history from both ends when used with --max-count.
    Which between after and before do you think corresponds to the
    traditional --reverse that allowed you to only see the newest
    part of the history?

I doubt that the population to answer correctly would not exceed a
half by large margin (if it is 50% then it means nobody understood
the difference correctly and they just flipped a coin).

I wonder --reverse=oldest and --reverse=newest is easier to teach
and explain?  I dunno.

Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Johannes Sixt]

Am 27.04.26 um 08:45 schrieb Junio C Hamano:
> I think "after" and "before" comes from "Do other things (including
> count limiting) and then apply reverse after all that" and would be
> very much understandable to those who know how the machinery works,
> but should mere mortals need to know the machinery only to use "git
> log"?

I fully share your sentiments regarding "after" and "before" being too
much tied to the machinery, but...

> I wonder --reverse=oldest and --reverse=newest is easier to teach
> and explain?  I dunno.
What does it mean to "revert the oldest"? Or "the newest"? If at all,
then this "newest" and "oldest" must be a restriction that applies to
--max-count in some way. Perhaps we need a --max-count-oldest option,
then --reverse does not have to be touched at all, because it is still
applied only after the set of commits to show has been determined.

-- Hannes

Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Junio C Hamano]

Johannes Sixt <j6t@kdbg.org> writes:

>> I wonder --reverse=oldest and --reverse=newest is easier to teach
>> and explain?  I dunno.

> What does it mean to "revert the oldest"? Or "the newest"?

I do not quite understand where the "revert" comes from, though.

> If at all,
> then this "newest" and "oldest" must be a restriction that applies to
> --max-count in some way. Perhaps we need a --max-count-oldest option,
> then --reverse does not have to be touched at all, because it is still
> applied only after the set of commits to show has been determined.

That makes two of us to suspect that this is more about --max-count
than --reverse.

cf. https://lore.kernel.org/git/xmqqv7dlr4yz.fsf@gitster.g/

"git log --max-count-oldest=3" will give us three oldest commit in
reverse chronological order, the set of commits shown are the same
with or without "--reverse", which makes tons of sense.

Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Chris Torek]

This topic has been rattling around in my head for a while, and I need
to get it out now. :-)

First, a few notes:

 * I'm going to delete most of the context because I want to go back
to first principles here. Instead, I'll list what I think are the real
issues.

 * I have always had a suspicion that `--max-count` / `-n` was "done
wrong" in the first place, it's just that it's generally invisibly
wrong.

 * While all the revision-walking machinery normally shows commits
"newest first", there are several fundamental issues with defining
"newest" anyway. A lot of Git newcomers find this terribly confusing
-- usually a few weeks or months into use of Git, really.

It's important to note that the rev-walk machinery uses a priority
queue, and that this is necessary because commits are in a directed
graph, which cannot be presented linearly unless you're willing to:
(a) add additional information (graph drawing, parent list, whatever),
or (b) discard information. The `git log` and `git rev-list`
documentation skimp a bit on this.

There is of course nothing wrong with discarding information when it's
irrelevant. In fact, that's the whole point of abstraction, to toss
out irrelevancies so that one can concentrate only on the relevant.
And that's what all the limiting options for revision walking are for!

Sorting options affect the order in which items go into the priority
queue. Limiting options affect which items go in, and sometimes, how
many items come out. Display options affect what we see when the items
come out, and this includes the sorting options since they come out in
the order they're in there.

Thus, `--reverse` is a *display* option (part of sorting), while
`--max-count` is a *limiting* option. It's just that, well, there's a
special case when they're combined.

Junio noted:

> That makes two of us to suspect that this is more about --max-count than --reverse.

And that's really the case here. Because `--max-count` was "done
wrong" initially, we have a slight problem. Had it been done as a
"window of items in the priority queue", we would always have gotten
the limited-to-N items remaining in the queue after the selection
process, displayed according to the display process. But when the
display is going to be "in the order of items in the queue" and the
limiting count is N items *and* the display doesn't reverse the queue,
it suffices to display the first N items and then quit entirely. This
is of course a nice space-and-time optimization.

As it turns out, the only display option that causes this optimization
to be invalid is (or might be) `--reverse`.

Unfortunately, fixing the problem by simply defeating the "keep N
items in the queue and only stop early (and maybe display as we go as
well) if we're allowed" optimization -- the one that was applied
prematurely, as it were -- will change the existing behavior of `git
log -n 10 --reverse` in any repository with more than 10 commits in
it. Had the over-optimization not been done, and someone wanted to add
a "gather only N into the queue and then stop traversing, and then
display" option, we could perhaps use `--stop-walk-(after|at)=n` as a
new option.

As far as I can tell, the gripe that this exposes the priority queue
mechanism is valid, but at best trivial, because knowing about the
priority queue is crucial anyway. Beginners can skip it for a little
while, but as soon as they find out that commits have two separate
date stamps, and learn about `--date-order`, `--author-date-order`,
and `--topo-order`, they need to learn about the queue.

If it's deemed acceptable to change the historic behavior of
`--max-count` combined with `--reverse`, I'd suggest simply adding
`--stop-walk-after` (perhaps with a slightly different name) to take
over the historic behavior of `--max-count`, and make `--max-count`
not over-optimize. If not, I'd suggest a new option, with a note in
the documentation that `--max-count` has this odd behavior when
combined with `--reverse`. Perhaps the new option could be called
`--prio-queue-size=n`. The implementation can still optimize this
(using the same code as before) when `--reverse` isn't in effect,
since the effect is only visible with `--reverse`.

Chris

Re: [PATCH v4 1/2] revision.c: implement -b-reverse=before for walks

[Mirko Faina]

On Mon, Apr 27, 2026 at 03:45:34PM +0900, Junio C Hamano 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
> 
> Can we rephrase "from the tail" somehow to reduce ambiguity?
> 
> Normally we generate a list of commits from newer to older, and you
> are saying that it is not possible to take the oldest three commits
> and show them from older to newer (i.e., in reverse).  But that, to
> some readers, is showing commits from the beginning end, not from
> the tail end.
> 
> Perhaps "... limited number of oldest commits impossible"?

Yes, will rephrase.

> > 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.
> 
> I think "after" and "before" comes from "Do other things (including
> count limiting) and then apply reverse after all that" and would be
> very much understandable to those who know how the machinery works,
> but should mere mortals need to know the machinery only to use "git
> log"?

No, they shouldn't, but...

> To put it another way, do you tnink experienced Git users who
> haven't seen the actual implementation of revision traversal can
> immediately answer this question:
> 
>     Now we have --reverse=after and --reverse=before to let you take
>     a limited history from both ends when used with --max-count.
>     Which between after and before do you think corresponds to the
>     traditional --reverse that allowed you to only see the newest
>     part of the history?

...while users might not have read the implementation (and they
shouldn't need to to use log) the order in which the class of options is
applied is already documented in the man pages, so having that knowledge
after and before do make sense despite not having seen the
implementation.

But...

> I doubt that the population to answer correctly would not exceed a
> half by large margin (if it is 50% then it means nobody understood
> the difference correctly and they just flipped a coin).
> 
> I wonder --reverse=oldest and --reverse=newest is easier to teach
> and explain?  I dunno.

...you're right, it is not immediately apparent, but neither are oldest
and newest. Unfortunately I don't think there's any name we can choose
that would make it so without having to read the caveats in the man
pages.

Since many have expressed that this issue is not really about reverse
but about max-count, like you initially assesed, then we should move
towards making changes to the max-count option.

The proposed --max-count-oldest doesn't seem right to me as
max-count-oldest is not about keeping the oldest commits (even tho
that's what we want to achieve when we interact with reverse) but about
when we want to apply max-count. Since [1],

> It might be that the right way to look at this new feature is not that
> "we are changing where reverse is applied", but "count limit is applied
> much later than usual"

maybe --max-count-later as in max count is being applied later than
usual? (either way the users will still need to reach for the man pages
for clarifications).

[1] https://lore.kernel.org/git/xmqqv7dlr4yz.fsf@gitster.g/

Re: [PATCH v4 1/2] revision.c: implement -b-reverse=before for walks

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

>> It might be that the right way to look at this new feature is not that
>> "we are changing where reverse is applied", but "count limit is applied
>> much later than usual"
>
> maybe --max-count-later as in max count is being applied later than
> usual? (either way the users will still need to reach for the man pages
> for clarifications).

I very much more prefer what J6t suggested, which (if I am
understanding him correctly) would make

    git log --max-count-oldest=3 $options

conceptually run "git log" (without count limit but with other
options like --grep, --author, --since, etc. applied) without
showing anything until the last three commits remains, and then
shows these last three commits.

    git log --max-count-oldest=3 --reverse $options

would show these same last three commits, but in the order opposite
to the first one.

Re: [PATCH v4 1/2] revision.c: implement --reverse=before for walks

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> +	} else if (skip_prefix(arg, "--reverse=", &optarg)) {
> +		if (!strcmp(optarg, "after"))
> +			revs->reverse = REVERSE_AFTER;
> +		else if(!strcmp(optarg, "before"))

Style: missing SP after "else if".

[PATCH v4 2/2] revision.c: reduce memory usage on reverse before

[Mirko Faina]

Due to the nature of --reverse=before we have to walk all of the history
and store each non-filtered processed commit, this can be expensive on
memory for projects with a long history. When --max-count is being used
we don't really have to keep every processed commit, we can discard
older commits (as in have been processed before than the ones we're now
considering, from a chronological commit order they are the newer
commits) as we surpass the --max-count limit.

Teach get_revision() to keep only the newer commits as we walk a
revision with --reverse=before and --max-count=<k>. We do this through a
simple queue. With N nodes and K as the --max-count argument, assuming K
< N, we go from a space complexity of O(N) to O(K). When it comes down
to time complexity, the queue has an amortized time of O(1) for pops, so
the complexity remains O(N).

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 revision.c | 42 ++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 40 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index d581f5e38e..41c3d185c5 100644
--- a/revision.c
+++ b/revision.c
@@ -4530,6 +4530,40 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	return c;
 }
 
+static void retrieve_with_window(struct rev_info *revs, int max_count,
+			  	 struct commit_list **reversed)
+{
+	struct commit *c;
+	struct commit_list *into_queue = NULL;
+	struct commit_list *outo_queue = NULL;
+	int into_count = 0;
+	int outo_count = 0;
+
+	while ((c = get_revision_internal(revs))) {
+		commit_list_insert(c, &into_queue);
+		into_count++;
+		if (into_count + outo_count > max_count) {
+			if (!outo_count) {
+				while (into_count) {
+					c = pop_commit(&into_queue);
+					into_count--;
+					commit_list_insert(c, &outo_queue);
+					outo_count++;
+				}
+			}
+			pop_commit(&outo_queue);
+			outo_count--;
+		}
+	}
+
+	while ((c = pop_commit(&outo_queue)))
+		commit_list_insert(c, reversed);
+	while ((c = pop_commit(&into_queue)))
+		commit_list_insert(c, &outo_queue);
+	while ((c = pop_commit(&outo_queue)))
+		commit_list_insert(c, reversed);
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c;
@@ -4546,8 +4580,12 @@ struct commit *get_revision(struct rev_info *revs)
 			revs->max_count = -1;
 
 		reversed = NULL;
-		while ((c = get_revision_internal(revs)))
-			commit_list_insert(c, &reversed);
+		if (revs->reverse == REVERSE_BEFORE && max_count >= 0) {
+			retrieve_with_window(revs, max_count, &reversed);
+		} else {
+			while ((c = get_revision_internal(revs)))
+				commit_list_insert(c, &reversed);
+		}
 		commit_list_free(revs->commits);
 		revs->commits = reversed;
 		revs->reverse_output_stage = 1;
-- 
2.54.0

Re: [PATCH v4 2/2] revision.c: reduce memory usage on reverse before

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

>  		reversed = NULL;
> -		while ((c = get_revision_internal(revs)))
> -			commit_list_insert(c, &reversed);
> +		if (revs->reverse == REVERSE_BEFORE && max_count >= 0) {
> +			retrieve_with_window(revs, max_count, &reversed);
> +		} else {
> +			while ((c = get_revision_internal(revs)))
> +				commit_list_insert(c, &reversed);
> +		}

Style: needless {} around single-statement body of if/else.

>  		commit_list_free(revs->commits);
>  		revs->commits = reversed;
>  		revs->reverse_output_stage = 1;

[PATCH v5] revision.c: implement --max-count-oldest

[Mirko Faina]

--max-count is a commit limiting option sets a maximum amount of commits
to be shown. If a user wants to see only the first N commits of the
history (the oldest commits) they'd have to combine --max-count with
--skip. This is not very user-friendly.

Teach get_revision() the --max-count-oldest option.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 Documentation/rev-list-options.adoc |  3 ++
 revision.c                          | 77 +++++++++++++++++++++++++++--
 revision.h                          |  2 +
 t/t4202-log.sh                      | 14 ++++++
 4 files changed, 93 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..736f34efab 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -18,6 +18,9 @@ ordering and formatting options, such as `--reverse`.
 `--max-count=<number>`::
 	Limit the output to _<number>_ commits.
 
+`--max-count-oldest=<number>`::
+	Limit the output to the _<number>_ oldest commits.
+
 `--skip=<number>`::
 	Skip _<number>_ commits before starting to show the commit output.
 
diff --git a/revision.c b/revision.c
index 599b3a66c3..3aaa77ced5 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 	}
 
 	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+		if (revs->max_count_type == 1)
+			die(_("can't use --max-count with --max-count-oldest"));
 		revs->max_count = parse_count(optarg);
 		revs->no_walk = 0;
+		revs->max_count_type = 0;
 		return argcount;
+	} else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
+		if (revs->max_count_type == 0 && revs->max_count != -1)
+			die(_("can't use --max-count with --max-count-oldest"));
+		if (revs->skip_count > 0)
+			die(_("con't use --max-count-oldest with --skip"));
+		revs->max_count = parse_count(optarg);
+		revs->no_walk = 0;
+		revs->max_count_type = 1;
+		revs->max_count_stage = 0;
 	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+		if (revs->max_count_type == 1)
+			die(_("con't use --max-count-oldest with --skip"));
 		revs->skip_count = parse_count(optarg);
 		return argcount;
 	} else if ((*arg == '-') && isdigit(arg[1])) {
@@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	return c;
 }
 
+static void retrieve_oldest_commits(struct rev_info *revs,
+				    struct commit_list **queue)
+{
+	struct commit *c;
+	int max_count = revs->max_count;
+	int queuei_count = 0;
+	int queueo_count = 0;
+	struct commit_list *queueo = NULL;
+	struct commit_list *queuei = NULL;
+	struct commit_list *reversed_queue = NULL;
+
+	revs->max_count = -1;
+	while ((c = get_revision_internal(revs))) {
+		c->object.flags &= ~SHOWN;
+		commit_list_insert(c, &queuei);
+		queuei_count++;
+		while (queuei_count + queueo_count > max_count) {
+			if (!queueo_count) {
+				while (queuei_count > 0) {
+					c = pop_commit(&queuei);
+					queuei_count--;
+					commit_list_insert(c, &queueo);
+					queueo_count++;
+				}
+			}
+			pop_commit(&queueo);
+			queueo_count--;
+		}
+	}
+
+	while ((c = pop_commit(&queueo)))
+		commit_list_insert(c, &reversed_queue);
+	while ((c = pop_commit(&queuei)))
+		commit_list_insert(c, &queueo);
+	while ((c = pop_commit(&queueo)))
+		commit_list_insert(c, &reversed_queue);
+
+	while ((c = pop_commit(&reversed_queue)))
+		commit_list_insert(c, queue);
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c;
 	struct commit_list *reversed;
+	struct commit_list *queue = NULL;
+
+	if (revs->max_count_type == 1 && !revs->max_count_stage) {
+		retrieve_oldest_commits(revs, &queue);
+		commit_list_free(revs->commits);
+		revs->commits = queue;
+		revs->max_count_stage = 1;
+	}
 
 	if (revs->reverse) {
 		reversed = NULL;
-		while ((c = get_revision_internal(revs)))
-			commit_list_insert(c, &reversed);
+		if (revs->max_count_type == 1)
+			while ((c = pop_commit(&revs->commits)))
+				commit_list_insert(c, &reversed);
+		else
+			while ((c = get_revision_internal(revs)))
+				commit_list_insert(c, &reversed);
 		commit_list_free(revs->commits);
 		revs->commits = reversed;
 		revs->reverse = 0;
@@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
 		return c;
 	}
 
-	c = get_revision_internal(revs);
+	if (revs->max_count_stage)
+		c = pop_commit(&revs->commits);
+	else
+		c = get_revision_internal(revs);
+
 	if (c && revs->graph)
 		graph_update(revs->graph, c);
 	if (!c) {
diff --git a/revision.h b/revision.h
index 584f1338b5..e157463cb1 100644
--- a/revision.h
+++ b/revision.h
@@ -309,6 +309,8 @@ struct rev_info {
 	/* special limits */
 	int skip_count;
 	int max_count;
+	unsigned int max_count_type:1;
+	unsigned int max_count_stage:1;
 	timestamp_t max_age;
 	timestamp_t max_age_as_filter;
 	timestamp_t min_age;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..668c231cf1 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,20 @@ test_expect_success 'log --graph with --name-status' '
 	test_cmp_graph --name-status tangle..reach
 '
 
+test_expect_success 'log --max-count-oldest=3 --oneline' '
+	test_when_finished rm expect &&
+	git log --oneline | tail -n3 >expect &&
+	git log --oneline --max-count-oldest=3 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
+	test_when_finished rm expect &&
+	git log --oneline | tail -n3 | tac >expect &&
+	git log --oneline --max-count-oldest=3 --reverse >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0

Re: [PATCH v5] revision.c: implement --max-count-oldest

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to combine --max-count with
> --skip. This is not very user-friendly.

To use "--skip=<n>" for this purose, you'd need to know how many
records are going to be omitted to begin with, but that means you'd
run the command without count limitation once only to find out how
many records there are.  "not very user-friendly" sounds like an
understatement of the year.

They can do with something silly like

    git rev-list ... |
    tail -n N |
    xargs -n1 git show ...

and that does count as "not very user-friendly", I would think.

> Teach get_revision() the --max-count-oldest option.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
>  Documentation/rev-list-options.adoc |  3 ++
>  revision.c                          | 77 +++++++++++++++++++++++++++--
>  revision.h                          |  2 +
>  t/t4202-log.sh                      | 14 ++++++
>  4 files changed, 93 insertions(+), 3 deletions(-)

It looks like this needs measurably smaller damage to the codebase
than the other --reverse=before approach ;-).

> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..736f34efab 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,9 @@ ordering and formatting options, such as `--reverse`.
>  `--max-count=<number>`::
>  	Limit the output to _<number>_ commits.
>  
> +`--max-count-oldest=<number>`::
> +	Limit the output to the _<number>_ oldest commits.
> +
>  `--skip=<number>`::
>  	Skip _<number>_ commits before starting to show the commit output.
>  
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>  	}
>  
>  	if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> +		if (revs->max_count_type == 1)
> +			die(_("can't use --max-count with --max-count-oldest"));
>  		revs->max_count = parse_count(optarg);
>  		revs->no_walk = 0;
> +		revs->max_count_type = 0;
>  		return argcount;
> +	} else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
> +		if (revs->max_count_type == 0 && revs->max_count != -1)
> +			die(_("can't use --max-count with --max-count-oldest"));
> +		if (revs->skip_count > 0)
> +			die(_("con't use --max-count-oldest with --skip"));
> +		revs->max_count = parse_count(optarg);
> +		revs->no_walk = 0;
> +		revs->max_count_type = 1;
> +		revs->max_count_stage = 0;
>  	} else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> +		if (revs->max_count_type == 1)
> +			die(_("con't use --max-count-oldest with --skip"));
>  		revs->skip_count = parse_count(optarg);
>  		return argcount;
>  	} else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
>  	return c;
>  }
>  
> +static void retrieve_oldest_commits(struct rev_info *revs,
> +				    struct commit_list **queue)
> +{
> +	struct commit *c;
> +	int max_count = revs->max_count;
> +	int queuei_count = 0;
> +	int queueo_count = 0;
> +	struct commit_list *queueo = NULL;
> +	struct commit_list *queuei = NULL;
> +	struct commit_list *reversed_queue = NULL;
> +
> +	revs->max_count = -1;
> +	while ((c = get_revision_internal(revs))) {
> +		c->object.flags &= ~SHOWN;
> +		commit_list_insert(c, &queuei);
> +		queuei_count++;
> +		while (queuei_count + queueo_count > max_count) {
> +			if (!queueo_count) {
> +				while (queuei_count > 0) {
> +					c = pop_commit(&queuei);
> +					queuei_count--;
> +					commit_list_insert(c, &queueo);
> +					queueo_count++;
> +				}
> +			}
> +			pop_commit(&queueo);
> +			queueo_count--;
> +		}
> +	}
> +
> +	while ((c = pop_commit(&queueo)))
> +		commit_list_insert(c, &reversed_queue);
> +	while ((c = pop_commit(&queuei)))
> +		commit_list_insert(c, &queueo);
> +	while ((c = pop_commit(&queueo)))
> +		commit_list_insert(c, &reversed_queue);
> +
> +	while ((c = pop_commit(&reversed_queue)))
> +		commit_list_insert(c, queue);
> +}
> +
>  struct commit *get_revision(struct rev_info *revs)
>  {
>  	struct commit *c;
>  	struct commit_list *reversed;
> +	struct commit_list *queue = NULL;
> +
> +	if (revs->max_count_type == 1 && !revs->max_count_stage) {
> +		retrieve_oldest_commits(revs, &queue);
> +		commit_list_free(revs->commits);
> +		revs->commits = queue;
> +		revs->max_count_stage = 1;
> +	}
>  
>  	if (revs->reverse) {
>  		reversed = NULL;
> -		while ((c = get_revision_internal(revs)))
> -			commit_list_insert(c, &reversed);
> +		if (revs->max_count_type == 1)
> +			while ((c = pop_commit(&revs->commits)))
> +				commit_list_insert(c, &reversed);
> +		else
> +			while ((c = get_revision_internal(revs)))
> +				commit_list_insert(c, &reversed);
>  		commit_list_free(revs->commits);
>  		revs->commits = reversed;
>  		revs->reverse = 0;
> @@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
>  		return c;
>  	}
>  
> -	c = get_revision_internal(revs);
> +	if (revs->max_count_stage)
> +		c = pop_commit(&revs->commits);
> +	else
> +		c = get_revision_internal(revs);
> +
>  	if (c && revs->graph)
>  		graph_update(revs->graph, c);
>  	if (!c) {
> diff --git a/revision.h b/revision.h
> index 584f1338b5..e157463cb1 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -309,6 +309,8 @@ struct rev_info {
>  	/* special limits */
>  	int skip_count;
>  	int max_count;
> +	unsigned int max_count_type:1;
> +	unsigned int max_count_stage:1;
>  	timestamp_t max_age;
>  	timestamp_t max_age_as_filter;
>  	timestamp_t min_age;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 05cee9e41b..668c231cf1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1882,6 +1882,20 @@ test_expect_success 'log --graph with --name-status' '
>  	test_cmp_graph --name-status tangle..reach
>  '
>  
> +test_expect_success 'log --max-count-oldest=3 --oneline' '
> +	test_when_finished rm expect &&
> +	git log --oneline | tail -n3 >expect &&
> +	git log --oneline --max-count-oldest=3 >actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> +	test_when_finished rm expect &&
> +	git log --oneline | tail -n3 | tac >expect &&
> +	git log --oneline --max-count-oldest=3 --reverse >actual &&
> +	test_cmp expect actual
> +'
> +
>  cat >expect <<-\EOF
>  * reach
>  |

Re: [PATCH v5] revision.c: implement --max-count-oldest

[Mirko Faina]

On Mon, May 04, 2026 at 02:19:47PM +0900, Junio C Hamano wrote:
> Mirko Faina <mroik@delayed.space> writes:
> 
> > --max-count is a commit limiting option sets a maximum amount of commits
> > to be shown. If a user wants to see only the first N commits of the
> > history (the oldest commits) they'd have to combine --max-count with
> > --skip. This is not very user-friendly.
> 
> To use "--skip=<n>" for this purose, you'd need to know how many
> records are going to be omitted to begin with, but that means you'd
> run the command without count limitation once only to find out how
> many records there are.  "not very user-friendly" sounds like an
> understatement of the year.
> 
> They can do with something silly like
> 
>     git rev-list ... |
>     tail -n N |
>     xargs -n1 git show ...
> 
> and that does count as "not very user-friendly", I would think.

Will reword in v6.

> > Teach get_revision() the --max-count-oldest option.
> >
> > Signed-off-by: Mirko Faina <mroik@delayed.space>
> > ---
> >  Documentation/rev-list-options.adoc |  3 ++
> >  revision.c                          | 77 +++++++++++++++++++++++++++--
> >  revision.h                          |  2 +
> >  t/t4202-log.sh                      | 14 ++++++
> >  4 files changed, 93 insertions(+), 3 deletions(-)
> 
> It looks like this needs measurably smaller damage to the codebase
> than the other --reverse=before approach ;-).

Yes, the control flow of the program is also easier to read.

Thanks

[PATCH v6] revision.c: implement --max-count-oldest

[Mirko Faina]

--max-count is a commit limiting option sets a maximum amount of commits
to be shown. If a user wants to see only the first N commits of the
history (the oldest commits) they'd have to do something like

    git log $(git rev-list HEAD | tail -n N | head -n 1)

This is not very user-friendly.

Teach get_revision() the --max-count-oldest option.

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
Since v5 I've reworded the commit message and rewrote the docs for
--max-count-oldest to be clearer on its functionality.

 Documentation/rev-list-options.adoc |  5 ++
 revision.c                          | 77 +++++++++++++++++++++++++++--
 revision.h                          |  2 +
 t/t4202-log.sh                      | 14 ++++++
 4 files changed, 95 insertions(+), 3 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..9f857cabcc 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
 `--max-count=<number>`::
    Limit the output to _<number>_ commits.
 
+`--max-count-oldest=<number>`::
+   Just like `--max-count=<number>`, it limits the output to _<number>_
+   commits. But instead of limiting to the first _<number>_ commits it
+   limits to the last _<number>_ commits.
+
 `--skip=<number>`::
    Skip _<number>_ commits before starting to show the commit output.
 
diff --git a/revision.c b/revision.c
index 599b3a66c3..3aaa77ced5 100644
--- a/revision.c
+++ b/revision.c
@@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
    }
 
    if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+       if (revs->max_count_type == 1)
+           die(_("can't use --max-count with --max-count-oldest"));
        revs->max_count = parse_count(optarg);
        revs->no_walk = 0;
+       revs->max_count_type = 0;
        return argcount;
+   } else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
+       if (revs->max_count_type == 0 && revs->max_count != -1)
+           die(_("can't use --max-count with --max-count-oldest"));
+       if (revs->skip_count > 0)
+           die(_("con't use --max-count-oldest with --skip"));
+       revs->max_count = parse_count(optarg);
+       revs->no_walk = 0;
+       revs->max_count_type = 1;
+       revs->max_count_stage = 0;
    } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+       if (revs->max_count_type == 1)
+           die(_("con't use --max-count-oldest with --skip"));
        revs->skip_count = parse_count(optarg);
        return argcount;
    } else if ((*arg == '-') && isdigit(arg[1])) {
@@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
    return c;
 }
 
+static void retrieve_oldest_commits(struct rev_info *revs,
+                   struct commit_list **queue)
+{
+   struct commit *c;
+   int max_count = revs->max_count;
+   int queuei_count = 0;
+   int queueo_count = 0;
+   struct commit_list *queueo = NULL;
+   struct commit_list *queuei = NULL;
+   struct commit_list *reversed_queue = NULL;
+
+   revs->max_count = -1;
+   while ((c = get_revision_internal(revs))) {
+       c->object.flags &= ~SHOWN;
+       commit_list_insert(c, &queuei);
+       queuei_count++;
+       while (queuei_count + queueo_count > max_count) {
+           if (!queueo_count) {
+               while (queuei_count > 0) {
+                   c = pop_commit(&queuei);
+                   queuei_count--;
+                   commit_list_insert(c, &queueo);
+                   queueo_count++;
+               }
+           }
+           pop_commit(&queueo);
+           queueo_count--;
+       }
+   }
+
+   while ((c = pop_commit(&queueo)))
+       commit_list_insert(c, &reversed_queue);
+   while ((c = pop_commit(&queuei)))
+       commit_list_insert(c, &queueo);
+   while ((c = pop_commit(&queueo)))
+       commit_list_insert(c, &reversed_queue);
+
+   while ((c = pop_commit(&reversed_queue)))
+       commit_list_insert(c, queue);
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
    struct commit *c;
    struct commit_list *reversed;
+   struct commit_list *queue = NULL;
+
+   if (revs->max_count_type == 1 && !revs->max_count_stage) {
+       retrieve_oldest_commits(revs, &queue);
+       commit_list_free(revs->commits);
+       revs->commits = queue;
+       revs->max_count_stage = 1;
+   }
 
    if (revs->reverse) {
        reversed = NULL;
-       while ((c = get_revision_internal(revs)))
-           commit_list_insert(c, &reversed);
+       if (revs->max_count_type == 1)
+           while ((c = pop_commit(&revs->commits)))
+               commit_list_insert(c, &reversed);
+       else
+           while ((c = get_revision_internal(revs)))
+               commit_list_insert(c, &reversed);
        commit_list_free(revs->commits);
        revs->commits = reversed;
        revs->reverse = 0;
@@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
        return c;
    }
 
-   c = get_revision_internal(revs);
+   if (revs->max_count_stage)
+       c = pop_commit(&revs->commits);
+   else
+       c = get_revision_internal(revs);
+
    if (c && revs->graph)
        graph_update(revs->graph, c);
    if (!c) {
diff --git a/revision.h b/revision.h
index 584f1338b5..e157463cb1 100644
--- a/revision.h
+++ b/revision.h
@@ -309,6 +309,8 @@ struct rev_info {
    /* special limits */
    int skip_count;
    int max_count;
+   unsigned int max_count_type:1;
+   unsigned int max_count_stage:1;
    timestamp_t max_age;
    timestamp_t max_age_as_filter;
    timestamp_t min_age;
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..668c231cf1 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,20 @@ test_expect_success 'log --graph with --name-status' '
    test_cmp_graph --name-status tangle..reach
 '
 
+test_expect_success 'log --max-count-oldest=3 --oneline' '
+   test_when_finished rm expect &&
+   git log --oneline | tail -n3 >expect &&
+   git log --oneline --max-count-oldest=3 >actual &&
+   test_cmp expect actual
+'
+
+test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
+   test_when_finished rm expect &&
+   git log --oneline | tail -n3 | tac >expect &&
+   git log --oneline --max-count-oldest=3 --reverse >actual &&
+   test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Johannes Sixt]

Am 05.05.26 um 23:54 schrieb Mirko Faina:
> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to do something like
> 
>     git log $(git rev-list HEAD | tail -n N | head -n 1)
> 
> This is not very user-friendly.
> 
> Teach get_revision() the --max-count-oldest option.
> 
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Since v5 I've reworded the commit message and rewrote the docs for
> --max-count-oldest to be clearer on its functionality.
> 
>  Documentation/rev-list-options.adoc |  5 ++
>  revision.c                          | 77 +++++++++++++++++++++++++++--
>  revision.h                          |  2 +
>  t/t4202-log.sh                      | 14 ++++++
>  4 files changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..9f857cabcc 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
>  `--max-count=<number>`::
>     Limit the output to _<number>_ commits.
>  
> +`--max-count-oldest=<number>`::
> +   Just like `--max-count=<number>`, it limits the output to _<number>_
> +   commits. But instead of limiting to the first _<number>_ commits it
> +   limits to the last _<number>_ commits.
> +

"Just like --max-count" is a surprising addendum in this sentence,
because the only thing they have in common is the limiting of commits,
which it repeats anyway. It's more like "Unlike --max-count, limits the
output to _<number>_ last commits."

BTW, this makes me think whether this kind of limiting could be
triggered by a negative argument to --max-count.

>  `--skip=<number>`::
>     Skip _<number>_ commits before starting to show the commit output.
>  
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>     }
>  
>     if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("can't use --max-count with --max-count-oldest"));

To help translators, the usual pattern is to say (here and later)

	die(_("options '%s' and '%s' cannot be used together"),
		"--max-count", "--max-count-oldest");

>         revs->max_count = parse_count(optarg);
>         revs->no_walk = 0;
> +       revs->max_count_type = 0;
>         return argcount;
> +   } else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
> +       if (revs->max_count_type == 0 && revs->max_count != -1)
> +           die(_("can't use --max-count with --max-count-oldest"));
> +       if (revs->skip_count > 0)
> +           die(_("con't use --max-count-oldest with --skip"));
> +       revs->max_count = parse_count(optarg);
> +       revs->no_walk = 0;
> +       revs->max_count_type = 1;
> +       revs->max_count_stage = 0;
>     } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("con't use --max-count-oldest with --skip"));
>         revs->skip_count = parse_count(optarg);
>         return argcount;
>     } else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
>     return c;
>  }
>  
> +static void retrieve_oldest_commits(struct rev_info *revs,
> +                   struct commit_list **queue)
> +{
> +   struct commit *c;
> +   int max_count = revs->max_count;
> +   int queuei_count = 0;
> +   int queueo_count = 0;
> +   struct commit_list *queueo = NULL;
> +   struct commit_list *queuei = NULL;
> +   struct commit_list *reversed_queue = NULL;
> +
> +   revs->max_count = -1;
> +   while ((c = get_revision_internal(revs))) {
> +       c->object.flags &= ~SHOWN;
> +       commit_list_insert(c, &queuei);
> +       queuei_count++;
> +       while (queuei_count + queueo_count > max_count) {
> +           if (!queueo_count) {
> +               while (queuei_count > 0) {
> +                   c = pop_commit(&queuei);
> +                   queuei_count--;
> +                   commit_list_insert(c, &queueo);
> +                   queueo_count++;
> +               }
> +           }
> +           pop_commit(&queueo);
> +           queueo_count--;
> +       }
> +   }
> +
> +   while ((c = pop_commit(&queueo)))
> +       commit_list_insert(c, &reversed_queue);
> +   while ((c = pop_commit(&queuei)))
> +       commit_list_insert(c, &queueo);
> +   while ((c = pop_commit(&queueo)))
> +       commit_list_insert(c, &reversed_queue);
> +
> +   while ((c = pop_commit(&reversed_queue)))
> +       commit_list_insert(c, queue);
> +}
> +
>  struct commit *get_revision(struct rev_info *revs)
>  {
>     struct commit *c;
>     struct commit_list *reversed;
> +   struct commit_list *queue = NULL;
> +
> +   if (revs->max_count_type == 1 && !revs->max_count_stage) {
> +       retrieve_oldest_commits(revs, &queue);
> +       commit_list_free(revs->commits);
> +       revs->commits = queue;
> +       revs->max_count_stage = 1;
> +   }
>  
>     if (revs->reverse) {
>         reversed = NULL;
> -       while ((c = get_revision_internal(revs)))
> -           commit_list_insert(c, &reversed);
> +       if (revs->max_count_type == 1)
> +           while ((c = pop_commit(&revs->commits)))
> +               commit_list_insert(c, &reversed);
> +       else
> +           while ((c = get_revision_internal(revs)))
> +               commit_list_insert(c, &reversed);
>         commit_list_free(revs->commits);
>         revs->commits = reversed;
>         revs->reverse = 0;

I would have expected that this kind of commit counting is handled at
the same spot where --max-count is handled, i.e., in
get_revision_internal(). It could make a difference in combination with
sorting options, --boundary, and --graph. The goal is that --max-count
and --max-count-oldest behave the same in this regard. (But I am in no
way an expert of the revision walker.)

> @@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
>         return c;
>     }
>  
> -   c = get_revision_internal(revs);
> +   if (revs->max_count_stage)
> +       c = pop_commit(&revs->commits);
> +   else
> +       c = get_revision_internal(revs);
> +
>     if (c && revs->graph)
>         graph_update(revs->graph, c);
>     if (!c) {
> diff --git a/revision.h b/revision.h
> index 584f1338b5..e157463cb1 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -309,6 +309,8 @@ struct rev_info {
>     /* special limits */
>     int skip_count;
>     int max_count;
> +   unsigned int max_count_type:1;
> +   unsigned int max_count_stage:1;
>     timestamp_t max_age;
>     timestamp_t max_age_as_filter;
>     timestamp_t min_age;
-- Hannes

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Mirko Faina]

On Wed, May 06, 2026 at 08:45:36AM +0200, Johannes Sixt wrote:
> > +`--max-count-oldest=<number>`::
> > +   Just like `--max-count=<number>`, it limits the output to _<number>_
> > +   commits. But instead of limiting to the first _<number>_ commits it
> > +   limits to the last _<number>_ commits.
> > +
> 
> "Just like --max-count" is a surprising addendum in this sentence,
> because the only thing they have in common is the limiting of commits,
> which it repeats anyway. It's more like "Unlike --max-count, limits the
> output to _<number>_ last commits."

Will fix in v7.

> BTW, this makes me think whether this kind of limiting could be
> triggered by a negative argument to --max-count.

Would be a good idea if it weren't for the fact that --max-count < 0 has
for a long time acted like no max count. I'd imagine many could be
asssuming this behaviour in their scripts.

> >  `--skip=<number>`::
> >     Skip _<number>_ commits before starting to show the commit output.
> >  
> > diff --git a/revision.c b/revision.c
> > index 599b3a66c3..3aaa77ced5 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
> >     }
> >  
> >     if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> > +       if (revs->max_count_type == 1)
> > +           die(_("can't use --max-count with --max-count-oldest"));
> 
> To help translators, the usual pattern is to say (here and later)
> 
> 	die(_("options '%s' and '%s' cannot be used together"),
> 		"--max-count", "--max-count-oldest");

Will do.

> > @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
> >     return c;
> >  }
> >  
> > +static void retrieve_oldest_commits(struct rev_info *revs,
> > +                   struct commit_list **queue)
> > +{
> > +   struct commit *c;
> > +   int max_count = revs->max_count;
> > +   int queuei_count = 0;
> > +   int queueo_count = 0;
> > +   struct commit_list *queueo = NULL;
> > +   struct commit_list *queuei = NULL;
> > +   struct commit_list *reversed_queue = NULL;
> > +
> > +   revs->max_count = -1;
> > +   while ((c = get_revision_internal(revs))) {
> > +       c->object.flags &= ~SHOWN;
> > +       commit_list_insert(c, &queuei);
> > +       queuei_count++;
> > +       while (queuei_count + queueo_count > max_count) {
> > +           if (!queueo_count) {
> > +               while (queuei_count > 0) {
> > +                   c = pop_commit(&queuei);
> > +                   queuei_count--;
> > +                   commit_list_insert(c, &queueo);
> > +                   queueo_count++;
> > +               }
> > +           }
> > +           pop_commit(&queueo);
> > +           queueo_count--;
> > +       }
> > +   }
> > +
> > +   while ((c = pop_commit(&queueo)))
> > +       commit_list_insert(c, &reversed_queue);
> > +   while ((c = pop_commit(&queuei)))
> > +       commit_list_insert(c, &queueo);
> > +   while ((c = pop_commit(&queueo)))
> > +       commit_list_insert(c, &reversed_queue);
> > +
> > +   while ((c = pop_commit(&reversed_queue)))
> > +       commit_list_insert(c, queue);
> > +}
> > +
> >  struct commit *get_revision(struct rev_info *revs)
> >  {
> >     struct commit *c;
> >     struct commit_list *reversed;
> > +   struct commit_list *queue = NULL;
> > +
> > +   if (revs->max_count_type == 1 && !revs->max_count_stage) {
> > +       retrieve_oldest_commits(revs, &queue);
> > +       commit_list_free(revs->commits);
> > +       revs->commits = queue;
> > +       revs->max_count_stage = 1;
> > +   }
> >  
> >     if (revs->reverse) {
> >         reversed = NULL;
> > -       while ((c = get_revision_internal(revs)))
> > -           commit_list_insert(c, &reversed);
> > +       if (revs->max_count_type == 1)
> > +           while ((c = pop_commit(&revs->commits)))
> > +               commit_list_insert(c, &reversed);
> > +       else
> > +           while ((c = get_revision_internal(revs)))
> > +               commit_list_insert(c, &reversed);
> >         commit_list_free(revs->commits);
> >         revs->commits = reversed;
> >         revs->reverse = 0;
> 
> I would have expected that this kind of commit counting is handled at
> the same spot where --max-count is handled, i.e., in
> get_revision_internal(). It could make a difference in combination with
> sorting options, --boundary, and --graph. The goal is that --max-count
> and --max-count-oldest behave the same in this regard. (But I am in no
> way an expert of the revision walker.)

It doesn't affect sorting options and the graph output by itself is
handled by setting the commits as not shown when we store them, but the
boundary option does break.

Will fix in v7.

Thank you

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

>> BTW, this makes me think whether this kind of limiting could be
>> triggered by a negative argument to --max-count.
>
> Would be a good idea if it weren't for the fact that --max-count < 0 has
> for a long time acted like no max count. I'd imagine many could be
> asssuming this behaviour in their scripts.

Many?  I am not sure.  

What do these script try to achieve by having "--max-count=-1"?  It
would be to defeat --max-count=<n> coming from elsewhere, but where?

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Mirko Faina]

On Thu, May 07, 2026 at 06:20:40PM +0900, Junio C Hamano wrote:
> Mirko Faina <mroik@delayed.space> writes:
> 
> >> BTW, this makes me think whether this kind of limiting could be
> >> triggered by a negative argument to --max-count.
> >
> > Would be a good idea if it weren't for the fact that --max-count < 0 has
> > for a long time acted like no max count. I'd imagine many could be
> > asssuming this behaviour in their scripts.
> 
> Many?  I am not sure.  
> 
> What do these script try to achieve by having "--max-count=-1"?  It
> would be to defeat --max-count=<n> coming from elsewhere, but where?

It's not necessarely to defeat a previous use of --max-count. If I know
that --max-count behaves in as certain way, in this case the same as not
having it when below 0, I can always have it in the command that I have
to run and just work with the argument (of course one would do this only
if it is easier to work with just the argument). That way I don't have
to conditionally add the option if it has to be enabled. Something like

	N=should_use_max_count_and_if_so_much_to_limit
	git rev-list --max-count=$N

Of course these script make use of behaviour that is not documented and
might not even be intended, so really their fault if it breaks.

This has been the behaviour of --max-count for a long time so I'm
assuming that there is a possibility that it will break many scripts.
But like I said, their fault if it breaks, if you think its not that
widespread I'll get rid of --max-count-oldest.

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Jean-Noël AVILA]

On Tuesday, 5 May 2026 23:54:56 CEST Mirko Faina wrote:
> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to do something like
> 
>     git log $(git rev-list HEAD | tail -n N | head -n 1)
> 
> This is not very user-friendly.
> 
> Teach get_revision() the --max-count-oldest option.
> 
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Since v5 I've reworded the commit message and rewrote the docs for
> --max-count-oldest to be clearer on its functionality.
> 
>  Documentation/rev-list-options.adoc |  5 ++
>  revision.c                          | 77 +++++++++++++++++++++++++++--
>  revision.h                          |  2 +
>  t/t4202-log.sh                      | 14 ++++++
>  4 files changed, 95 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/rev-list-options.adoc
> b/Documentation/rev-list-options.adoc index 2d195a1474..9f857cabcc 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
>  `--max-count=<number>`::
>     Limit the output to _<number>_ commits.
> 
> +`--max-count-oldest=<number>`::
> +   Just like `--max-count=<number>`, it limits the output to _<number>_
> +   commits. But instead of limiting to the first _<number>_ commits it
> +   limits to the last _<number>_ commits.
> +

Putting aside the discussion of --max-count=<neg-value> vs --max-count-
oldest=<value>, I do not think that defining --max-count-old with respect with 
--max-count is legible. It would be better to refine the definition of --max-
count (i.e. "Limit the output to the _<number>_ first commits") and just 
define --max-count-oldest on its own in the same manner. Referring to another 
entry is only practicable when it avoids repeating a long explanation. 
Otherwise, each entry's explanation should be as self-contained as possible.

>  `--skip=<number>`::
>     Skip _<number>_ commits before starting to show the commit output.
> 
> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info 
*revs, int
> argc, const char **arg }
> 
>     if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("can't use --max-count with --max-count-oldest"));
>         revs->max_count = parse_count(optarg);
>         revs->no_walk = 0;
> +       revs->max_count_type = 0;
>         return argcount;
> +   } else if ((argcount = parse_long_opt("max-count-oldest", argv, 
&optarg))) {
> +       if (revs->max_count_type == 0 && revs->max_count != -1)
> +           die(_("can't use --max-count with --max-count-oldest"));
> +       if (revs->skip_count > 0)
> +           die(_("con't use --max-count-oldest with --skip"));

Typo here (con't → can't). In any case, please prefer 
die_for_incompatible_opt2, to uniformize the messages and limit the number of 
translation strings.

Adding a test for these incompatibilities would be great too.

> +       revs->max_count = parse_count(optarg);
> +       revs->no_walk = 0;
> +       revs->max_count_type = 1;
> +       revs->max_count_stage = 0;
>     } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("con't use --max-count-oldest with --skip"));

ditto

>         revs->skip_count = parse_count(optarg);
>         return argcount;
>     } else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct 
rev_info
> *revs) return c;
>  }
> 

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Mirko Faina]

On Sat, May 09, 2026 at 02:46:26PM +0200, Jean-Noël AVILA wrote:
> On Tuesday, 5 May 2026 23:54:56 CEST Mirko Faina wrote:
> > --max-count is a commit limiting option sets a maximum amount of commits
> > to be shown. If a user wants to see only the first N commits of the
> > history (the oldest commits) they'd have to do something like
> > 
> >     git log $(git rev-list HEAD | tail -n N | head -n 1)
> > 
> > This is not very user-friendly.
> > 
> > Teach get_revision() the --max-count-oldest option.
> > 
> > Signed-off-by: Mirko Faina <mroik@delayed.space>
> > ---
> > Since v5 I've reworded the commit message and rewrote the docs for
> > --max-count-oldest to be clearer on its functionality.
> > 
> >  Documentation/rev-list-options.adoc |  5 ++
> >  revision.c                          | 77 +++++++++++++++++++++++++++--
> >  revision.h                          |  2 +
> >  t/t4202-log.sh                      | 14 ++++++
> >  4 files changed, 95 insertions(+), 3 deletions(-)
> > 
> > diff --git a/Documentation/rev-list-options.adoc
> > b/Documentation/rev-list-options.adoc index 2d195a1474..9f857cabcc 100644
> > --- a/Documentation/rev-list-options.adoc
> > +++ b/Documentation/rev-list-options.adoc
> > @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
> >  `--max-count=<number>`::
> >     Limit the output to _<number>_ commits.
> > 
> > +`--max-count-oldest=<number>`::
> > +   Just like `--max-count=<number>`, it limits the output to _<number>_
> > +   commits. But instead of limiting to the first _<number>_ commits it
> > +   limits to the last _<number>_ commits.
> > +
> 
> Putting aside the discussion of --max-count=<neg-value> vs --max-count-
> oldest=<value>, I do not think that defining --max-count-old with respect with 
> --max-count is legible. It would be better to refine the definition of --max-
> count (i.e. "Limit the output to the _<number>_ first commits") and just 
> define --max-count-oldest on its own in the same manner. Referring to another 
> entry is only practicable when it avoids repeating a long explanation. 
> Otherwise, each entry's explanation should be as self-contained as possible.

Will do in v7.

> >  `--skip=<number>`::
> >     Skip _<number>_ commits before starting to show the commit output.
> > 
> > diff --git a/revision.c b/revision.c
> > index 599b3a66c3..3aaa77ced5 100644
> > --- a/revision.c
> > +++ b/revision.c
> > @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info 
> *revs, int
> > argc, const char **arg }
> > 
> >     if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> > +       if (revs->max_count_type == 1)
> > +           die(_("can't use --max-count with --max-count-oldest"));
> >         revs->max_count = parse_count(optarg);
> >         revs->no_walk = 0;
> > +       revs->max_count_type = 0;
> >         return argcount;
> > +   } else if ((argcount = parse_long_opt("max-count-oldest", argv, 
> &optarg))) {
> > +       if (revs->max_count_type == 0 && revs->max_count != -1)
> > +           die(_("can't use --max-count with --max-count-oldest"));
> > +       if (revs->skip_count > 0)
> > +           die(_("con't use --max-count-oldest with --skip"));
> 
> Typo here (con't → can't). In any case, please prefer 
> die_for_incompatible_opt2, to uniformize the messages and limit the number of 
> translation strings.

Will do.

> Adding a test for these incompatibilities would be great too.

Yes, should've done that sooner. Will do.

> > +       revs->max_count = parse_count(optarg);
> > +       revs->no_walk = 0;
> > +       revs->max_count_type = 1;
> > +       revs->max_count_stage = 0;
> >     } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> > +       if (revs->max_count_type == 1)
> > +           die(_("con't use --max-count-oldest with --skip"));
> 
> ditto

Will do.

> >         revs->skip_count = parse_count(optarg);
> >         return argcount;
> >     } else if ((*arg == '-') && isdigit(arg[1])) {
> > @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct 
> rev_info
> > *revs) return c;
> >  }
> > 

Thank you

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to do something like
>
>     git log $(git rev-list HEAD | tail -n N | head -n 1)
>
> This is not very user-friendly.
>
> Teach get_revision() the --max-count-oldest option.
>
> Signed-off-by: Mirko Faina <mroik@delayed.space>
> ---
> Since v5 I've reworded the commit message and rewrote the docs for
> --max-count-oldest to be clearer on its functionality.
>
>  Documentation/rev-list-options.adoc |  5 ++
>  revision.c                          | 77 +++++++++++++++++++++++++++--
>  revision.h                          |  2 +
>  t/t4202-log.sh                      | 14 ++++++
>  4 files changed, 95 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..9f857cabcc 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -18,6 +18,11 @@ ordering and formatting options, such as `--reverse`.
>  `--max-count=<number>`::
>     Limit the output to _<number>_ commits.
>  
> +`--max-count-oldest=<number>`::
> +   Just like `--max-count=<number>`, it limits the output to _<number>_
> +   commits. But instead of limiting to the first _<number>_ commits it
> +   limits to the last _<number>_ commits.
> +
>  `--skip=<number>`::
>     Skip _<number>_ commits before starting to show the commit output.

Saving this message to a file and grepping for a tab finds nothing,
which indicates that the patch seems to be unsalvageably whitespace
broken, given that most of the context lines should use tabs for
indent.

What did you do differently this time?  The previous rounds did not
have this problem.


> diff --git a/revision.c b/revision.c
> index 599b3a66c3..3aaa77ced5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2339,10 +2339,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
>     }
>  
>     if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("can't use --max-count with --max-count-oldest"));
>         revs->max_count = parse_count(optarg);
>         revs->no_walk = 0;
> +       revs->max_count_type = 0;
>         return argcount;
> +   } else if ((argcount = parse_long_opt("max-count-oldest", argv, &optarg))) {
> +       if (revs->max_count_type == 0 && revs->max_count != -1)
> +           die(_("can't use --max-count with --max-count-oldest"));
> +       if (revs->skip_count > 0)
> +           die(_("con't use --max-count-oldest with --skip"));
> +       revs->max_count = parse_count(optarg);
> +       revs->no_walk = 0;
> +       revs->max_count_type = 1;
> +       revs->max_count_stage = 0;
>     } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
> +       if (revs->max_count_type == 1)
> +           die(_("con't use --max-count-oldest with --skip"));
>         revs->skip_count = parse_count(optarg);
>         return argcount;
>     } else if ((*arg == '-') && isdigit(arg[1])) {
> @@ -4521,15 +4535,68 @@ static struct commit *get_revision_internal(struct rev_info *revs)
>     return c;
>  }
>  
> +static void retrieve_oldest_commits(struct rev_info *revs,
> +                   struct commit_list **queue)
> +{
> +   struct commit *c;
> +   int max_count = revs->max_count;
> +   int queuei_count = 0;
> +   int queueo_count = 0;
> +   struct commit_list *queueo = NULL;
> +   struct commit_list *queuei = NULL;
> +   struct commit_list *reversed_queue = NULL;
> +
> +   revs->max_count = -1;
> +   while ((c = get_revision_internal(revs))) {
> +       c->object.flags &= ~SHOWN;
> +       commit_list_insert(c, &queuei);
> +       queuei_count++;
> +       while (queuei_count + queueo_count > max_count) {
> +           if (!queueo_count) {
> +               while (queuei_count > 0) {
> +                   c = pop_commit(&queuei);
> +                   queuei_count--;
> +                   commit_list_insert(c, &queueo);
> +                   queueo_count++;
> +               }
> +           }
> +           pop_commit(&queueo);
> +           queueo_count--;
> +       }
> +   }
> +
> +   while ((c = pop_commit(&queueo)))
> +       commit_list_insert(c, &reversed_queue);
> +   while ((c = pop_commit(&queuei)))
> +       commit_list_insert(c, &queueo);
> +   while ((c = pop_commit(&queueo)))
> +       commit_list_insert(c, &reversed_queue);
> +
> +   while ((c = pop_commit(&reversed_queue)))
> +       commit_list_insert(c, queue);
> +}
> +
>  struct commit *get_revision(struct rev_info *revs)
>  {
>     struct commit *c;
>     struct commit_list *reversed;
> +   struct commit_list *queue = NULL;
> +
> +   if (revs->max_count_type == 1 && !revs->max_count_stage) {
> +       retrieve_oldest_commits(revs, &queue);
> +       commit_list_free(revs->commits);
> +       revs->commits = queue;
> +       revs->max_count_stage = 1;
> +   }
>  
>     if (revs->reverse) {
>         reversed = NULL;
> -       while ((c = get_revision_internal(revs)))
> -           commit_list_insert(c, &reversed);
> +       if (revs->max_count_type == 1)
> +           while ((c = pop_commit(&revs->commits)))
> +               commit_list_insert(c, &reversed);
> +       else
> +           while ((c = get_revision_internal(revs)))
> +               commit_list_insert(c, &reversed);
>         commit_list_free(revs->commits);
>         revs->commits = reversed;
>         revs->reverse = 0;
> @@ -4543,7 +4610,11 @@ struct commit *get_revision(struct rev_info *revs)
>         return c;
>     }
>  
> -   c = get_revision_internal(revs);
> +   if (revs->max_count_stage)
> +       c = pop_commit(&revs->commits);
> +   else
> +       c = get_revision_internal(revs);
> +
>     if (c && revs->graph)
>         graph_update(revs->graph, c);
>     if (!c) {
> diff --git a/revision.h b/revision.h
> index 584f1338b5..e157463cb1 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -309,6 +309,8 @@ struct rev_info {
>     /* special limits */
>     int skip_count;
>     int max_count;
> +   unsigned int max_count_type:1;
> +   unsigned int max_count_stage:1;
>     timestamp_t max_age;
>     timestamp_t max_age_as_filter;
>     timestamp_t min_age;
> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
> index 05cee9e41b..668c231cf1 100755
> --- a/t/t4202-log.sh
> +++ b/t/t4202-log.sh
> @@ -1882,6 +1882,20 @@ test_expect_success 'log --graph with --name-status' '
>     test_cmp_graph --name-status tangle..reach
>  '
>  
> +test_expect_success 'log --max-count-oldest=3 --oneline' '
> +   test_when_finished rm expect &&
> +   git log --oneline | tail -n3 >expect &&
> +   git log --oneline --max-count-oldest=3 >actual &&
> +   test_cmp expect actual
> +'
> +
> +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> +   test_when_finished rm expect &&
> +   git log --oneline | tail -n3 | tac >expect &&
> +   git log --oneline --max-count-oldest=3 --reverse >actual &&
> +   test_cmp expect actual
> +'
> +
>  cat >expect <<-\EOF
>  * reach
>  |

Re: [PATCH v6] revision.c: implement --max-count-oldest

[Mirko Faina]

On Sun, May 10, 2026 at 06:01:15AM +0900, Junio C Hamano wrote:
> Saving this message to a file and grepping for a tab finds nothing,
> which indicates that the patch seems to be unsalvageably whitespace
> broken, given that most of the context lines should use tabs for
> indent.
> 
> What did you do differently this time?  The previous rounds did not
> have this problem.

Ah yes, I remember retab-bing with expandtab while writing stuff after
the commit message. At the time I must've not realized that it retabbed
the content of the patch as well (and I didn't even end up needing to
retab). Sorry about that.

I won't resend v6 as it will be discarded for v7 anyway.

Thank you

Re: [PATCH v5] revision.c: implement --max-count-oldest

[Junio C Hamano]

Mirko Faina <mroik@delayed.space> writes:

> --max-count is a commit limiting option sets a maximum amount of commits
> to be shown. If a user wants to see only the first N commits of the
> history (the oldest commits) they'd have to combine --max-count with
> --skip. This is not very user-friendly.
> ...
> +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> +	test_when_finished rm expect &&
> +	git log --oneline | tail -n3 | tac >expect &&
> +	git log --oneline --max-count-oldest=3 --reverse >actual &&
> +	test_cmp expect actual
> +'

"tac" is not portable, and breaks macOS CI jobs.

  https://github.com/git/git/actions/runs/25591146540/job/75128929633#step:4:2058

Wouldn't

  git log --oneline --reverse | head -n3 >expect

be equivalent?

Re: [PATCH v5] revision.c: implement --max-count-oldest

[Mirko Faina]

On Sat, May 09, 2026 at 08:01:15PM +0900, Junio C Hamano wrote:
> Mirko Faina <mroik@delayed.space> writes:
> 
> > --max-count is a commit limiting option sets a maximum amount of commits
> > to be shown. If a user wants to see only the first N commits of the
> > history (the oldest commits) they'd have to combine --max-count with
> > --skip. This is not very user-friendly.
> > ...
> > +test_expect_success 'log --max-count-oldest=3 --reverse --oneline' '
> > +	test_when_finished rm expect &&
> > +	git log --oneline | tail -n3 | tac >expect &&
> > +	git log --oneline --max-count-oldest=3 --reverse >actual &&
> > +	test_cmp expect actual
> > +'
> 
> "tac" is not portable, and breaks macOS CI jobs.
> 
>   https://github.com/git/git/actions/runs/25591146540/job/75128929633#step:4:2058
> 
> Wouldn't
> 
>   git log --oneline --reverse | head -n3 >expect
> 
> be equivalent?

Yes, will do.

Thank you

[PATCH v2 1/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

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 | 14 ++++--
 revision.c                          | 31 ++++++++++++--
 revision.h                          |  8 +++-
 t/t4202-log.sh                      | 66 +++++++++++++++++++++++++++++
 4 files changed, 111 insertions(+), 8 deletions(-)

diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
index 2d195a1474..7244e85108 100644
--- a/Documentation/rev-list-options.adoc
+++ b/Documentation/rev-list-options.adoc
@@ -914,10 +914,16 @@ 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`::
-	Output the commits chosen to be shown (see 'Commit Limiting'
-	section above) in reverse order. Cannot be combined with
-	`--walk-reflogs`.
+`--[no-]reverse[=(after|before)]`::
+	Accepts `after` or `before`. Cannot be combined with
+	`--walk-reflogs`. If `after`, output the commits chosen to be
+	shown (see 'Commit Limiting' section above) in reverse order. If
+	`before`, reverse the commits before filtering with `Commit
+	Limiting` options. This option can be used multiple times, last
+	one is applied. When the argument for `--reverse` is omitted, if
+	the current state is in no reverse, it defaults to `after`. If
+	it is in any reversed state, it restores the original ordering
+	by removing the reverse state.
 endif::git-shortlog[]
 
 ifndef::git-shortlog[]
diff --git a/revision.c b/revision.c
index 599b3a66c3..d581f5e38e 100644
--- a/revision.c
+++ b/revision.c
@@ -2686,7 +2686,16 @@ 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;
+		revs->reverse = !revs->reverse;
+	} else if (skip_prefix(arg, "--reverse=", &optarg)) {
+		if (!strcmp(optarg, "after"))
+			revs->reverse = REVERSE_AFTER;
+		else if(!strcmp(optarg, "before"))
+			revs->reverse = REVERSE_BEFORE;
+		else
+			die(_("unknown value for --reverse: %s"), optarg);
+	} else if (!strcmp(arg, "--no-reverse")) {
+		revs->reverse = NO_REVERSE;
 	} else if (!strcmp(arg, "--children")) {
 		revs->children.name = "children";
 		revs->limited = 1;
@@ -4525,19 +4534,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 == REVERSE_BEFORE)
+			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 == REVERSE_BEFORE)
+			revs->max_count = max_count;
 	}
 
 	if (revs->reverse_output_stage) {
+		if (revs->reverse == REVERSE_BEFORE && revs->max_count == 0)
+			return NULL;
+
 		c = pop_commit(&revs->commits);
+		if (revs->reverse == REVERSE_BEFORE)
+			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..02881577dc 100644
--- a/revision.h
+++ b/revision.h
@@ -121,6 +121,12 @@ struct ref_exclusions {
 struct oidset;
 struct topo_walk_info;
 
+enum rev_reverse {
+	NO_REVERSE = 0,
+	REVERSE_AFTER = 1,
+	REVERSE_BEFORE = 2,
+};
+
 struct rev_info {
 	/* Starting list */
 	struct commit_list *commits;
@@ -167,6 +173,7 @@ struct rev_info {
 			ignore_missing_links:1;
 
 	/* Traversal flags */
+	enum rev_reverse reverse:2;
 	unsigned int	dense:1,
 			prune:1,
 			no_walk:1,
@@ -196,7 +203,6 @@ struct rev_info {
 			rewrite_parents:1,
 			print_parents:1,
 			show_decorations:1,
-			reverse:1,
 			reverse_output_stage:1,
 			cherry_pick:1,
 			cherry_mark:1,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 05cee9e41b..3bfe2c99b8 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -1882,6 +1882,72 @@ 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 --reverse --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 --reverse --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
+'
+
+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
+'
+
+cat >expect <<-\EOF
+046b221 to remove
+c3f451c Merge tag 'reach'
+EOF
+
+test_expect_success 'log --reverse --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 --reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'log --reverse --no-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 --no-reverse --oneline --max-count=2 >actual &&
+	test_cmp expect actual
+'
+
 cat >expect <<-\EOF
 * reach
 |
-- 
2.54.0

Re: [PATCH v2 1/2] revision.c: implement --reverse=before for walks

[Jeff King]

On Wed, Apr 22, 2026 at 02:28:40AM +0200, Mirko Faina wrote:

> diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> index 2d195a1474..7244e85108 100644
> --- a/Documentation/rev-list-options.adoc
> +++ b/Documentation/rev-list-options.adoc
> @@ -914,10 +914,16 @@ 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`::
> -	Output the commits chosen to be shown (see 'Commit Limiting'
> -	section above) in reverse order. Cannot be combined with
> -	`--walk-reflogs`.
> +`--[no-]reverse[=(after|before)]`::
> +	Accepts `after` or `before`. Cannot be combined with
> +	`--walk-reflogs`. If `after`, output the commits chosen to be
> +	shown (see 'Commit Limiting' section above) in reverse order. If
> +	`before`, reverse the commits before filtering with `Commit
> +	Limiting` options. This option can be used multiple times, last
> +	one is applied. When the argument for `--reverse` is omitted, if
> +	the current state is in no reverse, it defaults to `after`. If
> +	it is in any reversed state, it restores the original ordering
> +	by removing the reverse state.

I think this is all correct, but I found the final sentences a bit hard
to follow (especially the phrase "reversed state"). Let me take a stab
at it.

  When multiple `--reverse=` options are given, the final option
  overrides any previous options. The `--reverse` option (with no
  specifier) behaves as `--reverse=after`, except that for historical
  reasons it negates any previous reversed state (so `--reverse
  --reverse` does nothing, nor does `--reverse=before --reverse`).

I dunno if that is any better, really. I hoped by mentioning "historical
reasons" that excuses any weirdness, and readers interested in sane
behavior can stop reading. ;)

So anyway, I offer it in case you find it useful or can pick out useful
bits from it.

-Peff

Re: [PATCH v2 1/2] revision.c: implement --reverse=before for walks

[Mirko Faina]

On Wed, Apr 22, 2026 at 06:44:42PM -0400, Jeff King wrote:
> On Wed, Apr 22, 2026 at 02:28:40AM +0200, Mirko Faina wrote:
> 
> > diff --git a/Documentation/rev-list-options.adoc b/Documentation/rev-list-options.adoc
> > index 2d195a1474..7244e85108 100644
> > --- a/Documentation/rev-list-options.adoc
> > +++ b/Documentation/rev-list-options.adoc
> > @@ -914,10 +914,16 @@ 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`::
> > -	Output the commits chosen to be shown (see 'Commit Limiting'
> > -	section above) in reverse order. Cannot be combined with
> > -	`--walk-reflogs`.
> > +`--[no-]reverse[=(after|before)]`::
> > +	Accepts `after` or `before`. Cannot be combined with
> > +	`--walk-reflogs`. If `after`, output the commits chosen to be
> > +	shown (see 'Commit Limiting' section above) in reverse order. If
> > +	`before`, reverse the commits before filtering with `Commit
> > +	Limiting` options. This option can be used multiple times, last
> > +	one is applied. When the argument for `--reverse` is omitted, if
> > +	the current state is in no reverse, it defaults to `after`. If
> > +	it is in any reversed state, it restores the original ordering
> > +	by removing the reverse state.
> 
> I think this is all correct, but I found the final sentences a bit hard
> to follow (especially the phrase "reversed state"). Let me take a stab
> at it.
> 
>   When multiple `--reverse=` options are given, the final option
>   overrides any previous options. The `--reverse` option (with no
>   specifier) behaves as `--reverse=after`, except that for historical
>   reasons it negates any previous reversed state (so `--reverse
>   --reverse` does nothing, nor does `--reverse=before --reverse`).
> 
> I dunno if that is any better, really. I hoped by mentioning "historical
> reasons" that excuses any weirdness, and readers interested in sane
> behavior can stop reading. ;)
> 
> So anyway, I offer it in case you find it useful or can pick out useful
> bits from it.

Yes, it does read better, thank you.

I'd personally put a "truth table" but that would be very verbose, and I
suspect most would not find it useful :D

Thank you

[PATCH v2 2/2] revision.c: reduce memory usage on reverse before

[Mirko Faina]

Due to the nature of --reverse=before we have to walk all of the history
and store each non-filtered processed commit, this can be expensive on
memory for projects with a long history. When --max-count is being used
we don't really have to keep every processed commit, we can discard
older commits (as in have been processed before than the ones we're now
considering, from a chronological commit order they are the newer
commits) as we surpass the --max-count limit.

Teach get_revision() to keep only the newer commits as we walk a
revision with --reverse=before and --max-count=<k>. We do this through a
simple queue. With N nodes and K as the --max-count argument, assuming K
< N, we go from a space complexity of O(N) to O(K). When it comes down
to time complexity, the queue has an ammortized time of O(1) for pops,
so the complexity remains O(N).

Signed-off-by: Mirko Faina <mroik@delayed.space>
---
 revision.c | 54 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
 1 file changed, 52 insertions(+), 2 deletions(-)

diff --git a/revision.c b/revision.c
index d581f5e38e..4730f21ea6 100644
--- a/revision.c
+++ b/revision.c
@@ -4530,6 +4530,52 @@ static struct commit *get_revision_internal(struct rev_info *revs)
 	return c;
 }
 
+static void retrieve_with_window(struct rev_info *revs, int max_count,
+			  struct commit_list **reversed)
+{
+	struct commit *c;
+	struct commit_list *into_queue = NULL;
+	struct commit_list *outo_queue = NULL;
+	int into_count = 0;
+	int outo_count = 0;
+
+	while ((c = get_revision_internal(revs))) {
+		commit_list_insert(c, &into_queue);
+		into_count++;
+		if (into_count + outo_count > max_count) {
+			if (!outo_count) {
+				while (into_count) {
+					c = pop_commit(&into_queue);
+					into_count--;
+					commit_list_insert(c, &outo_queue);
+					outo_count++;
+				}
+			}
+			pop_commit(&outo_queue);
+			outo_count--;
+		}
+	}
+
+	while (outo_count) {
+		c = pop_commit(&outo_queue);
+		outo_count--;
+		commit_list_insert(c, reversed);
+	}
+
+	while (into_count) {
+		c = pop_commit(&into_queue);
+		into_count--;
+		commit_list_insert(c, &outo_queue);
+		outo_count++;
+	}
+
+	while (outo_count) {
+		c = pop_commit(&outo_queue);
+		outo_count--;
+		commit_list_insert(c, reversed);
+	}
+}
+
 struct commit *get_revision(struct rev_info *revs)
 {
 	struct commit *c;
@@ -4546,8 +4592,12 @@ struct commit *get_revision(struct rev_info *revs)
 			revs->max_count = -1;
 
 		reversed = NULL;
-		while ((c = get_revision_internal(revs)))
-			commit_list_insert(c, &reversed);
+		if (revs->reverse == REVERSE_BEFORE && max_count != -1) {
+			retrieve_with_window(revs, max_count, &reversed);
+		} else {
+			while ((c = get_revision_internal(revs)))
+				commit_list_insert(c, &reversed);
+		}
 		commit_list_free(revs->commits);
 		revs->commits = reversed;
 		revs->reverse_output_stage = 1;
-- 
2.54.0