On Thu, Mar 24, 2022 at 11:27:34AM -0400, Derrick Stolee wrote:
> > This sounds quite tricky. In this case we may know which commit we
> > need to avoid (re)parsing to avoid the bug, but would it always be
> > the case? It feels almost like we want to unparse the commit
> > objects when we clear the grafts information in the previous patch,
> > doesn't it?
>
> I agree that the adjustment to paint_down_to_common() is a bit too
> coupled to this scenario, when we should just trust our commits to
> be valid if they have been parsed. We should always be able to
> parse our parents.
>
> Finding this bug is interesting, but I agree with Junio that a
> better fix would be to "unparse" a commit when modifying its graft
> in any way. That will universally fix it for any potential future
> commit walks that might be hit due to future code changes.
I agree completely with you both.
This made me think of some of the difficulties we encountered back in
ce16364e89 (commit.c: don't persist substituted parents when
unshallowing, 2020-07-08), particularly:
One way to fix this would be to reset the parsed object pool entirely
(flushing the cache and thus preventing subsequent reads from modifying
their parents) after unshallowing. That would produce a problem when
callers have a now-stale reference to the old pool, and so this patch
implements a different approach.
if I can recall back to when that patch was written, I think the issue
was that dumping the entire set of parsed objects caused us to have
stale references in the commit-graph machinery.
I'm not sure whether or not the same difficulties would be encountered
here, though. The shallow stuff is so tricky to me...
Thanks,
Taylor
