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

[Johannes Sixt <j6t@kdbg.org>]

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