Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal
- From
Jeff King <peff@peff.net>
- Date
- Sep 12, 2019, 14:03 UTC
- Message-ID
- <20190912140332.GD23031@sigill.intra.peff.net>
- In-Reply-To
- <20190912020848.GB76228@syl.lan>
On Wed, Sep 11, 2019 at 10:08:48PM -0400, Taylor Blau wrote:
Show 20 quoted lines
> > The test suite passes with my patch both with and without > > GIT_TEST_COMMIT_GRAPH=1. But to my surprise, it also passes if I delete > > the close_commit_graph() line added by 829a321569! > > > > So it's not clear to me whether this whole thing is truly unnecessary > > (and Stolee was just being overly cautious because the code is related > > to shallow-ness, even though it is OK doing a true-parent traversal > > itself), or if we just don't have good test coverage for the case that > > requires it. > > > > If it _is_ necessary, I'm a little worried there are other problems > > lurking. The whole issue is that we've seen and parsed some commits > > before we get to this shallow deepen-since code-path. So just disabling > > commit-graphs isn't enough. Even without them, we might have parsed some > > commits the old-fashioned way and filled in their parent pointers. Is > > that a problem? > > I am, too, but I don't think we should hold this patch up which is > obviously improving the situation in the meantime while we figure that > out.
Yeah, I'd agree here, unless we determine quickly that we do need the bigger fix, and somebody with a bit more knowledge of this shallow code offers a fix. I believe my patch is a strict improvement, and puts the commit-graph code path on par with the regular one.
-Peff