{"thread":{"id":"50824","subject":"[PATCH] diff: batch fetching of missing blobs","startedAt":"2019-03-26T22:09:12Z","lastAt":"2019-04-09T06:36:10Z","messageCount":29,"participants":["Jonathan Tan","SZEDER Gábor","Johannes Schindelin","Jeff King","Duy Nguyen","Christian Couder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"372552","messageId":"20190326220906.111879-1-jonathantanmy@google.com","threadId":"50824","inReplyTo":null,"subject":"[PATCH] diff: batch fetching of missing blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-26T22:09:06Z","receivedAt":"2019-03-26T22:09:12Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running a command like \"git show\" or \"git diff\" in a partial clone,\nbatch all missing blobs to be fetched as one request.\n\nThis is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\nblobs\", 2017-12-08), but for another command.\n\nSigned-off-by: Jonathan Tan \n---\nHere's an improvement for those having partial clones.\n\nI couldn't find a good place to place the test (a place that checks how\ndiff interfaces with the object store would be ideal), so I created a\nnew one. Let me know if there's a better place to put it.\n---\n diff.c | 27 +++++++++++++\n t/t4067-diff-partial-clone.sh | 76 +++++++++++++++++++++++++++++++++++\n 2 files changed, 103 insertions(+)\n create mode 100755 t/t4067-diff-partial-clone.sh\n\ndiff --git a/diff.c b/diff.c\nindex ec5c095199..0e08d05b14 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,6 +25,7 @@\n #include \"packfile.h\"\n #include \"parse-options.h\"\n #include \"help.h\"\n+#include \"fetch-object.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6067,6 +6068,32 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n \tif (o->color_moved)\n \t\to->emitted_symbols = &esm;\n \n+\tif (repository_format_partial_clone) {\n+\t\t/*\n+\t\t * Prefetch the diff pairs that are about to be flushed.\n+\t\t */\n+\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n+\t\tint fetch_if_missing_store = fetch_if_missing;\n+\n+\t\tfetch_if_missing = 0;\n+\t\tfor (i = 0; i < q->nr; i++) {\n+\t\t\tstruct diff_filepair *p = q->queue[i];\n+\t\t\tif (!check_pair_status(p))\n+\t\t\t\tcontinue;\n+\t\t\tif (p->one && p->one->oid_valid &&\n+\t\t\t !has_object_file(&p->one->oid))\n+\t\t\t\toid_array_append(&to_fetch, &p->one->oid);\n+\t\t\tif (p->two && p->two->oid_valid &&\n+\t\t\t !has_object_file(&p->two->oid))\n+\t\t\t\toid_array_append(&to_fetch, &p->two->oid);\n+\t\t}\n+\t\tif (to_fetch.nr)\n+\t\t\tfetch_objects(repository_format_partial_clone,\n+\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\tfetch_if_missing = fetch_if_missing_store;\n+\t\toid_array_clear(&to_fetch);\n+\t}\n+\n \tfor (i = 0; i < q->nr; i++) {\n \t\tstruct diff_filepair *p = q->queue[i];\n \t\tif (check_pair_status(p))\ndiff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nnew file mode 100755\nindex 0000000000..38f03be114\n--- /dev/null\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -0,0 +1,76 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff when reading objects in a partial clone'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'git show batches blobs' '\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff batches blobs' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo c >server/c &&\n+\techo d >server/d &&\n+\tgit -C server add c d &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff skips same-OID blobs' '\n+\trm -rf server client trace &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo another-a >server/a &&\n+\tgit -C server add a &&\n+\tgit -C server commit -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+\techo a | git hash-object --stdin >hash-old-a &&\n+\techo another-a | git hash-object --stdin >hash-new-a &&\n+\techo b | git hash-object --stdin >hash-b &&\n+\n+\t# Ensure that only a and another-a are fetched.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"want $(cat hash-old-a)\" trace &&\n+\tgrep \"want $(cat hash-new-a)\" trace &&\n+\t! grep \"want $(cat hash-b)\" trace\n+'\n+\n+test_done\n-- \n2.21.0.155.ge902e9bcae.dirty\n\n"},{"id":"372563","messageId":"20190327101055.GB32732@szeder.dev","threadId":"50824","inReplyTo":"20190326220906.111879-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: batch fetching of missing blobs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-27T10:10:55Z","receivedAt":"2019-03-27T10:11:01Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Tue, Mar 26, 2019 at 03:09:06PM -0700, Jonathan Tan wrote:\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n> index 0000000000..38f03be114\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,76 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> +\trm -rf server client trace &&\n\nPlease use 'test_when_finished' in the previous test to clean up after\nitself.\n\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo c >server/c &&\n> +\techo d >server/d &&\n> +\tgit -C server add c d &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> +\trm -rf server client trace &&\n\nLikewise.\n\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo another-a >server/a &&\n> +\tgit -C server add a &&\n> +\tgit -C server commit -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> +\techo a | git hash-object --stdin >hash-old-a &&\n> +\techo another-a | git hash-object --stdin >hash-new-a &&\n> +\techo b | git hash-object --stdin >hash-b &&\n> +\n> +\t# Ensure that only a and another-a are fetched.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"want $(cat hash-old-a)\" trace &&\n> +\tgrep \"want $(cat hash-new-a)\" trace &&\n> +\t! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_done\n> -- \n> 2.21.0.155.ge902e9bcae.dirty\n> \n"},{"id":"372589","messageId":"nycvar.QRO.7.76.6.1903272300020.41@tvgsbejvaqbjf.bet","threadId":"50824","inReplyTo":"20190326220906.111879-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: batch fetching of missing blobs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-03-27T22:02:04Z","receivedAt":"2019-03-27T22:02:09Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Tue, 26 Mar 2019, Jonathan Tan wrote:\n\n> When running a command like \"git show\" or \"git diff\" in a partial clone,\n> batch all missing blobs to be fetched as one request.\n>\n> This is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\n> blobs\", 2017-12-08), but for another command.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Here's an improvement for those having partial clones.\n>\n> I couldn't find a good place to place the test (a place that checks how\n> diff interfaces with the object store would be ideal), so I created a\n> new one. Let me know if there's a better place to put it.\n\nMy 2c: that's fine.\n\n> diff --git a/diff.c b/diff.c\n> index ec5c095199..0e08d05b14 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -25,6 +25,7 @@\n> #include \"packfile.h\"\n> #include \"parse-options.h\"\n> #include \"help.h\"\n> +#include \"fetch-object.h\"\n>\n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -6067,6 +6068,32 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n> \tif (o->color_moved)\n> \t\to->emitted_symbols = &esm;\n>\n> +\tif (repository_format_partial_clone) {\n> +\t\t/*\n> +\t\t * Prefetch the diff pairs that are about to be flushed.\n> +\t\t */\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\t\tint fetch_if_missing_store = fetch_if_missing;\n> +\n> +\t\tfetch_if_missing = 0;\n> +\t\tfor (i = 0; i < q->nr; i++) {\n> +\t\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\t\tif (!check_pair_status(p))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (p->one && p->one->oid_valid &&\n> +\t\t\t !has_object_file(&p->one->oid))\n> +\t\t\t\toid_array_append(&to_fetch, &p->one->oid);\n> +\t\t\tif (p->two && p->two->oid_valid &&\n> +\t\t\t !has_object_file(&p->two->oid))\n> +\t\t\t\toid_array_append(&to_fetch, &p->two->oid);\n> +\t\t}\n> +\t\tif (to_fetch.nr)\n> +\t\t\tfetch_objects(repository_format_partial_clone,\n> +\t\t\t\t to_fetch.oid, to_fetch.nr);\n> +\t\tfetch_if_missing = fetch_if_missing_store;\n\nI am slightly uneasy about the fact that this is totally *not*\nmulti-thread safe. If only we had a way to pass the `fetch_if_missing`\ninformation as a parameter to `has_object_file()`...\n\nDo you think that would be easy to do? If so, I think now would be as fine\nan opportunity as ever to introduce that.\n\nOtherwise the patch looks just fine to me.\n\nThanks,\nDscho\n\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\n> +\n> \tfor (i = 0; i < q->nr; i++) {\n> \t\tstruct diff_filepair *p = q->queue[i];\n> \t\tif (check_pair_status(p))\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n> index 0000000000..38f03be114\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,76 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> +\trm -rf server client trace &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo c >server/c &&\n> +\techo d >server/d &&\n> +\tgit -C server add c d &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> +\trm -rf server client trace &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo another-a >server/a &&\n> +\tgit -C server add a &&\n> +\tgit -C server commit -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> +\techo a | git hash-object --stdin >hash-old-a &&\n> +\techo another-a | git hash-object --stdin >hash-new-a &&\n> +\techo b | git hash-object --stdin >hash-b &&\n> +\n> +\t# Ensure that only a and another-a are fetched.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"want $(cat hash-old-a)\" trace &&\n> +\tgrep \"want $(cat hash-new-a)\" trace &&\n> +\t! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_done\n> --\n> 2.21.0.155.ge902e9bcae.dirty\n>\n>\n"},{"id":"372606","messageId":"20190328065252.GA1930@sigill.intra.peff.net","threadId":"50824","inReplyTo":"20190326220906.111879-1-jonathantanmy@google.com","subject":"Re: [PATCH] diff: batch fetching of missing blobs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-03-28T06:52:52Z","receivedAt":"2019-03-28T06:52:56Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Mar 26, 2019 at 03:09:06PM -0700, Jonathan Tan wrote:\n\n> When running a command like \"git show\" or \"git diff\" in a partial clone,\n> batch all missing blobs to be fetched as one request.\n> \n> This is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\n> blobs\", 2017-12-08), but for another command.\n\nSounds like a good idea, and this should make some cases much better\nwithout making any cases worse. Two observations about how we might do\neven better, though:\n\n> @@ -6067,6 +6068,32 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)\n> [...]\n\nAt this stage we're looking at actually diffing the contents themselves.\nBut we'd also potentially need the blobs during rename and break\ndetection. It's not always the same set of blobs (e.g., unless you've\ncranked up the copy-detection flags, renames only look at added/deleted\nentries). We could have each phase do its own bulk fetch, which\nworst-case gives us probably three fetches. But I wonder if we could\nfigure out a complete plausible set immediately after the tree diff.\n\n> +\tif (repository_format_partial_clone) {\n> +\t\t/*\n> +\t\t * Prefetch the diff pairs that are about to be flushed.\n> +\t\t */\n> +\t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> +\t\tint fetch_if_missing_store = fetch_if_missing;\n> +\n> +\t\tfetch_if_missing = 0;\n> +\t\tfor (i = 0; i < q->nr; i++) {\n> +\t\t\tstruct diff_filepair *p = q->queue[i];\n> +\t\t\tif (!check_pair_status(p))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (p->one && p->one->oid_valid &&\n> +\t\t\t !has_object_file(&p->one->oid))\n> +\t\t\t\toid_array_append(&to_fetch, &p->one->oid);\n> +\t\t\tif (p->two && p->two->oid_valid &&\n> +\t\t\t !has_object_file(&p->two->oid))\n> +\t\t\t\toid_array_append(&to_fetch, &p->two->oid);\n> +\t\t}\n\nThese has_object_file() calls may repeatedly re-scan the pack directory,\nonce per call. Since it's likely that some may be missing, that may be\na noticeable amount of wasted work for a big diff (still way less than\nindividually fetching each missing object, so it's a net win, but read\non).\n\nIf you use the QUICK flag, that avoids the re-scans, but we may miss\nerroneously say \"we don't have it\" if we race with a repack. For that,\nwe can either:\n\n 1. Just ignore it. It's relatively rare, and the worst case is that we\n re-fetch an object.\n\n 2. Do a series of QUICK checks, followed by a single\n reprepare_packed_git() if we had any missing, and then another\n series of QUICK checks. Then worst-case we have a single re-scan.\n\nSomething like:\n\n int object_is_missing_cb(const struct object_id *oid, void *data)\n {\n\treturn !has_object_file_with_flags(oid, OBJECT_INFO_QUICK);\n }\n ...\n\n /* collect all of the possible blobs we need */\n for (i = 0; i < q->nr; i++) {\n\t...\n\toid_array_append(&to_fetch, &p->one->oid);\n\toid_array_append(&to_fetch, &p->two->oid);\n }\n\n /* drop any we already have */\n oid_array_filter(&to_fetch, object_is_missing_cb, NULL);\n\n /* any missing ones might actually be a race; try again */\n if (to_fetch.nr) {\n\t reprepare_packed_git(the_repository);\n\t oid_array_filter(&to_fetch, object_is_missing_cb, NULL);\n }\n\n /* and now anything we have left is definitely not here */\n if (to_fetch.nr)\n\tfetch_objects(..., to_fetch.oid, to_fetch.nr).\n\nOne thing I noticed while writing this: we don't seem to do any\nde-duplication of the list (in yours or mine), and it doesn't look like\nfetch_objects() does either. I wonder if an oidset would be a better\ndata structure.\n\n-Peff\n"},{"id":"372731","messageId":"cover.1553895166.git.jonathantanmy@google.com","threadId":"50824","inReplyTo":"20190326220906.111879-1-jonathantanmy@google.com","subject":"[PATCH v2 0/2] Batch fetching of missing blobs in diff and show","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-29T21:39:26Z","receivedAt":"2019-03-29T21:39:35Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks, everyone for the review.\n\nChanges from v1:\n - used test_when_finished (Szeder)\n - used flag to inhibit fetching of missing objects (Dscho)\n - moved the prefetch so that it also works if we request rename\n detection, and included a test demonstrating that (not sure if that\n was what Peff meant)\n - used QUICK flag (I agree that the rescan is probably not that\n valuable here)\n\nPeff also suggested that I use an oidset instead of an oidarray in order\nto eliminate duplicates, but that makes it difficult to interface with\nfetch_objects(), which takes a pointer and a length (which is\nconvenient, because if we want to fetch a single object, we can just\npoint to it and use a length of 1). Maybe the best idea is to have\noidset maintain its own OID array, and have each hash entry store an\nindex instead of an OID. For now I added a NEEDSWORK.\n\nJonathan Tan (2):\n sha1-file: support OBJECT_INFO_FOR_PREFETCH\n diff: batch fetching of missing blobs\n\n diff.c | 32 +++++++++++\n object-store.h | 6 ++\n sha1-file.c | 3 +-\n t/t4067-diff-partial-clone.sh | 103 ++++++++++++++++++++++++++++++++++\n unpack-trees.c | 17 +++---\n 5 files changed, 152 insertions(+), 9 deletions(-)\n create mode 100755 t/t4067-diff-partial-clone.sh\n\nRange-diff against v1:\n-: ---------- > 1: 068861632b sha1-file: support OBJECT_INFO_FOR_PREFETCH\n1: d1e604239b ! 2: 44de02e584 diff: batch fetching of missing blobs\n @@ -9,12 +9,6 @@\n blobs\", 2017-12-08), but for another command.\n \n Signed-off-by: Jonathan Tan \n - ---\n - Here's an improvement for those having partial clones.\n -\n - I couldn't find a good place to place the test (a place that checks how\n - diff interfaces with the object store would be ideal), so I created a\n - new one. Let me know if there's a better place to put it.\n \n diff --git a/diff.c b/diff.c\n --- a/diff.c\n @@ -28,38 +22,45 @@\n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n @@\n - \tif (o->color_moved)\n - \t\to->emitted_symbols = &esm;\n + \tQSORT(q->queue, q->nr, diffnamecmp);\n + }\n \n ++static void add_if_missing(struct oid_array *to_fetch,\n ++\t\t\t const struct diff_filespec *filespec)\n ++{\n ++\tif (filespec && filespec->oid_valid &&\n ++\t oid_object_info_extended(the_repository, &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_std(struct diff_options *options)\n + {\n +\tif (repository_format_partial_clone) {\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 -+\t\tint fetch_if_missing_store = fetch_if_missing;\n +\n -+\t\tfetch_if_missing = 0;\n +\t\tfor (i = 0; i < q->nr; i++) {\n +\t\t\tstruct diff_filepair *p = q->queue[i];\n -+\t\t\tif (!check_pair_status(p))\n -+\t\t\t\tcontinue;\n -+\t\t\tif (p->one && p->one->oid_valid &&\n -+\t\t\t !has_object_file(&p->one->oid))\n -+\t\t\t\toid_array_append(&to_fetch, &p->one->oid);\n -+\t\t\tif (p->two && p->two->oid_valid &&\n -+\t\t\t !has_object_file(&p->two->oid))\n -+\t\t\t\toid_array_append(&to_fetch, &p->two->oid);\n ++\t\t\tadd_if_missing(&to_fetch, p->one);\n ++\t\t\tadd_if_missing(&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\tfetch_objects(repository_format_partial_clone,\n +\t\t\t\t to_fetch.oid, to_fetch.nr);\n -+\t\tfetch_if_missing = fetch_if_missing_store;\n +\t\toid_array_clear(&to_fetch);\n +\t}\n +\n - \tfor (i = 0; i < q->nr; i++) {\n - \t\tstruct diff_filepair *p = q->queue[i];\n - \t\tif (check_pair_status(p))\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);\n \n diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n new file mode 100755\n @@ -73,6 +74,8 @@\n +. ./test-lib.sh\n +\n +test_expect_success 'git show batches blobs' '\n ++\ttest_when_finished \"rm -rf server client trace\" &&\n ++\t\n +\ttest_create_repo server &&\n +\techo a >server/a &&\n +\techo b >server/b &&\n @@ -91,7 +94,7 @@\n +'\n +\n +test_expect_success 'diff batches blobs' '\n -+\trm -rf server client trace &&\n ++\ttest_when_finished \"rm -rf server client trace\" &&\n +\n +\ttest_create_repo server &&\n +\techo a >server/a &&\n @@ -115,7 +118,7 @@\n +'\n +\n +test_expect_success 'diff skips same-OID blobs' '\n -+\trm -rf server client trace &&\n ++\ttest_when_finished \"rm -rf server client trace\" &&\n +\n +\ttest_create_repo server &&\n +\techo a >server/a &&\n @@ -141,4 +144,29 @@\n +\t! grep \"want $(cat hash-b)\" trace\n +'\n +\n ++test_expect_success 'diff with rename detection batches blobs' '\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\\nbX\\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 that there is exactly 1 negotiation by checking that there is\n ++\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n ++\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n ++\tgrep \"similarity index\" out &&\n ++\tgrep \"git> done\" trace >done_lines &&\n ++\ttest_line_count = 1 done_lines\n ++'\n ++\n +test_done\n-- \n2.21.0.197.gd478713db0\n\n"},{"id":"372732","messageId":"068861632b85179d2a5a5ceb966e951a78b27141.1553895166.git.jonathantanmy@google.com","threadId":"50824","inReplyTo":"cover.1553895166.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] sha1-file: support OBJECT_INFO_FOR_PREFETCH","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-29T21:39:27Z","receivedAt":"2019-03-29T21:39:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Teach oid_object_info_extended() to support a new flag that inhibits\nfetching of missing objects. This is equivalent to setting\nfetch_is_missing to 0, calling oid_object_info_extended(), then setting\nfetch_if_missing to whatever it was before. Update unpack-trees.c to use\nthis new flag instead of repeatedly setting fetch_if_missing.\n\nThis new flag complicates things slightly in that there are now 2 ways\nto do the same thing. But this eliminates the need to repeatedly set a\nglobal variable, and more importantly, allows prefetching to be done in\nparallel (in the future); hence, this patch.\n\nSigned-off-by: Jonathan Tan \n---\n object-store.h | 6 ++++++\n sha1-file.c | 3 ++-\n unpack-trees.c | 17 +++++++++--------\n 3 files changed, 17 insertions(+), 9 deletions(-)\n\ndiff --git a/object-store.h b/object-store.h\nindex 14fc935bd1..dd3f9b75f0 100644\n--- a/object-store.h\n+++ b/object-store.h\n@@ -280,6 +280,12 @@ struct object_info {\n #define OBJECT_INFO_QUICK 8\n /* Do not check loose object */\n #define OBJECT_INFO_IGNORE_LOOSE 16\n+/*\n+ * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n+ * nonzero). This is meant for bulk prefetching of missing blobs in a partial\n+ * clone. Implies OBJECT_INFO_QUICK.\n+ */\n+#define OBJECT_INFO_FOR_PREFETCH (32 + OBJECT_INFO_QUICK)\n \n int oid_object_info_extended(struct repository *r,\n \t\t\t const struct object_id *,\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 494606f771..ad02649124 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -1370,7 +1370,8 @@ int oid_object_info_extended(struct repository *r, const struct object_id *oid,\n \n \t\t/* Check if it is a missing object */\n \t\tif (fetch_if_missing && repository_format_partial_clone &&\n-\t\t !already_retried && r == the_repository) {\n+\t\t !already_retried && r == the_repository &&\n+\t\t !(flags & OBJECT_INFO_FOR_PREFETCH)) {\n \t\t\t/*\n \t\t\t * TODO Investigate having fetch_object() return\n \t\t\t * TODO error/success and stopping the music here.\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 22c41a3ba8..381b0cd65e 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -404,20 +404,21 @@ static int check_updates(struct unpack_trees_options *o)\n \t\t * below.\n \t\t */\n \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n-\t\tint fetch_if_missing_store = fetch_if_missing;\n-\t\tfetch_if_missing = 0;\n \t\tfor (i = 0; i < index->cache_nr; i++) {\n \t\t\tstruct cache_entry *ce = index->cache[i];\n-\t\t\tif ((ce->ce_flags & CE_UPDATE) &&\n-\t\t\t !S_ISGITLINK(ce->ce_mode)) {\n-\t\t\t\tif (!has_object_file(&ce->oid))\n-\t\t\t\t\toid_array_append(&to_fetch, &ce->oid);\n-\t\t\t}\n+\n+\t\t\tif (!(ce->ce_flags & CE_UPDATE) ||\n+\t\t\t S_ISGITLINK(ce->ce_mode))\n+\t\t\t\tcontinue;\n+\t\t\tif (!oid_object_info_extended(the_repository, &ce->oid,\n+\t\t\t\t\t\t NULL,\n+\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\t\t\tcontinue;\n+\t\t\toid_array_append(&to_fetch, &ce->oid);\n \t\t}\n \t\tif (to_fetch.nr)\n \t\t\tfetch_objects(repository_format_partial_clone,\n \t\t\t\t to_fetch.oid, to_fetch.nr);\n-\t\tfetch_if_missing = fetch_if_missing_store;\n \t\toid_array_clear(&to_fetch);\n \t}\n \tfor (i = 0; i < index->cache_nr; i++) {\n-- \n2.21.0.197.gd478713db0\n\n"},{"id":"372733","messageId":"44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com","threadId":"50824","inReplyTo":"cover.1553895166.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-03-29T21:39:28Z","receivedAt":"2019-03-29T21:39:39Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When running a command like \"git show\" or \"git diff\" in a partial clone,\nbatch all missing blobs to be fetched as one request.\n\nThis is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\nblobs\", 2017-12-08), but for another command.\n\nSigned-off-by: Jonathan Tan \n---\n diff.c | 32 +++++++++++\n t/t4067-diff-partial-clone.sh | 103 ++++++++++++++++++++++++++++++++++\n 2 files changed, 135 insertions(+)\n create mode 100755 t/t4067-diff-partial-clone.sh\n\ndiff --git a/diff.c b/diff.c\nindex ec5c095199..1eccefb4ef 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,6 +25,7 @@\n #include \"packfile.h\"\n #include \"parse-options.h\"\n #include \"help.h\"\n+#include \"fetch-object.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6366,8 +6367,39 @@ void diffcore_fix_diff_index(void)\n \tQSORT(q->queue, q->nr, diffnamecmp);\n }\n \n+static void add_if_missing(struct oid_array *to_fetch,\n+\t\t\t const struct diff_filespec *filespec)\n+{\n+\tif (filespec && filespec->oid_valid &&\n+\t oid_object_info_extended(the_repository, &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_std(struct diff_options *options)\n {\n+\tif (repository_format_partial_clone) {\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+\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(&to_fetch, p->one);\n+\t\t\tadd_if_missing(&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\tfetch_objects(repository_format_partial_clone,\n+\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\toid_array_clear(&to_fetch);\n+\t}\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/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nnew file mode 100755\nindex 0000000000..349851be7d\n--- /dev/null\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff when reading objects in a partial clone'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'git show batches blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\t\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff batches blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo c >server/c &&\n+\techo d >server/d &&\n+\tgit -C server add c d &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff skips same-OID blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo another-a >server/a &&\n+\tgit -C server add a &&\n+\tgit -C server commit -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+\techo a | git hash-object --stdin >hash-old-a &&\n+\techo another-a | git hash-object --stdin >hash-new-a &&\n+\techo b | git hash-object --stdin >hash-b &&\n+\n+\t# Ensure that only a and another-a are fetched.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"want $(cat hash-old-a)\" trace &&\n+\tgrep \"want $(cat hash-new-a)\" trace &&\n+\t! grep \"want $(cat hash-b)\" trace\n+'\n+\n+test_expect_success 'diff with rename detection batches blobs' '\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\\nbX\\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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n+\tgrep \"similarity index\" out &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_done\n-- \n2.21.0.197.gd478713db0\n\n"},{"id":"373089","messageId":"20190404024710.GO32732@szeder.dev","threadId":"50824","inReplyTo":"44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-04-04T02:47:10Z","receivedAt":"2019-04-04T02:47:17Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Fri, Mar 29, 2019 at 02:39:28PM -0700, Jonathan Tan wrote:\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n> index 0000000000..349851be7d\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,103 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\t\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n\nThese patches and 'cc/multi-promisor' don't seem to work well\ntogether, and all tests checking 'test_line_count = 1 done_lines' in\nthis test script fail in current 'pu', because there are two\n\"git> done\" lines.\n\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo c >server/c &&\n> +\techo d >server/d &&\n> +\tgit -C server add c d &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo another-a >server/a &&\n> +\tgit -C server add a &&\n> +\tgit -C server commit -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> +\techo a | git hash-object --stdin >hash-old-a &&\n> +\techo another-a | git hash-object --stdin >hash-new-a &&\n> +\techo b | git hash-object --stdin >hash-b &&\n> +\n> +\t# Ensure that only a and another-a are fetched.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"want $(cat hash-old-a)\" trace &&\n> +\tgrep \"want $(cat hash-new-a)\" trace &&\n> +\t! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_expect_success 'diff with rename detection batches blobs' '\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\\nbX\\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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n> +\tgrep \"similarity index\" out &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_done\n> -- \n> 2.21.0.197.gd478713db0\n> \n"},{"id":"373190","messageId":"CACsJy8CgXLZxqab4vcP1jh3OMCGh1i=easb5BpCs1J8Uf_jsxw@mail.gmail.com","threadId":"50824","inReplyTo":"44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-05T09:39:34Z","receivedAt":"2019-04-05T09:40:03Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 30, 2019 at 4:40 AM Jonathan Tan wrote:\n>\n> When running a command like \"git show\" or \"git diff\" in a partial clone,\n> batch all missing blobs to be fetched as one request.\n>\n> This is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\n> blobs\", 2017-12-08), but for another command.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> diff.c | 32 +++++++++++\n> t/t4067-diff-partial-clone.sh | 103 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 135 insertions(+)\n> create mode 100755 t/t4067-diff-partial-clone.sh\n>\n> diff --git a/diff.c b/diff.c\n> index ec5c095199..1eccefb4ef 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -25,6 +25,7 @@\n> #include \"packfile.h\"\n> #include \"parse-options.h\"\n> #include \"help.h\"\n> +#include \"fetch-object.h\"\n>\n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -6366,8 +6367,39 @@ void diffcore_fix_diff_index(void)\n> QSORT(q->queue, q->nr, diffnamecmp);\n> }\n>\n> +static void add_if_missing(struct oid_array *to_fetch,\n> + const struct diff_filespec *filespec)\n> +{\n> + if (filespec && filespec->oid_valid &&\n> + oid_object_info_extended(the_repository, &filespec->oid, NULL,\n\nI'm quite sure we can pass 'struct repository *' around in diff code\nnow. I think it's the \"repo\" field in \"struct diff_options\". Please\nuse it and avoid more references to the_repository.\n\n> + OBJECT_INFO_FOR_PREFETCH))\n> + oid_array_append(to_fetch, &filespec->oid);\n> +}\n> +\n> void diffcore_std(struct diff_options *options)\n> {\n> + if (repository_format_partial_clone) {\n> + /*\n> + * Prefetch the diff pairs that are about to be flushed.\n> + */\n> + int i;\n> + struct diff_queue_struct *q = &diff_queued_diff;\n> + struct oid_array to_fetch = OID_ARRAY_INIT;\n> +\n> + for (i = 0; i < q->nr; i++) {\n> + struct diff_filepair *p = q->queue[i];\n> + add_if_missing(&to_fetch, p->one);\n> + add_if_missing(&to_fetch, p->two);\n> + }\n> + if (to_fetch.nr)\n> + /*\n> + * NEEDSWORK: Consider deduplicating the OIDs sent.\n> + */\n> + fetch_objects(repository_format_partial_clone,\n> + to_fetch.oid, to_fetch.nr);\n> + oid_array_clear(&to_fetch);\n> + }\n> +\n> /* NOTE please keep the following in sync with diff_tree_combined() */\n> if (options->skip_stat_unmatch)\n> diffcore_skip_stat_unmatch(options);\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n> index 0000000000..349851be7d\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,103 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + echo c >server/c &&\n> + echo d >server/d &&\n> + git -C server add c d &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + echo another-a >server/a &&\n> + git -C server add a &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + echo a | git hash-object --stdin >hash-old-a &&\n> + echo another-a | git hash-object --stdin >hash-new-a &&\n> + echo b | git hash-object --stdin >hash-b &&\n> +\n> + # Ensure that only a and another-a are fetched.\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> + grep \"want $(cat hash-old-a)\" trace &&\n> + grep \"want $(cat hash-new-a)\" trace &&\n> + ! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_expect_success 'diff with rename detection batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + printf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + rm server/b &&\n> + printf \"b\\nb\\nb\\nb\\nbX\\n\" >server/c &&\n> + git -C server add c &&\n> + git -C server commit -a -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n> + grep \"similarity index\" out &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_done\n> --\n> 2.21.0.197.gd478713db0\n>\n\n\n-- \nDuy\n"},{"id":"373198","messageId":"nycvar.QRO.7.76.6.1904051528130.41@tvgsbejvaqbjf.bet","threadId":"50824","inReplyTo":"20190404024710.GO32732@szeder.dev","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-05T13:38:47Z","receivedAt":"2019-04-05T13:39:02Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Thu, 4 Apr 2019, SZEDER Gábor wrote:\n\n> On Fri, Mar 29, 2019 at 02:39:28PM -0700, Jonathan Tan wrote:\n> > diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> > new file mode 100755\n> > index 0000000000..349851be7d\n> > --- /dev/null\n> > +++ b/t/t4067-diff-partial-clone.sh\n> > @@ -0,0 +1,103 @@\n> > +#!/bin/sh\n> > +\n> > +test_description='behavior of diff when reading objects in a partial clone'\n> > +\n> > +. ./test-lib.sh\n> > +\n> > +test_expect_success 'git show batches blobs' '\n> > +\ttest_when_finished \"rm -rf server client trace\" &&\n> > +\n> > +\ttest_create_repo server &&\n> > +\techo a >server/a &&\n> > +\techo b >server/b &&\n> > +\tgit -C server add a b &&\n> > +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> > +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> > +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> > +\tgrep \"git> done\" trace >done_lines &&\n> > +\ttest_line_count = 1 done_lines\n>\n> These patches and 'cc/multi-promisor' don't seem to work well\n> together, and all tests checking 'test_line_count = 1 done_lines' in\n> this test script fail in current 'pu', because there are two\n> \"git> done\" lines.\n\nI investigated a little further, and it would seem that it is neither this\npatch nor the cc/multi-promisor patches that introduce the problem, but\nthe merge between the two... The latter tries to get away from using the\nglobal variable `repository_format_partial_clone` while this patch\nintroduces another user.\n\nSo that merge between these two branches needs to become an \"evil merge\"\n(or should I say \"blessed merge\", or even a \"merge with a blessing\"?). I\nhave this patch, tentatively, for the `shears/pu` branch [*1*], which\nseems to fix the test here:\n\n-- snip --\nSubject: [PATCH] fixup??? Merge branch 'cc/multi-promisor' into pu\n\nThe `cc/multi-promisor` patch series and the\n`jt/batch-fetch-blobs-in-diff` patch series have a semantic conflict:\nthe former replaces checks for `repository_format_partial_clone` with\nchecks for `has_promisor_remote()`, while the latter introduces such a\ncheck.\n\nSigned-off-by: Johannes Schindelin \n---\n diff.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/diff.c b/diff.c\nindex fa12b5d04a..86278ce676 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@\n #include \"parse-options.h\"\n #include \"help.h\"\n #include \"fetch-object.h\"\n+#include \"promisor-remote.h\"\n\n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6493,7 +6494,7 @@ static void add_if_missing(struct oid_array *to_fetch,\n\n void diffcore_std(struct diff_options *options)\n {\n-\tif (repository_format_partial_clone) {\n+\tif (has_promisor_remote()) {\n \t\t/*\n \t\t * Prefetch the diff pairs that are about to be flushed.\n \t\t */\n-- snap --\n\nJunio, would you terribly mind adopting this?\n\nCiao,\nDscho\n\nFootnote *1*: I started again to maintain `shears/*` branches in\nhttps://github.com/git-for-windows/git which essentially are Git for\nWindows' patch thicket rebased to all four integration branches of\nupstream (or \"core\") Git. I try my best at keeping the CI green on those,\nmeaning that I liberally add commits on top that are neither in Git for\nWindows' `master` nor in core Git's branches.\n"},{"id":"373200","messageId":"nycvar.QRO.7.76.6.1904051611010.41@tvgsbejvaqbjf.bet","threadId":"50824","inReplyTo":"068861632b85179d2a5a5ceb966e951a78b27141.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] sha1-file: support OBJECT_INFO_FOR_PREFETCH","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-05T14:13:24Z","receivedAt":"2019-04-05T14:13:30Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Fri, 29 Mar 2019, Jonathan Tan wrote:\n\n> Teach oid_object_info_extended() to support a new flag that inhibits\n> fetching of missing objects. This is equivalent to setting\n> fetch_is_missing to 0, calling oid_object_info_extended(), then setting\n> fetch_if_missing to whatever it was before. Update unpack-trees.c to use\n> this new flag instead of repeatedly setting fetch_if_missing.\n>\n> This new flag complicates things slightly in that there are now 2 ways\n> to do the same thing.\n\nJust a note that I disagree with the latter part of the sentence: those\nare not 2 ways of doing the same thing, but they are two switches that\nessentially both have to be flipped to \"on\". They're just multiple gates.\n\nI do not ask you to rephrase it, merely registering a different opinion.\n\nThe patch looks good, I especially like the post-image of\n`check_updates()`, which looks much nicer (from my perspective, of\ncourse).\n\nThanks,\nDscho\n"},{"id":"373201","messageId":"nycvar.QRO.7.76.6.1904051615480.41@tvgsbejvaqbjf.bet","threadId":"50824","inReplyTo":"44de02e584f449481e6fb00cf35d74adf0192e9d.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-05T14:17:26Z","receivedAt":"2019-04-05T14:17:37Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Fri, 29 Mar 2019, Jonathan Tan wrote:\n\n> When running a command like \"git show\" or \"git diff\" in a partial clone,\n> batch all missing blobs to be fetched as one request.\n>\n> This is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\n> blobs\", 2017-12-08), but for another command.\n\nStill makes sense ;-)\n\n> diff --git a/diff.c b/diff.c\n> index ec5c095199..1eccefb4ef 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -25,6 +25,7 @@\n> #include \"packfile.h\"\n> #include \"parse-options.h\"\n> #include \"help.h\"\n> +#include \"fetch-object.h\"\n>\n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -6366,8 +6367,39 @@ void diffcore_fix_diff_index(void)\n> \tQSORT(q->queue, q->nr, diffnamecmp);\n> }\n>\n> +static void add_if_missing(struct oid_array *to_fetch,\n> +\t\t\t const struct diff_filespec *filespec)\n> +{\n> +\tif (filespec && filespec->oid_valid &&\n> +\t oid_object_info_extended(the_repository, &filespec->oid, NULL,\n> +\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n> +\t\toid_array_append(to_fetch, &filespec->oid);\n> +}\n\nThank you for introducing this, in looks more elegant to my eyes than the\nprevious iteration.\n\n> +\n> void diffcore_std(struct diff_options *options)\n> {\n> +\tif (repository_format_partial_clone) {\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> +\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(&to_fetch, p->one);\n> +\t\t\tadd_if_missing(&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\tfetch_objects(repository_format_partial_clone,\n> +\t\t\t\t to_fetch.oid, to_fetch.nr);\n> +\t\toid_array_clear(&to_fetch);\n> +\t}\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);\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n\nAlso: thank you very much for introducing this test script, to make sure\nthat things work as expected. Without it, we would not have detected any\nregression wrt multi-promisor.\n\nThis iteration looks good to me, thank you so much!\nDscho\n\n> index 0000000000..349851be7d\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,103 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo c >server/c &&\n> +\techo d >server/d &&\n> +\tgit -C server add c d &&\n> +\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> +\ttest_when_finished \"rm -rf server client trace\" &&\n> +\n> +\ttest_create_repo server &&\n> +\techo a >server/a &&\n> +\techo b >server/b &&\n> +\tgit -C server add a b &&\n> +\tgit -C server commit -m x &&\n> +\techo another-a >server/a &&\n> +\tgit -C server add a &&\n> +\tgit -C server commit -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> +\techo a | git hash-object --stdin >hash-old-a &&\n> +\techo another-a | git hash-object --stdin >hash-new-a &&\n> +\techo b | git hash-object --stdin >hash-b &&\n> +\n> +\t# Ensure that only a and another-a are fetched.\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> +\tgrep \"want $(cat hash-old-a)\" trace &&\n> +\tgrep \"want $(cat hash-new-a)\" trace &&\n> +\t! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_expect_success 'diff with rename detection batches blobs' '\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\\nbX\\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 that there is exactly 1 negotiation by checking that there is\n> +\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> +\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n> +\tgrep \"similarity index\" out &&\n> +\tgrep \"git> done\" trace >done_lines &&\n> +\ttest_line_count = 1 done_lines\n> +'\n> +\n> +test_done\n> --\n> 2.21.0.197.gd478713db0\n>\n>\n"},{"id":"373222","messageId":"20190405170934.20441-1-jonathantanmy@google.com","threadId":"50824","inReplyTo":"CACsJy8CgXLZxqab4vcP1jh3OMCGh1i=easb5BpCs1J8Uf_jsxw@mail.gmail.com","subject":"[PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2019-04-05T17:09:34Z","receivedAt":"2019-04-05T17:09:41Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This is a fixup on the tip of jt/batch-fetch-blobs-in-diff (571debe1d9).\n\nI don't know if Junio has already merged this branch to next (he marked\nthis as \"Will merge to 'next'\" in the \"What's Cooking\" email, but when I\nfetched, it hasn't been merged yet). If he has, we can use this commit\nmessage:\n\ndiff: propagate options->repo to add_if_missing\n\nAvoid a usage of the_repository by propagating the configured repository\nto add_if_missing(). Also, prefetch only if the repository being diffed\nis the_repository (because we do not support lazy fetching for any other\nrepository anyway).\n\nSigned-off-by: Jonathan Tan \n---\nThanks, Duy, for noticing this.\n---\n diff.c | 11 ++++++-----\n 1 file changed, 6 insertions(+), 5 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex 1eccefb4ef..811afbdfb1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -6367,18 +6367,19 @@ void diffcore_fix_diff_index(void)\n \tQSORT(q->queue, q->nr, diffnamecmp);\n }\n \n-static void add_if_missing(struct oid_array *to_fetch,\n+static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n \t\t\t const struct diff_filespec *filespec)\n {\n \tif (filespec && filespec->oid_valid &&\n-\t oid_object_info_extended(the_repository, &filespec->oid, NULL,\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_std(struct diff_options *options)\n {\n-\tif (repository_format_partial_clone) {\n+\tif (options->repo == the_repository &&\n+\t repository_format_partial_clone) {\n \t\t/*\n \t\t * Prefetch the diff pairs that are about to be flushed.\n \t\t */\n@@ -6388,8 +6389,8 @@ void diffcore_std(struct diff_options *options)\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(&to_fetch, p->one);\n-\t\t\tadd_if_missing(&to_fetch, p->two);\n+\t\t\tadd_if_missing(&to_fetch, options->repo, p->one);\n+\t\t\tadd_if_missing(&to_fetch, options->repo, p->two);\n \t\t}\n \t\tif (to_fetch.nr)\n \t\t\t/*\n-- \n2.21.0.392.gf8f6787159e-goog\n\n"},{"id":"373254","messageId":"nycvar.QRO.7.76.6.1904052214280.41@tvgsbejvaqbjf.bet","threadId":"50824","inReplyTo":"20190405170934.20441-1-jonathantanmy@google.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2019-04-05T20:16:10Z","receivedAt":"2019-04-05T20:16:13Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Jonathan,\n\nOn Fri, 5 Apr 2019, Jonathan Tan wrote:\n\n> This is a fixup on the tip of jt/batch-fetch-blobs-in-diff (571debe1d9).\n>\n> I don't know if Junio has already merged this branch to next (he marked\n> this as \"Will merge to 'next'\" in the \"What's Cooking\" email, but when I\n> fetched, it hasn't been merged yet). If he has, we can use this commit\n> message:\n>\n> diff: propagate options->repo to add_if_missing\n>\n> Avoid a usage of the_repository by propagating the configured repository\n> to add_if_missing(). Also, prefetch only if the repository being diffed\n> is the_repository (because we do not support lazy fetching for any other\n> repository anyway).\n\nTrue, and the introduction of `has_promisor_remotes()` should probably\nhelp that, by introducing a parameter of type `struct repository *r`.\n\n> Signed-off-by: Jonathan Tan \n\nThe patch and the commit message look good!\n\nThanks,\nDscho\n\n> diff --git a/diff.c b/diff.c\n> index 1eccefb4ef..811afbdfb1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6367,18 +6367,19 @@ void diffcore_fix_diff_index(void)\n> \tQSORT(q->queue, q->nr, diffnamecmp);\n> }\n>\n> -static void add_if_missing(struct oid_array *to_fetch,\n> +static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n> \t\t\t const struct diff_filespec *filespec)\n> {\n> \tif (filespec && filespec->oid_valid &&\n> -\t oid_object_info_extended(the_repository, &filespec->oid, NULL,\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_std(struct diff_options *options)\n> {\n> -\tif (repository_format_partial_clone) {\n> +\tif (options->repo == the_repository &&\n> +\t repository_format_partial_clone) {\n> \t\t/*\n> \t\t * Prefetch the diff pairs that are about to be flushed.\n> \t\t */\n> @@ -6388,8 +6389,8 @@ void diffcore_std(struct diff_options *options)\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(&to_fetch, p->one);\n> -\t\t\tadd_if_missing(&to_fetch, p->two);\n> +\t\t\tadd_if_missing(&to_fetch, options->repo, p->one);\n> +\t\t\tadd_if_missing(&to_fetch, options->repo, p->two);\n> \t\t}\n> \t\tif (to_fetch.nr)\n> \t\t\t/*\n> --\n> 2.21.0.392.gf8f6787159e-goog\n>\n>\n"},{"id":"373268","messageId":"20190405220005.GA10312@sigill.intra.peff.net","threadId":"50824","inReplyTo":"068861632b85179d2a5a5ceb966e951a78b27141.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] sha1-file: support OBJECT_INFO_FOR_PREFETCH","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T22:00:06Z","receivedAt":"2019-04-05T22:00:09Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2019 at 02:39:27PM -0700, Jonathan Tan wrote:\n\n> Teach oid_object_info_extended() to support a new flag that inhibits\n> fetching of missing objects. This is equivalent to setting\n> fetch_is_missing to 0, calling oid_object_info_extended(), then setting\n> fetch_if_missing to whatever it was before. Update unpack-trees.c to use\n> this new flag instead of repeatedly setting fetch_if_missing.\n> \n> This new flag complicates things slightly in that there are now 2 ways\n> to do the same thing. But this eliminates the need to repeatedly set a\n> global variable, and more importantly, allows prefetching to be done in\n> parallel (in the future); hence, this patch.\n\nSorry I'm a little late to review this. I don't have any critical\ncomments, so if this gets ignored, I'll live with it.\n\n> +/*\n> + * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> + * nonzero). This is meant for bulk prefetching of missing blobs in a partial\n> + * clone. Implies OBJECT_INFO_QUICK.\n> + */\n> +#define OBJECT_INFO_FOR_PREFETCH (32 + OBJECT_INFO_QUICK)\n\nMostly I found the name and semantics of this flag to be a little\nconfusing. Really what we want is to tell oid_object_info() not do any\non-demand fetching for us. That seems like a thing that we might\neventually want for other purposes (e.g., a diff operation that could\nproduce a real blob diff but would be happy outputting a less-detailed\ntree diff).\n\nIf it were just OBJECT_INFO_NO_FETCH or similar, that tells more clearly\nwhat it does, and would make sense in more contexts.\n\nI suspect that QUICK would be the norm when used with it, though I\nprobably would have kept the two orthogonal for the sake of simplicity\nand clarity.\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 22c41a3ba8..381b0cd65e 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -404,20 +404,21 @@ static int check_updates(struct unpack_trees_options *o)\n> \t\t * below.\n> \t\t */\n> \t\tstruct oid_array to_fetch = OID_ARRAY_INIT;\n> -\t\tint fetch_if_missing_store = fetch_if_missing;\n> -\t\tfetch_if_missing = 0;\n> \t\tfor (i = 0; i < index->cache_nr; i++) {\n> \t\t\tstruct cache_entry *ce = index->cache[i];\n> -\t\t\tif ((ce->ce_flags & CE_UPDATE) &&\n> -\t\t\t !S_ISGITLINK(ce->ce_mode)) {\n> -\t\t\t\tif (!has_object_file(&ce->oid))\n> -\t\t\t\t\toid_array_append(&to_fetch, &ce->oid);\n> -\t\t\t}\n> +\n> +\t\t\tif (!(ce->ce_flags & CE_UPDATE) ||\n> +\t\t\t S_ISGITLINK(ce->ce_mode))\n> +\t\t\t\tcontinue;\n> +\t\t\tif (!oid_object_info_extended(the_repository, &ce->oid,\n> +\t\t\t\t\t\t NULL,\n> +\t\t\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n> +\t\t\t\tcontinue;\n> +\t\t\toid_array_append(&to_fetch, &ce->oid);\n\nHere we get rid of the global set/restore dance, which is nice. But\nthere's also a behavior change, as we've picked up QUICK. I think that's\nprobably the right thing to do, but I was a bit surprised not to see any\ndiscussion in the commit message.\n\n-Peff\n"},{"id":"373269","messageId":"20190405221214.GB10312@sigill.intra.peff.net","threadId":"50824","inReplyTo":"cover.1553895166.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/2] Batch fetching of missing blobs in diff and show","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-05T22:12:15Z","receivedAt":"2019-04-05T22:12:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Mar 29, 2019 at 02:39:26PM -0700, Jonathan Tan wrote:\n\n> Thanks, everyone for the review.\n> \n> Changes from v1:\n> - used test_when_finished (Szeder)\n> - used flag to inhibit fetching of missing objects (Dscho)\n> - moved the prefetch so that it also works if we request rename\n> detection, and included a test demonstrating that (not sure if that\n> was what Peff meant)\n\nYes, putting it in diffcore_std() is probably OK. There may be one or\ntwo code paths that don't use that, but arguably they should, and this\nwill flush them out.\n\nYou could possibly get away with fetching less at that stage, since\nrenames would only need to see deleted/added files. But getting the\nminimal set gets ugly quickly, as you have to take into account copy\nsettings, or even which ones we found exact matches for (where we might\nnot even show a content diff at all).\n\nI think it's OK for this to be an approximation, and if anybody feels\nlike trying to narrow it down later, they can.\n\n> Peff also suggested that I use an oidset instead of an oidarray in order\n> to eliminate duplicates, but that makes it difficult to interface with\n> fetch_objects(), which takes a pointer and a length (which is\n> convenient, because if we want to fetch a single object, we can just\n> point to it and use a length of 1). Maybe the best idea is to have\n> oidset maintain its own OID array, and have each hash entry store an\n> index instead of an OID. For now I added a NEEDSWORK.\n\nThis is probably OK. There are two reasons to deduplicate:\n\n 1. If we expect a _lot_ of duplicates, it can be faster to de-dup as\n we go, rather than building up a giant list full of redundant\n entries (even if we later sort and de-dup it).\n\n But I don't think that's the case here. We'd expect relatively few\n duplicates in a single diff.\n\n 2. If the thing we feed the result to does not handle duplicates well.\n I'm not sure how fetch_objects() does with duplicates.\n\nIf we just care about (2), then an easy fix is to just ask oid-array to\ndedup. It's only mechanism right now is a for_each_unique() method,\nwhich is not used by fetch_objects(). But it would be easy to teach it\nto de-dup in place, like (completely untested):\n\ndiff --git a/sha1-array.c b/sha1-array.c\nindex d922e94e3f..aaea02e51e 100644\n--- a/sha1-array.c\n+++ b/sha1-array.c\n@@ -78,6 +78,23 @@ int oid_array_for_each_unique(struct oid_array *array,\n \treturn 0;\n }\n \n+int oid_array_dedup(struct oid_array *array)\n+{\n+\tint i, j;\n+\n+\tif (!array->sorted)\n+\t\toid_array_sort(array);\n+\n+\tfor (i = 0; i < array->nr; i++) {\n+\t\tif (i > 0 && oideq(array->oid + i, array->oid + i - 1))\n+\t\t\tcontinue;\n+\t\tif (i == j)\n+\t\t\tj++;\n+\t\telse\n+\t\t\tarray->oid[j++] = i;\n+\t}\n+}\n+\n void oid_array_filter(struct oid_array *array,\n \t\t for_each_oid_fn want,\n \t\t void *cb_data)\n\nThat could also be built on oid_array_for_each_unique(), but dealing\nwith the callbacks is probably worse than just reimplementing the few\nlines of logic.\n\n-Peff\n"},{"id":"373275","messageId":"CACsJy8AjyY1Azbf4VfnJMA3O8NNBV1P16dZRrHOUJdsYQHUu4g@mail.gmail.com","threadId":"50824","inReplyTo":"20190405170934.20441-1-jonathantanmy@google.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-06T04:17:46Z","receivedAt":"2019-04-06T04:21:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Apr 6, 2019 at 12:09 AM Jonathan Tan wrote:\n>\n> This is a fixup on the tip of jt/batch-fetch-blobs-in-diff (571debe1d9).\n>\n> I don't know if Junio has already merged this branch to next (he marked\n> this as \"Will merge to 'next'\" in the \"What's Cooking\" email, but when I\n> fetched, it hasn't been merged yet). If he has, we can use this commit\n> message:\n>\n> diff: propagate options->repo to add_if_missing\n>\n> Avoid a usage of the_repository by propagating the configured repository\n> to add_if_missing(). Also, prefetch only if the repository being diffed\n> is the_repository (because we do not support lazy fetching for any other\n> repository anyway).\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> Thanks, Duy, for noticing this.\n> ---\n> diff.c | 11 ++++++-----\n> 1 file changed, 6 insertions(+), 5 deletions(-)\n>\n> diff --git a/diff.c b/diff.c\n> index 1eccefb4ef..811afbdfb1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -6367,18 +6367,19 @@ void diffcore_fix_diff_index(void)\n> QSORT(q->queue, q->nr, diffnamecmp);\n> }\n>\n> -static void add_if_missing(struct oid_array *to_fetch,\n> +static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n\nOne last nit. \"struct repository *r\" is often the first argument. See\nhttps://public-inbox.org/git/xmqqsh2p6l43.fsf@gitster-ct.c.googlers.com/\n\n> const struct diff_filespec *filespec)\n> {\n> if (filespec && filespec->oid_valid &&\n> - oid_object_info_extended(the_repository, &filespec->oid, NULL,\n> + oid_object_info_extended(r, &filespec->oid, NULL,\n> OBJECT_INFO_FOR_PREFETCH))\n> oid_array_append(to_fetch, &filespec->oid);\n> }\n>\n> void diffcore_std(struct diff_options *options)\n> {\n> - if (repository_format_partial_clone) {\n> + if (options->repo == the_repository &&\n> + repository_format_partial_clone) {\n> /*\n> * Prefetch the diff pairs that are about to be flushed.\n> */\n> @@ -6388,8 +6389,8 @@ void diffcore_std(struct diff_options *options)\n>\n> for (i = 0; i < q->nr; i++) {\n> struct diff_filepair *p = q->queue[i];\n> - add_if_missing(&to_fetch, p->one);\n> - add_if_missing(&to_fetch, p->two);\n> + add_if_missing(&to_fetch, options->repo, p->one);\n> + add_if_missing(&to_fetch, options->repo, p->two);\n> }\n> if (to_fetch.nr)\n> /*\n> --\n> 2.21.0.392.gf8f6787159e-goog\n>\n\n\n-- \nDuy\n"},{"id":"373343","messageId":"CAP8UFD1qK61MtJ-XeMuwY4W2V0CdHeZQC9rQap1M+nEeaLmBJQ@mail.gmail.com","threadId":"50824","inReplyTo":"nycvar.QRO.7.76.6.1904051528130.41@tvgsbejvaqbjf.bet","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-07T06:00:08Z","receivedAt":"2019-04-07T06:00:24Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Hi,\n\nOn Fri, Apr 5, 2019 at 3:38 PM Johannes Schindelin\n wrote:\n\n> On Thu, 4 Apr 2019, SZEDER Gábor wrote:\n>\n> > On Fri, Mar 29, 2019 at 02:39:28PM -0700, Jonathan Tan wrote:\n> > > diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> > > new file mode 100755\n> > > index 0000000000..349851be7d\n> > > --- /dev/null\n> > > +++ b/t/t4067-diff-partial-clone.sh\n> > > @@ -0,0 +1,103 @@\n> > > +#!/bin/sh\n> > > +\n> > > +test_description='behavior of diff when reading objects in a partial clone'\n> > > +\n> > > +. ./test-lib.sh\n> > > +\n> > > +test_expect_success 'git show batches blobs' '\n> > > + test_when_finished \"rm -rf server client trace\" &&\n> > > +\n> > > + test_create_repo server &&\n> > > + echo a >server/a &&\n> > > + echo b >server/b &&\n> > > + git -C server add a b &&\n> > > + git -C server commit -m x &&\n> > > +\n> > > + test_config -C server uploadpack.allowfilter 1 &&\n> > > + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> > > + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> > > +\n> > > + # Ensure that there is exactly 1 negotiation by checking that there is\n> > > + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> > > + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> > > + grep \"git> done\" trace >done_lines &&\n> > > + test_line_count = 1 done_lines\n> >\n> > These patches and 'cc/multi-promisor' don't seem to work well\n> > together, and all tests checking 'test_line_count = 1 done_lines' in\n> > this test script fail in current 'pu', because there are two\n> > \"git> done\" lines.\n>\n> I investigated a little further, and it would seem that it is neither this\n> patch nor the cc/multi-promisor patches that introduce the problem, but\n> the merge between the two... The latter tries to get away from using the\n> global variable `repository_format_partial_clone` while this patch\n> introduces another user.\n\nThanks for investigating! Yeah, that's part of the problem.\n\nThe fix I would suggest is:\n\ndiff --git a/diff.c b/diff.c\nindex f685ab10b5..a2b1241f83 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -26,6 +26,7 @@\n #include \"parse-options.h\"\n #include \"help.h\"\n #include \"fetch-object.h\"\n+#include \"promisor-remote.h\"\n\n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6489,7 +6490,7 @@ static void add_if_missing(struct oid_array *to_fetch,\n\n void diffcore_std(struct diff_options *options)\n {\n- if (repository_format_partial_clone) {\n+ if (has_promisor_remote()) {\n /*\n * Prefetch the diff pairs that are about to be flushed.\n */\n@@ -6506,8 +6507,7 @@ void diffcore_std(struct diff_options *options)\n /*\n * NEEDSWORK: Consider deduplicating the OIDs sent.\n */\n- fetch_objects(repository_format_partial_clone,\n- to_fetch.oid, to_fetch.nr);\n+ promisor_remote_get_direct(to_fetch.oid, to_fetch.nr);\n oid_array_clear(&to_fetch);\n }\n\nI will send a new version with the above soon based on top of\njt/batch-fetch-blobs-in-diff in pu.\n"},{"id":"373385","messageId":"xmqqy34lb4vb.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"CAP8UFD1qK61MtJ-XeMuwY4W2V0CdHeZQC9rQap1M+nEeaLmBJQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T02:36:08Z","receivedAt":"2019-04-08T02:36:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Thanks for investigating! Yeah, that's part of the problem.\n>\n> The fix I would suggest is:\n>\n> diff --git a/diff.c b/diff.c\n> index f685ab10b5..a2b1241f83 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -26,6 +26,7 @@\n> #include \"parse-options.h\"\n> #include \"help.h\"\n> #include \"fetch-object.h\"\n> +#include \"promisor-remote.h\"\n\nThanks.\n\n>\n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -6489,7 +6490,7 @@ static void add_if_missing(struct oid_array *to_fetch,\n>\n> void diffcore_std(struct diff_options *options)\n> {\n> - if (repository_format_partial_clone) {\n> + if (has_promisor_remote()) {\n\nHmph, I see quite a few references to the variable disappears\nbetween next and pu. Is it that in the new world order, nobody\noutside the low-level object-access layer should look at the\nvariable directly, but instead ask the has_promisor_remote()\nfunction? If so, can we at least document that? Making it static\n(or at least renaming it) would have helped the compiler to notice\nthis semantic merge conflict better.\n\n> @@ -6506,8 +6507,7 @@ void diffcore_std(struct diff_options *options)\n> /*\n> * NEEDSWORK: Consider deduplicating the OIDs sent.\n> */\n> - fetch_objects(repository_format_partial_clone,\n> - to_fetch.oid, to_fetch.nr);\n> + promisor_remote_get_direct(to_fetch.oid, to_fetch.nr);\n\nLikewise between fetch_objects() and promisor_remote_get_direct().\nShouldn't the underlying fetch_objects be hidden from general\ncallers?\n\n> oid_array_clear(&to_fetch);\n> }\n>\n> I will send a new version with the above soon based on top of\n> jt/batch-fetch-blobs-in-diff in pu.\n"},{"id":"373387","messageId":"xmqqsgutb1m9.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"CACsJy8AjyY1Azbf4VfnJMA3O8NNBV1P16dZRrHOUJdsYQHUu4g@mail.gmail.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T03:46:22Z","receivedAt":"2019-04-08T03:46:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> -static void add_if_missing(struct oid_array *to_fetch,\n>> +static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n>\n> One last nit. \"struct repository *r\" is often the first argument. See\n\nYes, and that's easy enough to tweak locally ;-)\n\nThanks.\n\n"},{"id":"373388","messageId":"xmqqmul1b0pg.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"CACsJy8AjyY1Azbf4VfnJMA3O8NNBV1P16dZRrHOUJdsYQHUu4g@mail.gmail.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T04:06:03Z","receivedAt":"2019-04-08T04:06:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n>> Avoid a usage of the_repository by propagating the configured repository\n>> to add_if_missing(). Also, prefetch only if the repository being diffed\n>> is the_repository (because we do not support lazy fetching for any other\n>> repository anyway).\n\nIf we are willing to stay limited to the default repository anyway,\nallowing add_if_missing() to take an arbitrary repository does not\nreally matter, but before the caller of add_if_missing() befcomes\nready to work on an arbitrary repository, this change has to happen.\n\nTo update the caller, it seems to me that fetch_objects() must learn\nto take an arbitrary repository, but is that the only thing needed?\nAfter that, the function that the caller resides in and callchain\nupwards can learn to take a repository instance if we want to be\nable to diff inside an arbitrary repository.\n\nBut. Such a change still would not allow us to compare a tree in\none repository against a tree in another repository. It is likely\nthat a caller with such a need would simply make sure that objects\nin both repositories are available by using the in-core alternate\nobject store mechanism, making it a more-or-less moot point to be\nable to pass a repository instance through the callchain X-<. We\nprobably should make it, and spell it out somewhere in a long term\nvision shared among the developers, an explicit goal to get rid of\nthe internal (ab)use of the alternate object store mechanism.\n\nWith squashing the fix-up commit in, the 2/2 patch has become like\nso.\n\nThanks, both.\n\n-- >8 --\nFrom: Jonathan Tan \nDate: Fri, 5 Apr 2019 10:09:34 -0700\nSubject: [PATCH] diff: batch fetching of missing blobs\n\nWhen running a command like \"git show\" or \"git diff\" in a partial clone,\nbatch all missing blobs to be fetched as one request.\n\nThis is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\nblobs\", 2017-12-08), but for another command.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n diff.c | 33 +++++++++++\n t/t4067-diff-partial-clone.sh | 103 ++++++++++++++++++++++++++++++++++\n 2 files changed, 136 insertions(+)\n create mode 100755 t/t4067-diff-partial-clone.sh\n\ndiff --git a/diff.c b/diff.c\nindex ec5c095199..811afbdfb1 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -25,6 +25,7 @@\n #include \"packfile.h\"\n #include \"parse-options.h\"\n #include \"help.h\"\n+#include \"fetch-object.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -6366,8 +6367,40 @@ void diffcore_fix_diff_index(void)\n \tQSORT(q->queue, q->nr, diffnamecmp);\n }\n \n+static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n+\t\t\t const struct diff_filespec *filespec)\n+{\n+\tif (filespec && filespec->oid_valid &&\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_std(struct diff_options *options)\n {\n+\tif (options->repo == the_repository &&\n+\t repository_format_partial_clone) {\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+\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(&to_fetch, options->repo, p->one);\n+\t\t\tadd_if_missing(&to_fetch, options->repo, 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\tfetch_objects(repository_format_partial_clone,\n+\t\t\t\t to_fetch.oid, to_fetch.nr);\n+\t\toid_array_clear(&to_fetch);\n+\t}\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/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\nnew file mode 100755\nindex 0000000000..90c8fb2901\n--- /dev/null\n+++ b/t/t4067-diff-partial-clone.sh\n@@ -0,0 +1,103 @@\n+#!/bin/sh\n+\n+test_description='behavior of diff when reading objects in a partial clone'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'git show batches blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff batches blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo c >server/c &&\n+\techo d >server/d &&\n+\tgit -C server add c d &&\n+\tgit -C server commit -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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_expect_success 'diff skips same-OID blobs' '\n+\ttest_when_finished \"rm -rf server client trace\" &&\n+\n+\ttest_create_repo server &&\n+\techo a >server/a &&\n+\techo b >server/b &&\n+\tgit -C server add a b &&\n+\tgit -C server commit -m x &&\n+\techo another-a >server/a &&\n+\tgit -C server add a &&\n+\tgit -C server commit -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+\techo a | git hash-object --stdin >hash-old-a &&\n+\techo another-a | git hash-object --stdin >hash-new-a &&\n+\techo b | git hash-object --stdin >hash-b &&\n+\n+\t# Ensure that only a and another-a are fetched.\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n+\tgrep \"want $(cat hash-old-a)\" trace &&\n+\tgrep \"want $(cat hash-new-a)\" trace &&\n+\t! grep \"want $(cat hash-b)\" trace\n+'\n+\n+test_expect_success 'diff with rename detection batches blobs' '\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\\nbX\\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 that there is exactly 1 negotiation by checking that there is\n+\t# only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n+\tgrep \"similarity index\" out &&\n+\tgrep \"git> done\" trace >done_lines &&\n+\ttest_line_count = 1 done_lines\n+'\n+\n+test_done\n-- \n2.21.0-196-g041f5ea1cf\n\n"},{"id":"373396","messageId":"xmqqmul19h9j.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"xmqqy34lb4vb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T05:51:20Z","receivedAt":"2019-04-08T05:51:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Christian Couder writes:\n>\n>> Thanks for investigating! Yeah, that's part of the problem.\n>>\n>> The fix I would suggest is:\n>>\n>> diff --git a/diff.c b/diff.c\n>> index f685ab10b5..a2b1241f83 100644\n>> --- a/diff.c\n>> +++ b/diff.c\n>> @@ -26,6 +26,7 @@\n>> #include \"parse-options.h\"\n>> #include \"help.h\"\n>> #include \"fetch-object.h\"\n>> +#include \"promisor-remote.h\"\n>\n> Thanks.\n\nTogether with \"if we are forbidding the direct access to the\nrepository_format_partial_clone variable and the fetch_objects()\nfunciton, make it clear that is what is being done in the\nmulti-promisor topic\", I think a patch that adds this header should\nalso remove inclusion of \"fetch-object.h\".\n\n"},{"id":"373397","messageId":"xmqqftqt9gpz.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"xmqqmul19h9j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T06:03:04Z","receivedAt":"2019-04-08T06:03:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>>> #include \"fetch-object.h\"\n>>> +#include \"promisor-remote.h\"\n>>\n>> Thanks.\n>\n> Together with \"if we are forbidding the direct access to the\n> repository_format_partial_clone variable and the fetch_objects()\n> funciton, make it clear that is what is being done in the\n> multi-promisor topic\", I think a patch that adds this header should\n> also remove inclusion of \"fetch-object.h\".\n\nIn fact, your topic itself has the same issue. I'll queue the\nfollowing at the tip of the topic tentatively before merging it to\n'pu' with the fix we have been discussing around this thread.\n\nThanks.\n\n-- >8 --\nIn multi-promisor world, fetch_objects() should not be used\ndirectly; promisor_remote_get_direct() call replaces the helper.\n\n sha1-file.c | 1 -\n unpack-trees.c | 1 -\n 2 files changed, 2 deletions(-)\n\ndiff --git a/sha1-file.c b/sha1-file.c\nindex 715a2b882a..87ea8606f4 100644\n--- a/sha1-file.c\n+++ b/sha1-file.c\n@@ -30,7 +30,6 @@\n #include \"mergesort.h\"\n #include \"quote.h\"\n #include \"packfile.h\"\n-#include \"fetch-object.h\"\n #include \"object-store.h\"\n #include \"promisor-remote.h\"\n \ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 47353d85c3..ca0ab68c32 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -16,7 +16,6 @@\n #include \"submodule-config.h\"\n #include \"fsmonitor.h\"\n #include \"object-store.h\"\n-#include \"fetch-object.h\"\n #include \"promisor-remote.h\"\n \n /*\n-- \n2.21.0-196-g041f5ea1cf\n\n"},{"id":"373398","messageId":"CAP8UFD0gmaZzfK7taS=1hj=sCEpLFt_Az60TxYeUqp2A7r25JQ@mail.gmail.com","threadId":"50824","inReplyTo":"xmqqy34lb4vb.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-08T06:40:23Z","receivedAt":"2019-04-08T06:40:40Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 8, 2019 at 4:36 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > #ifdef NO_FAST_WORKING_DIRECTORY\n> > #define FAST_WORKING_DIRECTORY 0\n> > @@ -6489,7 +6490,7 @@ static void add_if_missing(struct oid_array *to_fetch,\n> >\n> > void diffcore_std(struct diff_options *options)\n> > {\n> > - if (repository_format_partial_clone) {\n> > + if (has_promisor_remote()) {\n>\n> Hmph, I see quite a few references to the variable disappears\n> between next and pu. Is it that in the new world order, nobody\n> outside the low-level object-access layer should look at the\n> variable directly, but instead ask the has_promisor_remote()\n> function?\n\nYeah, repository_format_partial_clone contains the remote configured\nin extensions.partialclone, while in the new world order there can be\nother promisor remotes than this one, and most of the code is only\ninterested in asking if we use any promisor remote.\n\n> If so, can we at least document that? Making it static\n> (or at least renaming it) would have helped the compiler to notice\n> this semantic merge conflict better.\n\nOk, I will see if I can make it static (or maybe rename it).\n\nPatch 3f82acbca2 (Use promisor_remote_get_direct() and\nhas_promisor_remote(), 2019-04-01) in cc/multi-promisor starts with:\n\n Use promisor_remote_get_direct() and has_promisor_remote()\n\n Instead of using the repository_format_partial_clone global\n and fetch_objects() directly, let's use has_promisor_remote()\n and promisor_remote_get_direct().\n\n This way all the configured promisor remotes will be taken\n into account, not only the one specified by\n extensions.partialClone.\n\nI will at least add something telling that in most cases\n\"repository_format_partial_clone\" and fetch_objects() shouldn't be\nused directly anymore.\n\n> > @@ -6506,8 +6507,7 @@ void diffcore_std(struct diff_options *options)\n> > /*\n> > * NEEDSWORK: Consider deduplicating the OIDs sent.\n> > */\n> > - fetch_objects(repository_format_partial_clone,\n> > - to_fetch.oid, to_fetch.nr);\n> > + promisor_remote_get_direct(to_fetch.oid, to_fetch.nr);\n>\n> Likewise between fetch_objects() and promisor_remote_get_direct().\n> Shouldn't the underlying fetch_objects be hidden from general\n> callers?\n\nI will see if I can do that, though it has to be used in promisor-remote.c.\n"},{"id":"373400","messageId":"CAP8UFD15V1yskNzPjROCcOvarz34NdwOoSM2JRh3zNJhNeXA=g@mail.gmail.com","threadId":"50824","inReplyTo":"xmqqftqt9gpz.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-08T06:45:46Z","receivedAt":"2019-04-08T06:46:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 8, 2019 at 8:03 AM Junio C Hamano wrote:\n>\n> Junio C Hamano writes:\n>\n> >>> #include \"fetch-object.h\"\n> >>> +#include \"promisor-remote.h\"\n> >>\n> >> Thanks.\n> >\n> > Together with \"if we are forbidding the direct access to the\n> > repository_format_partial_clone variable and the fetch_objects()\n> > funciton, make it clear that is what is being done in the\n> > multi-promisor topic\", I think a patch that adds this header should\n> > also remove inclusion of \"fetch-object.h\".\n>\n> In fact, your topic itself has the same issue.\n\nYeah sorry, this is the kind of things I easily forget.\n\n> I'll queue the\n> following at the tip of the topic tentatively before merging it to\n> 'pu' with the fix we have been discussing around this thread.\n\nThanks!\n"},{"id":"373404","messageId":"xmqqbm1h7wrj.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"CAP8UFD0gmaZzfK7taS=1hj=sCEpLFt_Az60TxYeUqp2A7r25JQ@mail.gmail.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-08T07:59:28Z","receivedAt":"2019-04-08T07:59:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Christian Couder writes:\n\n> Patch 3f82acbca2 (Use promisor_remote_get_direct() and\n> has_promisor_remote(), 2019-04-01) in cc/multi-promisor starts with:\n>\n> Use promisor_remote_get_direct() and has_promisor_remote()\n>\n> Instead of using the repository_format_partial_clone global\n> and fetch_objects() directly, let's use has_promisor_remote()\n> and promisor_remote_get_direct().\n>\n> This way all the configured promisor remotes will be taken\n> into account, not only the one specified by\n> extensions.partialClone.\n>\n> I will at least add something telling that in most cases\n> \"repository_format_partial_clone\" and fetch_objects() shouldn't be\n> used directly anymore.\n\nYes, that would be necessary not in the log, but in-code comments\nnext to now \"by-exception-only\" API functions and variables.\n\n>> > @@ -6506,8 +6507,7 @@ void diffcore_std(struct diff_options *options)\n>> > /*\n>> > * NEEDSWORK: Consider deduplicating the OIDs sent.\n>> > */\n>> > - fetch_objects(repository_format_partial_clone,\n>> > - to_fetch.oid, to_fetch.nr);\n>> > + promisor_remote_get_direct(to_fetch.oid, to_fetch.nr);\n>>\n>> Likewise between fetch_objects() and promisor_remote_get_direct().\n>> Shouldn't the underlying fetch_objects be hidden from general\n>> callers?\n>\n> I will see if I can do that, though it has to be used in promisor-remote.c.\n\nDoes fetch-objects.[ch] even need to exist after multi-promisor\nworld as an external interface? \n\nWouldn't it become an implementation detail of promisor-remote.[ch]\nAPI?\n"},{"id":"373408","messageId":"CAP8UFD0u8fd_s5wdsO4B=j7TE1U1=j=R7=v9-=wUy78ResoZBw@mail.gmail.com","threadId":"50824","inReplyTo":"xmqqbm1h7wrj.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2 2/2] diff: batch fetching of missing blobs","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2019-04-08T09:56:16Z","receivedAt":"2019-04-08T09:56:31Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Mon, Apr 8, 2019 at 9:59 AM Junio C Hamano wrote:\n>\n> Christian Couder writes:\n>\n> > I will at least add something telling that in most cases\n> > \"repository_format_partial_clone\" and fetch_objects() shouldn't be\n> > used directly anymore.\n>\n> Yes, that would be necessary not in the log, but in-code comments\n> next to now \"by-exception-only\" API functions and variables.\n\nOk, I will add in-code comments then, or ...\n\n> >> Likewise between fetch_objects() and promisor_remote_get_direct().\n> >> Shouldn't the underlying fetch_objects be hidden from general\n> >> callers?\n> >\n> > I will see if I can do that, though it has to be used in promisor-remote.c.\n>\n> Does fetch-objects.[ch] even need to exist after multi-promisor\n> world as an external interface?\n>\n> Wouldn't it become an implementation detail of promisor-remote.[ch]\n> API?\n\n... yeah, I will try this.\n"},{"id":"373409","messageId":"CACsJy8BHqaqOHVbwtONU5=RiG7Q8WNNAN5EGV_nm7NyNWeyuiQ@mail.gmail.com","threadId":"50824","inReplyTo":"xmqqmul1b0pg.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-08T09:58:35Z","receivedAt":"2019-04-08T09:59:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 8, 2019 at 11:06 AM Junio C Hamano wrote:\n>\n> Duy Nguyen writes:\n>\n> >> Avoid a usage of the_repository by propagating the configured repository\n> >> to add_if_missing(). Also, prefetch only if the repository being diffed\n> >> is the_repository (because we do not support lazy fetching for any other\n> >> repository anyway).\n>\n> If we are willing to stay limited to the default repository anyway,\n> allowing add_if_missing() to take an arbitrary repository does not\n> really matter, but before the caller of add_if_missing() befcomes\n> ready to work on an arbitrary repository, this change has to happen.\n>\n> To update the caller, it seems to me that fetch_objects() must learn\n> to take an arbitrary repository, but is that the only thing needed?\n> After that, the function that the caller resides in and callchain\n> upwards can learn to take a repository instance if we want to be\n> able to diff inside an arbitrary repository.\n>\n> But. Such a change still would not allow us to compare a tree in\n> one repository against a tree in another repository.\n\nI feel lost (and the answer \"go read partial clone code!\" is perfectly\nacceptable) but why would we need to diff trees of two different\nrepositories?\n\n> It is likely\n> that a caller with such a need would simply make sure that objects\n> in both repositories are available by using the in-core alternate\n> object store mechanism, making it a more-or-less moot point to be\n> able to pass a repository instance through the callchain X-<. We\n> probably should make it, and spell it out somewhere in a long term\n> vision shared among the developers, an explicit goal to get rid of\n> the internal (ab)use of the alternate object store mechanism.\n\nI think submodule code so far is doing this way. Though I don't see\nany reason we need it for submodule code. Objects are not supposed to\nbe shared between the super- and the sub-repo.\n\n>\n> With squashing the fix-up commit in, the 2/2 patch has become like\n> so.\n>\n> Thanks, both.\n>\n> -- >8 --\n> From: Jonathan Tan \n> Date: Fri, 5 Apr 2019 10:09:34 -0700\n> Subject: [PATCH] diff: batch fetching of missing blobs\n>\n> When running a command like \"git show\" or \"git diff\" in a partial clone,\n> batch all missing blobs to be fetched as one request.\n>\n> This is similar to c0c578b33c (\"unpack-trees: batch fetching of missing\n> blobs\", 2017-12-08), but for another command.\n>\n> Signed-off-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> ---\n> diff.c | 33 +++++++++++\n> t/t4067-diff-partial-clone.sh | 103 ++++++++++++++++++++++++++++++++++\n> 2 files changed, 136 insertions(+)\n> create mode 100755 t/t4067-diff-partial-clone.sh\n>\n> diff --git a/diff.c b/diff.c\n> index ec5c095199..811afbdfb1 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -25,6 +25,7 @@\n> #include \"packfile.h\"\n> #include \"parse-options.h\"\n> #include \"help.h\"\n> +#include \"fetch-object.h\"\n>\n> #ifdef NO_FAST_WORKING_DIRECTORY\n> #define FAST_WORKING_DIRECTORY 0\n> @@ -6366,8 +6367,40 @@ void diffcore_fix_diff_index(void)\n> QSORT(q->queue, q->nr, diffnamecmp);\n> }\n>\n> +static void add_if_missing(struct oid_array *to_fetch, struct repository *r,\n> + const struct diff_filespec *filespec)\n> +{\n> + if (filespec && filespec->oid_valid &&\n> + oid_object_info_extended(r, &filespec->oid, NULL,\n> + OBJECT_INFO_FOR_PREFETCH))\n> + oid_array_append(to_fetch, &filespec->oid);\n> +}\n> +\n> void diffcore_std(struct diff_options *options)\n> {\n> + if (options->repo == the_repository &&\n> + repository_format_partial_clone) {\n> + /*\n> + * Prefetch the diff pairs that are about to be flushed.\n> + */\n> + int i;\n> + struct diff_queue_struct *q = &diff_queued_diff;\n> + struct oid_array to_fetch = OID_ARRAY_INIT;\n> +\n> + for (i = 0; i < q->nr; i++) {\n> + struct diff_filepair *p = q->queue[i];\n> + add_if_missing(&to_fetch, options->repo, p->one);\n> + add_if_missing(&to_fetch, options->repo, p->two);\n> + }\n> + if (to_fetch.nr)\n> + /*\n> + * NEEDSWORK: Consider deduplicating the OIDs sent.\n> + */\n> + fetch_objects(repository_format_partial_clone,\n> + to_fetch.oid, to_fetch.nr);\n> + oid_array_clear(&to_fetch);\n> + }\n> +\n> /* NOTE please keep the following in sync with diff_tree_combined() */\n> if (options->skip_stat_unmatch)\n> diffcore_skip_stat_unmatch(options);\n> diff --git a/t/t4067-diff-partial-clone.sh b/t/t4067-diff-partial-clone.sh\n> new file mode 100755\n> index 0000000000..90c8fb2901\n> --- /dev/null\n> +++ b/t/t4067-diff-partial-clone.sh\n> @@ -0,0 +1,103 @@\n> +#!/bin/sh\n> +\n> +test_description='behavior of diff when reading objects in a partial clone'\n> +\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'git show batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client show HEAD &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + echo c >server/c &&\n> + echo d >server/d &&\n> + git -C server add c d &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_expect_success 'diff skips same-OID blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + echo b >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + echo another-a >server/a &&\n> + git -C server add a &&\n> + git -C server commit -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + echo a | git hash-object --stdin >hash-old-a &&\n> + echo another-a | git hash-object --stdin >hash-new-a &&\n> + echo b | git hash-object --stdin >hash-b &&\n> +\n> + # Ensure that only a and another-a are fetched.\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff HEAD^ HEAD &&\n> + grep \"want $(cat hash-old-a)\" trace &&\n> + grep \"want $(cat hash-new-a)\" trace &&\n> + ! grep \"want $(cat hash-b)\" trace\n> +'\n> +\n> +test_expect_success 'diff with rename detection batches blobs' '\n> + test_when_finished \"rm -rf server client trace\" &&\n> +\n> + test_create_repo server &&\n> + echo a >server/a &&\n> + printf \"b\\nb\\nb\\nb\\nb\\n\" >server/b &&\n> + git -C server add a b &&\n> + git -C server commit -m x &&\n> + rm server/b &&\n> + printf \"b\\nb\\nb\\nb\\nbX\\n\" >server/c &&\n> + git -C server add c &&\n> + git -C server commit -a -m x &&\n> +\n> + test_config -C server uploadpack.allowfilter 1 &&\n> + test_config -C server uploadpack.allowanysha1inwant 1 &&\n> + git clone --bare --filter=blob:limit=0 \"file://$(pwd)/server\" client &&\n> +\n> + # Ensure that there is exactly 1 negotiation by checking that there is\n> + # only 1 \"done\" line sent. (\"done\" marks the end of negotiation.)\n> + GIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client diff -M HEAD^ HEAD >out &&\n> + grep \"similarity index\" out &&\n> + grep \"git> done\" trace >done_lines &&\n> + test_line_count = 1 done_lines\n> +'\n> +\n> +test_done\n> --\n> 2.21.0-196-g041f5ea1cf\n>\n\n\n-- \nDuy\n"},{"id":"373459","messageId":"xmqq36mr8z3e.fsf@gitster-ct.c.googlers.com","threadId":"50824","inReplyTo":"CACsJy8BHqaqOHVbwtONU5=RiG7Q8WNNAN5EGV_nm7NyNWeyuiQ@mail.gmail.com","subject":"Re: [PATCH] fixup! diff: batch fetching of missing blobs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-09T06:36:05Z","receivedAt":"2019-04-09T06:36:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> I feel lost (and the answer \"go read partial clone code!\" is perfectly\n> acceptable) but why would we need to diff trees of two different\n> repositories?\n\nIt's just a speculation into far future to open possibilities of\nwhat we cannot do right now. There is no *NEED*, just like there is\nno need for a single process of git to be working in multiple\nrepositories at the same time (you can just fork another one into\nthe other repository).\n\n\n>> It is likely\n>> that a caller with such a need would simply make sure that objects\n>> in both repositories are available by using the in-core alternate\n>> object store mechanism, making it a more-or-less moot point to be\n>> able to pass a repository instance through the callchain X-<. We\n>> probably should make it, and spell it out somewhere in a long term\n>> vision shared among the developers, an explicit goal to get rid of\n>> the internal (ab)use of the alternate object store mechanism.\n>\n> I think submodule code so far is doing this way. Though I don't see\n> any reason we need it for submodule code. Objects are not supposed to\n> be shared between the super- and the sub-repo.\n\nYup, that is why I made that comment---if we were to pass repository\npointer throughout all the codepaths, one of the goal (eh, rather, a\nuseful yardstick to measure how close we are to the goal) should be\nthat we use less and less of that pattern to access objects from\nrandom repositories using the in-core alternate mechanism.\n"}]}