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

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