On Wed, Sep 11, 2019 at 08:18:46PM -0400, Jeff King wrote: > On Wed, Sep 11, 2019 at 08:04:14PM -0400, Jeff King wrote: > > > When the client has asked for certain shallow options like > > "deepen-since", we do a custom rev-list walk that pretends to be > > shallow. Before doing so, we have to disable the commit-graph, since it > > is not compatible with the shallow view of the repository. That's > > handled by 829a321569 (commit-graph: close_commit_graph before shallow > > walk, 2018-08-20). That commit literally closes and frees our > > repo->objects->commit_graph struct. > > A few notes and curiosities on my patch and this general area. > > 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. > [snip] Thanks, Taylor
