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

[Jeff King <peff@peff.net>]

On Thu, Jun 18, 2026 at 02:42:16PM +0200, Pablo Sabater wrote:

> > > +     for (cl = graph->revs->commits; cl; cl = cl->next) {
> > > +             if (get_commit_action(graph->revs, cl->item) != commit_show)
> > > +                     continue;
> [...]
> > 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.
> 
> Yeah, I haven't read dd4bc01c0a yet but from what you say it prob
> won't work anymore, I didn't know about that series, about the
> lookahead I think it could still work with some tweaks, the important
> part is to set the three lookahead flags.

Thanks for looking into it. I meant to also cc the Kristofer, the author
of dd4bc01c0a, for any thoughts (adding him now).

> From what I understood, we can only get the direct next commit, but no
> more reliably ordered.

Right. There are other queue implementations that could allow full
in-order traversal (e.g., a binary tree), but our prio_queue does not. I
suspect performance for other cases would suffer if we switched the
underlying data structure.

> The flags should be fine:
> 
> - 'is_next_visible' could need to traverse multiple entries, but it
> doesn't need them to be in order. We just need to know if something
> will be rendered after.

Yeah, this one seems easy. We are just setting a bit based on whether
we'd find any commit to show. So order doesn't matter.

> - 'next_has_column' only needs the first entry.

But this was the one I was worried about. Walking the linked list in
order will find us the next commit we're going to show, and the result
of the flag depends on graph_find_new_column_by_commit(). Is it OK to
find _any_ such commit?

(I'm looking at this purely based on reading the existing code, and
haven't really thought hard about the problem space).

> - 'is_next_visual_root' only needs the first entry to know if it could
> be a visual root, and also if it is not the last one (but we don't
> need them to be ordered for this last part).

This one just iterates looking for any other commit we'll show after the
next one. So finding any two entries would be equivalent to the current
code (though we only get to this loop if the first one passes the test
for graph_is_visual_root_candidate).

So if you say order doesn't matter for checking the column and the
visual-root-candidate function, I'm happy to believe you. It makes life
much easier. :)

> Should I work with 'next' as a base to have dd4bc01c0a? (Sorry I've
> just worked with master).

As Junio noted, that's already in master, so I think you're OK to just
base there.

But for future reference, no, you probably don't want to build off of
'next'. If your commit has a dependency on another topic it is best to
build directly off of that topic (and note it in the cover letter of the
series). That way you do not accidentally depend on other things in
'next' which might not ever make it to 'master' (and would thus hold
your topic hostage).

-Peff