From: Pablo Sabater Date: Thu, 18 Jun 2026 12:42:16 GMT Subject: Re: [PATCH v5 2/2] graph: indent visual root in graph Message-ID: In-Reply-To: <20260617202744.GA3465855@coredump.intra.peff.net> El mié, 17 jun 2026 a las 22:27, Jeff King () escribió: > > 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. Hi Jeff! 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. From what I understood, we can only get the direct next commit, but no more reliably ordered. 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. - 'next_has_column' only needs the first entry. - '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). Should I work with 'next' as a base to have dd4bc01c0a? (Sorry I've just worked with master). I'll try to make it work but if not, the lookahead works to avoid _redundant_ indentations, but it would still work correctly without it. > > 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 Thanks, Pablo