From: Kristofer Karlsson Date: Fri, 19 Jun 2026 07:34:16 GMT Subject: Re: [PATCH v5 2/2] graph: indent visual root in graph Message-ID: In-Reply-To: <20260618160504.GA818042@coredump.intra.peff.net> On Thu, 18 Jun 2026 at 18:05, Jeff King 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. 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. This would perhaps be an intermediate safety net -- once we have fully rolled it out, those functions could be removed again. 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. Thanks, Kristofer