[PATCH] Fix git rev-list --reverse --max-count=N

[Michael Spang]

Using --max-count with --reverse currently outputs the last N commits
in the final output rather than the first N commits. We want to
truncate the reversed list after the first few commits, rather than
truncating the initial list and reversing that.

Signed-off-by: Michael Spang <***@google.com>
---
revision.c | 20 +++++++++++++++++---
1 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/revision.c b/revision.c
index f54d43f..62135e0 100644
--- a/revision.c
+++ b/revision.c
@@ -1993,7 +1993,8 @@ static struct commit *get_revision_internal(struct rev_info *revs)
c = NULL;
break;
default:
- revs->max_count--;
+ if (!revs->reverse)
+ revs->max_count--;
}

if (c)
@@ -2055,8 +2056,21 @@ struct commit *get_revision(struct rev_info *revs)
revs->reverse_output_stage = 1;
}

- if (revs->reverse_output_stage)
- return pop_commit(&revs->commits);
+ if (revs->reverse_output_stage) {
+ c = pop_commit(&revs->commits);
+
+ switch (revs->max_count) {
+ case -1:
+ break;
+ case 0:
+ c = NULL;
+ break;
+ default:
+ revs->max_count--;
+ }
+
+ return c;
+ }

c = get_revision_internal(revs);
if (c && revs->graph)

[Johannes Sixt]

Post by Michael Spang
Using --max-count with --reverse currently outputs the last N commits
in the final output rather than the first N commits. We want to
truncate the reversed list after the first few commits, rather than
truncating the initial list and reversing that.

So when you have this history (A is oldest, D is newest):

A--B--C--D

and you say

git log --max-count=2 --reverse D

then you want

A
B

but I want

C
D

Why is your interpretation correct, an mine wrong?

-- Hannes

[Sverre Rabbelier]

Heya,

=A0 A--B--C--D
and you say
=A0 git log --max-count=3D2 --reverse D
then you want
=A0 A
=A0 B
but I want
=A0 C
=A0 D

And the current behavior is

B
A

Isn't it? I agree btw, that C D is the 'correct' result.

--=20
Cheers,

Sverre Rabbelier

[Michael Spang]

=A0 A--B--C--D
and you say
=A0 git log --max-count=3D2 --reverse D
then you want
=A0 A
=A0 B
but I want
=A0 C
=A0 D
Why is your interpretation correct, an mine wrong?

Perhaps not wrong, but for me it was unexpected. For whatever reason,
I expected "--reverse" to give you the illusion that you are iterating
from the beginning of the history, even if it's not actually possible
to iterate that way directly. In line with that, limiting the output
to N commits would give you the earliest N commits. If you instead
think "stop descending through the history once we have N commits" the
current behavior makes sense.

I talked to Junio and he says the current behavior is here to stay.

Michael

[Junio C Hamano]

Post by Johannes Sixt

Post by Michael Spang
Using --max-count with --reverse currently outputs the last N commits
in the final output rather than the first N commits. We want to
truncate the reversed list after the first few commits, rather than
truncating the initial list and reversing that.

A--B--C--D
and you say
git log --max-count=2 --reverse D
then you want
A
B
but I want
C
D
Why is your interpretation correct, an mine wrong?

The interaction between --max-count and --reverse was designed a long time
ago to support "I want to review the recent N commits in order to make
sure that they are natural and logical progression". So an unconditional
change of semantics to break that expectation this late in the game will
never be acceptable, and giving title to such a patch with a word "Fix"
won't fly well (it simply _breaks_ behaviour users have long learned to
expect).

It is a different matter to add an explicit command line option (and no
configuration to change it to default) to apply reversal before count
limiting.

[Michael Spang]

The interaction between --max-count and --reverse was designed a long=

time

ago to support "I want to review the recent N commits in order to mak=

e

sure that they are natural and logical progression". =A0So an uncondi=

tional

change of semantics to break that expectation this late in the game w=

ill

never be acceptable, and giving title to such a patch with a word "Fi=

x"

won't fly well (it simply _breaks_ behaviour users have long learned =

to

expect).

You have my apology for calling it a fix. I just don't have the same
visibility into what undocumented behavior users depend on that you
do, and so I thought it actually was one.

The patch can die here. I understand the current behavior now, and I
don't have a strong need for the behavior I expected (piping to head
works fine).

Michael