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

[Jeff King <peff@peff.net>]

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