From: Kristofer Karlsson Date: Mon, 22 Jun 2026 08:29:33 GMT Subject: Re: [PATCH v5 2/2] graph: indent visual root in graph Message-ID: In-Reply-To: <20260621180556.GD2206349@coredump.intra.peff.net> On Sun, 21 Jun 2026 at 20:05, Jeff King wrote: > 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). > > 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