{"thread":{"id":"56156","subject":"[PATCH 0/2] Another partial clone prefetch","startedAt":"2021-07-23T18:52:31Z","lastAt":"2021-07-26T13:01:09Z","messageCount":8,"participants":["Jonathan Tan","Elijah Newren","Junio C Hamano","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"431080","messageId":"cover.1627066238.git.jonathantanmy@google.com","threadId":"56156","inReplyTo":null,"subject":"[PATCH 0/2] Another partial clone prefetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-23T18:52:21Z","receivedAt":"2021-07-23T18:52:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Here's another instance in which we need to prefetch in order to avoid\nfetching objects one by one. More information is in patch 2's commit\nmessage.\n\nJonathan Tan (2):\n unpack-trees: refactor prefetching code\n cache-tree: prefetch in partial clone read-tree\n\n cache-tree.c | 11 ++++++++--\n cache.h | 9 ++++++++\n read-cache.c | 23 +++++++++++++++++++++\n t/t1022-read-tree-partial-clone.sh | 33 ++++++++++++++++++++++++++++++\n unpack-trees.c | 27 ++++++++----------------\n 5 files changed, 82 insertions(+), 21 deletions(-)\n create mode 100755 t/t1022-read-tree-partial-clone.sh\n\n-- \n2.32.0.432.gabb21c7263-goog\n\n"},{"id":"431081","messageId":"f502997d159c7a30862fab3c3b443291539b6f29.1627066238.git.jonathantanmy@google.com","threadId":"56156","inReplyTo":"cover.1627066238.git.jonathantanmy@google.com","subject":"[PATCH 1/2] unpack-trees: refactor prefetching code","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-23T18:52:22Z","receivedAt":"2021-07-23T18:52:31Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Refactor the prefetching code in unpack-trees.c into its own function,\nbecause it will be used elsewhere in a subsequent commit.\n\nSigned-off-by: Jonathan Tan \n---\n cache.h | 9 +++++++++\n read-cache.c | 23 +++++++++++++++++++++++\n unpack-trees.c | 27 ++++++++-------------------\n 3 files changed, 40 insertions(+), 19 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex ba04ff8bd3..6f952e22c6 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -410,6 +410,15 @@ struct cache_entry *dup_cache_entry(const struct cache_entry *ce, struct index_s\n */\n void validate_cache_entries(const struct index_state *istate);\n \n+/*\n+ * Bulk prefetch all missing cache entries that are not GITLINKs and that match\n+ * the given predicate. This function should only be called if\n+ * has_promisor_remote() returns true.\n+ */\n+typedef int (*must_prefetch_predicate)(const struct cache_entry *);\n+void prefetch_cache_entries(const struct index_state *istate,\n+\t\t\t must_prefetch_predicate must_prefetch);\n+\n #ifdef USE_THE_INDEX_COMPATIBILITY_MACROS\n extern struct index_state the_index;\n \ndiff --git a/read-cache.c b/read-cache.c\nindex ba2b012a6c..4e396bf17f 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -27,6 +27,7 @@\n #include \"progress.h\"\n #include \"sparse-index.h\"\n #include \"csum-file.h\"\n+#include \"promisor-remote.h\"\n \n /* Mask for the name length in ce_flags in the on-disk index */\n \n@@ -3657,3 +3658,25 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta\n \t\tstrbuf_add(sb, &buffer, sizeof(uint32_t));\n \t}\n }\n+\n+void prefetch_cache_entries(const struct index_state *istate,\n+\t\t\t must_prefetch_predicate must_prefetch)\n+{\n+\tint i;\n+\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\n+\tfor (i = 0; i < istate->cache_nr; i++) {\n+\t\tstruct cache_entry *ce = istate->cache[i];\n+\n+\t\tif (S_ISGITLINK(ce->ce_mode) || !must_prefetch(ce))\n+\t\t\tcontinue;\n+\t\tif (!oid_object_info_extended(the_repository, &ce->oid,\n+\t\t\t\t\t NULL,\n+\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\t\tcontinue;\n+\t\toid_array_append(&to_fetch, &ce->oid);\n+\t}\n+\tpromisor_remote_get_direct(the_repository,\n+\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\toid_array_clear(&to_fetch);\n+}\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f88a69f8e7..ed92794032 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -392,6 +392,11 @@ static void report_collided_checkout(struct index_state *index)\n \tstring_list_clear(&list, 0);\n }\n \n+static int must_checkout(const struct cache_entry *ce)\n+{\n+\treturn ce->ce_flags & CE_UPDATE;\n+}\n+\n static int check_updates(struct unpack_trees_options *o,\n \t\t\t struct index_state *index)\n {\n@@ -442,28 +447,12 @@ static int check_updates(struct unpack_trees_options *o,\n \tif (should_update_submodules())\n \t\tload_gitmodules_file(index, &state);\n \n-\tif (has_promisor_remote()) {\n+\tif (has_promisor_remote())\n \t\t/*\n \t\t * Prefetch the objects that are to be checked out in the loop\n \t\t * below.\n \t\t */\n-\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n-\t\tfor (i = 0; i < index->cache_nr; i++) {\n-\t\t\tstruct cache_entry *ce = index->cache[i];\n-\n-\t\t\tif (!(ce->ce_flags & CE_UPDATE) ||\n-\t\t\t S_ISGITLINK(ce->ce_mode))\n-\t\t\t\tcontinue;\n-\t\t\tif (!oid_object_info_extended(the_repository, &ce->oid,\n-\t\t\t\t\t\t NULL,\n-\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n-\t\t\t\tcontinue;\n-\t\t\toid_array_append(&to_fetch, &ce->oid);\n-\t\t}\n-\t\tpromisor_remote_get_direct(the_repository,\n-\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n-\t\toid_array_clear(&to_fetch);\n-\t}\n+\t\tprefetch_cache_entries(index, must_checkout);\n \n \tget_parallel_checkout_configs(&pc_workers, &pc_threshold);\n \n@@ -473,7 +462,7 @@ static int check_updates(struct unpack_trees_options *o,\n \tfor (i = 0; i < index->cache_nr; i++) {\n \t\tstruct cache_entry *ce = index->cache[i];\n \n-\t\tif (ce->ce_flags & CE_UPDATE) {\n+\t\tif (must_checkout(ce)) {\n \t\t\tsize_t last_pc_queue_size = pc_queue_size();\n \n \t\t\tif (ce->ce_flags & CE_WT_REMOVE)\n-- \n2.32.0.432.gabb21c7263-goog\n\n"},{"id":"431082","messageId":"f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com","threadId":"56156","inReplyTo":"cover.1627066238.git.jonathantanmy@google.com","subject":"[PATCH 2/2] cache-tree: prefetch in partial clone read-tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-23T18:52:23Z","receivedAt":"2021-07-23T18:52:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"\"git read-tree\" checks the existence of the blobs referenced by the\ngiven tree, but does not bulk prefetch them. Add a bulk prefetch.\n\nThe lack of prefetch here was noticed at $DAYJOB during a merge\ninvolving some specific commits, but I couldn't find a minimal merge\nthat didn't also trigger the prefetch in check_updates() in\nunpack-trees.c (and in all these cases, the lack of prefetch in\ncache-tree.c didn't matter because all the relevant blobs would have\nalready been prefetched by then). This is why I used read-tree here to\nexercise this code path.\n\nSigned-off-by: Jonathan Tan \n---\n cache-tree.c | 11 ++++++++--\n t/t1022-read-tree-partial-clone.sh | 33 ++++++++++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 2 deletions(-)\n create mode 100755 t/t1022-read-tree-partial-clone.sh\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 45e58666af..9ba2c7c6b2 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -237,6 +237,11 @@ int cache_tree_fully_valid(struct cache_tree *it)\n \treturn 1;\n }\n \n+static int must_check_existence(const struct cache_entry *ce)\n+{\n+\treturn !(has_promisor_remote() && ce_skip_worktree(ce));\n+}\n+\n static int update_one(struct cache_tree *it,\n \t\t struct cache_entry **cache,\n \t\t int entries,\n@@ -378,8 +383,7 @@ static int update_one(struct cache_tree *it,\n \t\t}\n \n \t\tce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n-\t\t\t(has_promisor_remote() &&\n-\t\t\t ce_skip_worktree(ce));\n+\t\t\t!must_check_existence(ce);\n \t\tif (is_null_oid(oid) ||\n \t\t (!ce_missing_ok && !has_object_file(oid))) {\n \t\t\tstrbuf_release(&buffer);\n@@ -466,6 +470,9 @@ int cache_tree_update(struct index_state *istate, int flags)\n \tif (!istate->cache_tree)\n \t\tistate->cache_tree = cache_tree();\n \n+\tif (!(flags & WRITE_TREE_MISSING_OK) && has_promisor_remote())\n+\t\tprefetch_cache_entries(istate, must_check_existence);\n+\n \ttrace_performance_enter();\n \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\ndiff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\nnew file mode 100755\nindex 0000000000..a763e27c7d\n--- /dev/null\n+++ b/t/t1022-read-tree-partial-clone.sh\n@@ -0,0 +1,33 @@\n+#!/bin/sh\n+\n+test_description='git read-tree in partial clones'\n+\n+TEST_NO_CREATE_REPO=1\n+\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'read-tree in partial clone prefetches in one batch' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\tgit init server &&\n+\techo foo >server/one &&\n+\techo bar >server/two &&\n+\tgit -C server add one two &&\n+\tgit -C server commit -m \"initial commit\" &&\n+\tTREE=$(git -C server rev-parse HEAD^{tree}) &&\n+\n+\tgit -C server config uploadpack.allowfilter 1 &&\n+\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n+\n+\t# \"done\" marks the end of negotiation (once per fetch). Expect that\n+\t# only one fetch occurs.\n+\tgrep \"fetch> done\" trace >donelines &&\n+\ttest_line_count = 1 donelines\n+'\n+\n+test_done\n-- \n2.32.0.432.gabb21c7263-goog\n\n"},{"id":"431083","messageId":"20210723185548.1784350-1-jonathantanmy@google.com","threadId":"56156","inReplyTo":"f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] cache-tree: prefetch in partial clone read-tree","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2021-07-23T18:55:48Z","receivedAt":"2021-07-23T18:55:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> \"git read-tree\" checks the existence of the blobs referenced by the\n> given tree, but does not bulk prefetch them. Add a bulk prefetch.\n> \n> The lack of prefetch here was noticed at $DAYJOB during a merge\n> involving some specific commits, but I couldn't find a minimal merge\n> that didn't also trigger the prefetch in check_updates() in\n> unpack-trees.c (and in all these cases, the lack of prefetch in\n> cache-tree.c didn't matter because all the relevant blobs would have\n> already been prefetched by then). This is why I used read-tree here to\n> exercise this code path.\n> \n> Signed-off-by: Jonathan Tan \n\nForgot to mention: the $DAYJOB case is the same case as in [1]. In [1] I\nnoticed that the object wasn't actually being used, so I disabled the\nobject existence check. But that's probably the wrong approach - if the\ncaller really didn't want the object's existence to be checked, they\ncould have used WRITE_TREE_MISSING_OK when calling cache_tree_update().\n\n[1] https://lore.kernel.org/git/cover.1627066238.git.jonathantanmy@google.com/\n"},{"id":"431095","messageId":"CABPp-BEUpXehjgh3-d4=r+V8CrYJxxBJ80Z8+EaJ=4ewvyGVDg@mail.gmail.com","threadId":"56156","inReplyTo":"f502997d159c7a30862fab3c3b443291539b6f29.1627066238.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] unpack-trees: refactor prefetching code","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T20:26:07Z","receivedAt":"2021-07-23T20:26:21Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 23, 2021 at 11:55 AM Jonathan Tan wrote:\n>\n> Refactor the prefetching code in unpack-trees.c into its own function,\n> because it will be used elsewhere in a subsequent commit.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> cache.h | 9 +++++++++\n> read-cache.c | 23 +++++++++++++++++++++++\n> unpack-trees.c | 27 ++++++++-------------------\n> 3 files changed, 40 insertions(+), 19 deletions(-)\n>\n> diff --git a/cache.h b/cache.h\n> index ba04ff8bd3..6f952e22c6 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -410,6 +410,15 @@ struct cache_entry *dup_cache_entry(const struct cache_entry *ce, struct index_s\n> */\n> void validate_cache_entries(const struct index_state *istate);\n>\n> +/*\n> + * Bulk prefetch all missing cache entries that are not GITLINKs and that match\n> + * the given predicate. This function should only be called if\n> + * has_promisor_remote() returns true.\n> + */\n> +typedef int (*must_prefetch_predicate)(const struct cache_entry *);\n> +void prefetch_cache_entries(const struct index_state *istate,\n> + must_prefetch_predicate must_prefetch);\n> +\n> #ifdef USE_THE_INDEX_COMPATIBILITY_MACROS\n> extern struct index_state the_index;\n>\n> diff --git a/read-cache.c b/read-cache.c\n> index ba2b012a6c..4e396bf17f 100644\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -27,6 +27,7 @@\n> #include \"progress.h\"\n> #include \"sparse-index.h\"\n> #include \"csum-file.h\"\n> +#include \"promisor-remote.h\"\n>\n> /* Mask for the name length in ce_flags in the on-disk index */\n>\n> @@ -3657,3 +3658,25 @@ static void write_ieot_extension(struct strbuf *sb, struct index_entry_offset_ta\n> strbuf_add(sb, &buffer, sizeof(uint32_t));\n> }\n> }\n> +\n> +void prefetch_cache_entries(const struct index_state *istate,\n> + must_prefetch_predicate must_prefetch)\n> +{\n> + int i;\n> + struct oid_array to_fetch = OID_ARRAY_INIT;\n> +\n> + for (i = 0; i < istate->cache_nr; i++) {\n> + struct cache_entry *ce = istate->cache[i];\n> +\n> + if (S_ISGITLINK(ce->ce_mode) || !must_prefetch(ce))\n> + continue;\n> + if (!oid_object_info_extended(the_repository, &ce->oid,\n> + NULL,\n> + OBJECT_INFO_FOR_PREFETCH))\n> + continue;\n> + oid_array_append(&to_fetch, &ce->oid);\n> + }\n> + promisor_remote_get_direct(the_repository,\n> + to_fetch.oid, to_fetch.nr);\n> + oid_array_clear(&to_fetch);\n> +}\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index f88a69f8e7..ed92794032 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -392,6 +392,11 @@ static void report_collided_checkout(struct index_state *index)\n> string_list_clear(&list, 0);\n> }\n>\n> +static int must_checkout(const struct cache_entry *ce)\n> +{\n> + return ce->ce_flags & CE_UPDATE;\n> +}\n> +\n> static int check_updates(struct unpack_trees_options *o,\n> struct index_state *index)\n> {\n> @@ -442,28 +447,12 @@ static int check_updates(struct unpack_trees_options *o,\n> if (should_update_submodules())\n> load_gitmodules_file(index, &state);\n>\n> - if (has_promisor_remote()) {\n> + if (has_promisor_remote())\n> /*\n> * Prefetch the objects that are to be checked out in the loop\n> * below.\n> */\n> - struct oid_array to_fetch = OID_ARRAY_INIT;\n> - for (i = 0; i < index->cache_nr; i++) {\n> - struct cache_entry *ce = index->cache[i];\n> -\n> - if (!(ce->ce_flags & CE_UPDATE) ||\n> - S_ISGITLINK(ce->ce_mode))\n> - continue;\n> - if (!oid_object_info_extended(the_repository, &ce->oid,\n> - NULL,\n> - OBJECT_INFO_FOR_PREFETCH))\n> - continue;\n> - oid_array_append(&to_fetch, &ce->oid);\n> - }\n> - promisor_remote_get_direct(the_repository,\n> - to_fetch.oid, to_fetch.nr);\n> - oid_array_clear(&to_fetch);\n> - }\n> + prefetch_cache_entries(index, must_checkout);\n>\n> get_parallel_checkout_configs(&pc_workers, &pc_threshold);\n>\n> @@ -473,7 +462,7 @@ static int check_updates(struct unpack_trees_options *o,\n> for (i = 0; i < index->cache_nr; i++) {\n> struct cache_entry *ce = index->cache[i];\n>\n> - if (ce->ce_flags & CE_UPDATE) {\n> + if (must_checkout(ce)) {\n> size_t last_pc_queue_size = pc_queue_size();\n>\n> if (ce->ce_flags & CE_WT_REMOVE)\n> --\n> 2.32.0.432.gabb21c7263-goog\n\nThis might have been slightly easier to review if you had introduced\nmust_checkout() first, and then made the new prefetch_cache_entries()\nin a separate commit, so that comparing old and new code I didn't have\nto try to deduce the separate steps. However, that's a really minor\npoint since must_checkout() is so small.\n\nAnyway, this patch looks good to me.\n"},{"id":"431098","messageId":"xmqqsg04loas.fsf@gitster.g","threadId":"56156","inReplyTo":"f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] cache-tree: prefetch in partial clone read-tree","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-23T21:20:59Z","receivedAt":"2021-07-23T21:21:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> @@ -466,6 +470,9 @@ int cache_tree_update(struct index_state *istate, int flags)\n> \tif (!istate->cache_tree)\n> \t\tistate->cache_tree = cache_tree();\n> \n> +\tif (!(flags & WRITE_TREE_MISSING_OK) && has_promisor_remote())\n> +\t\tprefetch_cache_entries(istate, must_check_existence);\n> +\n\nIt's so nice when a \"fix\" to an issue can be this simple.\n\nThanks; will queue.\n\n> \ttrace_performance_enter();\n> \ttrace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> \ti = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> diff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\n> new file mode 100755\n> index 0000000000..a763e27c7d\n> --- /dev/null\n> +++ b/t/t1022-read-tree-partial-clone.sh\n> @@ -0,0 +1,33 @@\n> +#!/bin/sh\n> +\n> +test_description='git read-tree in partial clones'\n> +\n> +TEST_NO_CREATE_REPO=1\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'read-tree in partial clone prefetches in one batch' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\tgit init server &&\n> +\techo foo >server/one &&\n> +\techo bar >server/two &&\n> +\tgit -C server add one two &&\n> +\tgit -C server commit -m \"initial commit\" &&\n> +\tTREE=$(git -C server rev-parse HEAD^{tree}) &&\n> +\n> +\tgit -C server config uploadpack.allowfilter 1 &&\n> +\tgit -C server config uploadpack.allowanysha1inwant 1 &&\n> +\tgit clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n> +\n> +\t# \"done\" marks the end of negotiation (once per fetch). Expect that\n> +\t# only one fetch occurs.\n> +\tgrep \"fetch> done\" trace >donelines &&\n> +\ttest_line_count = 1 donelines\n> +'\n> +\n> +test_done\n"},{"id":"431099","messageId":"CABPp-BHtJPp3qi3ww3EcxxzyCUU8DjS1ZWnEQfd4A2rqXyGUXg@mail.gmail.com","threadId":"56156","inReplyTo":"f4881b7455b9d33c8a53a91eda7fbdfc5d11382c.1627066238.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] cache-tree: prefetch in partial clone read-tree","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2021-07-23T21:34:34Z","receivedAt":"2021-07-23T21:34:48Z","isPatch":true,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"On Fri, Jul 23, 2021 at 11:55 AM Jonathan Tan wrote:\n>\n> \"git read-tree\" checks the existence of the blobs referenced by the\n> given tree, but does not bulk prefetch them. Add a bulk prefetch.\n>\n> The lack of prefetch here was noticed at $DAYJOB during a merge\n> involving some specific commits, but I couldn't find a minimal merge\n> that didn't also trigger the prefetch in check_updates() in\n> unpack-trees.c (and in all these cases, the lack of prefetch in\n> cache-tree.c didn't matter because all the relevant blobs would have\n> already been prefetched by then). This is why I used read-tree here to\n> exercise this code path.\n\nOkay, you have me stumped, I can't figure out what kind of a merge\nwould bypass the check_updates() in unpack-trees.c either. I was\ncurious about octopus or merge.autostash, but I just can't trigger it.\n\nUsing read-tree to trigger the case makes perfect sense, though.\n\n> Signed-off-by: Jonathan Tan \n> ---\n> cache-tree.c | 11 ++++++++--\n> t/t1022-read-tree-partial-clone.sh | 33 ++++++++++++++++++++++++++++++\n> 2 files changed, 42 insertions(+), 2 deletions(-)\n> create mode 100755 t/t1022-read-tree-partial-clone.sh\n>\n> diff --git a/cache-tree.c b/cache-tree.c\n> index 45e58666af..9ba2c7c6b2 100644\n> --- a/cache-tree.c\n> +++ b/cache-tree.c\n> @@ -237,6 +237,11 @@ int cache_tree_fully_valid(struct cache_tree *it)\n> return 1;\n> }\n>\n> +static int must_check_existence(const struct cache_entry *ce)\n> +{\n> + return !(has_promisor_remote() && ce_skip_worktree(ce));\n> +}\n> +\n> static int update_one(struct cache_tree *it,\n> struct cache_entry **cache,\n> int entries,\n> @@ -378,8 +383,7 @@ static int update_one(struct cache_tree *it,\n> }\n>\n> ce_missing_ok = mode == S_IFGITLINK || missing_ok ||\n> - (has_promisor_remote() &&\n> - ce_skip_worktree(ce));\n> + !must_check_existence(ce);\n> if (is_null_oid(oid) ||\n> (!ce_missing_ok && !has_object_file(oid))) {\n> strbuf_release(&buffer);\n> @@ -466,6 +470,9 @@ int cache_tree_update(struct index_state *istate, int flags)\n> if (!istate->cache_tree)\n> istate->cache_tree = cache_tree();\n>\n> + if (!(flags & WRITE_TREE_MISSING_OK) && has_promisor_remote())\n> + prefetch_cache_entries(istate, must_check_existence);\n> +\n\nNice that the fix is so simple.\n\n> trace_performance_enter();\n> trace2_region_enter(\"cache_tree\", \"update\", the_repository);\n> i = update_one(istate->cache_tree, istate->cache, istate->cache_nr,\n> diff --git a/t/t1022-read-tree-partial-clone.sh b/t/t1022-read-tree-partial-clone.sh\n> new file mode 100755\n> index 0000000000..a763e27c7d\n> --- /dev/null\n> +++ b/t/t1022-read-tree-partial-clone.sh\n> @@ -0,0 +1,33 @@\n> +#!/bin/sh\n> +\n> +test_description='git read-tree in partial clones'\n> +\n> +TEST_NO_CREATE_REPO=1\n> +\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'read-tree in partial clone prefetches in one batch' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + git init server &&\n> + echo foo >server/one &&\n> + echo bar >server/two &&\n> + git -C server add one two &&\n> + git -C server commit -m \"initial commit\" &&\n> + TREE=$(git -C server rev-parse HEAD^{tree}) &&\n> +\n> + git -C server config uploadpack.allowfilter 1 &&\n> + git -C server config uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:none \"file://$(pwd)/server\" client &&\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client read-tree $TREE &&\n> +\n> + # \"done\" marks the end of negotiation (once per fetch). Expect that\n> + # only one fetch occurs.\n> + grep \"fetch> done\" trace >donelines &&\n> + test_line_count = 1 donelines\n> +'\n> +\n> +test_done\n> --\n> 2.32.0.432.gabb21c7263-goog\n\nAny reason for preferring GIT_TRACE_PACKET over GIT_TRACE2_PERF and\nlooking for the reported fetch_count (or even the number of\nfetch_count lines)? Just curious.\n\nAnyway, looks good to me.\n"},{"id":"431185","messageId":"7f624239-907e-8e69-9419-a812efad9b9c@gmail.com","threadId":"56156","inReplyTo":"cover.1627066238.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Another partial clone prefetch","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2021-07-26T13:01:04Z","receivedAt":"2021-07-26T13:01:09Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 7/23/2021 2:52 PM, Jonathan Tan wrote:\n> Here's another instance in which we need to prefetch in order to avoid\n> fetching objects one by one. More information is in patch 2's commit\n> message.\n\nI know that Elijah and Junio already gave their reviews, but I'm here\nto chime in that this is a good pair of patches. Thanks!\n\n-Stolee\n"}]}