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

[Jeff King <peff@peff.net>]

On Fri, Jun 19, 2026 at 09:34:16AM +0200, Kristofer Karlsson wrote:

> On Thu, 18 Jun 2026 at 18:05, Jeff King <peff@peff.net> wrote:
> >
> > Thanks for looking into it. I meant to also cc the Kristofer, the author
> > of dd4bc01c0a, for any thoughts (adding him now).
> >
> 
> Thanks for the CC. I took a look at how this interacts with my
> change.
> 
> dd4bc01c0a doesn't hurt here I think, but future followup changes
> might. From what I can tell --graph triggers topo_order, so
> the walk mode is either REV_WALK_TOPO or REV_WALK_LIMITED
> and the prio_queue change only applies to REV_WALK_STREAMING.

I'm not so sure. If I merge 53967f242a (graph: indent visual root in
graph, 2026-06-13) into master (so that it has both your commit_queue
changes and Pablo's topic), and then apply 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:

  ./git log --graph -- Makefile

then we always see exactly one commit in the list, but an
ever-increasing number in the queue (up to ~4000). We do seem to be in
REV_WALK_TOPO mode, so I think we'd never return the commits via
get_revision(), but it is weird that we are sticking them in the queue
at all.

Looks like that happens via rewrite_parents(), which always writes into
commit_queue. I guess it doesn't matter because in topo mode we are
always pulling off of the topo_walk_info queue anyway? It does make me
wonder if there is a lurking bug around history simplification and
--topo-order, though.

> That said, graph_peek_next_visible() reaching directly into
> revs->commits feels fragile -- especially if we drop revs->commits
> in the future. One option would be to add a thin abstraction in
> revision.c that dispatches per walk mode, something like:
> 
>     int revision_has_more_commits(struct rev_info *revs)
>     {
>         if (revs->topo_walk_info)
>             return revs->topo_walk_info->topo_queue.nr > 0;
>         return revs->commits != NULL;
>     }
> 
>     struct commit *revision_peek_next_commit(struct rev_info *revs)
>     {
>         if (revs->topo_walk_info)
>             return prio_queue_peek(&revs->topo_walk_info->topo_queue);
>         if (revs->commits)
>             return revs->commits->item;
>         return NULL;
>     }
> 
> That way graph.c does not need to know which data structure the
> walker uses, and if the internals change later the API adapts in
> one place.

Yeah, I agree some abstraction would help. I think it would have to be
full iteration, though; the graph code wants to know if there is any
commit that is actually going to be shown, not just a potential single
next one. So we at least need to be able to iterate in arbitrary order.

> As for the multi-element peek question, I think I would either opt
> for draining into a buffer if it's really needed, though when looking
> at the code here I think multi-element peeking is not truly needed.
> It seems like the logic just checks if there is at least another
> element after the peek, but it does not try to read the actual value,
> so we can just check the queue size instead.

We do look at some characteristics of the commit we find by peeking, but
I'm not sure how much it matters if we get the _next_ commit that will
be shown, or if any arbitrary commit is OK.

-Peff