Re: [PATCH v6 6/6] list-objects-filter: implement filter tree:0

On Fri, Aug 17, 2018 at 3:28 PM Stefan Beller [off-list ref] wrote:

On Fri, Aug 17, 2018 at 3:20 PM Matthew DeVore [off-list ref] wrote:

quoted

On Fri, Aug 17, 2018 at 2:42 PM Stefan Beller [off-list ref] wrote:

quoted

On Wed, Aug 15, 2018 at 4:23 PM Matthew DeVore [off-list ref] wrote:

quoted

Teach list-objects the "tree:0" filter which allows for filtering
out all tree and blob objects (unless other objects are explicitly
specified by the user). The purpose of this patch is to allow smaller
partial clones.

The name of this filter - tree:0 - does not explicitly specify that
it also filters out all blobs, but this should not cause much confusion
because blobs are not at all useful without the trees that refer to
them.

I also consider only:commits as a name, but this is inaccurate because
it suggests that annotated tags are omitted, but actually they are
included.

Speaking of tag objects, it is possible to tag anything, including blobs.
Would a blob that is tagged (hence reachable without a tree) be not
filtered by tree:0 (or in the future any deeper depth) ?

I think so. If I try to fetch a tagged tree or blob, it should fetch
that object itself, since I'm referring to it explicitly in the git
pack-objects arguments (I mention fetch since git rev-list apparently
doesn't support specifying non-commits on the command line). This is
similar to how I can fetch a commit that would otherwise be filtered
*if* I specify it explicitly (rather than a child commit).

If you're fetching a tagged tree, then for depth=0, it will fetch the
given tree only, and not fetch any referents of an explicitly-given
tree. For depth=1, it will fetch all direct referents.

If you're fetching a commit, then for depth=0, you will not get any
tree objects, and for depth=1, you'll get only the root tree object
and none of its referrents. So the commit itself is like a "layer" in
the depth count.

That seems smart. Thanks!

quoted

quoted

I found this series a good read, despite my unfamiliarity of the
partial cloning.

One situation where I scratched my head for a second were previous patches
that  use "test_line_count = 0 rev_list_err" whereas using test_must_be_empty
would be an equally good choice (I am more used to the latter than the former)

Done. Here is an interdiff (sorry, the tab characters are not
maintained by my mail client):

heh. Thanks for switching the style; I should have emphasized that
(after reflection) I found them equally good, I am used to one
over the other more.

It seems marginally better to me. I also noticed a clean-up patch
going by that aggressively switched to test_must_be_empty wherever
possible: https://public-inbox.org/git/20180819215725.29001-1-szeder.dev@gmail.com/

OTOH, if it were up to me I would have just gotten rid of
test_must_be_empty and used an existing function with the right
argument, like `test_cmp /dev/null` - but using some form consistently
is the most important, whatever it is.

So if that is the only issue brought up, I would not even ask for a resend.

Thanks,
Stefan