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

[Matthew DeVore <matvore@google.com>]

On Tue, Aug 14, 2018 at 11:18 AM Jonathan Tan <jonathantanmy@google.com> wrote:
>
> > @@ -743,6 +743,9 @@ specification contained in <path>.
> >       A debug option to help with future "partial clone" development.
> >       This option specifies how missing objects are handled.
> >  +
> > +The form '--filter=tree:<depth>' omits all blobs and trees deeper than
> > +<depth> from the root tree. Currently, only <depth>=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=<path>' similarly uses a sparse-checkout
 specification contained in <path>.
++
+The form '--filter=tree:<depth>' omits all blobs and trees deeper than
+<depth> from the root tree. Currently, only <depth>=0 is supported.

 --no-filter::
  Turn off any previous `--filter=` argument.
@@ -743,9 +746,6 @@ specification contained in <path>.
  A debug option to help with future "partial clone" development.
  This option specifies how missing objects are handled.
 +
-The form '--filter=tree:<depth>' omits all blobs and trees deeper than
-<depth> from the root tree. Currently, only <depth>=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/t/t5317-pack-objects-filter-objects.sh
b/t/t5317-pack-objects-filter-objects.sh
index 65f2cf446..fe7c13a03 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' '

 test_expect_success 'setup for tests of tree:0' '
  mkdir r1/subtree &&
- echo "This is a file in a subtree" > r1/subtree/file &&
+ echo "This is a file in a subtree" >r1/subtree/file &&
  git -C r1 add subtree/file &&
  git -C r1 commit -m subtree
 '
@@ -95,8 +95,10 @@ test_expect_success 'grab tree directly when using tree:0' '
  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
+ 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
 '

 # Test blob:limit=<n>[kmg] filter.
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index d2859aba1..c3186d934 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -157,7 +157,7 @@ test_expect_success 'partial clone with
transfer.fsckobjects=1 uses index-pack -
 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 &&
+ echo "in dir" >src/dir/file.txt &&
  git -C src add dir/file.txt &&
  git -C src commit -m "file in dir" &&
  git -C src push -u srv master &&
@@ -166,8 +166,32 @@ test_expect_success 'use fsck before and after
manually fetching a missing subtr
  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
+
+ # Make sure we only have commits, and all trees and blobs are missing.
+ 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 &&
+
+ # Auto-fetch a tree with cat-file.
+ git -C dst cat-file -p $SUBTREE >tree_contents &&
+ grep file.txt tree_contents &&
+
+ # fsck still works after an auto-fetch of a tree.
+ git -C dst fsck &&
+
+ # Auto-fetch all remaining trees and blobs with --missing=error
+ git -C dst rev-list master --missing=error --objects >fetched_objects &&
+ test_line_count = 70 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
 '

 test_expect_success 'partial clone fetches blobs pointed to by refs
even if normally filtered out' '
@@ -186,28 +210,6 @@ test_expect_success 'partial clone fetches blobs
pointed to by refs even if norm
  git -C dst fsck
 '

-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
-'
-
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd