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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]

  Powered by Linux