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

On Tue, Aug 14, 2018 at 11:18 AM Jonathan Tan [off-list ref] wrote:

quoted

@@ -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?

quoted

+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.

quoted

+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.

quoted

+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.

quoted

+     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.

quoted

+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