{"thread":{"id":"53863","subject":"[PATCH] upload-pack: do not lazy-fetch \"have\" objects","startedAt":"2020-07-15T22:31:20Z","lastAt":"2020-07-20T17:42:05Z","messageCount":6,"participants":["Jonathan Tan","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"401610","messageId":"20200715223112.2018556-1-jonathantanmy@google.com","threadId":"53863","inReplyTo":null,"subject":"[PATCH] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-15T22:31:12Z","receivedAt":"2020-07-15T22:31:20Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When upload-pack receives a request containing \"have\" hashes, it (among\nother things) checks if the served repository has the corresponding\nobjects. However, it does not do so with the\nOBJECT_INFO_SKIP_FETCH_OBJECT flag, so if serving a partial clone, a\nlazy fetch will be triggered first.\n\nThis was discovered at $DAYJOB when a user fetched from a partial clone\n(into another partial clone - although this would also happen if the\nrepo to be fetched into is not a partial clone).\n\nTherefore, whenever \"have\" hashes are checked for existence, pass the\nOBJECT_INFO_SKIP_FETCH_OBJECT flag.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nThere is also the greater issue that if a lazy fetch fails, the fetch is\nusually fatal (and possibly always fatal - I haven't checked all the\ncode paths) when the calling code could just as easily continue without\nthe object (which is the case for upload-pack when checking \"have\"s),\nbut I haven't addressed that here.\n---\n t/t5616-partial-clone.sh | 38 ++++++++++++++++++++++++++++++++++++++\n upload-pack.c            |  5 +++--\n 2 files changed, 41 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a27452a51..37de0afb02 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -422,6 +422,44 @@ test_expect_success 'single-branch tag following respects partial clone' '\n \ttest_must_fail git -C single rev-parse --verify refs/tags/C\n '\n \n+test_expect_success 'fetch from a partial clone, protocol v0' '\n+\trm -rf server client trace &&\n+\n+\t# Pretend that the server is a partial clone\n+\tgit init server &&\n+\tgit -C server remote add a_remote \"file://$(pwd)/\" &&\n+\ttest_config -C server core.repositoryformatversion 1 &&\n+\ttest_config -C server extensions.partialclone a_remote &&\n+\ttest_config -C server protocol.version 0 &&\n+\ttest_commit -C server foo &&\n+\n+\t# Fetch from the server\n+\tgit init client &&\n+\ttest_config -C client protocol.version 0 &&\n+\ttest_commit -C client bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"file://$(pwd)/server\" &&\n+\t! grep \"version 2\" trace\n+'\n+\n+test_expect_success 'fetch from a partial clone, protocol v2' '\n+\trm -rf server client trace &&\n+\n+\t# Pretend that the server is a partial clone\n+\tgit init server &&\n+\tgit -C server remote add a_remote \"file://$(pwd)/\" &&\n+\ttest_config -C server core.repositoryformatversion 1 &&\n+\ttest_config -C server extensions.partialclone a_remote &&\n+\ttest_config -C server protocol.version 2 &&\n+\ttest_commit -C server foo &&\n+\n+\t# Fetch from the server\n+\tgit init client &&\n+\ttest_config -C client protocol.version 2 &&\n+\ttest_commit -C client bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"file://$(pwd)/server\" &&\n+\tgrep \"version 2\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 951a2b23aa..af9d621755 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -482,7 +482,7 @@ static int got_oid(struct upload_pack_data *data,\n {\n \tif (get_oid_hex(hex, oid))\n \t\tdie(\"git upload-pack: expected SHA1 object, got '%s'\", hex);\n-\tif (!has_object_file(oid))\n+\tif (!has_object_file_with_flags(oid, OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\treturn -1;\n \treturn do_got_oid(data, oid);\n }\n@@ -1423,7 +1423,8 @@ static int process_haves(struct upload_pack_data *data, struct oid_array *common\n \tfor (i = 0; i < data->haves.nr; i++) {\n \t\tconst struct object_id *oid = &data->haves.oid[i];\n \n-\t\tif (!has_object_file(oid))\n+\t\tif (!has_object_file_with_flags(oid,\n+\t\t\t\t\t\tOBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\tcontinue;\n \n \t\toid_array_append(common, oid);\n-- \n2.27.0.389.gc38d7665816-goog\n\n"},{"id":"401611","messageId":"xmqqpn8wie21.fsf@gitster.c.googlers.com","threadId":"53863","inReplyTo":"20200715223112.2018556-1-jonathantanmy@google.com","subject":"Re: [PATCH] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-15T22:55:18Z","receivedAt":"2020-07-15T22:55:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan <jonathantanmy@google.com> writes:\n\n> When upload-pack receives a request containing \"have\" hashes, it (among\n> other things) checks if the served repository has the corresponding\n> objects. However, it does not do so with the\n> OBJECT_INFO_SKIP_FETCH_OBJECT flag, so if serving a partial clone, a\n> lazy fetch will be triggered first.\n\nOK.  \n\nFixing issues hit by real users reactively is a necessary and good\nthing, but this is not the first time we patch callers of\nhas_object_file() for this kind of \"we are merely trying to\ndetermine the boundary of what we have, so that we know what we need\nto add to this repository\" queries, I am afraid.\n\nPerhaps it is a good idea to sweep all the hits from \"git grep -e\nhas_object_file \\*.c\" and audit the codebase to see if there are\nother problematic ones?\n\nFor example, list-objects.c::process_blob() tries to if the object\nexists when --exclude-promisor-objects is in effect so that it can\nreturn early if the object is missing and it is a promisor object.\nI would imagine that we would not want to lazy-fetch the object in\nthis case.\n\nThanks.  Will queue.\n\n"},{"id":"401620","messageId":"20200716104149.GA376357@coredump.intra.peff.net","threadId":"53863","inReplyTo":"xmqqpn8wie21.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-16T10:41:49Z","receivedAt":"2020-07-16T10:41:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 15, 2020 at 03:55:18PM -0700, Junio C Hamano wrote:\n\n> Fixing issues hit by real users reactively is a necessary and good\n> thing, but this is not the first time we patch callers of\n> has_object_file() for this kind of \"we are merely trying to\n> determine the boundary of what we have, so that we know what we need\n> to add to this repository\" queries, I am afraid.\n> \n> Perhaps it is a good idea to sweep all the hits from \"git grep -e\n> has_object_file \\*.c\" and audit the codebase to see if there are\n> other problematic ones?\n\nInterestingly, the case fixed here was mentioned after the three-dashes\nin this patch:\n\n  https://lore.kernel.org/git/20191127123211.GG22221@sigill.intra.peff.net/\n\nThat thread was about OBJECT_INFO_QUICK, but I think any \"we are finding\nthe boundary\" hunt should consider whether that's appropriate, too. In\nthat case they are really the same issue: we do not want to expend\neffort A on the unlikely chance P that we will find the object and save\nourselves effort B. The magnitude of A and B, and the probability P may\nbe different for the two optimizations, though.\n\nI think the instance fixed here _probably_ could be converted to QUICK\nas well. It saves extra refreshes of the pack directory when a client\nasks for an object we don't have. That's usually not too expensive, but\ncan be in some cases (many alternates, slow NFS filesystems, client has\nmany objects we don't). In the worst case, using QUICK would mean we can\nget fooled by a simultaneous repack into thinking we don't have an\nobject we _do_ have, and end up negotiating a worse pack for the client.\n\n> Thanks.  Will queue.\n\nThat said, yeah, I think Jonathan's patch is a strict improvement by\nitself.\n\n-Peff\n"},{"id":"401655","messageId":"xmqqk0z3gy5y.fsf@gitster.c.googlers.com","threadId":"53863","inReplyTo":"20200716104149.GA376357@coredump.intra.peff.net","subject":"Re: [PATCH] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-07-16T17:36:09Z","receivedAt":"2020-07-16T17:36:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think the instance fixed here _probably_ could be converted to QUICK\n> as well. It saves extra refreshes of the pack directory when a client\n> asks for an object we don't have. That's usually not too expensive, but\n> can be in some cases (many alternates, slow NFS filesystems, client has\n> many objects we don't). In the worst case, using QUICK would mean we can\n> get fooled by a simultaneous repack into thinking we don't have an\n> object we _do_ have, and end up negotiating a worse pack for the client.\n>\n>> Thanks.  Will queue.\n>\n> That said, yeah, I think Jonathan's patch is a strict improvement by\n> itself.\n\nI'll see what Jonathan would say, hoping that he'd consider if QUICK\nis appropriate or not sufficiently enough to result in an updated\npatch, that either has QUICK in the code or the reason why QUICK is\nnot used in the log message.\n\nThanks.\n"},{"id":"401658","messageId":"20200716180950.2852753-1-jonathantanmy@google.com","threadId":"53863","inReplyTo":"xmqqk0z3gy5y.fsf@gitster.c.googlers.com","subject":"[PATCH v2] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2020-07-16T18:09:50Z","receivedAt":"2020-07-16T18:10:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When upload-pack receives a request containing \"have\" hashes, it (among\nother things) checks if the served repository has the corresponding\nobjects. However, it does not do so with the\nOBJECT_INFO_SKIP_FETCH_OBJECT flag, so if serving a partial clone, a\nlazy fetch will be triggered first.\n\nThis was discovered at $DAYJOB when a user fetched from a partial clone\n(into another partial clone - although this would also happen if the\nrepo to be fetched into is not a partial clone).\n\nTherefore, whenever \"have\" hashes are checked for existence, pass the\nOBJECT_INFO_SKIP_FETCH_OBJECT flag. Also add the OBJECT_INFO_QUICK flag\nto improve performance, as it is typical that such objects do not exist\nin the serving repo, and the consequences of a false negative are minor\n(usually, a slightly larger pack sent).\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nDifferences from v1: OBJECT_INFO_QUICK is also used wherever\nOBJECT_INFO_SKIP_FETCH_OBJECT is added.\n\nUsing OBJECT_INFO_QUICK makes sense to me, so here's an updated patch.\n---\n t/t5616-partial-clone.sh | 38 ++++++++++++++++++++++++++++++++++++++\n upload-pack.c            |  6 ++++--\n 2 files changed, 42 insertions(+), 2 deletions(-)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 8a27452a51..37de0afb02 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -422,6 +422,44 @@ test_expect_success 'single-branch tag following respects partial clone' '\n \ttest_must_fail git -C single rev-parse --verify refs/tags/C\n '\n \n+test_expect_success 'fetch from a partial clone, protocol v0' '\n+\trm -rf server client trace &&\n+\n+\t# Pretend that the server is a partial clone\n+\tgit init server &&\n+\tgit -C server remote add a_remote \"file://$(pwd)/\" &&\n+\ttest_config -C server core.repositoryformatversion 1 &&\n+\ttest_config -C server extensions.partialclone a_remote &&\n+\ttest_config -C server protocol.version 0 &&\n+\ttest_commit -C server foo &&\n+\n+\t# Fetch from the server\n+\tgit init client &&\n+\ttest_config -C client protocol.version 0 &&\n+\ttest_commit -C client bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"file://$(pwd)/server\" &&\n+\t! grep \"version 2\" trace\n+'\n+\n+test_expect_success 'fetch from a partial clone, protocol v2' '\n+\trm -rf server client trace &&\n+\n+\t# Pretend that the server is a partial clone\n+\tgit init server &&\n+\tgit -C server remote add a_remote \"file://$(pwd)/\" &&\n+\ttest_config -C server core.repositoryformatversion 1 &&\n+\ttest_config -C server extensions.partialclone a_remote &&\n+\ttest_config -C server protocol.version 2 &&\n+\ttest_commit -C server foo &&\n+\n+\t# Fetch from the server\n+\tgit init client &&\n+\ttest_config -C client protocol.version 2 &&\n+\ttest_commit -C client bar &&\n+\tGIT_TRACE_PACKET=\"$(pwd)/trace\" git -C client fetch \"file://$(pwd)/server\" &&\n+\tgrep \"version 2\" trace\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \ndiff --git a/upload-pack.c b/upload-pack.c\nindex 951a2b23aa..8673741070 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -482,7 +482,8 @@ static int got_oid(struct upload_pack_data *data,\n {\n \tif (get_oid_hex(hex, oid))\n \t\tdie(\"git upload-pack: expected SHA1 object, got '%s'\", hex);\n-\tif (!has_object_file(oid))\n+\tif (!has_object_file_with_flags(oid,\n+\t\t\t\t\tOBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\treturn -1;\n \treturn do_got_oid(data, oid);\n }\n@@ -1423,7 +1424,8 @@ static int process_haves(struct upload_pack_data *data, struct oid_array *common\n \tfor (i = 0; i < data->haves.nr; i++) {\n \t\tconst struct object_id *oid = &data->haves.oid[i];\n \n-\t\tif (!has_object_file(oid))\n+\t\tif (!has_object_file_with_flags(oid,\n+\t\t\t\t\t\tOBJECT_INFO_QUICK | OBJECT_INFO_SKIP_FETCH_OBJECT))\n \t\t\tcontinue;\n \n \t\toid_array_append(common, oid);\n-- \n2.27.0.389.gc38d7665816-goog\n\n"},{"id":"401773","messageId":"20200720174203.GB2046890@coredump.intra.peff.net","threadId":"53863","inReplyTo":"20200716180950.2852753-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] upload-pack: do not lazy-fetch \"have\" objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-07-20T17:42:03Z","receivedAt":"2020-07-20T17:42:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 16, 2020 at 11:09:50AM -0700, Jonathan Tan wrote:\n\n> Differences from v1: OBJECT_INFO_QUICK is also used wherever\n> OBJECT_INFO_SKIP_FETCH_OBJECT is added.\n> \n> Using OBJECT_INFO_QUICK makes sense to me, so here's an updated patch.\n\nYeah, I think this is reasonable, and the patch looks good to me.\n\n-Peff\n"}]}