On Wed, Sep 11, 2019 at 10:08:48PM -0400, Taylor Blau wrote: > > 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
