{"thread":{"id":"53139","subject":"[PATCH] diff: restrict when prefetching occurs","startedAt":"2020-03-31T02:04:26Z","lastAt":"2020-04-07T23:44:28Z","messageCount":26,"participants":["Jonathan Tan","Derrick Stolee","Junio C Hamano","Garima Singh"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"394401","messageId":"20200331020418.55640-1-jonathantanmy@google.com","threadId":"53139","inReplyTo":null,"subject":"[PATCH] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-31T02:04:18Z","receivedAt":"2020-03-31T02:04:26Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08)\noptimized \"diff\" by prefetching blobs in a partial clone, but there are\nsome cases wherein blobs do not need to be prefetched. In particular, if\n(1) no blob data is included in the output of the diff, (2)\nbreak-rewrite detection is not requested, and (3) no inexact rename\ndetection is needed, then no blobs are read at all.\n\nTherefore, in such a case, do not prefetch. Change diffcore_std() to\nonly prefetch if (1) and/or (2) is not true (currently, it always\nprefetches); change diffcore_rename() to prefetch if (3) is not true and\nno prefetch has yet occurred.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\nI decided to revisit [1] because at $DAYJOB there was a new case that\nrequired this. I used Peff's code from [2] for the rebase part, and also\nautomatically prefetch if blob data will be included in the output of\nthe diff and/or break-rewrite detection is requested.\n\n[1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@google.com/\n[2] https://lore.kernel.org/git/20200130055136.GA2184413@coredump.intra.peff.net/\n---\n diff.c | 26 +++++++++++++++----\n diffcore-rename.c | 40 +++++++++++++++++++++++++++-\n diffcore.h | 2 +-\n t/t4067-diff-partial-clone.sh | 49 +++++++++++++++++++++++++++++++++++\n 4 files changed, 110 insertions(+), 7 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1010d806f5..19c5d638d6 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6507,10 +6507,24 @@ static void add_if_missing(struct repository *r,\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (options->repo == the_repository && has_promisor_remote()) {\n-\t\t/*\n-\t\t * Prefetch the diff pairs that are about to be flushed.\n-\t\t */\n+\tint prefetched = 0;\n+\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n+\t\tDIFF_FORMAT_NUMSTAT |\n+\t\tDIFF_FORMAT_PATCH |\n+\t\tDIFF_FORMAT_SHORTSTAT |\n+\t\tDIFF_FORMAT_DIRSTAT;\n+\n+\t/*\n+\t * Check if the user requested a blob-data-requiring diff output and/or\n+\t * break-rewrite detection (which requires blob data). If yes, prefetch\n+\t * the diff pairs.\n+\t *\n+\t * If no prefetching occurs, diffcore_rename() will prefetch if it\n+\t * decides that it needs inexact rename detection.\n+\t */\n+\tif (options->repo == the_repository && has_promisor_remote() &&\n+\t (options->output_format & output_formats_to_prefetch ||\n+\t (!options->found_follow && options->break_opt != -1))) {\n \t\tint i;\n \t\tstruct diff_queue_struct *q = &diff_queued_diff;\n \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n@@ -6520,6 +6534,8 @@ void diffcore_std(struct diff_options *options)\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n \t\t}\n+\t\tprefetched = 1;\n+\n \t\tif (to_fetch.nr)\n \t\t\t/*\n \t\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n@@ -6538,7 +6554,7 @@ void diffcore_std(struct diff_options *options)\n \t\t\tdiffcore_break(options->repo,\n \t\t\t\t options->break_opt);\n \t\tif (options->detect_rename)\n-\t\t\tdiffcore_rename(options);\n+\t\t\tdiffcore_rename(options, prefetched);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_merge_broken();\n \t}\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex e189f407af..962565f066 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -7,6 +7,7 @@\n #include \"object-store.h\"\n #include \"hashmap.h\"\n #include \"progress.h\"\n+#include \"promisor-remote.h\"\n \n /* Table of rename/copy destinations */\n \n@@ -448,7 +449,18 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n \treturn count;\n }\n \n-void diffcore_rename(struct diff_options *options)\n+static void add_if_missing(struct repository *r,\n+\t\t\t struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec)\n+{\n+\tif (filespec && filespec->oid_valid &&\n+\t !S_ISGITLINK(filespec->mode) &&\n+\t oid_object_info_extended(r, &filespec->oid, NULL,\n+\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\toid_array_append(to_fetch, &filespec->oid);\n+}\n+\n+void diffcore_rename(struct diff_options *options, int prefetched)\n {\n \tint detect_rename = options->detect_rename;\n \tint minimum_score = options->rename_score;\n@@ -538,6 +550,32 @@ void diffcore_rename(struct diff_options *options)\n \t\tbreak;\n \t}\n \n+\tif (!prefetched) {\n+\t\t/*\n+\t\t * At this point we know there's actual work to do: we have rename\n+\t\t * destinations that didn't find an exact match, and we have potential\n+\t\t * sources. So we'll have to do inexact rename detection, which\n+\t\t * requires looking at the blobs.\n+\t\t *\n+\t\t * If we haven't already prefetched, it's worth pre-fetching\n+\t\t * them as a group now.\n+\t\t */\n+\t\tint i;\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\n+\t\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\t\tif (rename_dst[i].pair)\n+\t\t\t\tcontinue; /* already found exact match */\n+\t\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n+\t\t}\n+\t\tfor (i = 0; i < rename_src_nr; i++)\n+\t\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n+\t\tif (to_fetch.nr)\n+\t\t\tpromisor_remote_get_direct(options->repo,\n+\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n \tif (options->show_rename_progress) {\n \t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\ndiff --git a/diffcore.h b/diffcore.h\nindex 7c07347e42..9f69506574 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -144,7 +144,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n void diffcore_break(struct repository *, int);\n-void diffcore_rename(struct diff_options *);\n+void diffcore_rename(struct diff_options *, int prefetched);\n void diffcore_merge_broken(void);\n void diffcore_pickaxe(struct diff_options *);\n void diffcore_order(const char *orderfile);\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 4831ad35e6..7acb64727d 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -131,4 +131,53 @@ test_expect_success 'diff with rename detection batches blobs' '\n \ttest_line_count = 1 done_lines\n '\n \n+test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\trm server/b &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/c &&\n+\tgit -C server add c &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace\n+'\n+\n+test_expect_success 'diff --break-rewrites fetches only if necessary, and batches blobs if it does' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\tprintf \"c\\nc\\nc\\nc\\nc\\n\" >server/b &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace &&\n+\n+\t# But with --break-rewrites, ensure that there is exactly 1 negotiation\n+\t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n+\t# end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n test_done\n-- \n2.26.0.rc2.310.g2932bb562d-goog\n\n"},{"id":"394411","messageId":"b956528c-412b-2f38-bd90-1fa2ae4b8604@gmail.com","threadId":"53139","inReplyTo":"20200331020418.55640-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: restrict when prefetching occurs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-31T12:14:49Z","receivedAt":"2020-03-31T12:14:56Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/30/2020 10:04 PM, Jonathan Tan wrote:\n> Commit 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08)\n> optimized \"diff\" by prefetching blobs in a partial clone, but there are\n> some cases wherein blobs do not need to be prefetched. In particular, if\n> (1) no blob data is included in the output of the diff, (2)\n> break-rewrite detection is not requested, and (3) no inexact rename\n> detection is needed, then no blobs are read at all.\n> \n> Therefore, in such a case, do not prefetch. Change diffcore_std() to\n> only prefetch if (1) and/or (2) is not true (currently, it always\n> prefetches); change diffcore_rename() to prefetch if (3) is not true and\n> no prefetch has yet occurred.\n\nThis conflicts with [3], so please keep that in mind.\n\nMaybe [3] should be adjusted to assume this patch, because that change\nis mostly about disabling the batch download when no renames are required.\nAs Peff said [2] the full rename detection trigger is \"overly broad\".\n\nHowever, the changed-path Bloom filters are an excellent test for this\npatch, as computing them in a partial clone will trigger downloading all\nblobs without [3].\n\n[3] https://lore.kernel.org/git/55824cda89c1dca7756c8c2d831d6e115f4a9ddb.1585528298.git.gitgitgadget@gmail.com/T/#u\n\n> [1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@google.com/\n> [2] https://lore.kernel.org/git/20200130055136.GA2184413@coredump.intra.peff.net/\n> ---\n> diff.c | 26 +++++++++++++++----\n> diffcore-rename.c | 40 +++++++++++++++++++++++++++-\n> diffcore.h | 2 +-\n> t/t4067-diff-partial-clone.sh | 49 +++++++++++++++++++++++++++++++++++\n> 4 files changed, 110 insertions(+), 7 deletions(-)\n> \n> diff --git a/diff.c b/diff.c\n> index 1010d806f5..19c5d638d6 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6507,10 +6507,24 @@ static void add_if_missing(struct repository *r,\n> \n> void diffcore_std(struct diff_options *options)\n> {\n> -\tif (options->repo == the_repository && has_promisor_remote()) {\n> -\t\t/*\n> -\t\t * Prefetch the diff pairs that are about to be flushed.\n> -\t\t */\n> +\tint prefetched = 0;\n> +\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n> +\t\tDIFF_FORMAT_NUMSTAT |\n> +\t\tDIFF_FORMAT_PATCH |\n> +\t\tDIFF_FORMAT_SHORTSTAT |\n> +\t\tDIFF_FORMAT_DIRSTAT;\n> +\n> +\t/*\n> +\t * Check if the user requested a blob-data-requiring diff output and/or\n> +\t * break-rewrite detection (which requires blob data). If yes, prefetch\n> +\t * the diff pairs.\n> +\t *\n> +\t * If no prefetching occurs, diffcore_rename() will prefetch if it\n> +\t * decides that it needs inexact rename detection.\n> +\t */\n> +\tif (options->repo == the_repository && has_promisor_remote() &&\n> +\t (options->output_format & output_formats_to_prefetch ||\n> +\t (!options->found_follow && options->break_opt != -1))) {\n> \t\tint i;\n> \t\tstruct diff_queue_struct *q = &diff_queued_diff;\n> \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> @@ -6520,6 +6534,8 @@ void diffcore_std(struct diff_options *options)\n> \t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n> \t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n> \t\t}\n> +\t\tprefetched = 1;\n> +\n> \t\tif (to_fetch.nr)\n\nIt was difficult to see from the context, but the next line is\n\"do the prefetch\", so \"prefetched = 1\" makes sense here, even\nif to_fetch.nr is zero. We've already done the work to see that\na batch download is not needed.\n\n> \t\t\t/*\n> \t\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n> @@ -6538,7 +6554,7 @@ void diffcore_std(struct diff_options *options)\n> \t\t\tdiffcore_break(options->repo,\n> \t\t\t\t options->break_opt);\n> \t\tif (options->detect_rename)\n> -\t\t\tdiffcore_rename(options);\n> +\t\t\tdiffcore_rename(options, prefetched);\n> \t\tif (options->break_opt != -1)\n> \t\t\tdiffcore_merge_broken();\n> \t}\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index e189f407af..962565f066 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -7,6 +7,7 @@\n> #include \"object-store.h\"\n> #include \"hashmap.h\"\n> #include \"progress.h\"\n> +#include \"promisor-remote.h\"\n> \n> /* Table of rename/copy destinations */\n> \n> @@ -448,7 +449,18 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n> \treturn count;\n> }\n> \n> -void diffcore_rename(struct diff_options *options)\n> +static void add_if_missing(struct repository *r,\n> +\t\t\t struct oid_array *to_fetch,\n> +\t\t\t const struct diff_filespec *filespec)\n> +{\n> +\tif (filespec && filespec->oid_valid &&\n> +\t !S_ISGITLINK(filespec->mode) &&\n> +\t oid_object_info_extended(r, &filespec->oid, NULL,\n> +\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n> +\t\toid_array_append(to_fetch, &filespec->oid);\n> +}\n> +\n> +void diffcore_rename(struct diff_options *options, int prefetched)\n> {\n> \tint detect_rename = options->detect_rename;\n> \tint minimum_score = options->rename_score;\n> @@ -538,6 +550,32 @@ void diffcore_rename(struct diff_options *options)\n> \t\tbreak;\n> \t}\n> \n> +\tif (!prefetched) {\n> +\t\t/*\n> +\t\t * At this point we know there's actual work to do: we have rename\n> +\t\t * destinations that didn't find an exact match, and we have potential\n> +\t\t * sources. So we'll have to do inexact rename detection, which\n> +\t\t * requires looking at the blobs.\n> +\t\t *\n> +\t\t * If we haven't already prefetched, it's worth pre-fetching\n> +\t\t * them as a group now.\n> +\t\t */\n> +\t\tint i;\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\n> +\t\tfor (i = 0; i < rename_dst_nr; i++) {\n> +\t\t\tif (rename_dst[i].pair)\n> +\t\t\t\tcontinue; /* already found exact match */\n> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n\nCould this be reversed instead to avoid the \"continue\"?\n\n\tif (!rename_dst[i].pair)\n\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n\n> +\t\t}\n> +\t\tfor (i = 0; i < rename_src_nr; i++)\n> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n\nDoes this not have the equivalent \"rename_src[i].pair\" logic for exact\nmatches?\n\n> +\t\tif (to_fetch.nr)\n> +\t\t\tpromisor_remote_get_direct(options->repo,\n> +\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n\nPerhaps promisor_remote_get_direct() could have the check for\nnr == 0 to exit early instead of putting that upon all the\ncallers?\n\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\n> +\n> \tif (options->show_rename_progress) {\n> \t\tprogress = start_delayed_progress(\n> \t\t\t\t_(\"Performing inexact rename detection\"),\n> diff --git a/diffcore.h b/diffcore.h\n> index 7c07347e42..9f69506574 100644\n> --- a/diffcore.h\n> +++ b/diffcore.h\n> @@ -144,7 +144,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n> void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n> \n> void diffcore_break(struct repository *, int);\n> -void diffcore_rename(struct diff_options *);\n> +void diffcore_rename(struct diff_options *, int prefetched);\n> void diffcore_merge_broken(void);\n> void diffcore_pickaxe(struct diff_options *);\n> void diffcore_order(const char *orderfile);\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> index 4831ad35e6..7acb64727d 100755\n> --- a/t/t4067-diff-partial-clone.sh\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -131,4 +131,53 @@ test_expect_success 'diff with rename detection batches blobs' '\n> \ttest_line_count = 1 done_lines\n> '\n> \n> +test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n\n> +\trm server/b &&\n> +\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/c &&\n\nWould \"mv server/b server/c\" make it more clear that\nthis is an exact rename?\n\n> +\tgit -C server add c &&\n> +\tgit -C server commit -a -m x &&\n> +\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> +\t# Ensure no fetches.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n> +\t! test_path_exists trace\n> +'\n> +\n> +test_expect_success 'diff --break-rewrites fetches only if necessary, and batches blobs if it does' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\tprintf \"c\\nc\\nc\\nc\\nc\\n\" >server/b &&\n> +\tgit -C server commit -a -m x &&\n> +\n> +\ttest_config -C server uploadpack.allowfilter 1 &&\n> +\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n> +\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> +\t# Ensure no fetches.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n> +\t! test_path_exists trace &&\n> +\n> +\t# But with --break-rewrites, ensure that there is exactly 1 negotiation\n> +\t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n> +\t# end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> test_done\n> \n\nThanks,\n-Stolee\n\n"},{"id":"394423","messageId":"20200331165058.53637-1-jonathantanmy@google.com","threadId":"53139","inReplyTo":"b956528c-412b-2f38-bd90-1fa2ae4b8604@gmail.com","subject":"Re: [PATCH] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-03-31T16:50:58Z","receivedAt":"2020-03-31T16:51:04Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> This conflicts with [3], so please keep that in mind.\n> \n> Maybe [3] should be adjusted to assume this patch, because that change\n> is mostly about disabling the batch download when no renames are required.\n> As Peff said [2] the full rename detection trigger is \"overly broad\".\n> \n> However, the changed-path Bloom filters are an excellent test for this\n> patch, as computing them in a partial clone will trigger downloading all\n> blobs without [3].\n> \n> [3] https://lore.kernel.org/git/55824cda89c1dca7756c8c2d831d6e115f4a9ddb.1585528298.git.gitgitgadget@gmail.com/T/#u\n> \n> > [1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@google.com/\n> > [2] https://lore.kernel.org/git/20200130055136.GA2184413@coredump.intra.peff.net/\n\nThanks for the pointer. Yes, I think that [3] should be adjusted to\nassume this patch.\n\n> > +\t\tfor (i = 0; i < rename_dst_nr; i++) {\n> > +\t\t\tif (rename_dst[i].pair)\n> > +\t\t\t\tcontinue; /* already found exact match */\n> > +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n> \n> Could this be reversed instead to avoid the \"continue\"?\n\nHmm...I prefer the \"return early\" approach, but can change it if others\nprefer to avoid the \"continue\" here.\n\n> \tif (!rename_dst[i].pair)\n> \t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n> \n> > +\t\t}\n> > +\t\tfor (i = 0; i < rename_src_nr; i++)\n> > +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n> \n> Does this not have the equivalent \"rename_src[i].pair\" logic for exact\n> matches?\n\nThanks for the catch. There's no \"pair\" in rename_src[i], but the\nequivalent is \"if (skip_unmodified &&\ndiff_unmodified_pair(rename_src[i].p))\", which you can see in the \"for\"\nloop later in the function. I've added this.\n\n> > +\t\tif (to_fetch.nr)\n> > +\t\t\tpromisor_remote_get_direct(options->repo,\n> > +\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n> \n> Perhaps promisor_remote_get_direct() could have the check for\n> nr == 0 to exit early instead of putting that upon all the\n> callers?\n\nThe 2nd param is a pointer to an array, and I think it would be strange\nto pass a pointer to a 0-size region of memory anywhere, so I'll leave\nit as it is.\n\n> > +test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n> > +\ttest_when_finished \"rm -rf server client trace\" &&\n> > +\n> > +\ttest_create_repo server &&\n> > +\techo a >server/a &&\n> > +\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n> > +\tgit -C server add a b &&\n> > +\tgit -C server commit -m x &&\n> \n> > +\trm server/b &&\n> > +\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/c &&\n> \n> Would \"mv server/b server/c\" make it more clear that\n> this is an exact rename?\n\nTrue. Will do.\n\nThanks for the review.\n"},{"id":"394426","messageId":"d1995983-c5b2-8d44-3949-10286b3f7c0e@gmail.com","threadId":"53139","inReplyTo":"20200331165058.53637-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: restrict when prefetching occurs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-03-31T17:48:10Z","receivedAt":"2020-03-31T17:48:16Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 3/31/2020 12:50 PM, Jonathan Tan wrote:\n>> This conflicts with [3], so please keep that in mind.\n>>\n>> Maybe [3] should be adjusted to assume this patch, because that change\n>> is mostly about disabling the batch download when no renames are required.\n>> As Peff said [2] the full rename detection trigger is \"overly broad\".\n>>\n>> However, the changed-path Bloom filters are an excellent test for this\n>> patch, as computing them in a partial clone will trigger downloading all\n>> blobs without [3].\n>>\n>> [3] https://lore.kernel.org/git/55824cda89c1dca7756c8c2d831d6e115f4a9ddb.1585528298.git.gitgitgadget@gmail.com/T/#u\n>>\n>>> [1] https://lore.kernel.org/git/20200128213508.31661-1-jonathantanmy@google.com/\n>>> [2] https://lore.kernel.org/git/20200130055136.GA2184413@coredump.intra.peff.net/\n> \n> Thanks for the pointer. Yes, I think that [3] should be adjusted to\n> assume this patch.\n> \n>>> +\t\tfor (i = 0; i < rename_dst_nr; i++) {\n>>> +\t\t\tif (rename_dst[i].pair)\n>>> +\t\t\t\tcontinue; /* already found exact match */\n>>> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n>>\n>> Could this be reversed instead to avoid the \"continue\"?\n> \n> Hmm...I prefer the \"return early\" approach, but can change it if others\n> prefer to avoid the \"continue\" here.\n\nThe \"return early\" approach is great and makes sense unless there is\nonly one line of code happening in the other case. Not sure if there\nis any potential that the non-continue case grows in size or not.\n\nDoesn't hurt that much to have the \"return early\" approach, as you\nwrote it.\n\n>> \tif (!rename_dst[i].pair)\n>> \t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n>>\n>>> +\t\t}\n>>> +\t\tfor (i = 0; i < rename_src_nr; i++)\n>>> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n>>\n>> Does this not have the equivalent \"rename_src[i].pair\" logic for exact\n>> matches?\n> \n> Thanks for the catch. There's no \"pair\" in rename_src[i], but the\n> equivalent is \"if (skip_unmodified &&\n> diff_unmodified_pair(rename_src[i].p))\", which you can see in the \"for\"\n> loop later in the function. I've added this.\n> \n>>> +\t\tif (to_fetch.nr)\n>>> +\t\t\tpromisor_remote_get_direct(options->repo,\n>>> +\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n>>\n>> Perhaps promisor_remote_get_direct() could have the check for\n>> nr == 0 to exit early instead of putting that upon all the\n>> callers?\n> \n> The 2nd param is a pointer to an array, and I think it would be strange\n> to pass a pointer to a 0-size region of memory anywhere, so I'll leave\n> it as it is.\n\nWell, I would assume that to_fetch.oid is either NULL or is alloc'd\nlarger than to_fetch.nr when there are no added objects.\n\nThis is now the fourth location where we if (to_fetch.nr) promisor_remote_get_direct()\nso we have already violated the rule of three.\n\nMy preference would be to insert a patch before this that halts the\npromisor_remote_get_direct() call on an nr of 0 and deletes the \"if (nr)\"\nconditions from the three existing callers. Then this patch could use\nthe logic without ever adding the \"if (nr)\".\n\nThanks,\n-Stolee\n"},{"id":"394427","messageId":"xmqqpncs75mu.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"20200331020418.55640-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-31T18:15:21Z","receivedAt":"2020-03-31T18:15:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index e189f407af..962565f066 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> ...\n> @@ -448,7 +449,18 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n> \treturn count;\n> }\n> \n> +static void add_if_missing(struct repository *r,\n> +\t\t\t struct oid_array *to_fetch,\n> +\t\t\t const struct diff_filespec *filespec)\n> +{\n> +\tif (filespec && filespec->oid_valid &&\n> +\t !S_ISGITLINK(filespec->mode) &&\n> +\t oid_object_info_extended(r, &filespec->oid, NULL,\n> +\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n> +\t\toid_array_append(to_fetch, &filespec->oid);\n> +}\n\nDo not copy&paste the exact code from elsewhere. It is a sure way\nto guarantee that they will drift apart over time. Rename the one\nin diff.c to something a bit more appropriate to be a global name\n(e.g. diff_prepare_prefetch() or somesuch), make it extern in\n and use it here.\n\nThanks.\n"},{"id":"394428","messageId":"xmqqlfng75cl.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"d1995983-c5b2-8d44-3949-10286b3f7c0e@gmail.com","subject":"Re: [PATCH] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-03-31T18:21:30Z","receivedAt":"2020-03-31T18:21:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n>>>> +\t\tfor (i = 0; i < rename_dst_nr; i++) {\n>>>> +\t\t\tif (rename_dst[i].pair)\n>>>> +\t\t\t\tcontinue; /* already found exact match */\n>>>> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n>>>\n>>> Could this be reversed instead to avoid the \"continue\"?\n>> \n>> Hmm...I prefer the \"return early\" approach, but can change it if others\n>> prefer to avoid the \"continue\" here.\n>\n> The \"return early\" approach is great and makes sense unless there is\n> only one line of code happening in the other case. Not sure if there\n> is any potential that the non-continue case grows in size or not.\n>\n> Doesn't hurt that much to have the \"return early\" approach, as you\n> wrote it.\n\nEven with just one statement after the continue, in this particular\ncase, the logic seems to flow a bit more naturally. \"Let's see each\nitem in this list. ah, this has already been processed so let's\nmove on. otherwise, we may need to do something a bit more.\" It\nalso saves one indentation level for the logic that matters ;-)\n\n>>>> +\t\tfor (i = 0; i < rename_src_nr; i++)\n>>>> +\t\t\tadd_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n>>>\n>>> Does this not have the equivalent \"rename_src[i].pair\" logic for exact\n>>> matches?\n\nOne source blob can be copied to multiple destination path, with and\nwithout modification, but we currently do not detect the case where\na destination blob is a concatenation of two source blobs. So we\ncan optimize the destination side (\"we are done with it, no need to\nlook---we won't find anything better anyway as we've found the exact\ncopy source\") but we cannot do the same optimization on the source\nside (\"yes, this one was copied to path A, but path B may have a\ncopy with slight modification), I would think.\n"},{"id":"394621","messageId":"cover.1585854639.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"20200331020418.55640-1-jonathantanmy@google.com","subject":"[PATCH v2 0/2] Restrict when prefetcing occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-02T19:19:15Z","receivedAt":"2020-04-02T19:19:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, everyone, for your review.\n\nNew in v2:\n - added restriction on fetching rename_src's blob, following Stolee's\n comment\n - folded oid_nr==0 check into promisor_remote_get_direct(), following\n Stolee's comment\n - used \"mv server/b server/c\", following Stolee's comment\n - made diff_add_if_missing() public function, following Junio's comment\n\nI didn't change the \"continue\" part that Stolee suggested [1].\n\n[1] https://lore.kernel.org/git/xmqqlfng75cl.fsf@gitster.c.googlers.com/\n\nJonathan Tan (2):\n promisor-remote: accept 0 as oid_nr in function\n diff: restrict when prefetching occurs\n\n builtin/index-pack.c | 5 ++--\n diff.c | 49 +++++++++++++++++++++++------------\n diffcore-rename.c | 37 +++++++++++++++++++++++++-\n diffcore.h | 10 ++++++-\n promisor-remote.c | 3 +++\n promisor-remote.h | 8 ++++++\n t/t4067-diff-partial-clone.sh | 48 ++++++++++++++++++++++++++++++++++\n unpack-trees.c | 5 ++--\n 8 files changed, 141 insertions(+), 24 deletions(-)\n\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"394622","messageId":"474eb27d9c136fb69e961546004cfb531d722e2c.1585854639.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1585854639.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] promisor-remote: accept 0 as oid_nr in function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-02T19:19:16Z","receivedAt":"2020-04-02T19:19:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There are 3 callers to promisor_remote_get_direct() that first check if\nthe number of objects to be fetched is equal to 0. Fold that check into\npromisor_remote_get_direct(), and in doing so, be explicit as to what\npromisor_remote_get_direct() does if oid_nr is 0 (it returns 0, success,\nimmediately).\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 5 ++---\n diff.c | 11 +++++------\n promisor-remote.c | 3 +++\n promisor-remote.h | 8 ++++++++\n unpack-trees.c | 5 ++---\n 5 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d967d188a3..f176dd28c8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1368,9 +1368,8 @@ static void fix_unresolved_deltas(struct hashfile *f)\n \t\t\t\tcontinue;\n \t\t\toid_array_append(&to_fetch, &d->oid);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\tpromisor_remote_get_direct(the_repository,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\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 \ndiff --git a/diff.c b/diff.c\nindex 1010d806f5..f01b4d91b8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6520,12 +6520,11 @@ void diffcore_std(struct diff_options *options)\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\t/*\n-\t\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n-\t\t\t */\n-\t\t\tpromisor_remote_get_direct(options->repo,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\t/*\n+\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n+\t\t */\n+\t\tpromisor_remote_get_direct(options->repo,\n+\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n \t\toid_array_clear(&to_fetch);\n \t}\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9f338c945f..2155dfe657 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -241,6 +241,9 @@ int promisor_remote_get_direct(struct repository *repo,\n \tint to_free = 0;\n \tint res = -1;\n \n+\tif (oid_nr == 0)\n+\t\treturn 0;\n+\n \tpromisor_remote_init();\n \n \tfor (r = promisors; r; r = r->next) {\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 737bac3a33..6343c47d18 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -20,6 +20,14 @@ struct promisor_remote {\n void promisor_remote_reinit(void);\n struct promisor_remote *promisor_remote_find(const char *remote_name);\n int has_promisor_remote(void);\n+\n+/*\n+ * Fetches all requested objects from all promisor remotes, trying them one at\n+ * a time until all objects are fetched. Returns 0 upon success, and non-zero\n+ * otherwise.\n+ *\n+ * If oid_nr is 0, this function returns 0 (success) immediately.\n+ */\n int promisor_remote_get_direct(struct repository *repo,\n \t\t\t const struct object_id *oids,\n \t\t\t int oid_nr);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f618a644ef..4c3191b947 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -423,9 +423,8 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t\tcontinue;\n \t\t\toid_array_append(&to_fetch, &ce->oid);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\tpromisor_remote_get_direct(the_repository,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\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 \tfor (i = 0; i < index->cache_nr; i++) {\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"394623","messageId":"a3322cdedf019126305fcead5918d523a1b2dfbc.1585854639.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1585854639.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-02T19:19:17Z","receivedAt":"2020-04-02T19:19:32Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08)\noptimized \"diff\" by prefetching blobs in a partial clone, but there are\nsome cases wherein blobs do not need to be prefetched. In particular, if\n(1) no blob data is included in the output of the diff, (2)\nbreak-rewrite detection is not requested, and (3) no inexact rename\ndetection is needed, then no blobs are read at all.\n\nTherefore, in such a case, do not prefetch. Change diffcore_std() to\nonly prefetch if (1) and/or (2) is not true (currently, it always\nprefetches); change diffcore_rename() to prefetch if (3) is not true and\nno prefetch has yet occurred.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n diff.c | 38 +++++++++++++++++++--------\n diffcore-rename.c | 37 ++++++++++++++++++++++++++-\n diffcore.h | 10 +++++++-\n t/t4067-diff-partial-clone.sh | 48 +++++++++++++++++++++++++++++++++++\n 4 files changed, 121 insertions(+), 12 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f01b4d91b8..857f02f481 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6494,9 +6494,9 @@ void diffcore_fix_diff_index(void)\n \tQSORT(q->queue, q->nr, diffnamecmp);\n }\n \n-static void add_if_missing(struct repository *r,\n-\t\t\t struct oid_array *to_fetch,\n-\t\t\t const struct diff_filespec *filespec)\n+void diff_add_if_missing(struct repository *r,\n+\t\t\t struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec)\n {\n \tif (filespec && filespec->oid_valid &&\n \t !S_ISGITLINK(filespec->mode) &&\n@@ -6507,24 +6507,42 @@ static void add_if_missing(struct repository *r,\n \n void diffcore_std(struct diff_options *options)\n {\n-\tif (options->repo == the_repository && has_promisor_remote()) {\n-\t\t/*\n-\t\t * Prefetch the diff pairs that are about to be flushed.\n-\t\t */\n+\tint prefetched = 0;\n+\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n+\t\tDIFF_FORMAT_NUMSTAT |\n+\t\tDIFF_FORMAT_PATCH |\n+\t\tDIFF_FORMAT_SHORTSTAT |\n+\t\tDIFF_FORMAT_DIRSTAT;\n+\n+\t/*\n+\t * Check if the user requested a blob-data-requiring diff output and/or\n+\t * break-rewrite detection (which requires blob data). If yes, prefetch\n+\t * the diff pairs.\n+\t *\n+\t * If no prefetching occurs, diffcore_rename() will prefetch if it\n+\t * decides that it needs inexact rename detection.\n+\t */\n+\tif (options->repo == the_repository && has_promisor_remote() &&\n+\t (options->output_format & output_formats_to_prefetch ||\n+\t (!options->found_follow && options->break_opt != -1))) {\n \t\tint i;\n \t\tstruct diff_queue_struct *q = &diff_queued_diff;\n \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n \n \t\tfor (i = 0; i < q->nr; i++) {\n \t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n-\t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n+\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->one);\n+\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->two);\n \t\t}\n+\n+\t\tprefetched = 1;\n+\n \t\t/*\n \t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n \t\t */\n \t\tpromisor_remote_get_direct(options->repo,\n \t\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\n \t\toid_array_clear(&to_fetch);\n \t}\n \n@@ -6537,7 +6555,7 @@ void diffcore_std(struct diff_options *options)\n \t\t\tdiffcore_break(options->repo,\n \t\t\t\t options->break_opt);\n \t\tif (options->detect_rename)\n-\t\t\tdiffcore_rename(options);\n+\t\t\tdiffcore_rename(options, prefetched);\n \t\tif (options->break_opt != -1)\n \t\t\tdiffcore_merge_broken();\n \t}\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex e189f407af..79ac1b4bee 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -7,6 +7,7 @@\n #include \"object-store.h\"\n #include \"hashmap.h\"\n #include \"progress.h\"\n+#include \"promisor-remote.h\"\n \n /* Table of rename/copy destinations */\n \n@@ -448,7 +449,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n \treturn count;\n }\n \n-void diffcore_rename(struct diff_options *options)\n+void diffcore_rename(struct diff_options *options, int prefetched)\n {\n \tint detect_rename = options->detect_rename;\n \tint minimum_score = options->rename_score;\n@@ -538,6 +539,40 @@ void diffcore_rename(struct diff_options *options)\n \t\tbreak;\n \t}\n \n+\tif (!prefetched) {\n+\t\t/*\n+\t\t * At this point we know there's actual work to do: we have rename\n+\t\t * destinations that didn't find an exact match, and we have potential\n+\t\t * sources. So we'll have to do inexact rename detection, which\n+\t\t * requires looking at the blobs.\n+\t\t *\n+\t\t * If we haven't already prefetched, it's worth pre-fetching\n+\t\t * them as a group now.\n+\t\t */\n+\t\tint i;\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\n+\t\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\t\tif (rename_dst[i].pair)\n+\t\t\t\tcontinue; /* already found exact match */\n+\t\t\tdiff_add_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n+\t\t}\n+\t\tfor (i = 0; i < rename_src_nr; i++) {\n+\t\t\tif (skip_unmodified &&\n+\t\t\t diff_unmodified_pair(rename_src[i].p))\n+\t\t\t\t/*\n+\t\t\t\t * The \"for\" loop below will not need these\n+\t\t\t\t * blobs, so skip prefetching.\n+\t\t\t\t */\n+\t\t\t\tcontinue;\n+\t\t\tdiff_add_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n+\t\t}\n+\t\tif (to_fetch.nr)\n+\t\t\tpromisor_remote_get_direct(options->repo,\n+\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n \tif (options->show_rename_progress) {\n \t\tprogress = start_delayed_progress(\n \t\t\t\t_(\"Performing inexact rename detection\"),\ndiff --git a/diffcore.h b/diffcore.h\nindex 7c07347e42..d7af6ab018 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -144,7 +144,7 @@ struct diff_filepair *diff_queue(struct diff_queue_struct *,\n void diff_q(struct diff_queue_struct *, struct diff_filepair *);\n \n void diffcore_break(struct repository *, int);\n-void diffcore_rename(struct diff_options *);\n+void diffcore_rename(struct diff_options *, int prefetched);\n void diffcore_merge_broken(void);\n void diffcore_pickaxe(struct diff_options *);\n void diffcore_order(const char *orderfile);\n@@ -182,4 +182,12 @@ int diffcore_count_changes(struct repository *r,\n \t\t\t unsigned long *src_copied,\n \t\t\t unsigned long *literal_added);\n \n+/*\n+ * If filespec contains an OID and if that object is missing from the given\n+ * repository, add that OID to to_fetch.\n+ */\n+void diff_add_if_missing(struct repository *r,\n+\t\t\t struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec);\n+\n #endif\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 4831ad35e6..c1ed1c2fc4 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -131,4 +131,52 @@ test_expect_success 'diff with rename detection batches blobs' '\n \ttest_line_count = 1 done_lines\n '\n \n+test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\tmv server/b server/c &&\n+\tgit -C server add c &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace\n+'\n+\n+test_expect_success 'diff --break-rewrites fetches only if necessary, and batches blobs if it does' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\tprintf \"c\\nc\\nc\\nc\\nc\\n\" >server/b &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace &&\n+\n+\t# But with --break-rewrites, ensure that there is exactly 1 negotiation\n+\t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n+\t# end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n test_done\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"394624","messageId":"xmqqblo93c31.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"474eb27d9c136fb69e961546004cfb531d722e2c.1585854639.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] promisor-remote: accept 0 as oid_nr in function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T19:46:26Z","receivedAt":"2020-04-02T19:46:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> There are 3 callers to promisor_remote_get_direct() that first check if\n> the number of objects to be fetched is equal to 0. Fold that check into\n> promisor_remote_get_direct(), and in doing so, be explicit as to what\n> promisor_remote_get_direct() does if oid_nr is 0 (it returns 0, success,\n> immediately).\n\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> builtin/index-pack.c | 5 ++---\n> diff.c | 11 +++++------\n> promisor-remote.c | 3 +++\n> promisor-remote.h | 8 ++++++++\n> unpack-trees.c | 5 ++---\n> 5 files changed, 20 insertions(+), 12 deletions(-)\n\nNice simplification.\n\n> +/*\n> + * Fetches all requested objects from all promisor remotes, trying them one at\n> + * a time until all objects are fetched. Returns 0 upon success, and non-zero\n> + * otherwise.\n\nGood.\n\n> + * If oid_nr is 0, this function returns 0 (success) immediately.\n\nIs this worth saying? If you ask to lazily grab 0 objects, it is\nprobably clear that no object would be read before the helper\nreturns.\n\nWhen oid_nr==0 you are allowed to pass oids==NULL, but otherwise,\noids==NULL would be an error. Is that the kind of difference you\nwanted to point out, I wonder?\n\n> + */\n> int promisor_remote_get_direct(struct repository *repo,\n> \t\t\t const struct object_id *oids,\n> \t\t\t int oid_nr);\n"},{"id":"394625","messageId":"xmqq7dyx3b1o.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"a3322cdedf019126305fcead5918d523a1b2dfbc.1585854639.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T20:08:51Z","receivedAt":"2020-04-02T20:09:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> +\tint prefetched = 0;\n> +\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n> +\t\tDIFF_FORMAT_NUMSTAT |\n> +\t\tDIFF_FORMAT_PATCH |\n> +\t\tDIFF_FORMAT_SHORTSTAT |\n> +\t\tDIFF_FORMAT_DIRSTAT;\n\nWould this want to be a \"const int\" (or even #define), I wonder. I\ndo not care too much between the two, but leaving it as a variable\nmakes me a bit nervous.\n\n> +\t/*\n> +\t * Check if the user requested a blob-data-requiring diff output and/or\n> +\t * break-rewrite detection (which requires blob data). If yes, prefetch\n> +\t * the diff pairs.\n> +\t *\n> +\t * If no prefetching occurs, diffcore_rename() will prefetch if it\n> +\t * decides that it needs inexact rename detection.\n> +\t */\n\nName-only etc. that Derrick mentioned in the other thread would be\nrelevant only when rename detection is active, and you'd do that in\ndiffcore_rename(). Good.\n\n> +\tif (options->repo == the_repository && has_promisor_remote() &&\n> +\t (options->output_format & output_formats_to_prefetch ||\n> +\t (!options->found_follow && options->break_opt != -1))) {\n> \t\tint i;\n> \t\tstruct diff_queue_struct *q = &diff_queued_diff;\n> \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> \n> \t\tfor (i = 0; i < q->nr; i++) {\n> \t\t\tstruct diff_filepair *p = q->queue[i];\n> -\t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n> -\t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n> +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->one);\n> +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->two);\n> \t\t}\n> +\n> +\t\tprefetched = 1;\n> +\n\nWouldn't it logically make more sense to do this after calling\npromisor_remote_get_direct() and if to_fetch.nr is not 0, ...\n\n> \t\t/*\n> \t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n> \t\t */\n> \t\tpromisor_remote_get_direct(options->repo,\n> \t\t\t\t\t to_fetch.oid, to_fetch.nr);\n> +\n\n... namely, here?\n\nWhen (q->nr != 0), to_fetch.nr may not be zero, I suspect, but the\noriginal code before [1/2] protected against to_fetch.nr==0 case, so\n...?\n\n> \t\toid_array_clear(&to_fetch);\n> \t}\n> \n> @@ -6537,7 +6555,7 @@ void diffcore_std(struct diff_options *options)\n> \t\t\tdiffcore_break(options->repo,\n> \t\t\t\t options->break_opt);\n> \t\tif (options->detect_rename)\n> -\t\t\tdiffcore_rename(options);\n> +\t\t\tdiffcore_rename(options, prefetched);\n> \t\tif (options->break_opt != -1)\n> \t\t\tdiffcore_merge_broken();\n> \t}\n> diff --git a/diffcore-rename.c b/diffcore-rename.c\n> index e189f407af..79ac1b4bee 100644\n> --- a/diffcore-rename.c\n> +++ b/diffcore-rename.c\n> @@ -7,6 +7,7 @@\n> #include \"object-store.h\"\n> #include \"hashmap.h\"\n> #include \"progress.h\"\n> +#include \"promisor-remote.h\"\n> \n> /* Table of rename/copy destinations */\n> \n> @@ -448,7 +449,7 @@ static int find_renames(struct diff_score *mx, int dst_cnt, int minimum_score, i\n> \treturn count;\n> }\n> \n> -void diffcore_rename(struct diff_options *options)\n> +void diffcore_rename(struct diff_options *options, int prefetched)\n> {\n> \tint detect_rename = options->detect_rename;\n> \tint minimum_score = options->rename_score;\n> @@ -538,6 +539,40 @@ void diffcore_rename(struct diff_options *options)\n> \t\tbreak;\n> \t}\n> \n> +\tif (!prefetched) {\n> +\t\t/*\n> +\t\t * At this point we know there's actual work to do: we have rename\n> +\t\t * destinations that didn't find an exact match, and we have potential\n> +\t\t * sources. So we'll have to do inexact rename detection, which\n> +\t\t * requires looking at the blobs.\n> +\t\t *\n> +\t\t * If we haven't already prefetched, it's worth pre-fetching\n> +\t\t * them as a group now.\n> +\t\t */\n\nThis comment makes me wonder if it would be even better to\n\n - prepare an empty to_fetch OID array in the caller,\n\n - if the output format is one of the ones that wants prefetch, add\n object names to to_fetch in the caller, BUT not fetch there.\n\n - pass &to_fetch by the caller to this function, and this code here\n may add even more objects,\n\n - then do the prefetch here (so a single promisor interaction will\n grab objects the caller would have fetched before calling us and\n the ones we want here), and then clear the to_fetch array.\n\n - the caller, after seeing this function returns, checks to_fetch\n and if it is not empty, fetches (i.e. the caller prepared list of\n objects based on the output type, we ended up not calling this\n helper, and then finally the caller does the prefetch).\n\nThat way, the \"unless we have already prefetched\" logic can go, and\nwe can lose one indentation level, no?\n\n\n> +\t\tint i;\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\n> +\t\tfor (i = 0; i < rename_dst_nr; i++) {\n> +\t\t\tif (rename_dst[i].pair)\n> +\t\t\t\tcontinue; /* already found exact match */\n> +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, rename_dst[i].two);\n> +\t\t}\n> +\t\tfor (i = 0; i < rename_src_nr; i++) {\n> +\t\t\tif (skip_unmodified &&\n> +\t\t\t diff_unmodified_pair(rename_src[i].p))\n> +\t\t\t\t/*\n> +\t\t\t\t * The \"for\" loop below will not need these\n> +\t\t\t\t * blobs, so skip prefetching.\n> +\t\t\t\t */\n> +\t\t\t\tcontinue;\n> +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, rename_src[i].p->one);\n> +\t\t}\n> +\t\tif (to_fetch.nr)\n> +\t\t\tpromisor_remote_get_direct(options->repo,\n> +\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n\nYou no longer need the if(), no?\n\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\n"},{"id":"394626","messageId":"xmqq369l3a4a.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"cover.1585854639.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/2] Restrict when prefetcing occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T20:28:53Z","receivedAt":"2020-04-02T20:29:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks, everyone, for your review.\n>\n> New in v2:\n> - added restriction on fetching rename_src's blob, following Stolee's\n> comment\n> - folded oid_nr==0 check into promisor_remote_get_direct(), following\n> Stolee's comment\n> - used \"mv server/b server/c\", following Stolee's comment\n> - made diff_add_if_missing() public function, following Junio's comment\n>\n> I didn't change the \"continue\" part that Stolee suggested [1].\n>\n> [1] https://lore.kernel.org/git/xmqqlfng75cl.fsf@gitster.c.googlers.com/\n>\n> Jonathan Tan (2):\n> promisor-remote: accept 0 as oid_nr in function\n> diff: restrict when prefetching occurs\n>\n> builtin/index-pack.c | 5 ++--\n> diff.c | 49 +++++++++++++++++++++++------------\n> diffcore-rename.c | 37 +++++++++++++++++++++++++-\n> diffcore.h | 10 ++++++-\n> promisor-remote.c | 3 +++\n> promisor-remote.h | 8 ++++++\n> t/t4067-diff-partial-clone.sh | 48 ++++++++++++++++++++++++++++++++++\n> unpack-trees.c | 5 ++--\n> 8 files changed, 141 insertions(+), 24 deletions(-)\n\nI notice that a439b4ef (diff: skip batch object download when\npossible, 2020-03-30) by Garima seems to aim for something similar.\n\nI'll for now keep both topics with conflict resolution, but it may\nmake sense for you two to compare notes. \n\nI especially like the way this series enumerates the formats that\nmatter to prefetching and the way the change is localized in\ndiffcore_std(); the other topic splits a similar logic (with\ndifferent criteria) between diff_setup_done() and diffcore_std(),\nwhich I found suboptimal.\n\nThanks.\n"},{"id":"394629","messageId":"20200402230153.45407-1-jonathantanmy@google.com","threadId":"53139","inReplyTo":"xmqqblo93c31.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 1/2] promisor-remote: accept 0 as oid_nr in function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-02T23:01:53Z","receivedAt":"2020-04-02T23:01:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > + * If oid_nr is 0, this function returns 0 (success) immediately.\n> \n> Is this worth saying? If you ask to lazily grab 0 objects, it is\n> probably clear that no object would be read before the helper\n> returns.\n> \n> When oid_nr==0 you are allowed to pass oids==NULL, but otherwise,\n> oids==NULL would be an error. Is that the kind of difference you\n> wanted to point out, I wonder?\n\nThanks for taking a look. Yes that was the difference I wanted to point\nout. After some thought, maybe I'll replace it with \"oids points to an\narray of OIDs of size oid_nr. If oid_nr is 0, oids can be anything.\".\n"},{"id":"394630","messageId":"20200402230937.47323-1-jonathantanmy@google.com","threadId":"53139","inReplyTo":"xmqq7dyx3b1o.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-02T23:09:37Z","receivedAt":"2020-04-02T23:09:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > +\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n> > +\t\tDIFF_FORMAT_NUMSTAT |\n> > +\t\tDIFF_FORMAT_PATCH |\n> > +\t\tDIFF_FORMAT_SHORTSTAT |\n> > +\t\tDIFF_FORMAT_DIRSTAT;\n> \n> Would this want to be a \"const int\" (or even #define), I wonder. I\n> do not care too much between the two, but leaving it as a variable\n> makes me a bit nervous.\n\nOK, will switch to \"const int\".\n\n> > +\tif (options->repo == the_repository && has_promisor_remote() &&\n> > +\t (options->output_format & output_formats_to_prefetch ||\n> > +\t (!options->found_follow && options->break_opt != -1))) {\n> > \t\tint i;\n> > \t\tstruct diff_queue_struct *q = &diff_queued_diff;\n> > \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> > \n> > \t\tfor (i = 0; i < q->nr; i++) {\n> > \t\t\tstruct diff_filepair *p = q->queue[i];\n> > -\t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n> > -\t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n> > +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->one);\n> > +\t\t\tdiff_add_if_missing(options->repo, &to_fetch, p->two);\n> > \t\t}\n> > +\n> > +\t\tprefetched = 1;\n> > +\n> \n> Wouldn't it logically make more sense to do this after calling\n> promisor_remote_get_direct() and if to_fetch.nr is not 0, ...\n> \n> > \t\t/*\n> > \t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n> > \t\t */\n> > \t\tpromisor_remote_get_direct(options->repo,\n> > \t\t\t\t\t to_fetch.oid, to_fetch.nr);\n> > +\n> \n> ... namely, here?\n> \n> When (q->nr != 0), to_fetch.nr may not be zero, I suspect, but the\n> original code before [1/2] protected against to_fetch.nr==0 case, so\n> ...?\n\nMy idea is that this prefetch is a superset of what diffcore_rebase()\nwants to prefetch, so if we have already done the necessary logic here\n(even if nothing gets prefetched - which might be the case if we have\nall objects), we do not need to do it in diffcore_rebase().\n\n> > +\tif (!prefetched) {\n> > +\t\t/*\n> > +\t\t * At this point we know there's actual work to do: we have rename\n> > +\t\t * destinations that didn't find an exact match, and we have potential\n> > +\t\t * sources. So we'll have to do inexact rename detection, which\n> > +\t\t * requires looking at the blobs.\n> > +\t\t *\n> > +\t\t * If we haven't already prefetched, it's worth pre-fetching\n> > +\t\t * them as a group now.\n> > +\t\t */\n> \n> This comment makes me wonder if it would be even better to\n> \n> - prepare an empty to_fetch OID array in the caller,\n> \n> - if the output format is one of the ones that wants prefetch, add\n> object names to to_fetch in the caller, BUT not fetch there.\n> \n> - pass &to_fetch by the caller to this function, and this code here\n> may add even more objects,\n> \n> - then do the prefetch here (so a single promisor interaction will\n> grab objects the caller would have fetched before calling us and\n> the ones we want here), and then clear the to_fetch array.\n> \n> - the caller, after seeing this function returns, checks to_fetch\n> and if it is not empty, fetches (i.e. the caller prepared list of\n> objects based on the output type, we ended up not calling this\n> helper, and then finally the caller does the prefetch).\n> \n> That way, the \"unless we have already prefetched\" logic can go, and\n> we can lose one indentation level, no?\n\nThis means that the only prefetch occurs in diffcore_rename()? I don't\nthink this will work for 2 reasons:\n\n - diffcore_std() calls diffcore_break() (which also reads blobs) before\n diffcore_rename()\n - (more importantly) there's a code path in diffcore_std() that does\n not call diffcore_rename(), so we would still need some prefetching\n logic in diffcore_std() in case diffcore_rename() is not called\n\n> > +\t\tif (to_fetch.nr)\n> > +\t\t\tpromisor_remote_get_direct(options->repo,\n> > +\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n> \n> You no longer need the if(), no?\n\nAh...I'll remove the if().\n"},{"id":"394631","messageId":"xmqqy2rd1ndv.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"20200402230937.47323-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T23:25:16Z","receivedAt":"2020-04-02T23:25:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n>> This comment makes me wonder if it would be even better to\n>> \n>> - prepare an empty to_fetch OID array in the caller,\n>> \n>> - if the output format is one of the ones that wants prefetch, add\n>> object names to to_fetch in the caller, BUT not fetch there.\n>> \n>> - pass &to_fetch by the caller to this function, and this code here\n>> may add even more objects,\n>> \n>> - then do the prefetch here (so a single promisor interaction will\n>> grab objects the caller would have fetched before calling us and\n>> the ones we want here), and then clear the to_fetch array.\n>> \n>> - the caller, after seeing this function returns, checks to_fetch\n>> and if it is not empty, fetches (i.e. the caller prepared list of\n>> objects based on the output type, we ended up not calling this\n>> helper, and then finally the caller does the prefetch).\n>> \n>> That way, the \"unless we have already prefetched\" logic can go, and\n>> we can lose one indentation level, no?\n>\n> This means that the only prefetch occurs in diffcore_rename()?\n\nNo, but I phrased the last bullet item incorrectly. \"after seeing\nthis function returns\" is wrong, but what is in parentheses (i.e. if\nwe didn't call diffcore_rename) is correct.\n\n> I don't\n> think this will work for 2 reasons:\n>\n> - diffcore_std() calls diffcore_break() (which also reads blobs) before\n> diffcore_rename()\n\nAhh, I missed that part.\n\n> - (more importantly) there's a code path in diffcore_std() that does\n> not call diffcore_rename(), so we would still need some prefetching\n> logic in diffcore_std() in case diffcore_rename() is not called\n\nThat one I think is already covered.\n"},{"id":"394632","messageId":"xmqqsghl1m0p.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"20200402230937.47323-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-02T23:54:46Z","receivedAt":"2020-04-02T23:54:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> My idea is that this prefetch is a superset of what diffcore_rebase()\n> wants to prefetch, so if we have already done the necessary logic here\n> (even if nothing gets prefetched - which might be the case if we have\n> all objects), we do not need to do it in diffcore_rebase().\n\ns/rebase/rename/ I presume, but the above reasoning, while it may\nhappen to hold true right now, feels brittle. In other words\n\n - how do we know it would stay to be \"a superset\"?\n\n - would it change the picture if we later added a prefetch in\n diffcore_break(), just like you are doing so to diffcore_rename()\n in this patch?\n\nSo the revised version of my earlier \"wondering\" is if it would be\nmore future-proof (easier to teach different steps to prefetch for\ntheir own needs, without having to make an assumption like \"what\nthis step needs is sufficient for the other step\") to arrange the\ncodepath from diffcore_std() to its helpers like so:\n\n - prepare an empty to_fetch OID array in the caller,\n\n - if the output format is one of the ones that wants prefetch,\n add object names to to_fetch in the caller, but not fetch as\n long as the caller does not yet need the contents of the\n blobs.\n\n - pass &to_fetch from diffcore_std() to the helper functions in\n the diffcore family like diffcore_{break,rename}() have them\n also batch what they (may) want to prefetch in there. Delay\n fetching until they actually need to look at the blobs, and\n when they fetch, clear &to_fetch for the next helper.\n\n - diffcore_std() also would need to look at the blob eventually,\n perhaps after all the helpers it may call returns. Do the\n final prefetch if to_fetch is still not empty before it has to\n look at the blobs.\n\nThanks.\n"},{"id":"394720","messageId":"20200403213546.237273-1-jonathantanmy@google.com","threadId":"53139","inReplyTo":"xmqqsghl1m0p.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-03T21:35:46Z","receivedAt":"2020-04-03T21:35:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > My idea is that this prefetch is a superset of what diffcore_rebase()\n> > wants to prefetch, so if we have already done the necessary logic here\n> > (even if nothing gets prefetched - which might be the case if we have\n> > all objects), we do not need to do it in diffcore_rebase().\n> \n> s/rebase/rename/ I presume,\n\nAh, yes.\n\n> but the above reasoning, while it may\n> happen to hold true right now, feels brittle. In other words\n> \n> - how do we know it would stay to be \"a superset\"?\n> \n> - would it change the picture if we later added a prefetch in\n> diffcore_break(), just like you are doing so to diffcore_rename()\n> in this patch?\n> \n> So the revised version of my earlier \"wondering\" is if it would be\n> more future-proof (easier to teach different steps to prefetch for\n> their own needs, without having to make an assumption like \"what\n> this step needs is sufficient for the other step\") to arrange the\n> codepath from diffcore_std() to its helpers like so:\n> \n> - prepare an empty to_fetch OID array in the caller,\n> \n> - if the output format is one of the ones that wants prefetch,\n> add object names to to_fetch in the caller, but not fetch as\n> long as the caller does not yet need the contents of the\n> blobs.\n> \n> - pass &to_fetch from diffcore_std() to the helper functions in\n> the diffcore family like diffcore_{break,rename}() have them\n> also batch what they (may) want to prefetch in there. Delay\n> fetching until they actually need to look at the blobs, and\n> when they fetch, clear &to_fetch for the next helper.\n> \n> - diffcore_std() also would need to look at the blob eventually,\n> perhaps after all the helpers it may call returns. Do the\n> final prefetch if to_fetch is still not empty before it has to\n> look at the blobs.\n\nAh...that makes sense. Besides the accumulating of prefetch targets\n(which makes deduplication more necessary - I might make a\n\"sort_and_uniq\" function on oid_array that updates the oid_array\nin-place), looking at the code, it's not only diffcore_break() which\nmight need prefetching, but diffcore_skip_stat_unmatch() too (not to\nspeak of the functions that come after diffcore_rename()). The least\nbrittle way is probably to have diff_populate_filespec() do the\nprefetching. I'll take a further look.\n"},{"id":"394723","messageId":"xmqqr1x4xlpk.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"20200403213546.237273-1-jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: restrict when prefetching occurs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-03T22:12:39Z","receivedAt":"2020-04-03T22:12:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Ah...that makes sense. Besides the accumulating of prefetch targets\n> (which makes deduplication more necessary - I might make a\n> \"sort_and_uniq\" function on oid_array that updates the oid_array\n> in-place), looking at the code, it's not only diffcore_break() which\n> might need prefetching, but diffcore_skip_stat_unmatch() too (not to\n> speak of the functions that come after diffcore_rename()). The least\n> brittle way is probably to have diff_populate_filespec() do the\n> prefetching. I'll take a further look.\n\nIt might be a losing battle, unless we can somehow cleanly have a\ntwo pass approach where we ask various codepaths \"enumerate blobs\nthat you think you would need prefetching in this oid list\" before\nletting any of them actually look at blobs and perform their main\ntasks, do a single prefetch and then let the existing age-old code\ncall the diffcore transformations as if there is no need for it to\nworry about prefetching ;-)\n\nThanks.\n"},{"id":"394823","messageId":"7de2f54b-8704-a0e1-12aa-0ca9d3d70f6f@gmail.com","threadId":"53139","inReplyTo":"xmqq369l3a4a.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v2 0/2] Restrict when prefetcing occurs","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2020-04-06T11:44:07Z","receivedAt":"2020-04-06T11:44:15Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 4/2/2020 4:28 PM, Junio C Hamano wrote:\n> I notice that a439b4ef (diff: skip batch object download when\n> possible, 2020-03-30) by Garima seems to aim for something similar.\n> \n> I'll for now keep both topics with conflict resolution, but it may\n> make sense for you two to compare notes.\n\nI pointed this out in [1]. I think the right thing to do is for\nGarima's/my patch to rely on Jonathan's change. The commit needs\nto be modified, not simply ejected, but it could be separated from\nthe rest of Garima's series. It is only a performance fix for\nnormal clones, but is critical for partial clones.\n\nGarima: do you think it would be easy to remove that patch if/when\nyou do a v4 and I can make a new series based on yours and Jonathan's\nwith the rename setting?\n\n[1] https://lore.kernel.org/git/b956528c-412b-2f38-bd90-1fa2ae4b8604@gmail.com/\nThanks,\n-Stolee\n"},{"id":"394824","messageId":"7c877d3b-e6fb-c9b0-f403-09133270017d@gmail.com","threadId":"53139","inReplyTo":"7de2f54b-8704-a0e1-12aa-0ca9d3d70f6f@gmail.com","subject":"Re: [PATCH v2 0/2] Restrict when prefetcing occurs","fromName":"Garima Singh","fromEmail":"garimasigit@gmail.com","sentAt":"2020-04-06T11:57:36Z","receivedAt":"2020-04-06T11:57:41Z","isPatch":true,"sender":{"key":"garimasigit@gmail.com","avatar":null},"body":"\nOn 4/6/2020 7:44 AM, Derrick Stolee wrote:\n> On 4/2/2020 4:28 PM, Junio C Hamano wrote:\n>> I notice that a439b4ef (diff: skip batch object download when\n>> possible, 2020-03-30) by Garima seems to aim for something similar.\n>>\n>> I'll for now keep both topics with conflict resolution, but it may\n>> make sense for you two to compare notes.\n> \n> I pointed this out in [1]. I think the right thing to do is for\n> Garima's/my patch to rely on Jonathan's change. The commit needs\n> to be modified, not simply ejected, but it could be separated from\n> the rest of Garima's series. It is only a performance fix for\n> normal clones, but is critical for partial clones.\n> \n> Garima: do you think it would be easy to remove that patch if/when\n> you do a v4 and I can make a new series based on yours and Jonathan's\n> with the rename setting?\n> \n\nSure. I was thinking about rebasing my series on top of Jonathan's \nand adjusting as necessary, but it might be easier to just remove it \nand then have a new series based on mine and Jonathan's, like you\nsuggested. \n\nThere hasn't been any feedback since I sent out v3. I will just re-roll\nv4 without this patch, to make sure pu no longer requires conflict\nresolution around the Bloom filter series. \n\nCheers! \nGarima Singh\n"},{"id":"395013","messageId":"cover.1586296510.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"20200331020418.55640-1-jonathantanmy@google.com","subject":"[PATCH v3 0/4] Restrict when prefetcing occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-07T22:11:39Z","receivedAt":"2020-04-07T22:11:49Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio wrote in [1]:\n\n> s/rebase/rename/ I presume, but the above reasoning, while it may\n> happen to hold true right now, feels brittle. In other words\n> \n> - how do we know it would stay to be \"a superset\"?\n> \n> - would it change the picture if we later added a prefetch in\n> diffcore_break(), just like you are doing so to diffcore_rename()\n> in this patch?\n\nand suggested that each function be capable of prefetching. I've done\nthat for most functions in this new version.\n\nTo avoid the potential slowdown of each function doing its own object\nexistence checks (that is, looping through all the relevant OIDs and\nthen prefetching based on whether it found one missing), what I did is\nto teach diff_populate_filespec() to retry whenever it attempts to read\na missing object, calling a callback before its 2nd (and final) try. I\nthen taught the functions called by diffcore_std() to pass a prefetching\nfunction as this callback.\n\nThe functions I've taught include diffcore_skip_stat_unmatch(). I\ncouldn't figure out how to trigger this behavior in a test (I can see\nthat the function is being run, but not how to make it read an object),\nbut I included the prefetching mechanism in this function anyway for\ncompleteness.\n\nThe previous version of my patch [2] made the assumption that the\nfetching done at the start of diffcore_std() is a superset of the\nfetching done by diffcore_rebase() - hence Junio's comment above about\nhow we would know that it would stay a superset. With this series, if\never that no longer holds (and we miss fixing it), rebase would only do\none additional bulk fetch (instead of fetching once for every missing\nobject).\n\n[1] https://lore.kernel.org/git/xmqqsghl1m0p.fsf@gitster.c.googlers.com/\n[2] https://lore.kernel.org/git/a3322cdedf019126305fcead5918d523a1b2dfbc.1585854639.git.jonathantanmy@google.com/\n\nJonathan Tan (4):\n promisor-remote: accept 0 as oid_nr in function\n diff: make diff_populate_filespec_options struct\n diff: refactor object read\n diff: restrict when prefetching occurs\n\n builtin/index-pack.c | 5 +-\n diff.c | 157 +++++++++++++++++++++++-----------\n diffcore-break.c | 12 ++-\n diffcore-rename.c | 64 ++++++++++++--\n diffcore.h | 30 ++++++-\n line-log.c | 6 +-\n promisor-remote.c | 3 +\n promisor-remote.h | 8 ++\n t/t4067-diff-partial-clone.sh | 48 +++++++++++\n unpack-trees.c | 5 +-\n 10 files changed, 267 insertions(+), 71 deletions(-)\n\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"395014","messageId":"474eb27d9c136fb69e961546004cfb531d722e2c.1586296510.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1586296510.git.jonathantanmy@google.com","subject":"[PATCH v3 1/4] promisor-remote: accept 0 as oid_nr in function","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-07T22:11:40Z","receivedAt":"2020-04-07T22:11:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"There are 3 callers to promisor_remote_get_direct() that first check if\nthe number of objects to be fetched is equal to 0. Fold that check into\npromisor_remote_get_direct(), and in doing so, be explicit as to what\npromisor_remote_get_direct() does if oid_nr is 0 (it returns 0, success,\nimmediately).\n\nSigned-off-by: Jonathan Tan \n---\n builtin/index-pack.c | 5 ++---\n diff.c | 11 +++++------\n promisor-remote.c | 3 +++\n promisor-remote.h | 8 ++++++++\n unpack-trees.c | 5 ++---\n 5 files changed, 20 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex d967d188a3..f176dd28c8 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -1368,9 +1368,8 @@ static void fix_unresolved_deltas(struct hashfile *f)\n \t\t\t\tcontinue;\n \t\t\toid_array_append(&to_fetch, &d->oid);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\tpromisor_remote_get_direct(the_repository,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\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 \ndiff --git a/diff.c b/diff.c\nindex 1010d806f5..f01b4d91b8 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6520,12 +6520,11 @@ void diffcore_std(struct diff_options *options)\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n \t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\t/*\n-\t\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n-\t\t\t */\n-\t\t\tpromisor_remote_get_direct(options->repo,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\t/*\n+\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n+\t\t */\n+\t\tpromisor_remote_get_direct(options->repo,\n+\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n \t\toid_array_clear(&to_fetch);\n \t}\n \ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 9f338c945f..2155dfe657 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -241,6 +241,9 @@ int promisor_remote_get_direct(struct repository *repo,\n \tint to_free = 0;\n \tint res = -1;\n \n+\tif (oid_nr == 0)\n+\t\treturn 0;\n+\n \tpromisor_remote_init();\n \n \tfor (r = promisors; r; r = r->next) {\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex 737bac3a33..6343c47d18 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -20,6 +20,14 @@ struct promisor_remote {\n void promisor_remote_reinit(void);\n struct promisor_remote *promisor_remote_find(const char *remote_name);\n int has_promisor_remote(void);\n+\n+/*\n+ * Fetches all requested objects from all promisor remotes, trying them one at\n+ * a time until all objects are fetched. Returns 0 upon success, and non-zero\n+ * otherwise.\n+ *\n+ * If oid_nr is 0, this function returns 0 (success) immediately.\n+ */\n int promisor_remote_get_direct(struct repository *repo,\n \t\t\t const struct object_id *oids,\n \t\t\t int oid_nr);\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex f618a644ef..4c3191b947 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -423,9 +423,8 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t\t\tcontinue;\n \t\t\toid_array_append(&to_fetch, &ce->oid);\n \t\t}\n-\t\tif (to_fetch.nr)\n-\t\t\tpromisor_remote_get_direct(the_repository,\n-\t\t\t\t\t\t to_fetch.oid, to_fetch.nr);\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 \tfor (i = 0; i < index->cache_nr; i++) {\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"395015","messageId":"c1973fd6308109b0cc99544500d8932222b66726.1586296510.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1586296510.git.jonathantanmy@google.com","subject":"[PATCH v3 2/4] diff: make diff_populate_filespec_options struct","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-07T22:11:41Z","receivedAt":"2020-04-07T22:11:53Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The behavior of diff_populate_filespec() currently can be customized\nthrough a bitflag, but a subsequent patch requires it to support a\nnon-boolean option. Replace the bitflag with an options struct.\n\nSigned-off-by: Jonathan Tan \n---\n diff.c | 54 ++++++++++++++++++++++++++++++-----------------\n diffcore-break.c | 4 ++--\n diffcore-rename.c | 13 +++++++-----\n diffcore.h | 9 +++++---\n line-log.c | 6 +++---\n 5 files changed, 54 insertions(+), 32 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f01b4d91b8..f337d837ac 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -573,7 +573,7 @@ static int fill_mmfile(struct repository *r, mmfile_t *mf,\n \t\tmf->size = 0;\n \t\treturn 0;\n \t}\n-\telse if (diff_populate_filespec(r, one, 0))\n+\telse if (diff_populate_filespec(r, one, NULL))\n \t\treturn -1;\n \n \tmf->ptr = one->data;\n@@ -585,9 +585,13 @@ static int fill_mmfile(struct repository *r, mmfile_t *mf,\n static unsigned long diff_filespec_size(struct repository *r,\n \t\t\t\t\tstruct diff_filespec *one)\n {\n+\tstruct diff_populate_filespec_options dpf_options = {\n+\t\t.check_size_only = 1,\n+\t};\n+\n \tif (!DIFF_FILE_VALID(one))\n \t\treturn 0;\n-\tdiff_populate_filespec(r, one, CHECK_SIZE_ONLY);\n+\tdiff_populate_filespec(r, one, &dpf_options);\n \treturn one->size;\n }\n \n@@ -3020,6 +3024,9 @@ static void show_dirstat(struct diff_options *options)\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tconst char *name;\n \t\tunsigned long copied, added, damage;\n+\t\tstruct diff_populate_filespec_options dpf_options = {\n+\t\t\t.check_size_only = 1,\n+\t\t};\n \n \t\tname = p->two->path ? p->two->path : p->one->path;\n \n@@ -3047,19 +3054,19 @@ static void show_dirstat(struct diff_options *options)\n \t\t}\n \n \t\tif (DIFF_FILE_VALID(p->one) && DIFF_FILE_VALID(p->two)) {\n-\t\t\tdiff_populate_filespec(options->repo, p->one, 0);\n-\t\t\tdiff_populate_filespec(options->repo, p->two, 0);\n+\t\t\tdiff_populate_filespec(options->repo, p->one, NULL);\n+\t\t\tdiff_populate_filespec(options->repo, p->two, NULL);\n \t\t\tdiffcore_count_changes(options->repo,\n \t\t\t\t\t p->one, p->two, NULL, NULL,\n \t\t\t\t\t &copied, &added);\n \t\t\tdiff_free_filespec_data(p->one);\n \t\t\tdiff_free_filespec_data(p->two);\n \t\t} else if (DIFF_FILE_VALID(p->one)) {\n-\t\t\tdiff_populate_filespec(options->repo, p->one, CHECK_SIZE_ONLY);\n+\t\t\tdiff_populate_filespec(options->repo, p->one, &dpf_options);\n \t\t\tcopied = added = 0;\n \t\t\tdiff_free_filespec_data(p->one);\n \t\t} else if (DIFF_FILE_VALID(p->two)) {\n-\t\t\tdiff_populate_filespec(options->repo, p->two, CHECK_SIZE_ONLY);\n+\t\t\tdiff_populate_filespec(options->repo, p->two, &dpf_options);\n \t\t\tcopied = 0;\n \t\t\tadded = p->two->size;\n \t\t\tdiff_free_filespec_data(p->two);\n@@ -3339,13 +3346,17 @@ static void emit_binary_diff(struct diff_options *o,\n int diff_filespec_is_binary(struct repository *r,\n \t\t\t struct diff_filespec *one)\n {\n+\tstruct diff_populate_filespec_options dpf_options = {\n+\t\t.check_binary = 1,\n+\t};\n+\n \tif (one->is_binary == -1) {\n \t\tdiff_filespec_load_driver(one, r->index);\n \t\tif (one->driver->binary != -1)\n \t\t\tone->is_binary = one->driver->binary;\n \t\telse {\n \t\t\tif (!one->data && DIFF_FILE_VALID(one))\n-\t\t\t\tdiff_populate_filespec(r, one, CHECK_BINARY);\n+\t\t\t\tdiff_populate_filespec(r, one, &dpf_options);\n \t\t\tif (one->is_binary == -1 && one->data)\n \t\t\t\tone->is_binary = buffer_is_binary(one->data,\n \t\t\t\t\t\tone->size);\n@@ -3677,8 +3688,8 @@ static void builtin_diffstat(const char *name_a, const char *name_b,\n \t}\n \n \telse if (complete_rewrite) {\n-\t\tdiff_populate_filespec(o->repo, one, 0);\n-\t\tdiff_populate_filespec(o->repo, two, 0);\n+\t\tdiff_populate_filespec(o->repo, one, NULL);\n+\t\tdiff_populate_filespec(o->repo, two, NULL);\n \t\tdata->deleted = count_lines(one->data, one->size);\n \t\tdata->added = count_lines(two->data, two->size);\n \t}\n@@ -3914,9 +3925,10 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only)\n */\n int diff_populate_filespec(struct repository *r,\n \t\t\t struct diff_filespec *s,\n-\t\t\t unsigned int flags)\n+\t\t\t const struct diff_populate_filespec_options *options)\n {\n-\tint size_only = flags & CHECK_SIZE_ONLY;\n+\tint size_only = options ? options->check_size_only : 0;\n+\tint check_binary = options ? options->check_binary : 0;\n \tint err = 0;\n \tint conv_flags = global_conv_flags_eol;\n \t/*\n@@ -3986,7 +3998,7 @@ int diff_populate_filespec(struct repository *r,\n \t\t * opening the file and inspecting the contents, this\n \t\t * is probably fine.\n \t\t */\n-\t\tif ((flags & CHECK_BINARY) &&\n+\t\tif (check_binary &&\n \t\t s->size > big_file_threshold && s->is_binary == -1) {\n \t\t\ts->is_binary = 1;\n \t\t\treturn 0;\n@@ -4012,7 +4024,7 @@ int diff_populate_filespec(struct repository *r,\n \t}\n \telse {\n \t\tenum object_type type;\n-\t\tif (size_only || (flags & CHECK_BINARY)) {\n+\t\tif (size_only || check_binary) {\n \t\t\ttype = oid_object_info(r, &s->oid, &s->size);\n \t\t\tif (type < 0)\n \t\t\t\tdie(\"unable to read %s\",\n@@ -4144,7 +4156,7 @@ static struct diff_tempfile *prepare_temp_file(struct repository *r,\n \t\treturn temp;\n \t}\n \telse {\n-\t\tif (diff_populate_filespec(r, one, 0))\n+\t\tif (diff_populate_filespec(r, one, NULL))\n \t\t\tdie(\"cannot read data blob for %s\", one->path);\n \t\tprep_temp_blob(r->index, name, temp,\n \t\t\t one->data, one->size,\n@@ -6410,9 +6422,9 @@ static int diff_filespec_is_identical(struct repository *r,\n {\n \tif (S_ISGITLINK(one->mode))\n \t\treturn 0;\n-\tif (diff_populate_filespec(r, one, 0))\n+\tif (diff_populate_filespec(r, one, NULL))\n \t\treturn 0;\n-\tif (diff_populate_filespec(r, two, 0))\n+\tif (diff_populate_filespec(r, two, NULL))\n \t\treturn 0;\n \treturn !memcmp(one->data, two->data, one->size);\n }\n@@ -6420,6 +6432,10 @@ static int diff_filespec_is_identical(struct repository *r,\n static int diff_filespec_check_stat_unmatch(struct repository *r,\n \t\t\t\t\t struct diff_filepair *p)\n {\n+\tstruct diff_populate_filespec_options dpf_options = {\n+\t\t.check_size_only = 1,\n+\t};\n+\n \tif (p->done_skip_stat_unmatch)\n \t\treturn p->skip_stat_unmatch_result;\n \n@@ -6442,8 +6458,8 @@ static int diff_filespec_check_stat_unmatch(struct repository *r,\n \t !DIFF_FILE_VALID(p->two) ||\n \t (p->one->oid_valid && p->two->oid_valid) ||\n \t (p->one->mode != p->two->mode) ||\n-\t diff_populate_filespec(r, p->one, CHECK_SIZE_ONLY) ||\n-\t diff_populate_filespec(r, p->two, CHECK_SIZE_ONLY) ||\n+\t diff_populate_filespec(r, p->one, &dpf_options) ||\n+\t diff_populate_filespec(r, p->two, &dpf_options) ||\n \t (p->one->size != p->two->size) ||\n \t !diff_filespec_is_identical(r, p->one, p->two)) /* (2) */\n \t\tp->skip_stat_unmatch_result = 1;\n@@ -6773,7 +6789,7 @@ size_t fill_textconv(struct repository *r,\n \t\t\t*outbuf = \"\";\n \t\t\treturn 0;\n \t\t}\n-\t\tif (diff_populate_filespec(r, df, 0))\n+\t\tif (diff_populate_filespec(r, df, NULL))\n \t\t\tdie(\"unable to read files to diff\");\n \t\t*outbuf = df->data;\n \t\treturn df->size;\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex 9d20a6a6fc..e8f6322c6a 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -62,8 +62,8 @@ static int should_break(struct repository *r,\n \t oideq(&src->oid, &dst->oid))\n \t\treturn 0; /* they are the same */\n \n-\tif (diff_populate_filespec(r, src, 0) ||\n-\t diff_populate_filespec(r, dst, 0))\n+\tif (diff_populate_filespec(r, src, NULL) ||\n+\t diff_populate_filespec(r, dst, NULL))\n \t\treturn 0; /* error but caught downstream */\n \n \tmax_size = ((src->size > dst->size) ? src->size : dst->size);\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex e189f407af..bf4c0b8740 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -148,6 +148,9 @@ static int estimate_similarity(struct repository *r,\n \t */\n \tunsigned long max_size, delta_size, base_size, src_copied, literal_added;\n \tint score;\n+\tstruct diff_populate_filespec_options dpf_options = {\n+\t\t.check_size_only = 1\n+\t};\n \n \t/* We deal only with regular files. Symlink renames are handled\n \t * only when they are exact matches --- in other words, no edits\n@@ -166,10 +169,10 @@ static int estimate_similarity(struct repository *r,\n \t * say whether the size is valid or not!)\n \t */\n \tif (!src->cnt_data &&\n-\t diff_populate_filespec(r, src, CHECK_SIZE_ONLY))\n+\t diff_populate_filespec(r, src, &dpf_options))\n \t\treturn 0;\n \tif (!dst->cnt_data &&\n-\t diff_populate_filespec(r, dst, CHECK_SIZE_ONLY))\n+\t diff_populate_filespec(r, dst, &dpf_options))\n \t\treturn 0;\n \n \tmax_size = ((src->size > dst->size) ? src->size : dst->size);\n@@ -187,9 +190,9 @@ static int estimate_similarity(struct repository *r,\n \tif (max_size * (MAX_SCORE-minimum_score) < delta_size * MAX_SCORE)\n \t\treturn 0;\n \n-\tif (!src->cnt_data && diff_populate_filespec(r, src, 0))\n+\tif (!src->cnt_data && diff_populate_filespec(r, src, NULL))\n \t\treturn 0;\n-\tif (!dst->cnt_data && diff_populate_filespec(r, dst, 0))\n+\tif (!dst->cnt_data && diff_populate_filespec(r, dst, NULL))\n \t\treturn 0;\n \n \tif (diffcore_count_changes(r, src, dst,\n@@ -261,7 +264,7 @@ static unsigned int hash_filespec(struct repository *r,\n \t\t\t\t struct diff_filespec *filespec)\n {\n \tif (!filespec->oid_valid) {\n-\t\tif (diff_populate_filespec(r, filespec, 0))\n+\t\tif (diff_populate_filespec(r, filespec, NULL))\n \t\t\treturn 0;\n \t\thash_object_file(r->hash_algo, filespec->data, filespec->size,\n \t\t\t\t \"blob\", &filespec->oid);\ndiff --git a/diffcore.h b/diffcore.h\nindex 7c07347e42..3b2020ce93 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -65,9 +65,12 @@ void free_filespec(struct diff_filespec *);\n void fill_filespec(struct diff_filespec *, const struct object_id *,\n \t\t int, unsigned short);\n \n-#define CHECK_SIZE_ONLY 1\n-#define CHECK_BINARY 2\n-int diff_populate_filespec(struct repository *, struct diff_filespec *, unsigned int);\n+struct diff_populate_filespec_options {\n+\tunsigned check_size_only : 1;\n+\tunsigned check_binary : 1;\n+};\n+int diff_populate_filespec(struct repository *, struct diff_filespec *,\n+\t\t\t const struct diff_populate_filespec_options *);\n void diff_free_filespec_data(struct diff_filespec *);\n void diff_free_filespec_blob(struct diff_filespec *);\n int diff_filespec_is_binary(struct repository *, struct diff_filespec *);\ndiff --git a/line-log.c b/line-log.c\nindex 9010e00950..40e1738dbb 100644\n--- a/line-log.c\n+++ b/line-log.c\n@@ -519,7 +519,7 @@ static void fill_line_ends(struct repository *r,\n \tunsigned long *ends = NULL;\n \tchar *data = NULL;\n \n-\tif (diff_populate_filespec(r, spec, 0))\n+\tif (diff_populate_filespec(r, spec, NULL))\n \t\tdie(\"Cannot read blob %s\", oid_to_hex(&spec->oid));\n \n \tALLOC_ARRAY(ends, size);\n@@ -1045,12 +1045,12 @@ static int process_diff_filepair(struct rev_info *rev,\n \t\treturn 0;\n \n \tassert(pair->two->oid_valid);\n-\tdiff_populate_filespec(rev->diffopt.repo, pair->two, 0);\n+\tdiff_populate_filespec(rev->diffopt.repo, pair->two, NULL);\n \tfile_target.ptr = pair->two->data;\n \tfile_target.size = pair->two->size;\n \n \tif (pair->one->oid_valid) {\n-\t\tdiff_populate_filespec(rev->diffopt.repo, pair->one, 0);\n+\t\tdiff_populate_filespec(rev->diffopt.repo, pair->one, NULL);\n \t\tfile_parent.ptr = pair->one->data;\n \t\tfile_parent.size = pair->one->size;\n \t} else {\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"395016","messageId":"34c239aa07233d5fc71c1e3cc2ea0ad32ad2bf78.1586296510.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1586296510.git.jonathantanmy@google.com","subject":"[PATCH v3 3/4] diff: refactor object read","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-07T22:11:42Z","receivedAt":"2020-04-07T22:11:56Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Refactor the object reads in diff_populate_filespec() to have the first\nobject read not be in an if/else branch, because in a future patch, a\nretry will be added to that first object read.\n\nSigned-off-by: Jonathan Tan \n---\n diff.c | 29 +++++++++++++++++++++--------\n 1 file changed, 21 insertions(+), 8 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f337d837ac..8db981b906 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4023,12 +4023,22 @@ int diff_populate_filespec(struct repository *r,\n \t\t}\n \t}\n \telse {\n-\t\tenum object_type type;\n+\t\tstruct object_info info = {\n+\t\t\t.sizep = &s->size\n+\t\t};\n+\n+\t\tif (!(size_only || check_binary))\n+\t\t\t/*\n+\t\t\t * Set contentp, since there is no chance that merely\n+\t\t\t * the size is sufficient.\n+\t\t\t */\n+\t\t\tinfo.contentp = &s->data;\n+\n+\t\tif (oid_object_info_extended(r, &s->oid, &info,\n+\t\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE))\n+\t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n+\n \t\tif (size_only || check_binary) {\n-\t\t\ttype = oid_object_info(r, &s->oid, &s->size);\n-\t\t\tif (type < 0)\n-\t\t\t\tdie(\"unable to read %s\",\n-\t\t\t\t oid_to_hex(&s->oid));\n \t\t\tif (size_only)\n \t\t\t\treturn 0;\n \t\t\tif (s->size > big_file_threshold && s->is_binary == -1) {\n@@ -4036,9 +4046,12 @@ int diff_populate_filespec(struct repository *r,\n \t\t\t\treturn 0;\n \t\t\t}\n \t\t}\n-\t\ts->data = repo_read_object_file(r, &s->oid, &type, &s->size);\n-\t\tif (!s->data)\n-\t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n+\t\tif (!info.contentp) {\n+\t\t\tinfo.contentp = &s->data;\n+\t\t\tif (oid_object_info_extended(r, &s->oid, &info,\n+\t\t\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE))\n+\t\t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n+\t\t}\n \t\ts->should_free = 1;\n \t}\n \treturn 0;\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"395017","messageId":"ee3373bc6a27ee1c4015abc84b634c036048e3f0.1586296510.git.jonathantanmy@google.com","threadId":"53139","inReplyTo":"cover.1586296510.git.jonathantanmy@google.com","subject":"[PATCH v3 4/4] diff: restrict when prefetching occurs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-04-07T22:11:43Z","receivedAt":"2020-04-07T22:11:59Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Commit 7fbbcb21b1 (\"diff: batch fetching of missing blobs\", 2019-04-08)\noptimized \"diff\" by prefetching blobs in a partial clone, but there are\nsome cases wherein blobs do not need to be prefetched. In these cases,\nany command that uses the diff machinery will unnecessarily fetch blobs.\n\ndiffcore_std() may read blobs when it calls the following functions:\n (1) diffcore_skip_stat_unmatch() (controlled by the config variable\n diff.autorefreshindex)\n (2) diffcore_break() and diffcore_merge_broken() (for break-rewrite\n detection)\n (3) diffcore_rename() (for rename detection)\n (4) diffcore_pickaxe() (for detecting addition/deletion of specified\n string)\n\nInstead of always prefetching blobs, teach diffcore_skip_stat_unmatch(),\ndiffcore_break(), and diffcore_rename() to prefetch blobs upon the first\nread of a missing object. This covers (1), (2), and (3): to cover the\nrest, teach diffcore_std() to prefetch if the output type is one that\nincludes blob data (and hence blob data will be required later anyway),\nor if it knows that (4) will be run.\n\nHelped-by: Jeff King \nSigned-off-by: Jonathan Tan \n---\n diff.c | 73 ++++++++++++++++++++++++-----------\n diffcore-break.c | 12 +++++-\n diffcore-rename.c | 55 ++++++++++++++++++++++++--\n diffcore.h | 21 ++++++++++\n t/t4067-diff-partial-clone.sh | 48 +++++++++++++++++++++++\n 5 files changed, 181 insertions(+), 28 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 8db981b906..d1ad6a3c4a 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -4034,10 +4034,18 @@ int diff_populate_filespec(struct repository *r,\n \t\t\t */\n \t\t\tinfo.contentp = &s->data;\n \n+\t\tif (options && options->missing_object_cb) {\n+\t\t\tif (!oid_object_info_extended(r, &s->oid, &info,\n+\t\t\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE |\n+\t\t\t\t\t\t OBJECT_INFO_SKIP_FETCH_OBJECT))\n+\t\t\t\tgoto object_read;\n+\t\t\toptions->missing_object_cb(options->missing_object_data);\n+\t\t}\n \t\tif (oid_object_info_extended(r, &s->oid, &info,\n \t\t\t\t\t OBJECT_INFO_LOOKUP_REPLACE))\n \t\t\tdie(\"unable to read %s\", oid_to_hex(&s->oid));\n \n+object_read:\n \t\tif (size_only || check_binary) {\n \t\t\tif (size_only)\n \t\t\t\treturn 0;\n@@ -6447,6 +6455,8 @@ static int diff_filespec_check_stat_unmatch(struct repository *r,\n {\n \tstruct diff_populate_filespec_options dpf_options = {\n \t\t.check_size_only = 1,\n+\t\t.missing_object_cb = diff_queued_diff_prefetch,\n+\t\t.missing_object_data = r,\n \t};\n \n \tif (p->done_skip_stat_unmatch)\n@@ -6523,9 +6533,9 @@ void diffcore_fix_diff_index(void)\n \tQSORT(q->queue, q->nr, diffnamecmp);\n }\n \n-static void add_if_missing(struct repository *r,\n-\t\t\t struct oid_array *to_fetch,\n-\t\t\t const struct diff_filespec *filespec)\n+void diff_add_if_missing(struct repository *r,\n+\t\t\t struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec)\n {\n \tif (filespec && filespec->oid_valid &&\n \t !S_ISGITLINK(filespec->mode) &&\n@@ -6534,29 +6544,48 @@ static void add_if_missing(struct repository *r,\n \t\toid_array_append(to_fetch, &filespec->oid);\n }\n \n-void diffcore_std(struct diff_options *options)\n+void diff_queued_diff_prefetch(void *repository)\n {\n-\tif (options->repo == the_repository && has_promisor_remote()) {\n-\t\t/*\n-\t\t * Prefetch the diff pairs that are about to be flushed.\n-\t\t */\n-\t\tint i;\n-\t\tstruct diff_queue_struct *q = &diff_queued_diff;\n-\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\tstruct repository *repo = repository;\n+\tint i;\n+\tstruct diff_queue_struct *q = &diff_queued_diff;\n+\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n \n-\t\tfor (i = 0; i < q->nr; i++) {\n-\t\t\tstruct diff_filepair *p = q->queue[i];\n-\t\t\tadd_if_missing(options->repo, &to_fetch, p->one);\n-\t\t\tadd_if_missing(options->repo, &to_fetch, p->two);\n-\t\t}\n-\t\t/*\n-\t\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n-\t\t */\n-\t\tpromisor_remote_get_direct(options->repo,\n-\t\t\t\t\t to_fetch.oid, to_fetch.nr);\n-\t\toid_array_clear(&to_fetch);\n+\tfor (i = 0; i < q->nr; i++) {\n+\t\tstruct diff_filepair *p = q->queue[i];\n+\t\tdiff_add_if_missing(repo, &to_fetch, p->one);\n+\t\tdiff_add_if_missing(repo, &to_fetch, p->two);\n \t}\n \n+\t/*\n+\t * NEEDSWORK: Consider deduplicating the OIDs sent.\n+\t */\n+\tpromisor_remote_get_direct(repo, to_fetch.oid, to_fetch.nr);\n+\n+\toid_array_clear(&to_fetch);\n+}\n+\n+void diffcore_std(struct diff_options *options)\n+{\n+\tint output_formats_to_prefetch = DIFF_FORMAT_DIFFSTAT |\n+\t\tDIFF_FORMAT_NUMSTAT |\n+\t\tDIFF_FORMAT_PATCH |\n+\t\tDIFF_FORMAT_SHORTSTAT |\n+\t\tDIFF_FORMAT_DIRSTAT;\n+\n+\t/*\n+\t * Check if the user requested a blob-data-requiring diff output and/or\n+\t * break-rewrite detection (which requires blob data). If yes, prefetch\n+\t * the diff pairs.\n+\t *\n+\t * If no prefetching occurs, diffcore_rename() will prefetch if it\n+\t * decides that it needs inexact rename detection.\n+\t */\n+\tif (options->repo == the_repository && has_promisor_remote() &&\n+\t (options->output_format & output_formats_to_prefetch ||\n+\t options->pickaxe_opts & DIFF_PICKAXE_KINDS_MASK))\n+\t\tdiff_queued_diff_prefetch(options->repo);\n+\n \t/* NOTE please keep the following in sync with diff_tree_combined() */\n \tif (options->skip_stat_unmatch)\n \t\tdiffcore_skip_stat_unmatch(options);\ndiff --git a/diffcore-break.c b/diffcore-break.c\nindex e8f6322c6a..0d4a14964d 100644\n--- a/diffcore-break.c\n+++ b/diffcore-break.c\n@@ -4,6 +4,7 @@\n #include \"cache.h\"\n #include \"diff.h\"\n #include \"diffcore.h\"\n+#include \"promisor-remote.h\"\n \n static int should_break(struct repository *r,\n \t\t\tstruct diff_filespec *src,\n@@ -49,6 +50,8 @@ static int should_break(struct repository *r,\n \tunsigned long delta_size, max_size;\n \tunsigned long src_copied, literal_added, src_removed;\n \n+\tstruct diff_populate_filespec_options options = { 0 };\n+\n \t*merge_score_p = 0; /* assume no deletion --- \"do not break\"\n \t\t\t * is the default.\n \t\t\t */\n@@ -62,8 +65,13 @@ static int should_break(struct repository *r,\n \t oideq(&src->oid, &dst->oid))\n \t\treturn 0; /* they are the same */\n \n-\tif (diff_populate_filespec(r, src, NULL) ||\n-\t diff_populate_filespec(r, dst, NULL))\n+\tif (r == the_repository && has_promisor_remote()) {\n+\t\toptions.missing_object_cb = diff_queued_diff_prefetch;\n+\t\toptions.missing_object_data = r;\n+\t}\n+\n+\tif (diff_populate_filespec(r, src, &options) ||\n+\t diff_populate_filespec(r, dst, &options))\n \t\treturn 0; /* error but caught downstream */\n \n \tmax_size = ((src->size > dst->size) ? src->size : dst->size);\ndiff --git a/diffcore-rename.c b/diffcore-rename.c\nindex bf4c0b8740..99e63e90f8 100644\n--- a/diffcore-rename.c\n+++ b/diffcore-rename.c\n@@ -1,4 +1,5 @@\n /*\n+ *\n * Copyright (C) 2005 Junio C Hamano\n */\n #include \"cache.h\"\n@@ -7,6 +8,7 @@\n #include \"object-store.h\"\n #include \"hashmap.h\"\n #include \"progress.h\"\n+#include \"promisor-remote.h\"\n \n /* Table of rename/copy destinations */\n \n@@ -128,10 +130,46 @@ struct diff_score {\n \tshort name_score;\n };\n \n+struct prefetch_options {\n+\tstruct repository *repo;\n+\tint skip_unmodified;\n+};\n+static void prefetch(void *prefetch_options)\n+{\n+\tstruct prefetch_options *options = prefetch_options;\n+\tint i;\n+\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\n+\tfor (i = 0; i < rename_dst_nr; i++) {\n+\t\tif (rename_dst[i].pair)\n+\t\t\t/*\n+\t\t\t * The loop in diffcore_rename() will not need these\n+\t\t\t * blobs, so skip prefetching.\n+\t\t\t */\n+\t\t\tcontinue; /* already found exact match */\n+\t\tdiff_add_if_missing(options->repo, &to_fetch,\n+\t\t\t\t rename_dst[i].two);\n+\t}\n+\tfor (i = 0; i < rename_src_nr; i++) {\n+\t\tif (options->skip_unmodified &&\n+\t\t diff_unmodified_pair(rename_src[i].p))\n+\t\t\t/*\n+\t\t\t * The loop in diffcore_rename() will not need these\n+\t\t\t * blobs, so skip prefetching.\n+\t\t\t */\n+\t\t\tcontinue;\n+\t\tdiff_add_if_missing(options->repo, &to_fetch,\n+\t\t\t\t rename_src[i].p->one);\n+\t}\n+\tpromisor_remote_get_direct(options->repo, to_fetch.oid, to_fetch.nr);\n+\toid_array_clear(&to_fetch);\n+}\n+\n static int estimate_similarity(struct repository *r,\n \t\t\t struct diff_filespec *src,\n \t\t\t struct diff_filespec *dst,\n-\t\t\t int minimum_score)\n+\t\t\t int minimum_score,\n+\t\t\t int skip_unmodified)\n {\n \t/* src points at a file that existed in the original tree (or\n \t * optionally a file in the destination tree) and dst points\n@@ -151,6 +189,12 @@ static int estimate_similarity(struct repository *r,\n \tstruct diff_populate_filespec_options dpf_options = {\n \t\t.check_size_only = 1\n \t};\n+\tstruct prefetch_options prefetch_options = {r, skip_unmodified};\n+\n+\tif (r == the_repository && has_promisor_remote()) {\n+\t\tdpf_options.missing_object_cb = prefetch;\n+\t\tdpf_options.missing_object_data = &prefetch_options;\n+\t}\n \n \t/* We deal only with regular files. Symlink renames are handled\n \t * only when they are exact matches --- in other words, no edits\n@@ -190,9 +234,11 @@ static int estimate_similarity(struct repository *r,\n \tif (max_size * (MAX_SCORE-minimum_score) < delta_size * MAX_SCORE)\n \t\treturn 0;\n \n-\tif (!src->cnt_data && diff_populate_filespec(r, src, NULL))\n+\tdpf_options.check_size_only = 0;\n+\n+\tif (!src->cnt_data && diff_populate_filespec(r, src, &dpf_options))\n \t\treturn 0;\n-\tif (!dst->cnt_data && diff_populate_filespec(r, dst, NULL))\n+\tif (!dst->cnt_data && diff_populate_filespec(r, dst, &dpf_options))\n \t\treturn 0;\n \n \tif (diffcore_count_changes(r, src, dst,\n@@ -569,7 +615,8 @@ void diffcore_rename(struct diff_options *options)\n \n \t\t\tthis_src.score = estimate_similarity(options->repo,\n \t\t\t\t\t\t\t one, two,\n-\t\t\t\t\t\t\t minimum_score);\n+\t\t\t\t\t\t\t minimum_score,\n+\t\t\t\t\t\t\t skip_unmodified);\n \t\t\tthis_src.name_score = basename_same(one, two);\n \t\t\tthis_src.dst = i;\n \t\t\tthis_src.src = j;\ndiff --git a/diffcore.h b/diffcore.h\nindex 3b2020ce93..d2a63c5c71 100644\n--- a/diffcore.h\n+++ b/diffcore.h\n@@ -65,9 +65,22 @@ void free_filespec(struct diff_filespec *);\n void fill_filespec(struct diff_filespec *, const struct object_id *,\n \t\t int, unsigned short);\n \n+/*\n+ * Prefetch the entries in diff_queued_diff. The parameter is a pointer to a\n+ * struct repository.\n+ */\n+void diff_queued_diff_prefetch(void *repository);\n+\n struct diff_populate_filespec_options {\n \tunsigned check_size_only : 1;\n \tunsigned check_binary : 1;\n+\n+\t/*\n+\t * If an object is missing, diff_populate_filespec() will invoke this\n+\t * callback before attempting to read that object again.\n+\t */\n+\tvoid (*missing_object_cb)(void *);\n+\tvoid *missing_object_data;\n };\n int diff_populate_filespec(struct repository *, struct diff_filespec *,\n \t\t\t const struct diff_populate_filespec_options *);\n@@ -185,4 +198,12 @@ int diffcore_count_changes(struct repository *r,\n \t\t\t unsigned long *src_copied,\n \t\t\t unsigned long *literal_added);\n \n+/*\n+ * If filespec contains an OID and if that object is missing from the given\n+ * repository, add that OID to to_fetch.\n+ */\n+void diff_add_if_missing(struct repository *r,\n+\t\t\t struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec);\n+\n #endif\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nindex 4831ad35e6..c1ed1c2fc4 100755\n--- a/t/t4067-diff-partial-clone.sh\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -131,4 +131,52 @@ test_expect_success 'diff with rename detection batches blobs' '\n \ttest_line_count = 1 done_lines\n '\n \n+test_expect_success 'diff does not fetch anything if inexact rename detection is not needed' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\tmv server/b server/c &&\n+\tgit -C server add c &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace\n+'\n+\n+test_expect_success 'diff --break-rewrites fetches only if necessary, and batches blobs if it does' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\tprintf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\tprintf \"c\\nc\\nc\\nc\\nc\\n\" >server/b &&\n+\tgit -C server commit -a -m x &&\n+\n+\ttest_config -C server uploadpack.allowfilter 1 &&\n+\ttest_config -C server uploadpack.allowanysha1inwant 1 &&\n+\tgit clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n+\n+\t# Ensure no fetches.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --raw -M HEAD^ HEAD &&\n+\t! test_path_exists trace &&\n+\n+\t# But with --break-rewrites, ensure that there is exactly 1 negotiation\n+\t# by checking that there is only 1 \"done\" line sent. (\"done\" marks the\n+\t# end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff --break-rewrites --raw -M HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n test_done\n-- \n2.26.0.292.g33ef6b2f38-goog\n\n"},{"id":"395027","messageId":"xmqqpncirhd9.fsf@gitster.c.googlers.com","threadId":"53139","inReplyTo":"c1973fd6308109b0cc99544500d8932222b66726.1586296510.git.jonathantanmy@google.com","subject":"Re: [PATCH v3 2/4] diff: make diff_populate_filespec_options struct","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-04-07T23:44:18Z","receivedAt":"2020-04-07T23:44:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> The behavior of diff_populate_filespec() currently can be customized\n> through a bitflag, but a subsequent patch requires it to support a\n> non-boolean option. Replace the bitflag with an options struct.\n\nHmph, clever :-).\n\n> +\tstruct diff_populate_filespec_options dpf_options = {\n> +\t\t.check_size_only = 1,\n> +\t};\n\nI would have called this instance of d-p-f-o \"check_size_only\",\nwhich would make the site that uses it ...\n\n> \tif (!DIFF_FILE_VALID(one))\n> \t\treturn 0;\n> -\tdiff_populate_filespec(r, one, CHECK_SIZE_ONLY);\n> +\tdiff_populate_filespec(r, one, &dpf_options);\n\n... easier to understand, especially if we can made it constant, but\nthat would probably contradict the plan to add more fields to the\nstructure, so let's see how it goes.\n\n> @@ -3339,13 +3346,17 @@ static void emit_binary_diff(struct diff_options *o,\n> int diff_filespec_is_binary(struct repository *r,\n> \t\t\t struct diff_filespec *one)\n> {\n> +\tstruct diff_populate_filespec_options dpf_options = {\n> +\t\t.check_binary = 1,\n> +\t};\n> +\n\nThe same comment applies to here, too.\n\n"}]}