From: Chandra Pratap <chandrapratap3519@gmail.com>
Date: Mon, 06 Jul 2026 09:49:31 GMT
Subject: Re: [PATCH v7 2/3] graph: add a 2 commit buffer for lookahead
Message-ID: <CA+J6zkQFsTA3QfU5VVjQ=KhJCg_pCrTgW9zinAUC4D9YwsyOkQ@mail.gmail.com>
In-Reply-To: <20260704-ps-pre-commit-indent-v7-2-a94706cc8376@gmail.com>

On Sat, 4 Jul 2026 at 14:24, Pablo Sabater <pabloosabaterr@gmail.com> wrote:
>
> In a subsequent commit the graph renderer needs to know if the next
> commit is a visual root or if it is the last commit to be shown. This
> requires peeking 2 commits ahead.
>
> Commits are pre-fetched at get_revision_internal() where they are also
> marked as SHOWN.
>
> Update graph_is_interesting() so it considers commits inside the
> lookahead as interesting as well.

Nit: lookahead -> lookahead buffer.

> Helped-by: Kristofer Karlsson <krka@spotify.com>
> Signed-off-by: Pablo Sabater <pabloosabaterr@gmail.com>
> ---
>  graph.c    | 47 +++++++++++++++++++++++++++++++++++++++++++++++
>  graph.h    | 17 +++++++++++++++++
>  revision.c | 17 ++++++++++++++++-
>  3 files changed, 80 insertions(+), 1 deletion(-)
>
> diff --git a/graph.c b/graph.c
> index 842282685f..300ae67669 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -315,6 +315,14 @@ struct git_graph {
>          * diff_output_prefix_callback().
>          */
>         struct strbuf prefix_buf;
> +
> +       /*
> +        * Lookahead buffer: up to 2 pre-fetched commits that will be shown.
> +        * Populated by get_revision() so graph_peek_next_visible() can use
> +        * actual walk results instead of peeking at rev_info internals.
> +        */
> +       struct commit *lookahead[2];
> +       int lookahead_nr;
>  };
>
>  static inline int graph_needs_truncation(struct git_graph *graph, int lane)
> @@ -388,6 +396,9 @@ struct git_graph *graph_init(struct rev_info *opt)
>         graph->num_columns = 0;
>         graph->num_new_columns = 0;
>         graph->mapping_size = 0;
> +       graph->lookahead[0] = NULL;
> +       graph->lookahead[1] = NULL;

Style: Manually NULLing out each entry doesn't look quite right to me.
Maybe do something like this instead?

memset(graph->lookahead, 0, sizeof(graph->lookahead));

Although for an array of only two elements, manually NULLing is still quite
readable and avoids the minor function-call overhead of memset().

Feel free to ignore this if you want.

> +       graph->lookahead_nr = 0;
>         /*
>          * Start the column color at the maximum value, since we'll
>          * always increment it for the first commit we output.
> @@ -456,6 +467,15 @@ static void graph_ensure_capacity(struct git_graph *graph, int num_columns)
>   */
>  static int graph_is_interesting(struct git_graph *graph, struct commit *commit)
>  {
> +       /*
> +        * Commits in the lookahead buffer have been pre-fetched by
> +        * get_revision() and will be shown in the future. They already
> +        * have the SHOWN flag set by get_revision_internal(), but the
> +        * graph still needs to treat them as interesting parents.
> +        */
> +       for (int i = 0; i < graph->lookahead_nr; i++)
> +               if (graph->lookahead[i] == commit)
> +                       return 1;
>         /*
>          * If revs->boundary is set, commits whose children have
>          * been shown are always interesting, even if they have the
> @@ -763,6 +783,33 @@ static int graph_needs_pre_commit_line(struct git_graph *graph)
>                graph->expansion_row < graph_num_expansion_rows(graph);
>  }
>
> +struct commit *graph_pop_lookahead(struct git_graph *graph)
> +{
> +       struct commit *c;
> +
> +       if (!graph->lookahead_nr)
> +               return NULL;
> +
> +       c = graph->lookahead[0];
> +       graph->lookahead[0] = graph->lookahead[1];
> +       graph->lookahead[1] = NULL;

Do we need to NULL out the retrieved buffer entries? If so, it is
worthwhile asserting that the entire buffer is NULLed out in the
!graph->lookahead_nr check above.

> +       graph->lookahead_nr--;
> +       return c;
> +}

Not the best engineering practice, but I guess it is fine to constrain
the logic to _only_ a 2-entry buffer since that's what we'll always
deal with anyway.

> +
> +int graph_get_lookahead_room(struct git_graph *graph)
> +{
> +       return 2 - graph->lookahead_nr;

We should use ARRAY_SIZE(graph->lookahead) instead of hardcoding
the value 2.
[snip]