On Tue, Jul 05, 2022 at 06:28:47PM -0400, Taylor Blau wrote:
> > This re-writes the commit data to:
> >
> > {
> > graph_pos = 0,
> > generation = 17631
> > }
>
> Nicely explained. To me, it seems like the problem is that we're reusing
> the same slab to:
>
> - store data that we're going to write as a part of commit-graph
> generation, and
> - store auxiliary data about commits that we have *read* from a
> commit-graph
>
> A complete fix might be to use a separate slab to store data that we
> read from data that we are about to write, to avoid polluting the latter
> with the former.
Yeah, that was my first instinct, too. But from a skim of the code, I
think that may be complicated, as the two uses don't seem to be
differentiated well. Or at least it resisted some basic hacky efforts I
made (e.g., splitting commit_graph_data_at() from the more limited
readers). I'm not that familiar with the commit-graph writing code; I
suspect Stolee might have something intelligent to say.
> In the meantime, a more minimal fix would be to avoid reading and
> overwriting the generation data where we can avoid it. AFAICT, this is
> the only spot that would need to be changed. The following patch does
> the trick for me:
Yeah, it fixes it for me, too, but it leaves me with many questions. ;)
Specifically:
- if it's so easy to get the position, why do we have the position in
the slab in the first place? Or is it for the cases where we're
writing (though then the question is: why do we fill it for the read
case, then)?
- if I understand correctly, the real sin here was calling
commit_graph_position() during the write. How can we find
other potential problem spots? From my (again, admittedly not well
developed) view of the code, it seems like an intentional choice
that you could intermingle computed and from-disk results.
At any rate, thanks to both you and Will for moving this forward.
-Peff
