From: Jeff Hostetler Date: Fri, 23 Jun 2017 17:16:30 GMT Subject: Re: [PATCH 1/3] list-objects: add filter_blob to traverse_commit_list Message-ID: <72257781-5a2c-cdd6-3fb3-7c5e6d883653@jeffhostetler.com> In-Reply-To: <20170622151028.07827b50@twelve2.svl.corp.google.com> On 6/22/2017 6:10 PM, Jonathan Tan wrote: > On Thu, 22 Jun 2017 14:45:26 -0700 > Jonathan Tan wrote: > >> On Thu, 22 Jun 2017 20:36:13 +0000 >> Jeff Hostetler wrote: >> >>> From: Jeff Hostetler >>> >>> In preparation for partial/sparse clone/fetch where the >>> server is allowed to omit large/all blobs from the packfile, >>> teach traverse_commit_list() to take a blob filter-proc that >>> controls when blobs are shown and marked as SEEN. >>> >>> Normally, traverse_commit_list() always marks visited blobs >>> as SEEN, so that the show_object() callback will never see >>> duplicates. Since a single blob OID may be referenced by >>> multiple pathnames, we may not be able to decide if a blob >>> should be excluded until later pathnames have been traversed. >>> With the filter-proc, the automatic deduping is disabled. >> >> Comparing this approach (this patch and the next one) against mine [1], >> I see that this has the advantage of (in pack-objects) avoiding the >> invocation of add_preferred_base_object() on blobs that are filtered >> out, avoiding polluting the "to_pack" data structure with information >> that we do not need. >> >> But it does add an extra place where blobs are filtered out (besides >> add_object_entry()), which makes it harder for the reader to keep track >> of what's going on. I took a brief look to see if filtering could be >> eliminated from add_object_entry(), but that function is called from >> many places, so I couldn't tell. >> >> Anyway, I think both approaches will work (this patch's and mine [1]). >> >> [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/ > > Also it should be mentioned somewhere that this does not cover the > bitmap case - a similar discussion should be included in one of the > patches, like I did in [1]. > > And looking back at my original cover letter [2], I wrote: > >> This is similar to [1] except that this > [...] >> is slightly more comprehensive in >> that the read_object_list_from_stdin() codepath is also covered in >> addition to the get_object_list() codepath. (Although, to be clear, >> upload-pack always passes "--revs" and thus only needs the >> get_object_list() codepath). > > (The [1] in the quote above refers to one of Jeff Hostetler's patches, > [QUOTE 1].) > > The same issue applies to this patch (since > read_object_list_from_stdin() invokes add_object_entry() directly > without going through the traversal mechanism) and probably warrants at > least some description in one of the commit messages. > > [1] https://public-inbox.org/git/6f7934621717141ce3bb6bc05cf1d59c7900ccc5.1496432147.git.jonathantanmy@google.com/ > [2] https://public-inbox.org/git/cover.1496361873.git.jonathantanmy@google.com/ > > [QUOTE 1] https://public-inbox.org/git/1488994685-37403-3-git-send-email-jeffhost@microsoft.com/ > Thanks for the quick feedback. I'll dig into each of these comments as I work on my next draft. Jeff