Re: [PATCH v11 8/8] list-objects-filter: implement filter tree:0
On Sat, Oct 6, 2018 at 5:10 PM Junio C Hamano wrote:
>
> As output made inside test_expect_{succcess,failure} are discarded
> by default and shown while debugging tests, there is no strong
> reason to use "grep -q" in our tests. I saw a few instances of
> "grep -q" added in this series including this one
>
> test_must_fail grep -q "$file_4" observed
>
> that should probably be
>
> ! grep "$file_4" observed
Yeah, I remember I read in the testing guidelines that you should just
use ! for non-Git commands since it's not our job to make sure these
tools are not crashing. Thank you for pointing this out.
>
> > + printf "blob\ncommit\ntree\n" >unique_types.expected &&
> > ...
> > + printf "blob\ntree\n" >expected &&
>
> Using test_write_lines is probably easier to read.
Done. Below is an interdiff. Let me know if you want a reroll soon.
Otherwise, I will send one later this week.
- Matt
diff --git a/t/t5317-pack-objects-filter-objects.sh
b/t/t5317-pack-objects-filter-objects.sh
index 510d3537f..d9dccf4d4 100755
--- a/t/t5317-pack-objects-filter-objects.sh
+++ b/t/t5317-pack-objects-filter-objects.sh
@@ -69,7 +69,7 @@ test_expect_success 'get an error for missing tree object' '
test_must_fail git -C r5 pack-objects --rev --stdout
2>bad_tree <<-EOF &&
HEAD
EOF
-grep -q "bad tree object" bad_tree
+grep "bad tree object" bad_tree
'
test_expect_success 'setup for tests of tree:0' '
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 53fbf7db8..392caa08f 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -192,7 +192,7 @@ test_expect_success 'use fsck before and after
manually fetching a missing subtr
xargs -n1 git -C dst cat-file -t >fetched_types &&
sort -u fetched_types >unique_types.observed &&
-printf "blob\ncommit\ntree\n" >unique_types.expected &&
+test_write_lines blob commit tree >unique_types.expected &&
test_cmp unique_types.expected unique_types.observed
'
diff --git a/t/t6112-rev-list-filters-objects.sh
b/t/t6112-rev-list-filters-objects.sh
index c8e3d87c4..08e0c7db6 100755
--- a/t/t6112-rev-list-filters-objects.sh
+++ b/t/t6112-rev-list-filters-objects.sh
@@ -38,8 +38,8 @@ test_expect_success 'specify blob explicitly
prevents filtering' '
awk -f print_2.awk) &&
git -C r1 rev-list --objects --filter=blob:none HEAD $file_3
>observed &&
-grep -q "$file_3" observed &&
-test_must_fail grep -q "$file_4" observed
+grep "$file_3" observed &&
+! grep "$file_4" observed
'
test_expect_success 'verify emitted+omitted == all' '
@@ -240,7 +240,7 @@ test_expect_success 'verify tree:0 includes trees
in "filtered" output' '
xargs -n1 git -C r3 cat-file -t >unsorted_filtered_types &&
sort -u unsorted_filtered_types >filtered_types &&
-printf "blob\ntree\n" >expected &&
+test_write_lines blob tree >expected &&
test_cmp expected filtered_types
'
Re: [PATCH v11 8/8] list-objects-filter: implement filter tree:0
Matthew DeVore writes:
> 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
Thanks.
> diff --git a/Documentation/rev-list-options.txt
> b/Documentation/rev-list-options.txt
> index 7b273635d..5f1672913 100644
> --- a/Documentation/rev-list-options.txt
> +++ b/Documentation/rev-list-options.txt
> @@ -731,6 +731,11 @@ 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 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.
OK.
> diff --git a/t/t5317-pack-objects-filter-objects.sh
> b/t/t5317-pack-objects-filter-objects.sh
> index 9839b48c1..510d3537f 100755
> --- a/t/t5317-pack-objects-filter-objects.sh
> +++ b/t/t5317-pack-objects-filter-objects.sh
> @@ -72,6 +72,34 @@ test_expect_success 'get an error for missing tree object'
> '
> grep -q "bad tree object" bad_tree
> '
As output made inside test_expect_{succcess,failure} are discarded
by default and shown while debugging tests, there is no strong
reason to use "grep -q" in our tests. I saw a few instances of
"grep -q" added in this series including this one
test_must_fail grep -q "$file_4" observed
that should probably be
! grep "$file_4" observed
> + printf "blob\ncommit\ntree\n" >unique_types.expected &&
> ...
> + printf "blob\ntree\n" >expected &&
Using test_write_lines is probably easier to read.
Other than these two minor classes of nits, the series look quite
well cooked to me.
Thanks.
[PATCH v11 8/8] 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 considered 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 | 5 +++
list-objects-filter-options.c | 13 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 ++
t/t5317-pack-objects-filter-objects.sh | 28 +++
t/t5616-partial-clone.sh | 42 ++
t/t6112-rev-list-filters-objects.sh| 15
7 files changed, 153 insertions(+)
diff --git a/Documentation/rev-list-options.txt
b/Documentation/rev-list-options.txt
index 7b273635d..5f1672913 100644
--- a/Documentation/rev-list-options.txt
+++ b/Documentation/rev-list-options.txt
@@ -731,6 +731,11 @@ 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 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.
diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index d259bdb2c..e8da2e858 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -49,6 +49,19 @@ static int gently_parse_list_objects_filter(
return 0;
}
+ } else if (skip_prefix(arg, "tree:", )) {
+ unsigned long depth;
+ if (!git_parse_ulong(v0, ) || depth != 0) {
+ if (errbuf) {
+ strbuf_addstr(
+ errbuf,
+ _("only 'tree:0' is supported"));
+ }
+ return 1;
+ }
+ 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 5f8b1a002..09b2b05d5 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -79,6 +79,54 @@ 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:
+ BUG("unknown filter_situation: %d", filter_situation);
+
+ 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;
+
