From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:32
This patch series does two things:
a. (patches 1-2) introduces "--filter=only:commits" which filters trees and blobs
b. (patches 3-5) better support for promisor trees in the rev-list command
The intention is to enable initial partial clones to be very tiny by only
including commits. Patches 3-5 are necessary because, even though it has already
been possible to have partial clones with trees missing (there are tests for it),
there have not been any filters which support this yet, so it seemed necessary
to make rev-list handle this case better.
Thank you,
Matthew DeVore (5):
revision: invert meaning of the USER_GIVEN flag
list-objects-filter: implement filter only:commits
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
rev-list: handle missing tree objects properly
Documentation/rev-list-options.txt | 2 +
builtin/rev-list.c | 12 +-
list-objects-filter-options.c | 4 +
list-objects-filter-options.h | 1 +
list-objects-filter.c | 43 +++--
list-objects.c | 226 +++++++++++++------------
revision.c | 1 -
revision.h | 11 +-
t/t5317-pack-objects-filter-objects.sh | 30 ++++
t/t5616-partial-clone.sh | 27 +++
t/t6112-rev-list-filters-objects.sh | 13 ++
11 files changed, 242 insertions(+), 128 deletions(-)
--
2.18.0.597.ga71716f1ad-goog
From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:36
Abandon the previous approach of mutating all new objects implicitly in
add_pending_object by inverting the meaning of the bit (it is now
NOT_USER_GIVEN) and only setting the flag when we need to.
This more accurately tracks if a tree was provided directly by the user.
Without this patch, the root tree of all commits were erroneously
considered to be USER_GIVEN, which meant they cannot be filtered. This
distinction is important in the next patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:40
Teach list-objects the "only:commits" filter which allows for filtering
out all non-commit and non-annotated tag 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 - only:commits - is a bit inaccurate because it
still allows annotated tags to pass through. I chose it because it was
the only concise name I could think of that was pretty descriptive. I
considered and decided against "tree:none" because the code and
documentation for filters seems to lack the concept of "you're filtering
this, so we'll implicitly filter all referents of this." So "tree:none"
is vague, since some may think it filters blobs too, while some may not.
"only:commits" is specific and makes it easier to match it to a
potential use case.
Signed-off-by: Matthew DeVore <redacted>
---
Documentation/rev-list-options.txt | 2 ++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 43 ++++++++++++++++++--------
t/t5317-pack-objects-filter-objects.sh | 30 ++++++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 ++++++++
6 files changed, 80 insertions(+), 13 deletions(-)
@@ -743,6 +743,8 @@ 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=only:commits' omits all blobs and trees.++ The form '--missing=error' requests that rev-list stop with an error if a missing object is encountered. This is the default action. +
@@ -26,38 +26,39 @@#define FILTER_SHOWN_BUT_REVISIT (1<<21)/*-*Afilterforlist-objectstoomitALLblobsfromthetraversal.-*AndtoOPTIONALLYcollectalistoftheomittedOIDs.+*Afilterforlist-objectstoomitALLblobsfromthetraversal,andpossibly+*treesaswell.+*CanOPTIONALLYcollectalistoftheomittedOIDs.*/-structfilter_blobs_none_data{+structfilter_none_of_type_data{+unsignedomit_trees:1;structoidset*omits;};-staticenumlist_objects_filter_resultfilter_blobs_none(+staticenumlist_objects_filter_resultfilter_none_of_type(enumlist_objects_filter_situationfilter_situation,structobject*obj,constchar*pathname,constchar*filename,void*filter_data_){-structfilter_blobs_none_data*filter_data=filter_data_;+structfilter_none_of_type_data*filter_data=filter_data_;switch(filter_situation){default:die("unknown filter_situation");returnLOFR_ZERO;-caseLOFS_BEGIN_TREE:-assert(obj->type==OBJ_TREE);-/* always include all tree objects */-returnLOFR_MARK_SEEN|LOFR_DO_SHOW;-caseLOFS_END_TREE:assert(obj->type==OBJ_TREE);returnLOFR_ZERO;+caseLOFS_BEGIN_TREE:+assert(obj->type==OBJ_TREE);+if(!filter_data->omit_trees)+returnLOFR_MARK_SEEN|LOFR_DO_SHOW;+caseLOFS_BLOB:-assert(obj->type==OBJ_BLOB);assert((obj->flags&SEEN)==0);if(filter_data->omits)
@@ -59,6 +59,36 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'setup for tests of only:commits''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify only:commits packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=only:commits>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack\+|grep-E"tree|blob"\+|sort>observed&&+test_line_count=0observed+'++test_expect_success'grab tree directly when using only:commits''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=only:commits>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack\+|grep-E"tree|blob"\+|sort>observed&&+test_line_count=1observed+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test only:commits filter.++test_expect_success'verify only:commits includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=only:commits\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:42
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 152 +++++++++++++++++++++++--------------------------
1 file changed, 71 insertions(+), 81 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:45
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 73 +++++++++++++++++++++++++++++---------------------
1 file changed, 43 insertions(+), 30 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-09 22:45:47
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 12 ++++++++----
list-objects.c | 8 ++++++--
revision.h | 1 +
t/t5616-partial-clone.sh | 27 +++++++++++++++++++++++++++
4 files changed, 42 insertions(+), 6 deletions(-)
@@ -128,6 +128,7 @@ struct rev_info {first_parent_only:1,line_level_traverse:1,tree_blobs_in_commit_order:1,+show_missing_trees:1,/* for internal use only */exclude_promisor_objects:1;
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use only:commits to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=only:commits"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+ ."$TEST_DIRECTORY"/lib-httpd.sh start_httpd
From: Jonathan Tan <hidden> Date: 2018-08-10 00:14:54
Teach list-objects the "only:commits" filter which allows for filtering
out all non-commit and non-annotated tag 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 - only:commits - is a bit inaccurate because it
still allows annotated tags to pass through. I chose it because it was
the only concise name I could think of that was pretty descriptive. I
considered and decided against "tree:none" because the code and
documentation for filters seems to lack the concept of "you're filtering
this, so we'll implicitly filter all referents of this." So "tree:none"
is vague, since some may think it filters blobs too, while some may not.
"only:commits" is specific and makes it easier to match it to a
potential use case.
I'll do a fuller review tomorrow, but here are my initial thoughts.
I'm undecided about whether "only:commits" or "tree:none" is better -
one argument in favor of the latter is that blobs are not of much use
without any trees referring to them, so it makes sense that omitting
trees means omitting blobs. But that requires some thought and is not
immediately obvious.
/*
- * A filter for list-objects to omit ALL blobs from the traversal.
- * And to OPTIONALLY collect a list of the omitted OIDs.
+ * A filter for list-objects to omit ALL blobs from the traversal, and possibly
+ * trees as well.
+ * Can OPTIONALLY collect a list of the omitted OIDs.
*/
-struct filter_blobs_none_data {
+struct filter_none_of_type_data {
+ unsigned omit_trees : 1;
struct oidset *omits;
};
I know that it's documented above that blobs are always omitted, but
maybe it's worth it to add a comment /* blobs are always omitted */.
- case LOFS_BEGIN_TREE:
- assert(obj->type == OBJ_TREE);
- /* always include all tree objects */
- return LOFR_MARK_SEEN | LOFR_DO_SHOW;
-
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
return LOFR_ZERO;
+ case LOFS_BEGIN_TREE:
+ assert(obj->type == OBJ_TREE);
+ if (!filter_data->omit_trees)
+ return LOFR_MARK_SEEN | LOFR_DO_SHOW;
+
case LOFS_BLOB:
- assert(obj->type == OBJ_BLOB);
assert((obj->flags & SEEN) == 0);
Moving the case LOFS_BEGIN_TREE and removing the assert is unnecessary,
I think.
Also, there's fallthrough. If that's on purpose, add /* fallthrough */,
although I think that it complicates the code unnecessarily here.
Bash pipes conceal return codes. Here it's OK, but it might be better to
write the verify-pack on its own line and then '! grep -E "tree|blob"' -
you don't need to sort or test_line_count.
+test_expect_success 'grab tree directly when using only:commits' '
+ # We should get the tree specified directly but not its blobs or subtrees.
+ git -C r1 pack-objects --rev --stdout --filter=only:commits >commitsonly.pack <<-EOF &&
+ HEAD:
+ EOF
+ git -C r1 index-pack ../commitsonly.pack &&
+ git -C r1 verify-pack -v ../commitsonly.pack \
+ | grep -E "tree|blob" \
+ | sort >observed &&
+ test_line_count = 1 observed
+'
Similar comment as above, except you can redirect the output of grep to
a file, then test_line_count on that file. No need for sort.
Once again, I'll do a fuller review tomorrow.
These are fine (obj->type is populated), because the types of objects
are known during traversal.
- if (obj->type == OBJ_BLOB && !has_object_file(&obj->oid)) {
+ if (!has_object_file(&obj->oid)) {
finish_object__ma(obj);
return 1;
And this is also fine, because finish_object__ma can now handle any
object type.
+ revs.show_missing_trees = 1;
(and elsewhere)
Could we just show missing trees all the time? We do that for blobs and
already rely on the caller (eventually, show_object() in
builtin/rev-list.c) to determine whether the object actually exists or
not; we could do the same for trees. This allows us to not include this
extra knob.
quoted hunk
- if (parse_tree_gently(tree, gently) < 0) {
+ parse_result = parse_tree_gently(tree, gently);
+ if (parse_result < 0 && !revs->show_missing_trees) {
if (revs->ignore_missing_links)
return;
@@ -182,7 +185,8 @@ static void process_tree(struct traversal_context *ctx, if (base->len) strbuf_addch(base, '/');- process_tree_contents(ctx, tree, base);+ if (parse_result >= 0)+ process_tree_contents(ctx, tree, base); if ((obj->flags & NOT_USER_GIVEN) && ctx->filter_fn) { r = ctx->filter_fn(LOFS_END_TREE, obj,
Is it possible to call the appropriate callbacks and then return
immediately, instead of going through the whole function checking
parse_result when necessary? When doing the latter, the reader needs to
keep on checking if each function still works if the tree is
unparseable.
From: Jonathan Tan <hidden> Date: 2018-08-10 18:43:48
Abandon the previous approach of mutating all new objects implicitly in
add_pending_object by inverting the meaning of the bit (it is now
NOT_USER_GIVEN) and only setting the flag when we need to.
This more accurately tracks if a tree was provided directly by the user.
Without this patch, the root tree of all commits were erroneously
considered to be USER_GIVEN, which meant they cannot be filtered. This
distinction is important in the next patch.
After rereading this patch, I think the thought process is:
- the existing code inaccurately makes root trees of commits USER_GIVEN
- instead of trying to fix that, it is easier to invert the meaning of this
flag, and since we only need to track trees and blobs, let's do so in this
patch
So a better commit message might be:
revision: mark non-user-given objects instead
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects are not
supported yet.)
The patch itself looks good to me.
From: Jonathan Tan <hidden> Date: 2018-08-10 19:04:00
Matthew DeVore (5):
revision: invert meaning of the USER_GIVEN flag
list-objects-filter: implement filter only:commits
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
rev-list: handle missing tree objects properly
Firstly, run every patch with "make DEVELOPER=1" - there is at least one
"mixed declarations and code", which the Git coding style does not
allow.
I've already replied to patches 1, 2, and 5. Patches 3 and 4 look OK to
me and seem like good changes (patch 4, in addition to reducing
indentation, also reduces the scope of the local variables - so it is a
good change).
One last thing is that I'm not sure that this order of patches is the
best order - in particular, if I run the tests at the 5th patch using a
binary compiled at the 4th patch, I notice that cloning with
"--filter=only:commits" fails with a cryptic error "fatal: bad tree
object e891efadd67ca0c01b1c518a2fd91130d40f5904". This makes bisecting
for errors difficult, but perhaps with this problem manifesting in only
a few commits, it is not so bad.
The ideal order is to put patches 3-5 before 1-2. I've tried the
rearrangement myself and found many instances where I had to rewrite
code because one patch introduces "ctx" and the other, NOT_USER_GIVEN.
So as a reviewer, I'm on the fence about suggesting that the patches be
reordered.
From: Matthew DeVore <hidden> Date: 2018-08-10 23:06:50
Changes applied, as suggested by jonathantanmy@google.com:
- Re-ordered patches so 3-5 actually come first
- Sadly, as a result of the above, many of the tests in the "treat
missing trees like missing blobs" patch had to be moved to the filter
implementation patch, since it doesn't seem possible to create
promisor objects that are really recognized as promisor objects in
tests (unless you actually do a partial clone). Overall, I thought
this ordering was more elegant, so I kept it.
- Reworded NOT_USER_GIVEN commit message as suggested
- Fixed style error list-objects.c (var dec and code mixed)
- Added missing /* fallthrough */ and explanation why
- Removed the show_missing_trees flag - now we won't show any error if
the only problem is the object is missing and it's a promisor object.
- Renamed only:commits to tree:none and updated commit message
accordingly
- Added /* blobs are always omitted */ comment in list-objects-filter.c
- Fixed up tests in t5317-pack-objects-filter-objects.sh to not use
unnecessary sorts, and to do more commands on lines of their own
rather than in pipes
Matthew DeVore (5):
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
rev-list: handle missing tree objects properly
revision: mark non-user-given objects instead
list-objects-filter: implement filter tree:none
Documentation/rev-list-options.txt | 2 +
builtin/rev-list.c | 10 +-
list-objects-filter-options.c | 4 +
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 +++--
list-objects.c | 236 +++++++++++++------------
revision.c | 1 -
revision.h | 10 +-
t/t5317-pack-objects-filter-objects.sh | 40 +++++
t/t5616-partial-clone.sh | 27 +++
t/t6112-rev-list-filters-objects.sh | 13 ++
11 files changed, 259 insertions(+), 134 deletions(-)
--
2.18.0.597.ga71716f1ad-goog
From: Matthew DeVore <hidden> Date: 2018-08-10 23:06:52
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-10 23:06:55
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 68 ++++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 27 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-10 23:06:58
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 10 ++++++----
list-objects.c | 17 +++++++++--------
t/t5317-pack-objects-filter-objects.sh | 13 +++++++++++++
3 files changed, 28 insertions(+), 12 deletions(-)
@@ -59,6 +59,19 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'get an error for missing tree object''+gitinitr5&&+echofoo>r5/foo&&+git-Cr5addfoo&&+git-Cr5commit-m"foo"&&+del=$(git-Cr5rev-parseHEAD^{tree}|sed"s|..|&/|")&&+rmr5/.git/objects/$del&&+test_must_failgit-Cr5pack-objects--rev--stdout2>bad_tree<<-EOF&&+HEAD+EOF+grep-q"bad tree object"bad_tree+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
From: Matthew DeVore <hidden> Date: 2018-08-10 23:07:01
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Matthew DeVore <hidden> Date: 2018-08-10 23:07:03
Teach list-objects the "tree:none" 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:none - 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.
Signed-off-by: Matthew DeVore <redacted>
---
Documentation/rev-list-options.txt | 2 ++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 +++++++++++++++++++-------
t/t5317-pack-objects-filter-objects.sh | 27 ++++++++++++++
t/t5616-partial-clone.sh | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 110 insertions(+), 13 deletions(-)
@@ -743,6 +743,8 @@ 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:none' omits all blobs and trees.++ The form '--missing=error' requests that rev-list stop with an error if a missing object is encountered. This is the default action. +
@@ -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 */
@@ -26,38 +26,45 @@#define FILTER_SHOWN_BUT_REVISIT (1<<21)/*-*Afilterforlist-objectstoomitALLblobsfromthetraversal.-*AndtoOPTIONALLYcollectalistoftheomittedOIDs.+*Afilterforlist-objectstoomitALLblobsfromthetraversal,andpossibly+*treesaswell.+*CanOPTIONALLYcollectalistoftheomittedOIDs.*/-structfilter_blobs_none_data{+structfilter_none_of_type_data{+/* blobs are always omitted */+unsignedomit_trees:1;structoidset*omits;};-staticenumlist_objects_filter_resultfilter_blobs_none(+staticenumlist_objects_filter_resultfilter_none_of_type(enumlist_objects_filter_situationfilter_situation,structobject*obj,constchar*pathname,constchar*filename,void*filter_data_){-structfilter_blobs_none_data*filter_data=filter_data_;+structfilter_none_of_type_data*filter_data=filter_data_;switch(filter_situation){default:die("unknown filter_situation");returnLOFR_ZERO;-caseLOFS_BEGIN_TREE:-assert(obj->type==OBJ_TREE);-/* always include all tree objects */-returnLOFR_MARK_SEEN|LOFR_DO_SHOW;-caseLOFS_END_TREE:assert(obj->type==OBJ_TREE);returnLOFR_ZERO;+caseLOFS_BEGIN_TREE:+assert(obj->type==OBJ_TREE);+if(!filter_data->omit_trees)+returnLOFR_MARK_SEEN|LOFR_DO_SHOW;++/*+*Fallthroughtoinsertintoomittedlistfortreesaswellas+*blobs.+*/+/* fallthrough */caseLOFS_BLOB:-assert(obj->type==OBJ_BLOB);assert((obj->flags&SEEN)==0);if(filter_data->omits)
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:none''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:none packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:none''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:none to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:none"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+ ."$TEST_DIRECTORY"/lib-httpd.sh start_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:none filter.++test_expect_success'verify tree:none includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:none\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jeff Hostetler <hidden> Date: 2018-08-13 16:39:02
On 8/10/2018 7:06 PM, Matthew DeVore wrote:
quoted hunk
Teach list-objects the "tree:none" 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:none - 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.
Signed-off-by: Matthew DeVore <redacted>
---
Documentation/rev-list-options.txt | 2 ++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 +++++++++++++++++++-------
t/t5317-pack-objects-filter-objects.sh | 27 ++++++++++++++
t/t5616-partial-clone.sh | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 110 insertions(+), 13 deletions(-)
@@ -743,6 +743,8 @@ 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:none' omits all blobs and trees.++ The form '--missing=error' requests that rev-list stop with an error if a missing object is encountered. This is the default action. +
@@ -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 */
I'm not sure I'd convert the existing filter types.
When I created this file, I created a set of function pairs
for each filter type:
filter_<name>() and filter_<name>__init()
with the latter being added to the s_filters[] array and created
a choice enum having corresponding values
LOFC_<name>
Here you're adding a new _init() and LOFC_ key, but mapping both
the original "blob:none" and the new "tree:none" to a combined
filter function and blends these 2 modes.
Style-wise, I'd keep the original filters as they were and add a
new function pair for the new tree:none filter. Then you can
simplify the logic inside your new filter. For example, in your
filter "filter_data->omit_trees" will always be true, so you can
just do the "if (filter_data->omits) oidset_insert(...); return _SEEN"
and not have the fallthru stuff -- or get rid of the asserts() and put
the case labels together.
One of the things I wanted to do (when I found some free time) was to
add a "tree:none" and maybe a "tree:root" filter. (The latter only
including the root trees associated with the fetched commits, since
there are/were some places where we implicitly also load the root tree
when loading the commit object.) So in that vein, it might be that we
would want a "tree:<depth>" filter instead with 0 = none and 1 = root.
I wasn't ready to propose that when I did the filtering, but I had that
in mind. (And is partially why I suggest keeping your new filter
independent of the existing ones.)
Jeff
quoted hunk
-static enum list_objects_filter_result filter_blobs_none(
+static enum list_objects_filter_result filter_none_of_type(
enum list_objects_filter_situation filter_situation,
struct object *obj,
const char *pathname,
const char *filename,
void *filter_data_)
{
- struct filter_blobs_none_data *filter_data = filter_data_;
+ struct filter_none_of_type_data *filter_data = filter_data_;
switch (filter_situation) {
default:
die("unknown filter_situation");
return LOFR_ZERO;
- case LOFS_BEGIN_TREE:
- assert(obj->type == OBJ_TREE);
- /* always include all tree objects */
- return LOFR_MARK_SEEN | LOFR_DO_SHOW;
-
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
return LOFR_ZERO;
+ case LOFS_BEGIN_TREE:
+ assert(obj->type == OBJ_TREE);
+ if (!filter_data->omit_trees)
+ return LOFR_MARK_SEEN | LOFR_DO_SHOW;
+
+ /*
+ * Fallthrough to insert into omitted list for trees as well as
+ * blobs.
+ */
+ /* fallthrough */
case LOFS_BLOB:
- assert(obj->type == OBJ_BLOB);
assert((obj->flags & SEEN) == 0);
if (filter_data->omits)
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:none''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:none packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:none''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:none to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:none"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+."$TEST_DIRECTORY"/lib-httpd.shstart_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:none filter.++test_expect_success'verify tree:none includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:none\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Matthew DeVore <hidden> Date: 2018-08-13 18:14:52
Applied the following changes suggested by git@jeffhostetler.com:
- Change the filter name from tree:none to tree:0 to make room for
future improvements which allow filtering based on depth.
- Made a separate filter logic function and filter data struct for
tree:0 rather than share it with blob:none. I would usually prefer
making the code less redundant whenever possible, but since there
are plans to extend this in the future, it makes more sense to have
this separate.
Matthew DeVore (5):
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
rev-list: handle missing tree objects properly
revision: mark non-user-given objects instead
list-objects-filter: implement filter tree:0
Documentation/rev-list-options.txt | 3 +
builtin/rev-list.c | 10 +-
list-objects-filter-options.c | 4 +
list-objects-filter-options.h | 1 +
list-objects-filter.c | 50 ++++++
list-objects.c | 236 +++++++++++++------------
revision.c | 1 -
revision.h | 10 +-
t/t5317-pack-objects-filter-objects.sh | 40 +++++
t/t5616-partial-clone.sh | 27 +++
t/t6112-rev-list-filters-objects.sh | 13 ++
11 files changed, 274 insertions(+), 121 deletions(-)
--
2.18.0.597.ga71716f1ad-goog
From: Matthew DeVore <hidden> Date: 2018-08-13 18:14:54
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-13 18:14:59
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 68 ++++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 27 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-13 18:15:08
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 10 ++++++----
list-objects.c | 17 +++++++++--------
t/t5317-pack-objects-filter-objects.sh | 13 +++++++++++++
3 files changed, 28 insertions(+), 12 deletions(-)
@@ -59,6 +59,19 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'get an error for missing tree object''+gitinitr5&&+echofoo>r5/foo&&+git-Cr5addfoo&&+git-Cr5commit-m"foo"&&+del=$(git-Cr5rev-parseHEAD^{tree}|sed"s|..|&/|")&&+rmr5/.git/objects/$del&&+test_must_failgit-Cr5pack-objects--rev--stdout2>bad_tree<<-EOF&&+HEAD+EOF+grep-q"bad tree object"bad_tree+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
From: Matthew DeVore <hidden> Date: 2018-08-13 18:15:08
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Matthew DeVore <hidden> Date: 2018-08-13 18:15:08
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 <redacted>
---
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 | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 125 insertions(+)
@@ -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. +
@@ -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 */
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:0 to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+ ."$TEST_DIRECTORY"/lib-httpd.sh start_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jonathan Tan <hidden> Date: 2018-08-13 18:20:32
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
Indeed, and I'm not sure why the message was there in the first place -
if parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the "die" call, so any message printed by parse_tree_gently() seems
superfluous.
It might be better to add an additional commit that removes the "gently"
condition (in other words, always parsing gently), with a message
explaining the above. Also, in that commit, I prefer not to add the
"/*quiet_on_missing*/" explanation (we don't seem to do that in Git
code); I also know that the ">= 0" is a holdover from the existing "< 0"
code, but we don't need to do that either.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
is_promisor_object() should. If you still have the code you used to
verify that, can you share it? In particular, pay attention to the path
of the repo - promise_and_delete is hardcoded to use one particular
path.
Whether you test in this patch or in the last patch, make sure that the
following are tested:
git rev-list --missing=error, allow-any, allow-promisor, print
git rev-list --exclude-promisor-objects
Also, test when a tree pointed to by a commit is missing, and when a
tree pointed to by a tree is missing.
quoted hunk
@@ -152,20 +151,21 @@ static void process_tree(struct traversal_context *ctx, die("bad tree object"); if (obj->flags & (UNINTERESTING | SEEN)) return;- if (parse_tree_gently(tree, gently) < 0) {+ parsed = parse_tree_gently(tree, /*quiet_on_missing=*/1) >= 0;+ if (!parsed) { if (revs->ignore_missing_links) return;+ if (!is_promisor_object(&obj->oid))+ die("bad tree object %s", oid_to_hex(&obj->oid));+ /* * Pre-filter known-missing tree objects when explicitly * requested. This may cause the actual filter to report * an incomplete list of missing objects. */- if (revs->exclude_promisor_objects &&- is_promisor_object(&obj->oid))+ if (revs->exclude_promisor_objects) return;-- die("bad tree object %s", oid_to_hex(&obj->oid)); }
The missing mechanism (for error, allow-any, print) should work without
needing to consult whether an object is a promisor object or not - it
should just print whatever is missing, so the "if
(!is_promisor_object..." line looks out of place.
In my original review [1], I suggested that we always show a tree if we
have its hash - if we don't have the object, we just recurse into it.
This would be the same as your patch, except that the 'die("bad tree
object...' is totally removed instead of merely moved. I still think
this solution has some merit - all the tests still pass (except that we
need to check for "unable to read" instead of "bad tree object" in error
messages), but I just realized that it might still be backwards
incompatible in that a basic "rev-list --objects" would now succeed
instead of fail if a tree was missing (I haven't tested this though).
We might need a flag called "do_not_die_on_missing_tree" (much like your
original idea of "show_missing_trees") so that callers that are prepared
to deal with missing trees can set this. Sorry for the churn. You can
document it as such:
Blobs are shown without regard for their existence. But not so for
trees: unless exclude_promisor_objects is set and the tree in question
is a promisor object, or ignore_missing_links is set (and in this case,
the tree in question may or may not be a promisor object), the revision
walker dies with a "bad tree object" message when encountering a
missing tree.
For callers that can handle missing trees and want them to be
filterable and showable, set this to true. The revision walker will
filter and show such a missing tree as usual, but will not attempt to
recurse into this tree object.
[1] https://public-inbox.org/git/20180810002411.13447-1-jonathantanmy@google.com/
From: Jonathan Tan <hidden> Date: 2018-08-13 18:29:58
- case LOFS_BEGIN_TREE:
- assert(obj->type == OBJ_TREE);
- /* always include all tree objects */
- return LOFR_MARK_SEEN | LOFR_DO_SHOW;
-
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
return LOFR_ZERO;
+ case LOFS_BEGIN_TREE:
+ assert(obj->type == OBJ_TREE);
+ if (!filter_data->omit_trees)
+ return LOFR_MARK_SEEN | LOFR_DO_SHOW;
+
+ /*
+ * Fallthrough to insert into omitted list for trees as well as
+ * blobs.
+ */
+ /* fallthrough */
case LOFS_BLOB:
- assert(obj->type == OBJ_BLOB);
assert((obj->flags & SEEN) == 0);
After looking at the resulting file, I don't think saving a few lines of
code (to add the OID, then return LOFR_MARK_SEEN) is worth rearranging
the cases and falling through. Can you just add the OID-adding code to
the LOFS_BEGIN_TREE case?
We also need to verify that the resulting partial clone works - after
all relevant tests, can you also ensure that:
- fsck works
- a cat-file on an indirectly missing tree works (i.e. if you have
commit -> A -> B and both A and B are missing, cat-file the B)
- fsck still works after the cat-file
There is another potential issue about expanding the documentation of
the pack protocol because we now support a new type of filter, but that
is fine because the protocol currently points us to the rev-list
documentation (which is updated). We probably need a way for clients to
query servers about which filters they support, but that is definitely
beyond the scope of this patch set.
As stated in my review of patch 3, also test the other --missing
arguments.
Patches 1, 2, and 4 look good to me. (Writing this here so that I don't
need to send one e-mail for each.)
From: Matthew DeVore <hidden> Date: 2018-08-14 00:22:53
Resending this in plain-text mode so that git@vger.kernel.org won't
bounce it. Sorry for those of you receiving this twice.
On Mon, Aug 13, 2018 at 11:20 AM Jonathan Tan [off-list ref] wrote:
quoted
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
Indeed, and I'm not sure why the message was there in the first place -
if parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the "die" call, so any message printed by parse_tree_gently() seems
superfluous.
It might be better to add an additional commit that removes the "gently"
condition (in other words, always parsing gently), with a message
explaining the above. Also, in that commit, I prefer not to add the
"/*quiet_on_missing*/" explanation (we don't seem to do that in Git
code); I also know that the ">= 0" is a holdover from the existing "< 0"
code, but we don't need to do that either.
Good idea. I've added a new commit which replaces the calculation with
a hard-coded "1"
I don't understand about the ">= 0". What should I replace it with?
Maybe you mean the return is never positive so I can change:
parse_tree_gently(tree, 1) >= 0
to:
!parse_tree_gently(tree, 1)
?
quoted
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
is_promisor_object() should. If you still have the code you used to
verify that, can you share it? In particular, pay attention to the path
of the repo - promise_and_delete is hardcoded to use one particular
path.
It turns out I wasn't setting the extensions.partial_clone config in
my test, and that's why everything wasn't working. So I've moved all
the tests feasible back to the earlier commit. Cool :)
Whether you test in this patch or in the last patch, make sure that the
following are tested:
git rev-list --missing=error, allow-any, allow-promisor, print
git rev-list --exclude-promisor-objects
Added --missing=print, --missing=allow-any, and
--exclude-promisor-objects to t0410
--missing=allow-promisor did some seem sufficiently interesting or
different from allow-any to justify adding it.
I had to put missing=error into the commit that introduces the tree:0
filter, since that flag causes an automatic attempt to fetch the
missing object, which t0410 does not seem to support. So added test
case "auto-fetching of trees with --missing=error" to t5616.
Also, test when a tree pointed to by a commit is missing, and when a
tree pointed to by a tree is missing.
Former is done multiple times already, added latter to t0410 as
"missing non-root tree object and rev-list."
quoted
@@ -152,20 +151,21 @@ static void process_tree(struct traversal_context *ctx, die("bad tree object"); if (obj->flags & (UNINTERESTING | SEEN)) return;- if (parse_tree_gently(tree, gently) < 0) {+ parsed = parse_tree_gently(tree, /*quiet_on_missing=*/1) >= 0;+ if (!parsed) { if (revs->ignore_missing_links) return;+ if (!is_promisor_object(&obj->oid))+ die("bad tree object %s", oid_to_hex(&obj->oid));+ /* * Pre-filter known-missing tree objects when explicitly * requested. This may cause the actual filter to report * an incomplete list of missing objects. */- if (revs->exclude_promisor_objects &&- is_promisor_object(&obj->oid))+ if (revs->exclude_promisor_objects) return;-- die("bad tree object %s", oid_to_hex(&obj->oid)); }
The missing mechanism (for error, allow-any, print) should work without
needing to consult whether an object is a promisor object or not - it
should just print whatever is missing, so the "if
(!is_promisor_object..." line looks out of place.
Done. I considered that a missing object which is not a promisor is a
serious error, so I had it die here. But now that I've added the
do_not_die_on_missing_tree flag, it's more natural to keep the
previous promisor check as-is. Also, is_promisor_object is an
expensive check, and it would be better to skip it during the common
execution path (which should be when exclude_promisor_objects, an
internal-use-only flag, is *not* set, which means we never call
is_promisor_object.
In my original review [1], I suggested that we always show a tree if we
have its hash - if we don't have the object, we just recurse into it.
This would be the same as your patch, except that the 'die("bad tree
object...' is totally removed instead of merely moved. I still think
this solution has some merit - all the tests still pass (except that we
need to check for "unable to read" instead of "bad tree object" in error
messages), but I just realized that it might still be backwards
incompatible in that a basic "rev-list --objects" would now succeed
instead of fail if a tree was missing (I haven't tested this though).
The presence of the die if !is_promisor_object is what justified the
changing of the parse_tree_gently to always be gently, since it is
what showed the OID. Can we really remove both? Maybe in a different
patch set, since I'm no longer touching that line?
We might need a flag called "do_not_die_on_missing_tree" (much like your
original idea of "show_missing_trees") so that callers that are prepared
to deal with missing trees can set this. Sorry for the churn. You can
document it as such:
Added it, but not with a command-line flag, only in rev-info.h. We can
always add a flag later if people have been relying on the existing
behavior of git rev-list to balk at missing trees. (That seems
unlikely though, considering there is no filter to enable that before
this patchset).
Blobs are shown without regard for their existence. But not so for
trees: unless exclude_promisor_objects is set and the tree in question
is a promisor object, or ignore_missing_links is set (and in this case,
the tree in question may or may not be a promisor object), the revision
walker dies with a "bad tree object" message when encountering a
missing tree.
For callers that can handle missing trees and want them to be
filterable and showable, set this to true. The revision walker will
filter and show such a missing tree as usual, but will not attempt to
recurse into this tree object.
[1] https://public-inbox.org/git/20180810002411.13447-1-jonathantanmy@google.com/
From: Matthew DeVore <hidden> Date: 2018-08-14 00:55:54
On Mon, Aug 13, 2018 at 11:29 AM Jonathan Tan [off-list ref] wrote:
quoted
- case LOFS_BEGIN_TREE:
- assert(obj->type == OBJ_TREE);
- /* always include all tree objects */
- return LOFR_MARK_SEEN | LOFR_DO_SHOW;
-
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
return LOFR_ZERO;
+ case LOFS_BEGIN_TREE:
+ assert(obj->type == OBJ_TREE);
+ if (!filter_data->omit_trees)
+ return LOFR_MARK_SEEN | LOFR_DO_SHOW;
+
+ /*
+ * Fallthrough to insert into omitted list for trees as well as
+ * blobs.
+ */
+ /* fallthrough */
case LOFS_BLOB:
- assert(obj->type == OBJ_BLOB);
assert((obj->flags & SEEN) == 0);
After looking at the resulting file, I don't think saving a few lines of
code (to add the OID, then return LOFR_MARK_SEEN) is worth rearranging
the cases and falling through. Can you just add the OID-adding code to
the LOFS_BEGIN_TREE case?
I've followed Jeff's suggestion of splitting up the functions, so
though the code is now redundant, it is ready for some future
improvements that are planned, and it's more clear and consistent with
the other filter functions.
We also need to verify that the resulting partial clone works - after
all relevant tests, can you also ensure that:
- fsck works
- a cat-file on an indirectly missing tree works (i.e. if you have
commit -> A -> B and both A and B are missing, cat-file the B)
- fsck still works after the cat-file
Done - added it to t5616 in the last commit of the patchset.
There is another potential issue about expanding the documentation of
the pack protocol because we now support a new type of filter, but that
is fine because the protocol currently points us to the rev-list
documentation (which is updated). We probably need a way for clients to
query servers about which filters they support, but that is definitely
beyond the scope of this patch set.
From: Matthew DeVore <hidden> Date: 2018-08-14 00:57:16
On Mon, Aug 13, 2018 at 9:38 AM Jeff Hostetler [off-list ref] wrote:
On 8/10/2018 7:06 PM, Matthew DeVore wrote:
quoted
Teach list-objects the "tree:none" 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:none - 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.
Signed-off-by: Matthew DeVore <redacted>
---
Documentation/rev-list-options.txt | 2 ++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 49 +++++++++++++++++++-------
t/t5317-pack-objects-filter-objects.sh | 27 ++++++++++++++
t/t5616-partial-clone.sh | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 110 insertions(+), 13 deletions(-)
@@ -743,6 +743,8 @@ 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:none' omits all blobs and trees.++ The form '--missing=error' requests that rev-list stop with an error if a missing object is encountered. This is the default action. +
@@ -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 */
I'm not sure I'd convert the existing filter types.
When I created this file, I created a set of function pairs
for each filter type:
filter_<name>() and filter_<name>__init()
with the latter being added to the s_filters[] array and created
a choice enum having corresponding values
LOFC_<name>
Here you're adding a new _init() and LOFC_ key, but mapping both
the original "blob:none" and the new "tree:none" to a combined
filter function and blends these 2 modes.
Style-wise, I'd keep the original filters as they were and add a
new function pair for the new tree:none filter. Then you can
simplify the logic inside your new filter. For example, in your
filter "filter_data->omit_trees" will always be true, so you can
just do the "if (filter_data->omits) oidset_insert(...); return _SEEN"
and not have the fallthru stuff -- or get rid of the asserts() and put
the case labels together.
One of the things I wanted to do (when I found some free time) was to
add a "tree:none" and maybe a "tree:root" filter. (The latter only
including the root trees associated with the fetched commits, since
there are/were some places where we implicitly also load the root tree
when loading the commit object.) So in that vein, it might be that we
would want a "tree:<depth>" filter instead with 0 = none and 1 = root.
I wasn't ready to propose that when I did the filtering, but I had that
in mind. (And is partially why I suggest keeping your new filter
independent of the existing ones.)
That's fair. I've split up the functions to be completely separate,
and changed the filter name to tree:0 so it can later be extended as
you suggest.
Jeff
quoted
-static enum list_objects_filter_result filter_blobs_none(
+static enum list_objects_filter_result filter_none_of_type(
enum list_objects_filter_situation filter_situation,
struct object *obj,
const char *pathname,
const char *filename,
void *filter_data_)
{
- struct filter_blobs_none_data *filter_data = filter_data_;
+ struct filter_none_of_type_data *filter_data = filter_data_;
switch (filter_situation) {
default:
die("unknown filter_situation");
return LOFR_ZERO;
- case LOFS_BEGIN_TREE:
- assert(obj->type == OBJ_TREE);
- /* always include all tree objects */
- return LOFR_MARK_SEEN | LOFR_DO_SHOW;
-
case LOFS_END_TREE:
assert(obj->type == OBJ_TREE);
return LOFR_ZERO;
+ case LOFS_BEGIN_TREE:
+ assert(obj->type == OBJ_TREE);
+ if (!filter_data->omit_trees)
+ return LOFR_MARK_SEEN | LOFR_DO_SHOW;
+
+ /*
+ * Fallthrough to insert into omitted list for trees as well as
+ * blobs.
+ */
+ /* fallthrough */
case LOFS_BLOB:
- assert(obj->type == OBJ_BLOB);
assert((obj->flags & SEEN) == 0);
if (filter_data->omits)
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:none''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:none packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:none''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:none>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:none to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:none"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+."$TEST_DIRECTORY"/lib-httpd.shstart_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:none filter.++test_expect_success'verify tree:none includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:none\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jeff Hostetler <hidden> Date: 2018-08-14 15:13:44
On 8/13/2018 2:14 PM, Matthew DeVore wrote:
quoted hunk
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 <redacted>
---
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 | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 125 insertions(+)
@@ -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. +
@@ -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 */
@@ -80,6 +80,55 @@ static void *filter_blobs_none__init(returnd;}+/*+*Afilterforlist-objectstoomitALLtreesandblobsfromthetraversal.+*CanOPTIONALLYcollectalistoftheomittedOIDs.+*/+structfilter_trees_none_data{+structoidset*omits;+};++staticenumlist_objects_filter_resultfilter_trees_none(+enumlist_objects_filter_situationfilter_situation,+structobject*obj,+constchar*pathname,+constchar*filename,+void*filter_data_)+{+structfilter_trees_none_data*filter_data=filter_data_;++switch(filter_situation){+default:+die("unknown filter_situation");+returnLOFR_ZERO;++caseLOFS_BEGIN_TREE:+caseLOFS_BLOB:+if(filter_data->omits)+oidset_insert(filter_data->omits,&obj->oid);+returnLOFR_MARK_SEEN;/* but not LOFR_DO_SHOW (hard omit) */++caseLOFS_END_TREE:+assert(obj->type==OBJ_TREE);+returnLOFR_ZERO;++}+}
There are a couple of options here:
[] If really want to omit all trees and blobs (and we DO NOT want
the oidset of everything omitted), then we might be able to
shortcut the traversal and speed things up.
{} add a LOFR_SKIP_TREE bit to list_objects_filter_result
{} test this bit process_tree() and avoid the init_tree_desc() and
the while loop and some adjacent setup/tear-down code.
{} make this filter something like:
case LOFS_BEGIN_TREE:
if (filter_data->omits) {
oidset_insert(filter_data->omits, &obj->oid);
return LOFR_MARK_SEEN; /* ... (hard omit) */
} else
return LOFR_SKIP_TREE;
case LOFS_BLOB:
if (filter_data->omits) {
oidset_insert(filter_data->omits, &obj->oid);
return LOFR_MARK_SEEN; /* ... (hard omit) */
else
assert(...should not happen...);
[] Later, if we choose to actually support a depth>0, we'll probably
want a different filter function to conditionally include/exclude
blobs, include shallow tree[node]s, and do some of the provisional-
omit logic on deep tree[nodes] (in case a tree appears at multiple
places/depths in the history). But that can wait.
Jeff
quoted hunk
+
+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.
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:0 to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+."$TEST_DIRECTORY"/lib-httpd.shstart_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jonathan Tan <hidden> Date: 2018-08-14 16:03:09
I don't understand about the ">= 0". What should I replace it with?
Maybe you mean the return is never positive so I can change:
parse_tree_gently(tree, 1) >= 0
to:
!parse_tree_gently(tree, 1)
?
Sorry for the lack of clarity - that is what I meant.
quoted
The missing mechanism (for error, allow-any, print) should work without
needing to consult whether an object is a promisor object or not - it
should just print whatever is missing, so the "if
(!is_promisor_object..." line looks out of place.
Done. I considered that a missing object which is not a promisor is a
serious error, so I had it die here.
It is a serious error, but as far as I can tell, that is what the
--missing flags are supposed to help diagnose (so we can't die since we
need the diagnoses to be printed). See, for example, 'rev-list W/
--missing=print' in t6112 - the "r1" repository does not have partial
clone enabled (which I verified by inserting a test_pause then cat-ting
r1/.git/config), but nothing dies.
But now that I've added the
do_not_die_on_missing_tree flag, it's more natural to keep the
previous promisor check as-is.
OK, I'll take a look once you send out v4.
Also, is_promisor_object is an
expensive check, and it would be better to skip it during the common
execution path (which should be when exclude_promisor_objects, an
internal-use-only flag, is *not* set, which means we never call
is_promisor_object.
That's true.
quoted
In my original review [1], I suggested that we always show a tree if we
have its hash - if we don't have the object, we just recurse into it.
This would be the same as your patch, except that the 'die("bad tree
object...' is totally removed instead of merely moved. I still think
this solution has some merit - all the tests still pass (except that we
need to check for "unable to read" instead of "bad tree object" in error
messages), but I just realized that it might still be backwards
incompatible in that a basic "rev-list --objects" would now succeed
instead of fail if a tree was missing (I haven't tested this though).
The presence of the die if !is_promisor_object is what justified the
changing of the parse_tree_gently to always be gently, since it is
what showed the OID. Can we really remove both? Maybe in a different
patch set, since I'm no longer touching that line?
That's true - the idea of removing both needs more thought, and if we
were to do so, we definitely can do it in a different patch set.
quoted
We might need a flag called "do_not_die_on_missing_tree" (much like your
original idea of "show_missing_trees") so that callers that are prepared
to deal with missing trees can set this. Sorry for the churn. You can
document it as such:
Added it, but not with a command-line flag, only in rev-info.h. We can
always add a flag later if people have been relying on the existing
behavior of git rev-list to balk at missing trees. (That seems
unlikely though, considering there is no filter to enable that before
this patchset).
By flag, I indeed meant in rev-info.h - sorry for the confusion. That
sounds good.
From: Matthew DeVore <hidden> Date: 2018-08-14 17:29:12
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-14 17:29:16
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 68 ++++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 27 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-14 17:29:20
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 11 +++--
list-objects.c | 17 +++++--
revision.h | 1 +
t/t0410-partial-clone.sh | 66 ++++++++++++++++++++++++++
t/t5317-pack-objects-filter-objects.sh | 13 +++++
5 files changed, 101 insertions(+), 7 deletions(-)
@@ -124,6 +124,7 @@ struct rev_info {first_parent_only:1,line_level_traverse:1,tree_blobs_in_commit_order:1,+do_not_die_on_missing_tree:1,/* for internal use only */exclude_promisor_objects:1;
@@ -186,6 +186,72 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '!grep$FOOout'+test_expect_success'show missing tree objects with --missing=print''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++TREE=$(git-Creporev-parsebar^{tree})&&++promise_and_delete$TREE&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&+git-Creporev-list--quiet--missing=print--objectsHEAD>missing_objs2>rev_list_err&&+echo"?$TREE">expected&&+test_cmpexpectedmissing_objs&&++# do not complain when a missing tree cannot be parsed+!grep-q"Could not read "rev_list_err+'++test_expect_success'missing tree objects with --missing=allow-any and --exclude-promisor-objects''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++promise_and_delete$(git-Creporev-parsebar^{tree})&&+promise_and_delete$(git-Creporev-parsefoo^{tree})&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, 3 blobs, and 1 tree+test_line_count=7objs&&++# Do the same for --exclude-promisor-objects, but with all trees gone.+promise_and_delete$(git-Creporev-parsebaz^{tree})&&+git-Creporev-list--exclude-promisor-objects--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, no blobs or trees+test_line_count=3objs+'++test_expect_success'missing non-root tree object and rev-list''+rm-rfrepo&&+test_create_reporepo&&+mkdirrepo/dir&&+echofoo>repo/dir/foo&&+git-Crepoadddir/foo&&+git-Crepocommit-m"commit dir/foo"&&++promise_and_delete$(git-Creporev-parseHEAD:dir)&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 1 commit and 1 tree+test_line_count=2objs+'+ test_expect_success'rev-list stops traversal at missing and promised tree''rm-rfrepo&&test_create_reporepo&&
@@ -59,6 +59,19 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'get an error for missing tree object''+gitinitr5&&+echofoo>r5/foo&&+git-Cr5addfoo&&+git-Cr5commit-m"foo"&&+del=$(git-Cr5rev-parseHEAD^{tree}|sed"s|..|&/|")&&+rmr5/.git/objects/$del&&+test_must_failgit-Cr5pack-objects--rev--stdout2>bad_tree<<-EOF&&+HEAD+EOF+grep-q"bad tree object"bad_tree+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
From: Matthew DeVore <hidden> Date: 2018-08-14 17:29:22
If parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the die("bad tree object...") call, so any message printed by
parse_tree_gently() is superfluous.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-14 17:29:22
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Jonathan Tan <hidden> Date: 2018-08-14 18:06:33
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
The last sentence is difficult to understand - probably better to say
that all --missing= arguments and --exclude-promisor-objects work for
missing trees like they currently do for blobs (and do not fixate on
just --missing=error). And also demonstrate this in tests, like in
t6612.
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
These two paragraphs are no longer applicable, I think.
Is this correct? I would have expected this to be set only if --missing
was set.
- process_tree_contents(ctx, tree, base);
+ /*
+ * NEEDSWORK: we should not have to process this tree's contents if the
+ * filter wants to exclude all its contents AND the filter doesn't need
+ * to collect the omitted OIDs. We should add a LOFR_SKIP_TREE bit which
+ * allows skipping all children.
+ */
+ if (parsed)
+ process_tree_contents(ctx, tree, base);
I agree with Jeff Hostetler in [1] that a LOFR_SKIP_TREE bit is
desirable, but I don't think that this patch is the right place to
introduce this NEEDSWORK. For me, this patch is about skipping iterating
over the contents of a tree because the tree does not exist; this
NEEDSWORK is about skipping iterating over the contents of a tree
because we don't want its contents, and it is quite confusing to
conflate the two.
[1] https://public-inbox.org/git/d751d56b-84bb-a03d-5f2a-7dbaf8d947cc@jeffhostetler.com/
I know that the other flags don't have documentation, but I think it's
worth documenting this one because it is rather complicated. I have
provided a sample one in my earlier review - feel free to use that or
come up with your own.
@@ -186,6 +186,72 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '!grep$FOOout'+test_expect_success'show missing tree objects with --missing=print''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++TREE=$(git-Creporev-parsebar^{tree})&&++promise_and_delete$TREE&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&+git-Creporev-list--quiet--missing=print--objectsHEAD>missing_objs2>rev_list_err&&+echo"?$TREE">expected&&+test_cmpexpectedmissing_objs&&++# do not complain when a missing tree cannot be parsed+!grep-q"Could not read "rev_list_err+'
I think that the --exclude-promisor-tests can go in t0410 as you have
done, but the --missing tests (except for --missing=allow-promisor)
should go in t6112. (And like the existing --missing tests, they should
be done without setting extensions.partialclone.)
As for --missing=allow-promisor, I don't see them being tested anywhere
:-( so feel free to make a suggestion. I would put them in t6112 for
easy comparison with the other --missing tests.
From: Matthew DeVore <hidden> Date: 2018-08-14 18:09:06
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 <redacted>
---
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(+)
@@ -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. +
@@ -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 */
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -154,6 +154,22 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -grep"git index-pack.*--fsck-objects"trace'+test_expect_success'use fsck before and after manually fetching a missing subtree''+# push new commit so server has a subtree+mkdirsrc/dir&&+echo"in dir">src/dir/file.txt&&+git-Csrcadddir/file.txt&&+git-Csrccommit-m"file in dir"&&+git-Csrcpush-usrvmaster&&+SUBTREE=$(git-Csrcrev-parseHEAD:dir)&&++rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstfsck&&+git-Cdstcat-file-p$SUBTREE>tree_contents2>err&&+git-Cdstfsck+'+ test_expect_success'partial clone fetches blobs pointed to by refs even if normally filtered out''rm-rfsrcdst&&gitinitsrc&&
@@ -170,6 +186,28 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:0 to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'auto-fetching of trees with --missing=error''+git-Cdstrev-listmaster--missing=error--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+printf"blob\ncommit\ntree\n">unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'+ ."$TEST_DIRECTORY"/lib-httpd.sh start_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Matthew DeVore <hidden> Date: 2018-08-14 18:12:42
On Tue, Aug 14, 2018 at 8:13 AM Jeff Hostetler [off-list ref] wrote:
On 8/13/2018 2:14 PM, Matthew DeVore 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.
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 <redacted>
---
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 | 27 ++++++++++++++
t/t6112-rev-list-filters-objects.sh | 13 +++++++
7 files changed, 125 insertions(+)
@@ -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. +
@@ -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 */
@@ -80,6 +80,55 @@ static void *filter_blobs_none__init(returnd;}+/*+*Afilterforlist-objectstoomitALLtreesandblobsfromthetraversal.+*CanOPTIONALLYcollectalistoftheomittedOIDs.+*/+structfilter_trees_none_data{+structoidset*omits;+};++staticenumlist_objects_filter_resultfilter_trees_none(+enumlist_objects_filter_situationfilter_situation,+structobject*obj,+constchar*pathname,+constchar*filename,+void*filter_data_)+{+structfilter_trees_none_data*filter_data=filter_data_;++switch(filter_situation){+default:+die("unknown filter_situation");+returnLOFR_ZERO;++caseLOFS_BEGIN_TREE:+caseLOFS_BLOB:+if(filter_data->omits)+oidset_insert(filter_data->omits,&obj->oid);+returnLOFR_MARK_SEEN;/* but not LOFR_DO_SHOW (hard omit) */++caseLOFS_END_TREE:+assert(obj->type==OBJ_TREE);+returnLOFR_ZERO;++}+}
There are a couple of options here:
[] If really want to omit all trees and blobs (and we DO NOT want
the oidset of everything omitted), then we might be able to
shortcut the traversal and speed things up.
{} add a LOFR_SKIP_TREE bit to list_objects_filter_result
{} test this bit process_tree() and avoid the init_tree_desc() and
the while loop and some adjacent setup/tear-down code.
{} make this filter something like:
case LOFS_BEGIN_TREE:
if (filter_data->omits) {
oidset_insert(filter_data->omits, &obj->oid);
return LOFR_MARK_SEEN; /* ... (hard omit) */
} else
return LOFR_SKIP_TREE;
case LOFS_BLOB:
if (filter_data->omits) {
oidset_insert(filter_data->omits, &obj->oid);
return LOFR_MARK_SEEN; /* ... (hard omit) */
else
assert(...should not happen...);
I like this - it will considerably reduce the amount of work the
server needs to do on a partial clone. I'd prefer to do this in a
follow-up patchset, so I added a NEEDSWORK in the commit that adds
proper handling for filtered tree objects. I want to make sure the
unit tests are thorough when I apply your suggestion, and this
patchset is already a bit more complex than I was expecting.
[] Later, if we choose to actually support a depth>0, we'll probably
want a different filter function to conditionally include/exclude
blobs, include shallow tree[node]s, and do some of the provisional-
omit logic on deep tree[nodes] (in case a tree appears at multiple
places/depths in the history). But that can wait.
Jeff
quoted
+
+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.
@@ -72,6 +72,33 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+grep-E"tree|blob"objs>trees_and_blobs&&+test_line_count=1trees_and_blobs+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -170,6 +170,33 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if normgit-Cdstfsck'+test_expect_success'can use tree:0 to filter partial clone''+rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+catfetched_objects\+|awk-fprint_1.awk\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'++test_expect_success'show missing tree objects with --missing=print''+git-Cdstrev-listmaster--missing=print--quiet--objects>missing_objs&&+sed"s/?//"missing_objs\+|xargs-n1git-Csrv.barecat-file-t\+>missing_types&&+sort-umissing_types>missing_types.uniq&&+echotree>expected&&+test_cmpmissing_types.uniqexpected+'++test_expect_success'do not complain when a missing tree cannot be parsed''+git-Cdstrev-listmaster--missing=print--quiet--objects2>rev_list_err>&2&&+!grep-q"Could not read "rev_list_err+'+."$TEST_DIRECTORY"/lib-httpd.shstart_httpd
@@ -196,6 +196,19 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'++# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jonathan Tan <hidden> Date: 2018-08-14 18:18:47
quoted hunk
@@ -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.
+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 &&
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.
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
From: Matthew DeVore <hidden> Date: 2018-08-14 20:00:49
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 &&
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.
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
@@ -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. +
@@ -74,7 +74,7 @@ test_expect_success 'get an error for missing tree object' ' test_expect_success'setup for tests of tree:0''mkdirr1/subtree&&-echo"This is a file in a subtree">r1/subtree/file&&+echo"This is a file in a subtree">r1/subtree/file&&git-Cr1addsubtree/file&&git-Cr1commit-msubtree'
@@ -95,8 +95,10 @@ test_expect_success 'grab tree directly when using tree:0' 'EOFgit-Cr1index-pack../commitsonly.pack&&git-Cr1verify-pack-v../commitsonly.pack>objs&&-grep-E"tree|blob"objs>trees_and_blobs&&-test_line_count=1trees_and_blobs+grep-E"tree|blob"objs\+|awk-fprint_1.awk>trees_and_blobs&&+git-Cr1rev-parseHEAD:>expected&&+test_cmptrees_and_blobsexpected'# Test blob:limit=<n>[kmg] filter.
From: Jeff King <hidden> Date: 2018-08-14 20:01:58
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:<depth>' omits all blobs and trees deeper than
+<depth> from the root tree. Currently, only <depth>=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
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.
From: Matthew DeVore <hidden> Date: 2018-08-14 22:43:40
On Tue, Aug 14, 2018 at 11:06 AM Jonathan Tan [off-list ref] wrote:
quoted
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. A missing tree
will cause an error if --missing indicates an error should be caused,
and the hash is printed even if the tree is missing.
The last sentence is difficult to understand - probably better to say
that all --missing= arguments and --exclude-promisor-objects work for
missing trees like they currently do for blobs (and do not fixate on
just --missing=error). And also demonstrate this in tests, like in
t6612.
Fixed the commit message. And for the tests, in t0410 I changed the
--missing=allow-any to --missing-allow-promisor, and in t6112 I added
--missing=allow-any and --missing=print test cases.
quoted
In list-objects.c we no longer print a message to stderr if a tree
object is missing (quiet_on_missing is always true). I couldn't find
any place where this would matter, or where the caller of
traverse_commit_list would need to be fixed to show the error. However,
in the future it would be trivial to make the caller show the message if
we needed to.
This is not tested very thoroughly, since we cannot create promisor
objects in tests without using an actual partial clone. t0410 has a
promise_and_delete utility function, but the is_promisor_object function
does not return 1 for objects deleted in this way. More tests will will
come in a patch that implements a filter that can be used with git
clone.
These two paragraphs are no longer applicable, I think.
Is this correct? I would have expected this to be set only if --missing
was set.
If --missing is not set, then we want to fetch missing objects
automatically, and then die if we fail to do that, which is what
happens for blobs. So we don't want to die in list-objects.c. If we
fail to fetch, then we will die on line 213 in rev-list.c.
quoted
- process_tree_contents(ctx, tree, base);
+ /*
+ * NEEDSWORK: we should not have to process this tree's contents if the
+ * filter wants to exclude all its contents AND the filter doesn't need
+ * to collect the omitted OIDs. We should add a LOFR_SKIP_TREE bit which
+ * allows skipping all children.
+ */
+ if (parsed)
+ process_tree_contents(ctx, tree, base);
I agree with Jeff Hostetler in [1] that a LOFR_SKIP_TREE bit is
desirable, but I don't think that this patch is the right place to
introduce this NEEDSWORK. For me, this patch is about skipping iterating
over the contents of a tree because the tree does not exist; this
NEEDSWORK is about skipping iterating over the contents of a tree
because we don't want its contents, and it is quite confusing to
conflate the two.
I know that the other flags don't have documentation, but I think it's
worth documenting this one because it is rather complicated. I have
provided a sample one in my earlier review - feel free to use that or
come up with your own.
Added your wording to revision.h without major change.
@@ -186,6 +186,72 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '!grep$FOOout'+test_expect_success'show missing tree objects with --missing=print''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++TREE=$(git-Creporev-parsebar^{tree})&&++promise_and_delete$TREE&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&+git-Creporev-list--quiet--missing=print--objectsHEAD>missing_objs2>rev_list_err&&+echo"?$TREE">expected&&+test_cmpexpectedmissing_objs&&++# do not complain when a missing tree cannot be parsed+!grep-q"Could not read "rev_list_err+'
I think that the --exclude-promisor-tests can go in t0410 as you have
done, but the --missing tests (except for --missing=allow-promisor)
should go in t6112. (And like the existing --missing tests, they should
be done without setting extensions.partialclone.)
Done.
As for --missing=allow-promisor, I don't see them being tested anywhere
:-( so feel free to make a suggestion. I would put them in t6112 for
easy comparison with the other --missing tests.
Kept my allow-promisor test in t0410 since it requires partial clone
to be turned on in the config, and because it is pretty similar to
--exclude-promisor-objects.
Is this correct? I would have expected this to be set only if --missing
was set.
If --missing is not set, then we want to fetch missing objects
automatically, and then die if we fail to do that, which is what
happens for blobs.
This is true, and should already be handled. Pay attention to when
fetch_if_missing is set in builtin/rev-list.c.
do_not_die_on_missing_tree should probably be set to 1 whenever
fetch_if_missing is set to 0, I think.
(I acknowledge that the usage of this global variable is confusing, but
I couldn't think of a better way to implement this when I did. Perhaps
when the object store refactoring is done, this can be a store-specific
setting instead of a global variable.)
So we don't want to die in list-objects.c. If we
fail to fetch, then we will die on line 213 in rev-list.c.
Why don't we want to die in list-objects.c? When --missing=error is
passed, fetch_if_missing retains its default value of 1, so
parse_tree_gently() will attempt to fetch it - and if it fails, I think
it's appropriate to die in list-objects.c (and this should be the
current behavior). On other values, e.g. --missing=allow-any, there is
no autofetch (since fetch_if_missing is 0), so it is correct not to die
in list-objects.c.
quoted
As for --missing=allow-promisor, I don't see them being tested anywhere
:-( so feel free to make a suggestion. I would put them in t6112 for
easy comparison with the other --missing tests.
Kept my allow-promisor test in t0410 since it requires partial clone
to be turned on in the config, and because it is pretty similar to
--exclude-promisor-objects.
From: Jonathan Tan <hidden> Date: 2018-08-14 23:14:31
quoted
So we don't want to die in list-objects.c. If we
fail to fetch, then we will die on line 213 in rev-list.c.
Why don't we want to die in list-objects.c? When --missing=error is
passed, fetch_if_missing retains its default value of 1, so
parse_tree_gently() will attempt to fetch it - and if it fails, I think
it's appropriate to die in list-objects.c (and this should be the
current behavior). On other values, e.g. --missing=allow-any, there is
no autofetch (since fetch_if_missing is 0), so it is correct not to die
in list-objects.c.
After some in-office discussion, I should have checked line 213 in
builtin/rev-list.c more thorougly. Indeed it is OK not to die in
list-objects.c here, since builtin/rev-list.c already knows how to
handle missing objects in the --missing=error circumstance.
From: Matthew DeVore <hidden> Date: 2018-08-14 23:55:49
On Tue, Aug 14, 2018 at 1:01 PM Jeff King [off-list ref] wrote:
On Tue, Aug 14, 2018 at 10:28:13AM -0700, Matthew DeVore wrote:
quoted
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:
quoted
+The form '--filter=tree:<depth>' omits all blobs and trees deeper than
+<depth> from the root tree. Currently, only <depth>=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
@@ -732,8 +732,10 @@ 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.+The form '--filter=tree:<depth>' omits all blobs and trees whose depth+from the root tree is >= <depth> (minimum depth if an object is located+at multiple depths in the commits traversed). Currently, only <depth>=0+is supported, which omits all blobs and trees. --no-filter:: Turn off any previous `--filter=` argument.
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:07
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:09
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 68 ++++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 27 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:12
If parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the die("bad tree object...") call, so any message printed by
parse_tree_gently() is superfluous.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:15
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. The --missing=*
and --exclude-promisor-objects flags now work for trees as they already
do for blobs. This is demonstrated in t6112.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 11 ++++---
list-objects.c | 11 +++++--
revision.h | 15 +++++++++
t/t0410-partial-clone.sh | 45 ++++++++++++++++++++++++++
t/t5317-pack-objects-filter-objects.sh | 13 ++++++++
t/t6112-rev-list-filters-objects.sh | 17 ++++++++++
6 files changed, 105 insertions(+), 7 deletions(-)
@@ -125,6 +125,21 @@ struct rev_info {line_level_traverse:1,tree_blobs_in_commit_order:1,+/*+*Blobsareshownwithoutregardfortheirexistence.+*Butnotsofortrees:unlessexclude_promisor_objects+*issetandthetreeinquestionisapromisorobject;+*ORignore_missing_linksisset,therevisionwalker+*dieswitha"bad tree object HASH"messagewhen+*encounteringamissingtree.Forcallersthatcan+*handlemissingtreesandwantthemtobefilterable+*andshowable,setthistotrue.Therevisionwalker+*willfilterandshowsuchamissingtreeasusual,+*butwillnotattempttorecurseintothistree+*object.+*/+do_not_die_on_missing_tree:1,+/* for internal use only */exclude_promisor_objects:1;
@@ -186,6 +186,51 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '!grep$FOOout'+test_expect_success'missing tree objects with --missing=allow-promisor and --exclude-promisor-objects''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++promise_and_delete$(git-Creporev-parsebar^{tree})&&+promise_and_delete$(git-Creporev-parsefoo^{tree})&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-promisor--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, 3 blobs, and 1 tree+test_line_count=7objs&&++# Do the same for --exclude-promisor-objects, but with all trees gone.+promise_and_delete$(git-Creporev-parsebaz^{tree})&&+git-Creporev-list--exclude-promisor-objects--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, no blobs or trees+test_line_count=3objs+'++test_expect_success'missing non-root tree object and rev-list''+rm-rfrepo&&+test_create_reporepo&&+mkdirrepo/dir&&+echofoo>repo/dir/foo&&+git-Crepoadddir/foo&&+git-Crepocommit-m"commit dir/foo"&&++promise_and_delete$(git-Creporev-parseHEAD:dir)&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 1 commit and 1 tree+test_line_count=2objs+'+ test_expect_success'rev-list stops traversal at missing and promised tree''rm-rfrepo&&test_create_reporepo&&
@@ -59,6 +59,19 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'get an error for missing tree object''+gitinitr5&&+echofoo>r5/foo&&+git-Cr5addfoo&&+git-Cr5commit-m"foo"&&+del=$(git-Cr5rev-parseHEAD^{tree}|sed"s|..|&/|")&&+rmr5/.git/objects/$del&&+test_must_failgit-Cr5pack-objects--rev--stdout2>bad_tree<<-EOF&&+HEAD+EOF+grep-q"bad tree object"bad_tree+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -196,6 +196,23 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+test_expect_success'rev-list W/ --missing=print and --missing=allow-any for trees''+TREE=$(git-Cr3rev-parseHEAD:dir1)&&++rmr3/.git/objects/$(echo$TREE|sed"s|^..|&/|")&&++git-Cr3rev-list--quiet--missing=print--objectsHEAD>missing_objs2>rev_list_err&&+echo"?$TREE">expected&&+test_cmpexpectedmissing_objs&&++# do not complain when a missing tree cannot be parsed+test_line_count=0rev_list_err&&++git-Cr3rev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+!grep$TREEobjs&&+test_line_count=0rev_list_err+'+# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:17
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Matthew DeVore <hidden> Date: 2018-08-15 00:23:20
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 <redacted>
---
Documentation/rev-list-options.txt | 5 +++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 50 ++++++++++++++++++++++++++
t/t5317-pack-objects-filter-objects.sh | 28 +++++++++++++++
t/t5616-partial-clone.sh | 38 ++++++++++++++++++++
t/t6112-rev-list-filters-objects.sh | 12 +++++++
7 files changed, 138 insertions(+)
@@ -731,6 +731,11 @@ 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 whose depth+from the root tree is >= <depth> (minimum depth if an object is located+at multiple depths in the commits traversed). Currently, only <depth>=0+is supported, which omits all blobs and trees. --no-filter:: Turn off any previous `--filter=` argument.
@@ -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 */
@@ -72,6 +72,34 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+awk-e"/tree|blob/{print \$1}"objs>trees_and_blobs&&+git-Cr1rev-parseHEAD:>expected&&+test_cmptrees_and_blobsexpected+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -154,6 +154,44 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -grep"git index-pack.*--fsck-objects"trace'+test_expect_success'use fsck before and after manually fetching a missing subtree''+# push new commit so server has a subtree+mkdirsrc/dir&&+echo"in dir">src/dir/file.txt&&+git-Csrcadddir/file.txt&&+git-Csrccommit-m"file in dir"&&+git-Csrcpush-usrvmaster&&+SUBTREE=$(git-Csrcrev-parseHEAD:dir)&&++rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstfsck&&++# Make sure we only have commits, and all trees and blobs are missing.+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+awk-fprint_1.awkfetched_objects\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected&&++# Auto-fetch a tree with cat-file.+git-Cdstcat-file-p$SUBTREE>tree_contents&&+grepfile.txttree_contents&&++# fsck still works after an auto-fetch of a tree.+git-Cdstfsck&&++# Auto-fetch all remaining trees and blobs with --missing=error+git-Cdstrev-listmaster--missing=error--objects>fetched_objects&&+test_line_count=70fetched_objects&&+awk-fprint_1.awkfetched_objects\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+printf"blob\ncommit\ntree\n">unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'+ test_expect_success'partial clone fetches blobs pointed to by refs even if normally filtered out''rm-rfsrcdst&&gitinitsrc&&
@@ -213,6 +213,18 @@ test_expect_success 'rev-list W/ --missing=print and --missing=allow-any for tretest_line_count=0rev_list_err'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'+# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Jeff King <hidden> Date: 2018-08-15 01:22:28
On Tue, Aug 14, 2018 at 04:55:34PM -0700, Matthew DeVore wrote:
quoted
- 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").
quoted hunk
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
@@ -732,8 +732,10 @@ 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.+The form '--filter=tree:<depth>' omits all blobs and trees whose depth+from the root tree is >= <depth> (minimum depth if an object is located+at multiple depths in the commits traversed). Currently, only <depth>=0+is supported, which omits all blobs and trees.
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:09
Applied suggestion from Junio about removing -e flag from awk invocation.
Sending an updated patchset now since I haven't heard any other comments for a
while, and I don't believe Jonathan, the most active reviewer, has any more
concerns.
Matthew DeVore (6):
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
list-objects: always parse trees gently
rev-list: handle missing tree objects properly
revision: mark non-user-given objects instead
list-objects-filter: implement filter tree:0
Documentation/rev-list-options.txt | 5 +
builtin/rev-list.c | 11 +-
list-objects-filter-options.c | 4 +
list-objects-filter-options.h | 1 +
list-objects-filter.c | 50 ++++++
list-objects.c | 232 +++++++++++++------------
revision.c | 1 -
revision.h | 25 ++-
t/t0410-partial-clone.sh | 45 +++++
t/t5317-pack-objects-filter-objects.sh | 41 +++++
t/t5616-partial-clone.sh | 38 ++++
t/t6112-rev-list-filters-objects.sh | 29 ++++
12 files changed, 364 insertions(+), 118 deletions(-)
--
2.18.0.865.gffc8e1a3cd6-goog
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:19
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:19
If parsing fails when revs->ignore_missing_links and
revs->exclude_promisor_objects are both false, we print the OID anyway
in the die("bad tree object...") call, so any message printed by
parse_tree_gently() is superfluous.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:19
This will be used in a follow-up patch to reduce indentation needed when
invoking the logic conditionally. i.e. rather than:
if (foo) {
while (...) {
/* this is very indented */
}
}
we will have:
if (foo)
process_tree_contents(...);
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 68 ++++++++++++++++++++++++++++++--------------------
1 file changed, 41 insertions(+), 27 deletions(-)
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:20
Previously, we assumed only blob objects could be missing. This patch
makes rev-list handle missing trees like missing blobs. The --missing=*
and --exclude-promisor-objects flags now work for trees as they already
do for blobs. This is demonstrated in t6112.
Signed-off-by: Matthew DeVore <redacted>
---
builtin/rev-list.c | 11 ++++---
list-objects.c | 11 +++++--
revision.h | 15 +++++++++
t/t0410-partial-clone.sh | 45 ++++++++++++++++++++++++++
t/t5317-pack-objects-filter-objects.sh | 13 ++++++++
t/t6112-rev-list-filters-objects.sh | 17 ++++++++++
6 files changed, 105 insertions(+), 7 deletions(-)
@@ -125,6 +125,21 @@ struct rev_info {line_level_traverse:1,tree_blobs_in_commit_order:1,+/*+*Blobsareshownwithoutregardfortheirexistence.+*Butnotsofortrees:unlessexclude_promisor_objects+*issetandthetreeinquestionisapromisorobject;+*ORignore_missing_linksisset,therevisionwalker+*dieswitha"bad tree object HASH"messagewhen+*encounteringamissingtree.Forcallersthatcan+*handlemissingtreesandwantthemtobefilterable+*andshowable,setthistotrue.Therevisionwalker+*willfilterandshowsuchamissingtreeasusual,+*butwillnotattempttorecurseintothistree+*object.+*/+do_not_die_on_missing_tree:1,+/* for internal use only */exclude_promisor_objects:1;
@@ -186,6 +186,51 @@ test_expect_success 'rev-list stops traversal at missing and promised commit' '!grep$FOOout'+test_expect_success'missing tree objects with --missing=allow-promisor and --exclude-promisor-objects''+rm-rfrepo&&+test_create_reporepo&&+test_commit-Crepofoo&&+test_commit-Crepobar&&+test_commit-Crepobaz&&++promise_and_delete$(git-Creporev-parsebar^{tree})&&+promise_and_delete$(git-Creporev-parsefoo^{tree})&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-promisor--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, 3 blobs, and 1 tree+test_line_count=7objs&&++# Do the same for --exclude-promisor-objects, but with all trees gone.+promise_and_delete$(git-Creporev-parsebaz^{tree})&&+git-Creporev-list--exclude-promisor-objects--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 3 commits, no blobs or trees+test_line_count=3objs+'++test_expect_success'missing non-root tree object and rev-list''+rm-rfrepo&&+test_create_reporepo&&+mkdirrepo/dir&&+echofoo>repo/dir/foo&&+git-Crepoadddir/foo&&+git-Crepocommit-m"commit dir/foo"&&++promise_and_delete$(git-Creporev-parseHEAD:dir)&&++git-Crepoconfigcore.repositoryformatversion1&&+git-Crepoconfigextensions.partialclone"arbitrary string"&&++git-Creporev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+test_line_count=0rev_list_err&&+# 1 commit and 1 tree+test_line_count=2objs+'+ test_expect_success'rev-list stops traversal at missing and promised tree''rm-rfrepo&&test_create_reporepo&&
@@ -59,6 +59,19 @@ test_expect_success 'verify normal and blob:none packfiles have same commits/tretest_cmpobservedexpected'+test_expect_success'get an error for missing tree object''+gitinitr5&&+echofoo>r5/foo&&+git-Cr5addfoo&&+git-Cr5commit-m"foo"&&+del=$(git-Cr5rev-parseHEAD^{tree}|sed"s|..|&/|")&&+rmr5/.git/objects/$del&&+test_must_failgit-Cr5pack-objects--rev--stdout2>bad_tree<<-EOF&&+HEAD+EOF+grep-q"bad tree object"bad_tree+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -196,6 +196,23 @@ test_expect_success 'verify sparse:oid=oid-ish omits top-level files' 'test_cmpobservedexpected'+test_expect_success'rev-list W/ --missing=print and --missing=allow-any for trees''+TREE=$(git-Cr3rev-parseHEAD:dir1)&&++rmr3/.git/objects/$(echo$TREE|sed"s|^..|&/|")&&++git-Cr3rev-list--quiet--missing=print--objectsHEAD>missing_objs2>rev_list_err&&+echo"?$TREE">expected&&+test_cmpexpectedmissing_objs&&++# do not complain when a missing tree cannot be parsed+test_line_count=0rev_list_err&&++git-Cr3rev-list--missing=allow-any--objectsHEAD>objs2>rev_list_err&&+!grep$TREEobjs&&+test_line_count=0rev_list_err+'+# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:21
Currently, list-objects.c incorrectly treats all root trees of commits
as USER_GIVEN. Also, it would be easier to mark objects that are
non-user-given instead of user-given, since the places in the code
where we access an object through a reference are more obvious than
the places where we access an object that was given by the user.
Resolve these two problems by introducing a flag NOT_USER_GIVEN that
marks blobs and trees that are non-user-given, replacing USER_GIVEN.
(Only blobs and trees are marked because this mark is only used when
filtering objects, and filtering of other types of objects is not
supported yet.)
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 31 ++++++++++++++++++-------------
revision.c | 1 -
revision.h | 10 +++++++---
3 files changed, 25 insertions(+), 17 deletions(-)
@@ -8,7 +8,11 @@#include"diff.h"#include"commit-slab-decl.h"-/* Remember to update object flag allocation in object.h */+/* Remember to update object flag allocation in object.h+*NEEDSWORK:NOT_USER_GIVENdoesn'tapplytocommitsbecauseweonlysupport+*filteringtreesandblobs,butitmaybeusefultosupportfilteringcommits+*inthefuture.+*/#define SEEN (1u<<0)#define UNINTERESTING (1u<<1)#define TREESAME (1u<<2)
@@ -20,9 +24,9 @@#define SYMMETRIC_LEFT (1u<<8)#define PATCHSAME (1u<<9)#define BOTTOM (1u<<10)-#define USER_GIVEN (1u<<25) /* given directly by the user */+#define NOT_USER_GIVEN (1u<<25) /* tree or blob not given directly by user */#define TRACK_LINEAR (1u<<26)-#define ALL_REV_FLAGS (((1u<<11)-1) | USER_GIVEN | TRACK_LINEAR)+#define ALL_REV_FLAGS (((1u<<11)-1) | NOT_USER_GIVEN | TRACK_LINEAR)#define DECORATE_SHORT_REFS 1#define DECORATE_FULL_REFS 2
From: Matthew DeVore <hidden> Date: 2018-08-15 23:23:24
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 <redacted>
---
Documentation/rev-list-options.txt | 5 +++
list-objects-filter-options.c | 4 +++
list-objects-filter-options.h | 1 +
list-objects-filter.c | 50 ++++++++++++++++++++++++++
t/t5317-pack-objects-filter-objects.sh | 28 +++++++++++++++
t/t5616-partial-clone.sh | 38 ++++++++++++++++++++
t/t6112-rev-list-filters-objects.sh | 12 +++++++
7 files changed, 138 insertions(+)
@@ -731,6 +731,11 @@ 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 whose depth+from the root tree is >= <depth> (minimum depth if an object is located+at multiple depths in the commits traversed). Currently, only <depth>=0+is supported, which omits all blobs and trees. --no-filter:: Turn off any previous `--filter=` argument.
@@ -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 */
@@ -72,6 +72,34 @@ test_expect_success 'get an error for missing tree object' 'grep-q"bad tree object"bad_tree'+test_expect_success'setup for tests of tree:0''+mkdirr1/subtree&&+echo"This is a file in a subtree">r1/subtree/file&&+git-Cr1addsubtree/file&&+git-Cr1commit-msubtree+'++test_expect_success'verify tree:0 packfile has no blobs or trees''+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+!grep-E"tree|blob"objs+'++test_expect_success'grab tree directly when using tree:0''+# We should get the tree specified directly but not its blobs or subtrees.+git-Cr1pack-objects--rev--stdout--filter=tree:0>commitsonly.pack<<-EOF&&+HEAD:+EOF+git-Cr1index-pack../commitsonly.pack&&+git-Cr1verify-pack-v../commitsonly.pack>objs&&+awk"/tree|blob/{print \$1}"objs>trees_and_blobs&&+git-Cr1rev-parseHEAD:>expected&&+test_cmptrees_and_blobsexpected+'+# Test blob:limit=<n>[kmg] filter.# We boundary test around the size parameter. The filter is strictly less than# the value, so size 500 and 1000 should have the same results, but 1001 should
@@ -154,6 +154,44 @@ test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack -grep"git index-pack.*--fsck-objects"trace'+test_expect_success'use fsck before and after manually fetching a missing subtree''+# push new commit so server has a subtree+mkdirsrc/dir&&+echo"in dir">src/dir/file.txt&&+git-Csrcadddir/file.txt&&+git-Csrccommit-m"file in dir"&&+git-Csrcpush-usrvmaster&&+SUBTREE=$(git-Csrcrev-parseHEAD:dir)&&++rm-rfdst&&+gitclone--no-checkout--filter=tree:0"file://$(pwd)/srv.bare"dst&&+git-Cdstfsck&&++# Make sure we only have commits, and all trees and blobs are missing.+git-Cdstrev-listmaster--missing=allow-any--objects>fetched_objects&&+awk-fprint_1.awkfetched_objects\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+echocommit>unique_types.expected&&+test_cmpunique_types.observedunique_types.expected&&++# Auto-fetch a tree with cat-file.+git-Cdstcat-file-p$SUBTREE>tree_contents&&+grepfile.txttree_contents&&++# fsck still works after an auto-fetch of a tree.+git-Cdstfsck&&++# Auto-fetch all remaining trees and blobs with --missing=error+git-Cdstrev-listmaster--missing=error--objects>fetched_objects&&+test_line_count=70fetched_objects&&+awk-fprint_1.awkfetched_objects\+|xargs-n1git-Cdstcat-file-t>fetched_types&&+sortfetched_types-u>unique_types.observed&&+printf"blob\ncommit\ntree\n">unique_types.expected&&+test_cmpunique_types.observedunique_types.expected+'+ test_expect_success'partial clone fetches blobs pointed to by refs even if normally filtered out''rm-rfsrcdst&&gitinitsrc&&
@@ -213,6 +213,18 @@ test_expect_success 'rev-list W/ --missing=print and --missing=allow-any for tretest_line_count=0rev_list_err'+# Test tree:0 filter.++test_expect_success'verify tree:0 includes trees in "filtered" output''+git-Cr3rev-listHEAD--quiet--objects--filter-print-omitted--filter=tree:0\+|awk-fprint_1.awk\+|seds/~//\+|xargs-n1git-Cr3cat-file-t\+|sort-u>filtered_types&&+printf"blob\ntree\n">expected&&+test_cmpfiltered_typesexpected+'+# Delete some loose objects and use rev-list, but WITHOUT any filtering.# This models previously omitted objects that we did not receive.
From: Stefan Beller <hidden> Date: 2018-08-17 21:42:54
On Wed, Aug 15, 2018 at 4:23 PM Matthew DeVore [off-list ref] wrote:
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 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)
Thanks,
Stefan
From: Matthew DeVore <hidden> Date: 2018-08-17 22:20:09
On Fri, Aug 17, 2018 at 2:42 PM Stefan Beller [off-list ref] wrote:
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.
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):
From: Stefan Beller <hidden> Date: 2018-08-17 22:28:25
On Fri, Aug 17, 2018 at 3:20 PM Matthew DeVore [off-list ref] wrote:
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
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.
So if that is the only issue brought up, I would not even ask for a resend.
Thanks,
Stefan
From: Matthew DeVore <hidden> Date: 2018-08-20 13:18:50
There were many instances in this file where it seemed like BUG would be
better, so I created a new commit before this one to switch them over. The
interdiff is below.
BTW, why are there so many instances of "die" without "_"? I expect all
errors that may be caused by a user to be localized.
I'm going by the output of this: grep -IrE '\Wdie\([^_]' --exclude-dir=t
From: Stefan Beller <hidden> Date: 2018-08-20 18:38:27
On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore [off-list ref] wrote:
There were many instances in this file where it seemed like BUG would be
better, so I created a new commit before this one to switch them over. The
interdiff is below.
BTW, why are there so many instances of "die" without "_"? I expect all
errors that may be caused by a user to be localized.
Well, there is the porcelain layer to be consumed by a human user
and the plumbing that is good for scripts. And in scripts you might want
to grep for certain errors and react to that, so a non-localized error
message makes the script possible to run in any localisation.
The BUG is strictly for things that are due to Gits internals,
not for problematic user input. Problematic user input
definitely wants a die(...), and depending on the plumbing/porcelain
layer it may need to be _(translatable).
I think BUG() would never go with translated strings.
quoted hunk
I'm going by the output of this: grep -IrE '\Wdie\([^_]' --exclude-dir=t
Up until here we just have replace the die by BUG in the default
case of the state machine switch. (We need the default due to strict
compile flags, but as filter_situation is an enum I thought we would not
as compilers are smart enough to see we got all values of the enum
covered).
I agree that keeping the defaults and having a BUG() is reasonable.
quoted hunk
case LOFS_BEGIN_TREE:
assert(obj->type == OBJ_TREE);
From: Matthew DeVore <hidden> Date: 2018-08-20 23:21:05
On Mon, Aug 20, 2018 at 11:38 AM Stefan Beller [off-list ref] wrote:
On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore [off-list ref] wrote:
quoted
There were many instances in this file where it seemed like BUG would be
better, so I created a new commit before this one to switch them over. The
interdiff is below.
BTW, why are there so many instances of "die" without "_"? I expect all
errors that may be caused by a user to be localized.
Well, there is the porcelain layer to be consumed by a human user
and the plumbing that is good for scripts. And in scripts you might want
to grep for certain errors and react to that, so a non-localized error
message makes the script possible to run in any localisation.
The BUG is strictly for things that are due to Gits internals,
not for problematic user input. Problematic user input
definitely wants a die(...), and depending on the plumbing/porcelain
layer it may need to be _(translatable).
Ah I see. Plumbing commands are not translated. Makes perfect sense now.
I think BUG() would never go with translated strings.
quoted
I'm going by the output of this: grep -IrE '\Wdie\([^_]' --exclude-dir=t
Up until here we just have replace the die by BUG in the default
case of the state machine switch. (We need the default due to strict
compile flags, but as filter_situation is an enum I thought we would not
as compilers are smart enough to see we got all values of the enum
covered).
At the risk of going on a tangent, I assumed this was because enums
are really ints, and the "default" is there in case the enum somehow
got assigned to an int without a corresponding value. Either because
of a cast from an int that was out-of-range, or new values that were
obtained from arithmetic or bitwise operations on the declared enum
values, which created undeclared values.
I agree that keeping the defaults and having a BUG() is reasonable.
quoted
case LOFS_BEGIN_TREE:
assert(obj->type == OBJ_TREE);
From: Matthew DeVore <hidden> Date: 2018-08-20 23:30:30
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
From: Stefan Beller <hidden> Date: 2018-08-21 00:30:04
quoted
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.
/dev/null, eh? It shows you don't use Windows on a day to day basis. ;-)
But yeah consistency is really good to have. :)
From: Stefan Beller <hidden> Date: 2018-08-21 00:36:29
At the risk of going on a tangent, I assumed this was because enums
are really ints, and the "default" is there in case the enum somehow
got assigned to an int without a corresponding value. Either because
of a cast from an int that was out-of-range, or new values that were
obtained from arithmetic or bitwise operations on the declared enum
values, which created undeclared values.
See
374166cb381 (grep: catch a missing enum in switch statement, 2017-05-25)
or a bit date, but nevertheless an interesting read:
b8527d5fa61 (wt-status: fix possible use of uninitialized variable, 2013-03-21)
...compilers these days are just too smart to reason about them :-)
On Mon, Aug 20, 2018 at 8:38 PM Stefan Beller [off-list ref] wrote:
On Mon, Aug 20, 2018 at 6:18 AM Matthew DeVore [off-list ref] wrote:
quoted
There were many instances in this file where it seemed like BUG would be
better, so I created a new commit before this one to switch them over. The
interdiff is below.
BTW, why are there so many instances of "die" without "_"? I expect all
errors that may be caused by a user to be localized.
Well, there is the porcelain layer to be consumed by a human user
and the plumbing that is good for scripts. And in scripts you might want
to grep for certain errors and react to that, so a non-localized error
message makes the script possible to run in any localisation.
I probably have a different view about this, but strings (as English
sentences) are for human only and should be translated. For machines
there should be well defined format (that just might look like
English), not totally free text. In some case, this format can be as
simple as the "error/warning/fatal" prefix, which is left
untranslated, but the rest should be. There is no guarantee that these
die() messages will not change in the future, even left untranslated.
--
Duy
From: Matthew DeVore <hidden> Date: 2018-09-04 18:06:04
I made the following changes since v6 of the patchset:
- (suggested by Duy Nguyen) add a new commit which replaces uses of die() with
BUG() in list-objects-filter.c wherever it corresponds to a coding error.
- Replace die() with BUG() in new code.
- Replace test_line_count = 0 with test_must_be_empty in new tests since the
trend seems to be, based on other RFCs in progress, that we are standardizing
on that phraseology. See:
https://public-inbox.org/git/20180819215725.29001-1-szeder.dev@gmail.com/
As asked in the last "What's cooking in git.git" post, the status of this patch
is:
- The original reviewer, Jonathan Tan, is on vacation and will be back later
this week.
- Stefan Beller has been reviewing the patchset in Jonathan's absence, and
stated that it's a good read despite not being familiar with the code:
https://public-inbox.org/git/CAGZ79kaWcGbyc2S5gOCU7NdvT4fN46jq4xK9MvTLAFBGhyuo2A@mail.gmail.com/
- I haven't updated this patch in a while since we have been in RC for a while,
but after this update I think it's ready. There hasn't been any comment or
request for change to the patchset recently.
Matthew DeVore (7):
list-objects: store common func args in struct
list-objects: refactor to process_tree_contents
list-objects: always parse trees gently
rev-list: handle missing tree objects properly
revision: mark non-user-given objects instead
list-objects-filter: use BUG rather than die
list-objects-filter: implement filter tree:0
Documentation/rev-list-options.txt | 5 +
builtin/rev-list.c | 11 +-
list-objects-filter-options.c | 4 +
list-objects-filter-options.h | 1 +
list-objects-filter.c | 60 ++++++-
list-objects.c | 232 +++++++++++++------------
revision.c | 1 -
revision.h | 25 ++-
t/t0410-partial-clone.sh | 45 +++++
t/t5317-pack-objects-filter-objects.sh | 41 +++++
t/t5616-partial-clone.sh | 38 ++++
t/t6112-rev-list-filters-objects.sh | 29 ++++
12 files changed, 367 insertions(+), 125 deletions(-)
--
2.19.0.rc1.350.ge57e33dbd1-goog
From: Matthew DeVore <hidden> Date: 2018-09-04 18:06:08
This will make utility functions easier to create, as done by the next
patch.
Signed-off-by: Matthew DeVore <redacted>
---
list-objects.c | 158 +++++++++++++++++++++++--------------------------
1 file changed, 74 insertions(+), 84 deletions(-)