Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Wed, Aug 15, 2018 at 9:17 AM Junio C Hamano wrote: > > Jeff King writes: > > > Right, I'd agree they probably want the minimum for that traversal. And > > for `rev-list --filter`, that's probably OK. But keep in mind the main > > goal for --filter is using it for fetches, and many servers do not > > perform the traversal at all. Instead they use reachability bitmaps to > > come up with the set of objects to send. The bitmaps have enough > > information to say "remove all trees from the set", but not enough to do > > any kind of depth-based calculation (not even "is this a root tree"). > > If the depth-based cutoff turns out to make sense (on which I > haven't formed an opinion yet), newer version of pack bitmaps could > store that information ;-) > > How are these "fitler" expressions negotiated between the fetcher > and uploader? Does a "fetch-patch" say "am I allowed to ask you to > filter with tree:4?" and refrain from using the option when > "upload-pack" says "no"? I couldn't find a feature like that for the existing features, but adding such a think seems reasonable to me. (thinking in terms of protocol v2,) There could be a filter-check command which takes a single argument: the filter string (like "tree:4"), and responds with a single line: either "ok" or "unsupported".
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Wed, Aug 15, 2018 at 9:14 AM Junio C Hamano wrote:
>
> Matthew DeVore writes:
>
> > Thank you. I changed it to this:
> > awk -e "/tree|blob/{print \$1}" objs >trees_and_blobs
>
> The "-e" option does not appear in
>
> http://pubs.opengroup.org/onlinepubs/9699919799/utilities/awk.html
>
> and I think you can safely drop it from your command line.
Fixed it, thank you. It will be in the next patchset version.
>
> If no -f option is specified, the first operand to awk shall be the
> text of the awk program. The application shall supply the program
> operand as a single argument to awk. If the text does not end in a
> , awk shall interpret the text as if it did.
>
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
Jeff King writes: > Right, I'd agree they probably want the minimum for that traversal. And > for `rev-list --filter`, that's probably OK. But keep in mind the main > goal for --filter is using it for fetches, and many servers do not > perform the traversal at all. Instead they use reachability bitmaps to > come up with the set of objects to send. The bitmaps have enough > information to say "remove all trees from the set", but not enough to do > any kind of depth-based calculation (not even "is this a root tree"). If the depth-based cutoff turns out to make sense (on which I haven't formed an opinion yet), newer version of pack bitmaps could store that information ;-) How are these "fitler" expressions negotiated between the fetcher and uploader? Does a "fetch-patch" say "am I allowed to ask you to filter with tree:4?" and refrain from using the option when "upload-pack" says "no"?
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
Matthew DeVore writes:
> Thank you. I changed it to this:
> awk -e "/tree|blob/{print \$1}" objs >trees_and_blobs
The "-e" option does not appear in
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/awk.html
and I think you can safely drop it from your command line.
If no -f option is specified, the first operand to awk shall be the
text of the awk program. The application shall supply the program
operand as a single argument to awk. If the text does not end in a
, awk shall interpret the text as if it did.
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Tue, Aug 14, 2018 at 04:55:34PM -0700, Matthew DeVore wrote: > > - it's expensive to compute, because you have to actually walk all of > > the possible commits and trees that could refer to it. This > > prohibits a lot of other optimizations like reachability bitmaps > > (though with some complexity you could cache the depths, too). > I think what the user likely wants is to use the minimum depth based > on the commits in the traversal, not every commit in the repo - is > this what you mean? Right, I'd agree they probably want the minimum for that traversal. And for `rev-list --filter`, that's probably OK. But keep in mind the main goal for --filter is using it for fetches, and many servers do not perform the traversal at all. Instead they use reachability bitmaps to come up with the set of objects to send. The bitmaps have enough information to say "remove all trees from the set", but not enough to do any kind of depth-based calculation (not even "is this a root tree"). > Makes sense. I changed it like this - > > diff --git a/Documentation/rev-list-options.txt > b/Documentation/rev-list-options.txt > index 0b5f77ad3..5f1672913 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -732,8 +732,10 @@ the requested refs. > The form '--filter=sparse:path=' similarly uses a sparse-checkout > specification contained in . > + > -The form '--filter=tree:' omits all blobs and trees deeper than > - from the root tree. Currently, only =0 is supported. > +The form '--filter=tree:' omits all blobs and trees whose depth > +from the root tree is >= (minimum depth if an object is located > +at multiple depths in the commits traversed). Currently, only =0 > +is supported, which omits all blobs and trees. Yes, I think that makes sense. Thanks! -Peff
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Tue, Aug 14, 2018 at 1:01 PM Jeff King wrote: > > On Tue, Aug 14, 2018 at 10:28:13AM -0700, Matthew DeVore wrote: > > > The name "tree:0" allows later filtering based on depth, i.e. "tree:1" > > would filter out all but the root tree and blobs. In order to avoid > > confusion between 0 and capital O, the documentation was worded in a > > somewhat round-about way that also hints at this future improvement to > > the feature. > > I'm OK with this as a name, since we're explicitly not supporting deeper > depths. But I'd note that "depth" is actually a tricky characteristic, > as it's not a property of the object itself, but rather who refers to > it. So: > > - it's expensive to compute, because you have to actually walk all of > the possible commits and trees that could refer to it. This > prohibits a lot of other optimizations like reachability bitmaps > (though with some complexity you could cache the depths, too). I think what the user likely wants is to use the minimum depth based on the commits in the traversal, not every commit in the repo - is this what you mean? > > - you have to define it as something like "the minimum depth at which > this object is found", since there may be multiple depths > > I think you can read that second definition between the lines of: > > > +The form '--filter=tree:' omits all blobs and trees deeper than > > + from the root tree. Currently, only =0 is supported. > > But I wonder if we should be more precise. It doesn't matter now, but it > may help set expectations if the feature does come later. > Makes sense. I changed it like this - diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 0b5f77ad3..5f1672913 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -732,8 +732,10 @@ the requested refs. The form '--filter=sparse:path=' similarly uses a sparse-checkout specification contained in . + -The form '--filter=tree:' omits all blobs and trees deeper than - from the root tree. Currently, only =0 is supported. +The form '--filter=tree:' omits all blobs and trees whose depth +from the root tree is >= (minimum depth if an object is located +at multiple depths in the commits traversed). Currently, only =0 +is supported, which omits all blobs and trees. --no-filter:: Turn off any previous `--filter=` argument. > -Peff
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Tue, Aug 14, 2018 at 1:55 PM Junio C Hamano wrote:
>
> Jonathan Tan writes:
>
> >> - grep -E "tree|blob" objs >trees_and_blobs &&
> >> - test_line_count = 1 trees_and_blobs
> >> + grep -E "tree|blob" objs \
> >> + | awk -f print_1.awk >trees_and_blobs &&
> >> + git -C r1 rev-parse HEAD: >expected &&
> >> + test_cmp trees_and_blobs expected
> >
> > Indent "| awk" (and similar lines in this patch) - although I guess it
> > is likely that you actually have it indented, and your e-mail client
> > modified the whitespace so that it looks like there is no indent.
>
> No, wrap lines like this
>
> command1 arg1 arg2 |
> command2 arg1 arg2 &&
>
> That way, you do not need backslash to continue line.
>
> Also think twice when you are seeing yourself piping output from
> "grep" to more powerful tools like "perl", "awk" or "sed". Often
> you can lose the upstream "grep".
Thank you. I changed it to this:
awk -e "/tree|blob/{print \$1}" objs >trees_and_blobs
About the line wrapping strategy, the files I've edited all did it
with the \ and the | at the start of subsequent lines - so I made my
code match that style. Otherwise I would have liked to use the style
you suggest...
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
Jonathan Tan writes: >> - grep -E "tree|blob" objs >trees_and_blobs && >> - test_line_count = 1 trees_and_blobs >> + grep -E "tree|blob" objs \ >> + | awk -f print_1.awk >trees_and_blobs && >> + git -C r1 rev-parse HEAD: >expected && >> + test_cmp trees_and_blobs expected > > Indent "| awk" (and similar lines in this patch) - although I guess it > is likely that you actually have it indented, and your e-mail client > modified the whitespace so that it looks like there is no indent. No, wrap lines like this command1 arg1 arg2 | command2 arg1 arg2 && That way, you do not need backslash to continue line. Also think twice when you are seeing yourself piping output from "grep" to more powerful tools like "perl", "awk" or "sed". Often you can lose the upstream "grep".
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
> - grep -E "tree|blob" objs >trees_and_blobs && > - test_line_count = 1 trees_and_blobs > + grep -E "tree|blob" objs \ > + | awk -f print_1.awk >trees_and_blobs && > + git -C r1 rev-parse HEAD: >expected && > + test_cmp trees_and_blobs expected Indent "| awk" (and similar lines in this patch) - although I guess it is likely that you actually have it indented, and your e-mail client modified the whitespace so that it looks like there is no indent. Other than that, this interdiff looks good to me. Thanks.
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Tue, Aug 14, 2018 at 10:28:13AM -0700, Matthew DeVore wrote: > The name "tree:0" allows later filtering based on depth, i.e. "tree:1" > would filter out all but the root tree and blobs. In order to avoid > confusion between 0 and capital O, the documentation was worded in a > somewhat round-about way that also hints at this future improvement to > the feature. I'm OK with this as a name, since we're explicitly not supporting deeper depths. But I'd note that "depth" is actually a tricky characteristic, as it's not a property of the object itself, but rather who refers to it. So: - it's expensive to compute, because you have to actually walk all of the possible commits and trees that could refer to it. This prohibits a lot of other optimizations like reachability bitmaps (though with some complexity you could cache the depths, too). - you have to define it as something like "the minimum depth at which this object is found", since there may be multiple depths I think you can read that second definition between the lines of: > +The form '--filter=tree:' omits all blobs and trees deeper than > + from the root tree. Currently, only =0 is supported. But I wonder if we should be more precise. It doesn't matter now, but it may help set expectations if the feature does come later. -Peff
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
On Tue, Aug 14, 2018 at 11:18 AM Jonathan Tan wrote:
>
> > @@ -743,6 +743,9 @@ specification contained in .
> > A debug option to help with future "partial clone" development.
> > This option specifies how missing objects are handled.
> > +
> > +The form '--filter=tree:' omits all blobs and trees deeper than
> > + from the root tree. Currently, only =0 is supported.
> > ++
> > The form '--missing=error' requests that rev-list stop with an error if
> > a missing object is encountered. This is the default action.
> > +
>
> The "--filter" documentation should go with the other "--filter"
> information, not right after --missing.
Fixed. My problem was that I didn't know what the + meant - I guess it
means that the paragraph before and after are in the same section?
>
> > +test_expect_success 'setup for tests of tree:0' '
> > + mkdir r1/subtree &&
> > + echo "This is a file in a subtree" > r1/subtree/file &&
> > + git -C r1 add subtree/file &&
> > + git -C r1 commit -m subtree
> > +'
>
> Style: no space after >
Fixed.
>
> > +test_expect_success 'grab tree directly when using tree:0' '
> > + # We should get the tree specified directly but not its blobs or
> > subtrees.
> > + git -C r1 pack-objects --rev --stdout --filter=tree:0
> > >commitsonly.pack <<-EOF &&
> > + HEAD:
> > + EOF
> > + git -C r1 index-pack ../commitsonly.pack &&
> > + git -C r1 verify-pack -v ../commitsonly.pack >objs &&
> > + grep -E "tree|blob" objs >trees_and_blobs &&
> > + test_line_count = 1 trees_and_blobs
> > +'
>
> Can we also verify that the SHA-1 in trees_and_blobs is what we
> expected?
Done - Now I'm comparing to the output of `git rev-parse HEAD:` and I
don't need the separate line count check either.
>
> > +test_expect_success 'use fsck before and after manually fetching a missing
> > subtree' '
> > + # push new commit so server has a subtree
> > + mkdir src/dir &&
> > + echo "in dir" > src/dir/file.txt &&
>
> No space after >
Fixed.
>
> > + git -C src add dir/file.txt &&
> > + git -C src commit -m "file in dir" &&
> > + git -C src push -u srv master &&
> > + SUBTREE=$(git -C src rev-parse HEAD:dir) &&
> > +
> > + rm -rf dst &&
> > + git clone --no-checkout --filter=tree:0 "file://$(pwd)/srv.bare" dst
> > &&
> > + git -C dst fsck &&
> > + git -C dst cat-file -p $SUBTREE >tree_contents 2>err &&
> > + git -C dst fsck
> > +'
>
> If you don't need to redirect to err, don't do so.
>
> Before the cat-file, also verify that the tree is missing, most likely
> through a "git rev-list" with "--missing=print".
That won't work though - the subtree's hash is not known because its
parent tree is not there. I've merged the three tests in this file,
and as a result am now using the check which makes sure the object
types are only "commit"
>
> And I would grep on the tree_contents to ensure that the filename
> ("file.txt") is there, so that we know that we got the correct tree.
Done.
>
> > +test_expect_success 'can use tree:0 to filter partial clone' '
> > + rm -rf dst &&
> > + git clone --no-checkout --filter=tree:0 "file://$(pwd)/srv.bare" dst
> > &&
> > + git -C dst rev-list master --missing=allow-any --objects
> > >fetched_objects &&
> > + cat fetched_objects \
> > + | awk -f print_1.awk \
> > + | xargs -n1 git -C dst cat-file -t >fetched_types &&
> > + sort fetched_types -u >unique_types.observed &&
> > + echo commit > unique_types.expected &&
> > + test_cmp unique_types.observed unique_types.expected
> > +'
> > +
> > +test_expect_success 'auto-fetching of trees with --missing=error' '
> > + git -C dst rev-list master --missing=error --objects >fetched_objects
> > &&
> > + cat fetched_objects \
> > + | awk -f print_1.awk \
> > + | xargs -n1 git -C dst cat-file -t >fetched_types &&
> > + sort fetched_types -u >unique_types.observed &&
> > + printf "blob\ncommit\ntree\n" >unique_types.expected &&
> > + test_cmp unique_types.observed unique_types.expected
> > +'
>
> These two tests seem redundant with the 'use fsck before and after
> manually fetching a missing subtree' test (after the latter is
> appropriately renamed). I think we only need to test this sequence once,
> which can be placed in one or spread over multiple tests:
>
> 1. partial clone with --filter=tree:0
> 2. fsck works
> 3. verify that trees are indeed missing
> 4. autofetch a tree
> 5. fsck still works
Done - that's much nicer. Thanks!
Here is an interdiff from v4 of the patch:
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 9e351ec2a..0b5f77ad3 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -731,6 +731,9 @@ the requested refs.
+
The form '--filter=sparse:path=' similarly uses a sparse-checkout
specification contained in .
++
+The form '--filter=tree:' omits all
Re: [PATCH v4 6/6] list-objects-filter: implement filter tree:0
> @@ -743,6 +743,9 @@ specification contained in .
> A debug option to help with future "partial clone" development.
> This option specifies how missing objects are handled.
> +
> +The form '--filter=tree:' omits all blobs and trees deeper than
> + from the root tree. Currently, only =0 is supported.
> ++
> The form '--missing=error' requests that rev-list stop with an error if
> a missing object is encountered. This is the default action.
> +
The "--filter" documentation should go with the other "--filter"
information, not right after --missing.
> +test_expect_success 'setup for tests of tree:0' '
> + mkdir r1/subtree &&
> + echo "This is a file in a subtree" > r1/subtree/file &&
> + git -C r1 add subtree/file &&
> + git -C r1 commit -m subtree
> +'
Style: no space after >
> +test_expect_success 'grab tree directly when using tree:0' '
> + # We should get the tree specified directly but not its blobs or
> subtrees.
> + git -C r1 pack-objects --rev --stdout --filter=tree:0 >commitsonly.pack
> <<-EOF &&
> + HEAD:
> + EOF
> + git -C r1 index-pack ../commitsonly.pack &&
> + git -C r1 verify-pack -v ../commitsonly.pack >objs &&
> + grep -E "tree|blob" objs >trees_and_blobs &&
> + test_line_count = 1 trees_and_blobs
> +'
Can we also verify that the SHA-1 in trees_and_blobs is what we
expected?
> +test_expect_success 'use fsck before and after manually fetching a missing
> subtree' '
> + # push new commit so server has a subtree
> + mkdir src/dir &&
> + echo "in dir" > src/dir/file.txt &&
No space after >
> + git -C src add dir/file.txt &&
> + git -C src commit -m "file in dir" &&
> + git -C src push -u srv master &&
> + SUBTREE=$(git -C src rev-parse HEAD:dir) &&
> +
> + rm -rf dst &&
> + git clone --no-checkout --filter=tree:0 "file://$(pwd)/srv.bare" dst &&
> + git -C dst fsck &&
> + git -C dst cat-file -p $SUBTREE >tree_contents 2>err &&
> + git -C dst fsck
> +'
If you don't need to redirect to err, don't do so.
Before the cat-file, also verify that the tree is missing, most likely
through a "git rev-list" with "--missing=print".
And I would grep on the tree_contents to ensure that the filename
("file.txt") is there, so that we know that we got the correct tree.
> +test_expect_success 'can use tree:0 to filter partial clone' '
> + rm -rf dst &&
> + git clone --no-checkout --filter=tree:0 "file://$(pwd)/srv.bare" dst &&
> + git -C dst rev-list master --missing=allow-any --objects
> >fetched_objects &&
> + cat fetched_objects \
> + | awk -f print_1.awk \
> + | xargs -n1 git -C dst cat-file -t >fetched_types &&
> + sort fetched_types -u >unique_types.observed &&
> + echo commit > unique_types.expected &&
> + test_cmp unique_types.observed unique_types.expected
> +'
> +
> +test_expect_success 'auto-fetching of trees with --missing=error' '
> + git -C dst rev-list master --missing=error --objects >fetched_objects &&
> + cat fetched_objects \
> + | awk -f print_1.awk \
> + | xargs -n1 git -C dst cat-file -t >fetched_types &&
> + sort fetched_types -u >unique_types.observed &&
> + printf "blob\ncommit\ntree\n" >unique_types.expected &&
> + test_cmp unique_types.observed unique_types.expected
> +'
These two tests seem redundant with the 'use fsck before and after
manually fetching a missing subtree' test (after the latter is
appropriately renamed). I think we only need to test this sequence once,
which can be placed in one or spread over multiple tests:
1. partial clone with --filter=tree:0
2. fsck works
3. verify that trees are indeed missing
4. autofetch a tree
5. fsck still works
[PATCH v4 6/6] list-objects-filter: implement filter tree:0
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.
The name "tree:0" allows later filtering based on depth, i.e. "tree:1"
would filter out all but the root tree and blobs. In order to avoid
confusion between 0 and capital O, the documentation was worded in a
somewhat round-about way that also hints at this future improvement to
the feature.
Signed-off-by: Matthew DeVore
---
Documentation/rev-list-options.txt | 3 ++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 50 ++
t/t5317-pack-objects-filter-objects.sh | 27 ++
t/t5616-partial-clone.sh | 38
t/t6112-rev-list-filters-objects.sh| 13 +++
7 files changed, 136 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 7b273635d..9e351ec2a 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -743,6 +743,9 @@ specification contained in .
A debug option to help with future "partial clone" development.
This option specifies how missing objects are handled.
+
+The form '--filter=tree:' omits all blobs and trees deeper than
+ from the root tree. Currently, only =0 is supported.
++
The form '--missing=error' requests that rev-list stop with an error if
a missing object is encountered. This is the default action.
+
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index c0e2bd6a0..a28382940 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -50,6 +50,10 @@ static int gently_parse_list_objects_filter(
return 0;
}
+ } else if (!strcmp(arg, "tree:0")) {
+ filter_options->choice = LOFC_TREE_NONE;
+ return 0;
+
} else if (skip_prefix(arg, "sparse:oid=", )) {
struct object_context oc;
struct object_id sparse_oid;
diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h
index a61f8..af64e5c66 100644
--- a/list-objects-filter-options.h
+++ b/list-objects-filter-options.h
@@ -10,6 +10,7 @@ enum list_objects_filter_choice {
LOFC_DISABLED = 0,
LOFC_BLOB_NONE,
LOFC_BLOB_LIMIT,
+ LOFC_TREE_NONE,
LOFC_SPARSE_OID,
LOFC_SPARSE_PATH,
LOFC__COUNT /* must be last */
diff --git a/list-objects-filter.c b/list-objects-filter.c
index a0ba78b20..8e3caf5bf 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -80,6 +80,55 @@ static void *filter_blobs_none__init(
return d;
}
+/*
+ * A filter for list-objects to omit ALL trees and blobs from the traversal.
+ * Can OPTIONALLY collect a list of the omitted OIDs.
+ */
+struct filter_trees_none_data {
+ struct oidset *omits;
+};
+
+static enum list_objects_filter_result filter_trees_none(
+ enum list_objects_filter_situation filter_situation,
+ struct object *obj,
+ const char *pathname,
+ const char *filename,
+ void *filter_data_)
+{
+ struct filter_trees_none_data *filter_data = filter_data_;
+
+ switch (filter_situation) {
+ default:
+ die("unknown filter_situation");
+ return LOFR_ZERO;
+
+ case LOFS_BEGIN_TREE:
+ case LOFS_BLOB:
+ if (filter_data->omits)
+ oidset_insert(filter_data->omits, >oid);
+ return LOFR_MARK_SEEN; /* but not LOFR_DO_SHOW (hard omit) */
+
+ case LOFS_END_TREE:
+ assert(obj->type == OBJ_TREE);
+ return LOFR_ZERO;
+
+ }
+}
+
+static void* filter_trees_none__init(
+ struct oidset *omitted,
+ struct list_objects_filter_options *filter_options,
+ filter_object_fn *filter_fn,
+ filter_free_fn *filter_free_fn)
+{
+ struct filter_trees_none_data *d = xcalloc(1, sizeof(*d));
+ d->omits = omitted;
+
+ *filter_fn = filter_trees_none;
+ *filter_free_fn = free;
+ return d;
+}
+
/*
* A filter for list-objects to omit large blobs.
* And to OPTIONALLY collect a list of the omitted OIDs.
@@ -374,6 +423,7 @@ static filter_init_fn s_filters[] = {
NULL,
filter_blobs_none__init,
filter_blobs_limit__init,
+ filter_trees_none__init,
filter_sparse_oid__init,
filter_sparse_path__init,
