From: Ben Knoble Date: Sun, 19 Apr 2026 12:06:24 GMT Subject: Re: [PATCH] revision.c: implement --reverse=before for walks Message-ID: In-Reply-To: <20260418164736.2367523-2-mroik@delayed.space> > Le 18 avr. 2026 à 12:57, Mirko Faina 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 > --- > 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 > >