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

From: Kristofer Karlsson <krka@spotify.com>
Date: Jun 22, 2026, 08:29 UTC
Message-ID: <CAL71e4NCV5uPJ-LsEQmy2R3gAjF8C60E=YL24tTABaxs+QBSXA@mail.gmail.com>
In-Reply-To: <20260621180556.GD2206349@coredump.intra.peff.net>

On Sun, 21 Jun 2026 at 20:05, Jeff King <peff@peff.net> wrote:

Show 5 quoted lines

> 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.

Thanks for the analysis. You are right that rewrite_one() leaks parents into commit_queue that are never consumed in topo mode. I have not explored the graph code very much, so I cannot say how this affects the lookahead.

I am thinking that revs->commits is somewhat multi-purpose -- it serves as initial tips, work queue, topo sort buffer, boundary staging, and reverse output buffer depending on mode and phase. Now that we have two representations (commits and commit_queue) it is both multi-purpose and unclear which one to use. That is not a great situation.

I originally just set out to optimize the prio queue usage and speed up expensive walks, but I think I also need to be a good citizen and help clean up some of the mess that comes with having two separate containers (I am not sure exactly how yet - maybe even adding _more containers but with more semantically clear purpose).

Show 10 quoted lines

> > 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.

I am not sure if arbitrary order is valid - I think simply having an intermediate buffer where the filtering has been applied would be sufficient. I think the peeking approach is wrong - peeking into the second element doesn't work since after processing the first element the second element could have changed. I prototyped something locally that uses a lookahead buffer instead and it seems to work and then we don't need to manually filter on get_commit_action - get_revision_internal will apply the right filtering. Will reply with that finding in the right place though.

Thanks, Kristofer