Re: [PATCH] commit-reach: do not parse and iterate minima

From: Taylor Blau <me@ttaylorr.com>
Date: Mar 24, 2022, 22:06 UTC
Message-ID: <YjzrZNQZ7cBIr+Kx@nand.local>
In-Reply-To: <34da1688-7d94-d54c-6b3a-3106d0f15380@github.com>

On Thu, Mar 24, 2022 at 11:27:34AM -0400, Derrick Stolee wrote:

Show 15 quoted lines

> > 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