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

[Mirko Faina <mroik@delayed.space>]

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