From: Pablo Sabater Date: Tue, 07 Jul 2026 06:31:35 GMT Subject: Re: [PATCH v7 2/3] graph: add a 2 commit buffer for lookahead Message-ID: In-Reply-To: El lun, 6 jul 2026 a las 17:33, Chandra Pratap () escribió: > > On Mon, 6 Jul 2026 at 19:15, Kristofer Karlsson wrote: > > > > The hardcoded size-2 lookahead buffer was my suggestion, > > so I am responding inline with my thoughts although Pablo is > > the right person for making further changes (if any). > > > > On Mon, 6 Jul 2026, Chandra Pratap wrote: > > > Do we need to NULL out the retrieved buffer entries? If so, it is > > > worthwhile asserting that the entire buffer is NULLed out in the > > > !graph->lookahead_nr check above. > > > > You're right, it's not technically needed, and there are many places > > in the repo where stale data remains in buffers, and it would be possible > > to do that here too. I don't think it matters much in practice though, > > and NULLing them out would perhaps prevent some accidental reuse on bugs > > (NULL would crash instead). It is not really needed to NULL because every time we access it (pop or the graph_is_interesting()) we are limited by graph->lookahead_nr, however I thought that it is better to have it NULL. Imagine that somehow the lookahead_nr is 1 when it should be 0, having NULL would segfault or if it doesn't at least we are sure that graph_is_interesting() won't re-process as interesting a commit left as stale on the buffer. Anyway, this is just speculation. I think it's better to leave it like this. > > > > As for asserting: rather than checking that empty slots are NULL > > (which just verifies our own cleanup), it might be more useful to > > assert that a slot is non-NULL when lookahead_nr says it should be > > populated, i.e. assert on read rather than on empty. But even that > > may be overkill for a 2-element internal buffer. > > True. But since we're already going through the pains of initializing the > buffer and NULLing it upon a pop, I'd much rather go the extra length > and verify what we're trying to do, shouldn't be that complicated anyway. > > Whether that means checking for NULL here, on a push, or on a read > is something I don't feel strongly about, either is fine with me. About asserting, I think that the best is, because we are popping, to check the first element only just in case we are in the imaginary scenario that lookahead_nr is lying, but because we pop, we don't really care about what's on the second entry. > > > > Not the best engineering practice, but I guess it is fine to constrain > > > the logic to _only_ a 2-entry buffer since that's what we'll always > > > deal with anyway. > > > > I did consider making it a proper ring buffer, but it felt like > > overkill (and I could not find any other existing ring buffer to > > piggy-back on in the repo), and the lookahead depth is > > structurally tied to the algorithm - we only ever need two more > > elements. > > > > It also helps that this is entirely internal to graph.c. If the > > buffer were part of a broader API, a less hardcoded approach > > would be more appropriate indeed. > > Agreed. > > > > We should use ARRAY_SIZE(graph->lookahead) instead of hardcoding > > > the value 2. > > > > Agreed, that is a nice improvement. What do you think Pablo? Yes, I'll do that on reroll. > > > > Thanks, > > Kristofer Not related with this feedback but worth saying: re-reading what's done on revision.c there is this if line: > if (!revs->max_count_stage && !revs->reverse_output_stage) Graph is not compatible with --reverse, so the right-side will always be true. About --max-count, I made a few tests and the lookahead behaves the same regardless of the number of commits to be shown (even if capped). So this whole if block can be dropped and we can try to populate the lookahead buffer always. Thanks both for the feedback and review, Pablo.