{"thread":{"id":"49411","subject":"[PATCH] fetch-pack: approximate no_dependents with filter","startedAt":"2018-09-24T15:45:22Z","lastAt":"2018-10-03T23:05:05Z","messageCount":7,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358760","messageId":"20180924154516.48704-1-jonathantanmy@google.com","threadId":"49411","inReplyTo":null,"subject":"[PATCH] fetch-pack: approximate no_dependents with filter","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-24T15:45:16Z","receivedAt":"2018-09-24T15:45:22Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Whenever a lazy fetch is performed for a tree object, any trees and\nblobs it directly or indirectly references will be fetched as well.\nThere is a \"no_dependents\" argument in struct fetch_pack_args that\nindicates that objects that the wanted object references need not be\nsent, but it currently has no effect other than to inhibit usage of\nobject flags.\n\nExtend the \"no_dependents\" argument to also exclude sending of objects\nas much as the current protocol allows: when fetching a tree, all trees\nit references will be sent (but not the blobs), and when fetching a\nblob, it will still be sent. (If this mechanism is used to fetch a\ncommit or any other non-blob object, all referenced objects, except\nblobs, will be sent.) The client neither needs to know or specify the\ntype of each object it wants.\n\nThe necessary code change is done in fetch_pack() instead of somewhere\ncloser to where the \"filter\" instruction is written to the wire so that\nonly one part of the code needs to be changed in order for users of all\nprotocol versions to benefit from this optimization.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThis was prompted by a user at $DAY_JOB who had a partial clone\nexcluding trees, and had a workflow that only required tree objects (and\nnot blobs).\n\nThis will hopefully make partial clones excluding trees (with the\n\"tree:0\" filter) a bit better, in that if an operation requires only\ntrees to be inspected, the required download is much smaller.\n---\n fetch-pack.c             | 14 ++++++++++++++\n fetch-pack.h             |  7 +++++++\n t/t0410-partial-clone.sh | 41 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 88a078e9b..c25b0f54c 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1598,6 +1598,20 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n+\tif (args->no_dependents && !args->filter_options.choice) {\n+\t\t/*\n+\t\t * The protocol does not support requesting that only the\n+\t\t * wanted objects be sent, so approximate this by setting a\n+\t\t * \"blob:none\" filter if no filter is already set. This works\n+\t\t * for all object types: note that wanted blobs will still be\n+\t\t * sent because they are directly specified as a \"want\".\n+\t\t *\n+\t\t * NEEDSWORK: Add an option in the protocol to request that\n+\t\t * only the wanted objects be sent, and implement it.\n+\t\t */\n+\t\tparse_list_objects_filter(&args->filter_options, \"blob:none\");\n+\t}\n+\n \tif (!ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 5b6e86880..43ec344d9 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -43,6 +43,13 @@ struct fetch_pack_args {\n \tunsigned from_promisor:1;\n \n \t/*\n+\t * Attempt to fetch only the wanted objects, and not any objects\n+\t * referred to by them. Due to protocol limitations, extraneous\n+\t * objects may still be included. (When fetching non-blob\n+\t * objects, only blobs are excluded; when fetching a blob, the\n+\t * blob itself will still be sent. The client does not need to\n+\t * know whether a wanted object is a blob or not.)\n+\t *\n \t * If 1, fetch_pack() will also not modify any object flags.\n \t * This allows fetch_pack() to safely be called by any function,\n \t * regardless of which object flags it uses (if any).\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 128130066..08a0c3651 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -170,6 +170,47 @@ test_expect_success 'fetching of missing objects' '\n \tgit verify-pack --verbose \"$IDX\" | grep \"$HASH\"\n '\n \n+test_expect_success 'fetching of missing blobs works' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\n+\tgit -C repo cat-file -p $(cat blobhash)\n+'\n+\n+test_expect_success 'fetching of missing trees does not fetch blobs' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit -C repo rev-parse foo^{tree} >treehash &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\tgit -C repo cat-file -p $(cat treehash) &&\n+\n+\t# Ensure that the tree, but not the blob, is fetched\n+\tgit -C repo rev-list --objects --missing=print $(cat treehash) >objects &&\n+\tgrep \"^$(cat treehash)\" objects &&\n+\tgrep \"^[?]$(cat blobhash)\" objects\n+'\n+\n test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358880","messageId":"xmqqh8idns9i.fsf@gitster-ct.c.googlers.com","threadId":"49411","inReplyTo":"20180924154516.48704-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: approximate no_dependents with filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-25T22:09:45Z","receivedAt":"2018-09-25T22:09:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> Whenever a lazy fetch is performed for a tree object, any trees and\n> blobs it directly or indirectly references will be fetched as well.\n> There is a \"no_dependents\" argument in struct fetch_pack_args that\n> indicates that objects that the wanted object references need not be\n> sent, but it currently has no effect other than to inhibit usage of\n> object flags.\n>\n> Extend the \"no_dependents\" argument to also exclude sending of objects\n> as much as the current protocol allows: when fetching a tree, all trees\n> it references will be sent (but not the blobs), and when fetching a\n> blob, it will still be sent. (If this mechanism is used to fetch a\n> commit or any other non-blob object, all referenced objects, except\n> blobs, will be sent.) The client neither needs to know or specify the\n> type of each object it wants.\n>\n> The necessary code change is done in fetch_pack() instead of somewhere\n> closer to where the \"filter\" instruction is written to the wire so that\n> only one part of the code needs to be changed in order for users of all\n> protocol versions to benefit from this optimization.\n\nIt is very clear how you are churning the code, but it is utterly\nunclear from the description what you perceived as a problem and why\nthis change is a good (if not the best) solution for that problem,\nat least to me.\n\nAfter reading the above description, I cannot shake the feeling that\nthis is tied too strongly to the tree:0 use case?  Does it help\nother use cases (e.g. would it be useful or harmful if a lazy clone\nwas done to exclude blobs that are larger than certain threshold, or\nobjects of all types that are not referenced by commits younger than\ncertain threshold)?\n\n"},{"id":"359087","messageId":"20180927183718.89804-1-jonathantanmy@google.com","threadId":"49411","inReplyTo":"xmqqh8idns9i.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch-pack: approximate no_dependents with filter","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-27T18:37:18Z","receivedAt":"2018-09-27T18:37:23Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> It is very clear how you are churning the code, but it is utterly\n> unclear from the description what you perceived as a problem and why\n> this change is a good (if not the best) solution for that problem,\n> at least to me.\n\nFirstly, thanks for your comments and questions - it's sometimes hard\nfor me to think of the questions someone else would ask when reading one\nof my patches. I have tried to rewrite the commit message (you can see\nit at the end of this e-mail) following your questions.\n\nThe new paragraph 1 addresses what I perceive as a problem, and the new\nparagraph 2 addresses the ideal and partial solution.\n\n> After reading the above description, I cannot shake the feeling that\n> this is tied too strongly to the tree:0 use case?  Does it help\n> other use cases (e.g. would it be useful or harmful if a lazy clone\n> was done to exclude blobs that are larger than certain threshold, or\n> objects of all types that are not referenced by commits younger than\n> certain threshold)?\n\nYes, it is solely for the tree:0 use case. But it doesn't hurt other use\ncases, as I have explained in new paragraph 3.\n\nI have retained old paragraph 3 as new paragraph 4, and removed old\nparagraph 2 as it mostly duplicates the comments in the code. New commit\nmessage follows:\n\n[start commit message]\n\nfetch-pack: exclude blobs when lazy-fetching trees\n\nA partial clone with missing trees can be obtained using \"git clone\n--filter=tree:none <repo>\". In such a repository, when a tree needs to\nbe lazily fetched, any tree or blob it directly or indirectly references\nis fetched as well, regardless of whether the original command required\nthose objects, or if the local repository already had some of them.\n\nThis is because the fetch protocol, which the lazy fetch uses, does not\nallow clients to request that only the wanted objects be sent, which\nwould be the ideal solution. This patch implements a partial solution:\nspecify the \"blob:none\" filter, somewhat reducing the fetch payload.\n\nThis change has no effect when lazily fetching blobs (due to how filters\nwork). And if lazily fetching a commit (such repositories are difficult\nto construct and is not a use case we support very well, but it is\npossible), referenced commits and trees are still fetched - only the\nblobs are not fetched.\n\nThe necessary code change is done in fetch_pack() instead of somewhere\ncloser to where the \"filter\" instruction is written to the wire so that\nonly one part of the code needs to be changed in order for users of all\nprotocol versions to benefit from this optimization.\n\n[end commit message]\n"},{"id":"359320","messageId":"xmqqftxs82yh.fsf@gitster-ct.c.googlers.com","threadId":"49411","inReplyTo":"20180924154516.48704-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch-pack: approximate no_dependents with filter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-29T20:26:46Z","receivedAt":"2018-09-29T20:26:51Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> This was prompted by a user at $DAY_JOB who had a partial clone\n> excluding trees, and had a workflow that only required tree objects (and\n> not blobs).\n>\n> This will hopefully make partial clones excluding trees (with the\n> \"tree:0\" filter) a bit better, in that if an operation requires only\n> trees to be inspected, the required download is much smaller.\n\nThis seems to break 5520 and 5616 when merged to 'pu'.  \n\nIt seems that merging master to md/filter-trees and then applying\nthis is sufficient to break 5616.\n"},{"id":"359595","messageId":"cover.1538607476.git.jonathantanmy@google.com","threadId":"49411","inReplyTo":"20180924154516.48704-1-jonathantanmy@google.com","subject":"[PATCH v2 0/2] Lazy fetch bug fix (and feature that reveals it)","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-03T23:04:51Z","receivedAt":"2018-10-03T23:05:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This seems to break 5520 and 5616 when merged to 'pu'.  \n> \n> It seems that merging master to md/filter-trees and then applying\n> this is sufficient to break 5616.\n\nI verified that 5616 is broken on [master + md/filter-trees + my patch],\nand after investigation, found a bug in the lazy fetch implementation.\nPatch 1 contains a fix, and more information about that bug.\n\nPatch 2 is the original patch, updated with the commit message I\nsuggested [1] in reply to Junio's comment.\n\nI bundled these patches together because patch 2 (in combination with\ncertain other commits) reveals the bug that patch 1 fixes, and to make\nit easier for others to verify that these patches together pass all\ntests when rebased on [master + md/filter-trees] or 'pu' (at least, as\nof the time of writing).\n\n[1] https://public-inbox.org/git/20180927183718.89804-1-jonathantanmy@google.com/\n\nJonathan Tan (2):\n  fetch-pack: avoid object flags if no_dependents\n  fetch-pack: exclude blobs when lazy-fetching trees\n\n fetch-pack.c             | 115 +++++++++++++++++++++++++--------------\n fetch-pack.h             |   7 +++\n t/t0410-partial-clone.sh |  41 ++++++++++++++\n 3 files changed, 121 insertions(+), 42 deletions(-)\n\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359596","messageId":"ac263ac3bec1aaf482248730afa126a8667fa8d5.1538607476.git.jonathantanmy@google.com","threadId":"49411","inReplyTo":"cover.1538607476.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] fetch-pack: avoid object flags if no_dependents","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-03T23:04:52Z","receivedAt":"2018-10-03T23:05:03Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetch_pack() is invoked as part of another Git command (due to a\nlazy fetch from a partial clone, for example), it uses object flags that\nmay already be used by the outer Git command.\n\nThe commit that introduced the lazy fetch feature (88e2f9ed8e\n(\"introduce fetch-object: fetch one promisor object\", 2017-12-05)) tried\nto avoid this overlap, but it did not avoid it totally. It was\nsuccessful in avoiding writing COMPLETE, but did not avoid reading\nCOMPLETE, and did not avoid writing and reading ALTERNATE.\n\nEnsure that no flags are written or read by fetch_pack() in the case\nwhere it is used to perform a lazy fetch. To do this, it is sufficient\nto avoid checking completeness of wanted refs (unnecessary in the case\nof lazy fetches), and to avoid negotiation-related work (in the current\nimplementation, already, no negotiation is performed). After that was\ndone, the lack of overlap was verified by checking all direct and\nindirect usages of COMPLETE and ALTERNATE - that they are read or\nwritten only if no_dependents is false.\n\nThere are other possible solutions to this issue:\n\n (1) Split fetch-pack.{c,h} into a flag-using part and a non-flag-using\n     part, and whenever no_dependents is set, only use the\n     non-flag-using part.\n (2) Make fetch_pack() be able to be used with arbitrary repository\n     objects. fetch_pack() should then create its own repository object\n     based on the given repository object, with its own object\n     hashtable, so that the flags do not conflict.\n\n(1) is possible but invasive - some functions would need to be split;\nand such invasiveness would potentially be unnecessary if we ever were\nto need (2) anyway. (2) would be useful if we were to support, say,\nsubmodules that were partial clones themselves, but I don't know when or\nif the Git project plans to support those.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c | 101 ++++++++++++++++++++++++++++++---------------------\n 1 file changed, 59 insertions(+), 42 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 75047a4b2a..b9b1211dda 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -253,8 +253,10 @@ static int find_common(struct fetch_negotiator *negotiator,\n \tif (args->stateless_rpc && multi_ack == 1)\n \t\tdie(_(\"--stateless-rpc requires multi_ack_detailed\"));\n \n-\tmark_tips(negotiator, args->negotiation_tips);\n-\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n+\tif (!args->no_dependents) {\n+\t\tmark_tips(negotiator, args->negotiation_tips);\n+\t\tfor_each_cached_alternate(negotiator, insert_one_alternate_object);\n+\t}\n \n \tfetching = 0;\n \tfor ( ; refs ; refs = refs->next) {\n@@ -271,8 +273,12 @@ static int find_common(struct fetch_negotiator *negotiator,\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n+\t\t *\n+\t\t * Do this only if args->no_dependents is false (if it is true,\n+\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (((o = lookup_object(the_repository, remote->hash)) != NULL) &&\n+\t\tif (!args->no_dependents &&\n+\t\t    ((o = lookup_object(the_repository, remote->hash)) != NULL) &&\n \t\t\t\t(o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -710,31 +716,29 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator,\n \n \toidset_clear(&loose_oid_set);\n \n-\tif (!args->no_dependents) {\n-\t\tif (!args->deepen) {\n-\t\t\tfor_each_ref(mark_complete_oid, NULL);\n-\t\t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n-\t\t\tcommit_list_sort_by_date(&complete);\n-\t\t\tif (cutoff)\n-\t\t\t\tmark_recent_complete_commits(args, cutoff);\n-\t\t}\n+\tif (!args->deepen) {\n+\t\tfor_each_ref(mark_complete_oid, NULL);\n+\t\tfor_each_cached_alternate(NULL, mark_alternate_complete);\n+\t\tcommit_list_sort_by_date(&complete);\n+\t\tif (cutoff)\n+\t\t\tmark_recent_complete_commits(args, cutoff);\n+\t}\n \n-\t\t/*\n-\t\t * Mark all complete remote refs as common refs.\n-\t\t * Don't mark them common yet; the server has to be told so first.\n-\t\t */\n-\t\tfor (ref = *refs; ref; ref = ref->next) {\n-\t\t\tstruct object *o = deref_tag(the_repository,\n-\t\t\t\t\t\t     lookup_object(the_repository,\n-\t\t\t\t\t\t     ref->old_oid.hash),\n-\t\t\t\t\t\t     NULL, 0);\n+\t/*\n+\t * Mark all complete remote refs as common refs.\n+\t * Don't mark them common yet; the server has to be told so first.\n+\t */\n+\tfor (ref = *refs; ref; ref = ref->next) {\n+\t\tstruct object *o = deref_tag(the_repository,\n+\t\t\t\t\t     lookup_object(the_repository,\n+\t\t\t\t\t     ref->old_oid.hash),\n+\t\t\t\t\t     NULL, 0);\n \n-\t\t\tif (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))\n-\t\t\t\tcontinue;\n+\t\tif (!o || o->type != OBJ_COMMIT || !(o->flags & COMPLETE))\n+\t\t\tcontinue;\n \n-\t\t\tnegotiator->known_common(negotiator,\n-\t\t\t\t\t\t (struct commit *)o);\n-\t\t}\n+\t\tnegotiator->known_common(negotiator,\n+\t\t\t\t\t (struct commit *)o);\n \t}\n \n \tsave_commit_buffer = old_save_commit_buffer;\n@@ -990,11 +994,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \tif (!server_supports(\"deepen-relative\") && args->deepen_relative)\n \t\tdie(_(\"Server does not support --deepen\"));\n \n-\tmark_complete_and_common_ref(&negotiator, args, &ref);\n-\tfilter_refs(args, &ref, sought, nr_sought);\n-\tif (everything_local(args, &ref)) {\n-\t\tpacket_flush(fd[1]);\n-\t\tgoto all_done;\n+\tif (!args->no_dependents) {\n+\t\tmark_complete_and_common_ref(&negotiator, args, &ref);\n+\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\tif (everything_local(args, &ref)) {\n+\t\t\tpacket_flush(fd[1]);\n+\t\t\tgoto all_done;\n+\t\t}\n+\t} else {\n+\t\tfilter_refs(args, &ref, sought, nr_sought);\n \t}\n \tif (find_common(&negotiator, args, fd, &oid, ref) < 0)\n \t\tif (!args->keep_pack)\n@@ -1040,7 +1048,7 @@ static void add_shallow_requests(struct strbuf *req_buf,\n \t}\n }\n \n-static void add_wants(const struct ref *wants, struct strbuf *req_buf)\n+static void add_wants(int no_dependents, const struct ref *wants, struct strbuf *req_buf)\n {\n \tint use_ref_in_want = server_supports_feature(\"fetch\", \"ref-in-want\", 0);\n \n@@ -1057,8 +1065,12 @@ static void add_wants(const struct ref *wants, struct strbuf *req_buf)\n \t\t * We use lookup_object here because we are only\n \t\t * interested in the case we *know* the object is\n \t\t * reachable and we have already scanned it.\n+\t\t *\n+\t\t * Do this only if args->no_dependents is false (if it is true,\n+\t\t * we cannot trust the object flags).\n \t\t */\n-\t\tif (((o = lookup_object(the_repository, remote->hash)) != NULL) &&\n+\t\tif (!no_dependents &&\n+\t\t    ((o = lookup_object(the_repository, remote->hash)) != NULL) &&\n \t\t    (o->flags & COMPLETE)) {\n \t\t\tcontinue;\n \t\t}\n@@ -1155,7 +1167,7 @@ static int send_fetch_request(struct fetch_negotiator *negotiator, int fd_out,\n \t}\n \n \t/* add wants */\n-\tadd_wants(wants, &req_buf);\n+\tadd_wants(args->no_dependents, wants, &req_buf);\n \n \tif (args->no_dependents) {\n \t\tpacket_buf_write(&req_buf, \"done\");\n@@ -1346,16 +1358,21 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,\n \t\t\t\targs->deepen = 1;\n \n \t\t\t/* Filter 'ref' by 'sought' and those that aren't local */\n-\t\t\tmark_complete_and_common_ref(&negotiator, args, &ref);\n-\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n-\t\t\tif (everything_local(args, &ref))\n-\t\t\t\tstate = FETCH_DONE;\n-\t\t\telse\n+\t\t\tif (!args->no_dependents) {\n+\t\t\t\tmark_complete_and_common_ref(&negotiator, args, &ref);\n+\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n+\t\t\t\tif (everything_local(args, &ref))\n+\t\t\t\t\tstate = FETCH_DONE;\n+\t\t\t\telse\n+\t\t\t\t\tstate = FETCH_SEND_REQUEST;\n+\n+\t\t\t\tmark_tips(&negotiator, args->negotiation_tips);\n+\t\t\t\tfor_each_cached_alternate(&negotiator,\n+\t\t\t\t\t\t\t  insert_one_alternate_object);\n+\t\t\t} else {\n+\t\t\t\tfilter_refs(args, &ref, sought, nr_sought);\n \t\t\t\tstate = FETCH_SEND_REQUEST;\n-\n-\t\t\tmark_tips(&negotiator, args->negotiation_tips);\n-\t\t\tfor_each_cached_alternate(&negotiator,\n-\t\t\t\t\t\t  insert_one_alternate_object);\n+\t\t\t}\n \t\t\tbreak;\n \t\tcase FETCH_SEND_REQUEST:\n \t\t\tif (send_fetch_request(&negotiator, fd[1], args, ref,\n-- \n2.19.0.605.g01d371f741-goog\n\n"},{"id":"359597","messageId":"1142e0a4e1db9f4f5c0cee41e936a36deb64bd5d.1538607476.git.jonathantanmy@google.com","threadId":"49411","inReplyTo":"cover.1538607476.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] fetch-pack: exclude blobs when lazy-fetching trees","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-10-03T23:04:53Z","receivedAt":"2018-10-03T23:05:05Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"A partial clone with missing trees can be obtained using \"git clone\n--filter=tree:none <repo>\". In such a repository, when a tree needs to\nbe lazily fetched, any tree or blob it directly or indirectly references\nis fetched as well, regardless of whether the original command required\nthose objects, or if the local repository already had some of them.\n\nThis is because the fetch protocol, which the lazy fetch uses, does not\nallow clients to request that only the wanted objects be sent, which\nwould be the ideal solution. This patch implements a partial solution:\nspecify the \"blob:none\" filter, somewhat reducing the fetch payload.\n\nThis change has no effect when lazily fetching blobs (due to how filters\nwork). And if lazily fetching a commit (such repositories are difficult\nto construct and is not a use case we support very well, but it is\npossible), referenced commits and trees are still fetched - only the\nblobs are not fetched.\n\nThe necessary code change is done in fetch_pack() instead of somewhere\ncloser to where the \"filter\" instruction is written to the wire so that\nonly one part of the code needs to be changed in order for users of all\nprotocol versions to benefit from this optimization.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c             | 14 ++++++++++++++\n fetch-pack.h             |  7 +++++++\n t/t0410-partial-clone.sh | 41 ++++++++++++++++++++++++++++++++++++++++\n 3 files changed, 62 insertions(+)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex b9b1211dda..5d82666a52 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -1615,6 +1615,20 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n+\tif (args->no_dependents && !args->filter_options.choice) {\n+\t\t/*\n+\t\t * The protocol does not support requesting that only the\n+\t\t * wanted objects be sent, so approximate this by setting a\n+\t\t * \"blob:none\" filter if no filter is already set. This works\n+\t\t * for all object types: note that wanted blobs will still be\n+\t\t * sent because they are directly specified as a \"want\".\n+\t\t *\n+\t\t * NEEDSWORK: Add an option in the protocol to request that\n+\t\t * only the wanted objects be sent, and implement it.\n+\t\t */\n+\t\tparse_list_objects_filter(&args->filter_options, \"blob:none\");\n+\t}\n+\n \tif (!ref) {\n \t\tpacket_flush(fd[1]);\n \t\tdie(_(\"no matching remote head\"));\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex 5b6e868802..43ec344d95 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -43,6 +43,13 @@ struct fetch_pack_args {\n \tunsigned from_promisor:1;\n \n \t/*\n+\t * Attempt to fetch only the wanted objects, and not any objects\n+\t * referred to by them. Due to protocol limitations, extraneous\n+\t * objects may still be included. (When fetching non-blob\n+\t * objects, only blobs are excluded; when fetching a blob, the\n+\t * blob itself will still be sent. The client does not need to\n+\t * know whether a wanted object is a blob or not.)\n+\t *\n \t * If 1, fetch_pack() will also not modify any object flags.\n \t * This allows fetch_pack() to safely be called by any function,\n \t * regardless of which object flags it uses (if any).\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex cfd0655ea1..c521d7d6c6 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -182,6 +182,47 @@ test_expect_success 'fetching of missing objects works with ref-in-want enabled'\n \tgrep \"git< fetch=.*ref-in-want\" trace\n '\n \n+test_expect_success 'fetching of missing blobs works' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\n+\tgit -C repo cat-file -p $(cat blobhash)\n+'\n+\n+test_expect_success 'fetching of missing trees does not fetch blobs' '\n+\trm -rf server repo &&\n+\ttest_create_repo server &&\n+\ttest_commit -C server foo &&\n+\tgit -C server repack -a -d --write-bitmap-index &&\n+\n+\tgit clone \"file://$(pwd)/server\" repo &&\n+\tgit -C repo rev-parse foo^{tree} >treehash &&\n+\tgit hash-object repo/foo.t >blobhash &&\n+\trm -rf repo/.git/objects/* &&\n+\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C repo config core.repositoryformatversion 1 &&\n+\tgit -C repo config extensions.partialclone \"origin\" &&\n+\tgit -C repo cat-file -p $(cat treehash) &&\n+\n+\t# Ensure that the tree, but not the blob, is fetched\n+\tgit -C repo rev-list --objects --missing=print $(cat treehash) >objects &&\n+\tgrep \"^$(cat treehash)\" objects &&\n+\tgrep \"^[?]$(cat blobhash)\" objects\n+'\n+\n test_expect_success 'rev-list stops traversal at missing and promised commit' '\n \trm -rf repo &&\n \ttest_create_repo repo &&\n-- \n2.19.0.605.g01d371f741-goog\n\n"}]}