Re: [PATCH v5 2/2] graph: indent visual root in graph

>
> On Sat, Jun 13, 2026 at 09:09:16PM +0200, Pablo Sabater wrote:
>
> > +/*
> > + * Iterates the commits queue searching for the next visible commit, once found
> > + * sets visibleness and visual-root flags.
> > + * Knowing if the next commit is also a visual root avoids redundant indentations
> > + *
> > + * NEEDSWORK: The queue is actively being modified by the walker, for each commit
> > + * its parents and itself get simplified and their flags set, but for the next
> > + * unrelated commit or the grandparents they are not simplified yet, which means
> > + * that a commit whose parents are all filtered will not be marked as a visual
> > + * root candidate at the lookahead.
> > + * This causes the lookahead to fail, failing to set the cascade flag to avoid
> > + * redundant indentations.
> > + * See 'test_expect_failure' at t4218-log-graph-indentation.sh.
> > + */
> > +static void graph_peek_next_visible(struct git_graph *graph,
> > +                                 struct graph_lookahead_flags *flags)
> > +{
> > +     struct commit_list *cl;
> > +
> > +     flags->is_next_visible = 0;
> > +     flags->is_next_visual_root = 0;
> > +     flags->next_has_column = 0;
> > +
> > +     for (cl = graph->revs->commits; cl; cl = cl->next) {
> > +             if (get_commit_action(graph->revs, cl->item) != commit_show)
> > +                     continue;
> > [...]
>
> I have a feeling this may interact badly with the prio-queue introduced
> by dd4bc01c0a (revision: use priority queue for non-limited streaming
> walks, 2026-05-27). In that commit, get_revision_1() sucks all of the
> commits from revs->commits into revs->commit_queue, and then traversal
> puts the parents into that queue, not the commits list.
>
> So during the traversal, revs->commits does not hold the complete queue
> anymore. I think it does see _some_ commits, since some get placed
> directly into revs->commits and then later moved next time
> get_revision() is called. But if we instrument the code like this:
>
> diff --git a/graph.c b/graph.c
> index e0d1e2a510..8a5f17a089 100644
> --- a/graph.c
> +++ b/graph.c
> @@ -926,6 +926,10 @@ static void graph_peek_next_visible(struct git_graph *graph,
>         flags->is_next_visual_root = 0;
>         flags->next_has_column = 0;
>
> +       warning("peeking at visible commits: %d in list, %d in queue",
> +               commit_list_count(graph->revs->commits),
> +               (int)graph->revs->commit_queue.nr);
> +
>         for (cl = graph->revs->commits; cl; cl = cl->next) {
>                 if (get_commit_action(graph->revs, cl->item) != commit_show)
>                         continue;
>
> and run something like:
>
>   ./git log --graph --oneline -- Makefile
>
> we can see that we're always considering just one commit, while there
> may be dozens or hundreds in the queue.
>
> I'm not sure what the solution is. This function wants to peek ahead in
> queue order, possibly through multiple entries. But a heap-based queue
> inherently only supports peeking at the first entry.

>
> None of the tests seem to fail, but I'm not sure if that's because I'm
> way off base in my analysis, or there's a gap in the test coverage, or
> if this case is part of the expect_failure ones mentioned in the
> comment.
>
> I noticed because I have another topic which drops the revs->commits
> list entirely (and just always uses the queue), which of course doesn't
> compile when merged with this (I merge with 'jch' for my daily driver,
> which now includes this patch).
>
> -Peff