| Message ID | 20180924154516.48704-1-jonathantanmy@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | fetch-pack: approximate no_dependents with filter | expand |
Jonathan Tan <jonathantanmy@google.com> writes: > Whenever a lazy fetch is performed for a tree object, any trees and > blobs it directly or indirectly references will be fetched as well. > There is a "no_dependents" argument in struct fetch_pack_args that > indicates that objects that the wanted object references need not be > sent, but it currently has no effect other than to inhibit usage of > object flags. > > Extend the "no_dependents" argument to also exclude sending of objects > as much as the current protocol allows: when fetching a tree, all trees > it references will be sent (but not the blobs), and when fetching a > blob, it will still be sent. (If this mechanism is used to fetch a > commit or any other non-blob object, all referenced objects, except > blobs, will be sent.) The client neither needs to know or specify the > type of each object it wants. > > The necessary code change is done in fetch_pack() instead of somewhere > closer to where the "filter" instruction is written to the wire so that > only one part of the code needs to be changed in order for users of all > protocol versions to benefit from this optimization. It is very clear how you are churning the code, but it is utterly unclear from the description what you perceived as a problem and why this change is a good (if not the best) solution for that problem, at least to me. After reading the above description, I cannot shake the feeling that this is tied too strongly to the tree:0 use case? Does it help other use cases (e.g. would it be useful or harmful if a lazy clone was done to exclude blobs that are larger than certain threshold, or objects of all types that are not referenced by commits younger than certain threshold)?
> It is very clear how you are churning the code, but it is utterly > unclear from the description what you perceived as a problem and why > this change is a good (if not the best) solution for that problem, > at least to me. Firstly, thanks for your comments and questions - it's sometimes hard for me to think of the questions someone else would ask when reading one of my patches. I have tried to rewrite the commit message (you can see it at the end of this e-mail) following your questions. The new paragraph 1 addresses what I perceive as a problem, and the new paragraph 2 addresses the ideal and partial solution. > After reading the above description, I cannot shake the feeling that > this is tied too strongly to the tree:0 use case? Does it help > other use cases (e.g. would it be useful or harmful if a lazy clone > was done to exclude blobs that are larger than certain threshold, or > objects of all types that are not referenced by commits younger than > certain threshold)? Yes, it is solely for the tree:0 use case. But it doesn't hurt other use cases, as I have explained in new paragraph 3. I have retained old paragraph 3 as new paragraph 4, and removed old paragraph 2 as it mostly duplicates the comments in the code. New commit message follows: [start commit message] fetch-pack: exclude blobs when lazy-fetching trees A partial clone with missing trees can be obtained using "git clone --filter=tree:none <repo>". In such a repository, when a tree needs to be lazily fetched, any tree or blob it directly or indirectly references is fetched as well, regardless of whether the original command required those objects, or if the local repository already had some of them. This is because the fetch protocol, which the lazy fetch uses, does not allow clients to request that only the wanted objects be sent, which would be the ideal solution. This patch implements a partial solution: specify the "blob:none" filter, somewhat reducing the fetch payload. This change has no effect when lazily fetching blobs (due to how filters work). And if lazily fetching a commit (such repositories are difficult to construct and is not a use case we support very well, but it is possible), referenced commits and trees are still fetched - only the blobs are not fetched. The necessary code change is done in fetch_pack() instead of somewhere closer to where the "filter" instruction is written to the wire so that only one part of the code needs to be changed in order for users of all protocol versions to benefit from this optimization. [end commit message]
Jonathan Tan <jonathantanmy@google.com> writes: > This was prompted by a user at $DAY_JOB who had a partial clone > excluding trees, and had a workflow that only required tree objects (and > not blobs). > > This will hopefully make partial clones excluding trees (with the > "tree:0" filter) a bit better, in that if an operation requires only > trees to be inspected, the required download is much smaller. This seems to break 5520 and 5616 when merged to 'pu'. It seems that merging master to md/filter-trees and then applying this is sufficient to break 5616.
diff --git a/fetch-pack.c b/fetch-pack.c index 88a078e9b..c25b0f54c 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1598,6 +1598,20 @@ struct ref *fetch_pack(struct fetch_pack_args *args, if (nr_sought) nr_sought = remove_duplicates_in_refs(sought, nr_sought); + if (args->no_dependents && !args->filter_options.choice) { + /* + * The protocol does not support requesting that only the + * wanted objects be sent, so approximate this by setting a + * "blob:none" filter if no filter is already set. This works + * for all object types: note that wanted blobs will still be + * sent because they are directly specified as a "want". + * + * NEEDSWORK: Add an option in the protocol to request that + * only the wanted objects be sent, and implement it. + */ + parse_list_objects_filter(&args->filter_options, "blob:none"); + } + if (!ref) { packet_flush(fd[1]); die(_("no matching remote head")); diff --git a/fetch-pack.h b/fetch-pack.h index 5b6e86880..43ec344d9 100644 --- a/fetch-pack.h +++ b/fetch-pack.h @@ -43,6 +43,13 @@ struct fetch_pack_args { unsigned from_promisor:1; /* + * Attempt to fetch only the wanted objects, and not any objects + * referred to by them. Due to protocol limitations, extraneous + * objects may still be included. (When fetching non-blob + * objects, only blobs are excluded; when fetching a blob, the + * blob itself will still be sent. The client does not need to + * know whether a wanted object is a blob or not.) + * * If 1, fetch_pack() will also not modify any object flags. * This allows fetch_pack() to safely be called by any function, * regardless of which object flags it uses (if any). diff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh index 128130066..08a0c3651 100755 --- a/t/t0410-partial-clone.sh +++ b/t/t0410-partial-clone.sh @@ -170,6 +170,47 @@ test_expect_success 'fetching of missing objects' ' git verify-pack --verbose "$IDX" | grep "$HASH" ' +test_expect_success 'fetching of missing blobs works' ' + rm -rf server repo && + test_create_repo server && + test_commit -C server foo && + git -C server repack -a -d --write-bitmap-index && + + git clone "file://$(pwd)/server" repo && + git hash-object repo/foo.t >blobhash && + rm -rf repo/.git/objects/* && + + git -C server config uploadpack.allowanysha1inwant 1 && + git -C server config uploadpack.allowfilter 1 && + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialclone "origin" && + + git -C repo cat-file -p $(cat blobhash) +' + +test_expect_success 'fetching of missing trees does not fetch blobs' ' + rm -rf server repo && + test_create_repo server && + test_commit -C server foo && + git -C server repack -a -d --write-bitmap-index && + + git clone "file://$(pwd)/server" repo && + git -C repo rev-parse foo^{tree} >treehash && + git hash-object repo/foo.t >blobhash && + rm -rf repo/.git/objects/* && + + git -C server config uploadpack.allowanysha1inwant 1 && + git -C server config uploadpack.allowfilter 1 && + git -C repo config core.repositoryformatversion 1 && + git -C repo config extensions.partialclone "origin" && + git -C repo cat-file -p $(cat treehash) && + + # Ensure that the tree, but not the blob, is fetched + git -C repo rev-list --objects --missing=print $(cat treehash) >objects && + grep "^$(cat treehash)" objects && + grep "^[?]$(cat blobhash)" objects +' + test_expect_success 'rev-list stops traversal at missing and promised commit' ' rm -rf repo && test_create_repo repo &&
Whenever a lazy fetch is performed for a tree object, any trees and blobs it directly or indirectly references will be fetched as well. There is a "no_dependents" argument in struct fetch_pack_args that indicates that objects that the wanted object references need not be sent, but it currently has no effect other than to inhibit usage of object flags. Extend the "no_dependents" argument to also exclude sending of objects as much as the current protocol allows: when fetching a tree, all trees it references will be sent (but not the blobs), and when fetching a blob, it will still be sent. (If this mechanism is used to fetch a commit or any other non-blob object, all referenced objects, except blobs, will be sent.) The client neither needs to know or specify the type of each object it wants. The necessary code change is done in fetch_pack() instead of somewhere closer to where the "filter" instruction is written to the wire so that only one part of the code needs to be changed in order for users of all protocol versions to benefit from this optimization. Signed-off-by: Jonathan Tan <jonathantanmy@google.com> --- This was prompted by a user at $DAY_JOB who had a partial clone excluding trees, and had a workflow that only required tree objects (and not blobs). This will hopefully make partial clones excluding trees (with the "tree:0" filter) a bit better, in that if an operation requires only trees to be inspected, the required download is much smaller. --- fetch-pack.c | 14 ++++++++++++++ fetch-pack.h | 7 +++++++ t/t0410-partial-clone.sh | 41 ++++++++++++++++++++++++++++++++++++++++ 3 files changed, 62 insertions(+)