Re: [PATCH] upload-pack: disable commit graph more gently for shallow traversal

[Jeff King <peff@peff.net>]

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