Re: [PATCH v7 2/3] graph: add a 2 commit buffer for lookahead

>
> El lun, 6 jul 2026 a las 17:33, Chandra Pratap
> (<chandrapratap3519@gmail.com>) escribió:
> >
> > On Mon, 6 Jul 2026 at 19:15, Kristofer Karlsson <krka@spotify.com> 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 <chandrapratap3519@gmail.com> 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.