{"thread":{"id":"49394","subject":"[PATCH] fetch: in partial clone, check presence of targets","startedAt":"2018-09-20T18:48:55Z","lastAt":"2018-09-21T20:19:27Z","messageCount":8,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"358529","messageId":"20180920184843.20898-1-jonathantanmy@google.com","threadId":"49394","inReplyTo":null,"subject":"[PATCH] fetch: in partial clone, check presence of targets","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-20T18:48:43Z","receivedAt":"2018-09-20T18:48:55Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching an object that is known as a promisor object to the local\nrepository, the connectivity check in quickfetch() in builtin/fetch.c\nsucceeds, causing object transfer to be bypassed. However, this should\nnot happen if that object is merely promised and not actually present.\n\nBecause this happens, when a user invokes \"git fetch origin \" on\nthe command-line, the object may not actually be fetched even\nthough the command returns an exit code of 0. This is a similar issue\n(but with a different cause) to the one fixed by a0c9016abd\n(\"upload-pack: send refs' objects despite \"filter\"\", 2018-07-09).\n\nTherefore, update quickfetch() to also directly check for the presence\nof all objects to be fetched.\n\nSigned-off-by: Jonathan Tan \n---\nIn this patch, I have striven to avoid piping from Git commands that may\nfail, following the guidelines in [1].\n\n[1] https://public-inbox.org/git/c625bfe2205d51b3158ef71e4bf472708642c146.1537223021.git.matvore@google.com/\n---\n builtin/fetch.c | 19 +++++++++++++++++++\n t/t5616-partial-clone.sh | 17 +++++++++++++++++\n 2 files changed, 36 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d21..e9640fe5a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -938,6 +938,25 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (deepen)\n \t\treturn -1;\n+\n+\tif (repository_format_partial_clone) {\n+\t\t/*\n+\t\t * For the purposes of the connectivity check,\n+\t\t * check_connected() considers all objects promised by\n+\t\t * promisor objects as existing, which means that the\n+\t\t * connectivity check would pass even if a target object\n+\t\t * in rm is merely promised and not present. When\n+\t\t * fetching objects, we need all of them to be present\n+\t\t * (in addition to having correct connectivity), so\n+\t\t * check them directly.\n+\t\t */\n+\t\tstruct ref *r;\n+\t\tfor (r = rm; r; r = r->next) {\n+\t\t\tif (!has_object_file(&r->old_oid))\n+\t\t\t\treturn -1;\n+\t\t}\n+\t}\n+\n \topt.quiet = 1;\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex bbbe7537d..359d27d02 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -170,6 +170,23 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm\n \tgit -C dst fsck\n '\n \n+test_expect_success 'fetch what is specified on CLI even if already promised' '\n+\trm -rf src dst.git &&\n+\tgit init src &&\n+\ttest_commit -C src foo &&\n+\ttest_config -C src uploadpack.allowfilter 1 &&\n+\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit hash-object --stdin blob &&\n+\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/src\" dst.git &&\n+\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_before &&\n+\tgrep \"?$(cat blob)\" missing_before &&\n+\tgit -C dst.git fetch origin $(cat blob) &&\n+\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_after &&\n+\t! grep \"?$(cat blob)\" missing_after\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358540","messageId":"xmqqh8ijzyep.fsf@gitster-ct.c.googlers.com","threadId":"49394","inReplyTo":"20180920184843.20898-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: in partial clone, check presence of targets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-20T20:50:22Z","receivedAt":"2018-09-20T20:50:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 61bec5d21..e9640fe5a 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -938,6 +938,25 @@ static int quickfetch(struct ref *ref_map)\n> \t */\n> \tif (deepen)\n> \t\treturn -1;\n> +\n> +\tif (repository_format_partial_clone) {\n> +\t\t/*\n> +\t\t * For the purposes of the connectivity check,\n> +\t\t * check_connected() considers all objects promised by\n> +\t\t * promisor objects as existing, which means that the\n> +\t\t * connectivity check would pass even if a target object\n> +\t\t * in rm is merely promised and not present. When\n> +\t\t * fetching objects, we need all of them to be present\n> +\t\t * (in addition to having correct connectivity), so\n> +\t\t * check them directly.\n> +\t\t */\n> +\t\tstruct ref *r;\n> +\t\tfor (r = rm; r; r = r->next) {\n> +\t\t\tif (!has_object_file(&r->old_oid))\n> +\t\t\t\treturn -1;\n> +\t\t}\n\nBecause check_connected() lies in the presense of \"promisor\", we can\ndefeat it this way, which makes sense. \n\nI wonder if it makes sense to do this check unconditionally, even\nwhen we are in a fully functioning clone. The check is quite cheap\nand can reject a ref_map that has an object we do not know about,\nwithout check_connected() having to ask the rev-list.\n\n> +\t}\n> +\n> \topt.quiet = 1;\n> \treturn check_connected(iterate_ref_map, &rm, &opt);\n> }\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index bbbe7537d..359d27d02 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -170,6 +170,23 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm\n> \tgit -C dst fsck\n> '\n> \n> +test_expect_success 'fetch what is specified on CLI even if already promised' '\n> +\trm -rf src dst.git &&\n> +\tgit init src &&\n> +\ttest_commit -C src foo &&\n> +\ttest_config -C src uploadpack.allowfilter 1 &&\n> +\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n> +\n> +\tgit hash-object --stdin blob &&\n> +\n> +\tgit clone --bare --filter=blob:none \"file://$(pwd)/src\" dst.git &&\n> +\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_before &&\n> +\tgrep \"?$(cat blob)\" missing_before &&\n> +\tgit -C dst.git fetch origin $(cat blob) &&\n> +\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_after &&\n> +\t! grep \"?$(cat blob)\" missing_after\n> +'\n> +\n> . \"$TEST_DIRECTORY\"/lib-httpd.sh\n> start_httpd\n"},{"id":"358543","messageId":"20180920221038.74193-1-jonathantanmy@google.com","threadId":"49394","inReplyTo":"xmqqh8ijzyep.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: in partial clone, check presence of targets","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-20T22:10:38Z","receivedAt":"2018-09-20T22:10:51Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan writes:\n> \n> > +\tif (repository_format_partial_clone) {\n> > +\t\t/*\n> > +\t\t * For the purposes of the connectivity check,\n> > +\t\t * check_connected() considers all objects promised by\n> > +\t\t * promisor objects as existing, which means that the\n> > +\t\t * connectivity check would pass even if a target object\n> > +\t\t * in rm is merely promised and not present. When\n> > +\t\t * fetching objects, we need all of them to be present\n> > +\t\t * (in addition to having correct connectivity), so\n> > +\t\t * check them directly.\n> > +\t\t */\n> > +\t\tstruct ref *r;\n> > +\t\tfor (r = rm; r; r = r->next) {\n> > +\t\t\tif (!has_object_file(&r->old_oid))\n> > +\t\t\t\treturn -1;\n> > +\t\t}\n> \n> Because check_connected() lies in the presense of \"promisor\", we can\n> defeat it this way, which makes sense. \n> \n> I wonder if it makes sense to do this check unconditionally, even\n> when we are in a fully functioning clone. The check is quite cheap\n> and can reject a ref_map that has an object we do not know about,\n> without check_connected() having to ask the rev-list.\n\nIt makes sense to me from a runtime point of view - the usual case, for\nme, is when we're missing at least one object that we're fetching, and\ndoing the cheap check even allows us to skip the expensive check.\n\nThe hard part for me lies in how to communicate to future readers of the\ncode that they cannot remove this section to simplify the code. We would\nneed a more complicated comment, something like this:\n\n /*\n * Check if all wanted objects are present.\n *\n * If the local repository is a partial clone, check_connected() is not\n * sufficient - it would pass even if a wanted object is merely\n * promised and not present. This is because, for the purposes of the\n * connectivity check, check_connected() considers all objects promised\n * by promisor objects as existing.\n *\n * If the local repository is not a partial clone, check_connected() is\n * sufficient, but this loop allows us to avoid the expensive\n * check_connected() in the usual case that at least one wanted object\n * is missing.\n */\n"},{"id":"358544","messageId":"xmqqy3bvycie.fsf@gitster-ct.c.googlers.com","threadId":"49394","inReplyTo":"20180920221038.74193-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: in partial clone, check presence of targets","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-20T23:28:41Z","receivedAt":"2018-09-20T23:28:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> The hard part for me lies in how to communicate to future readers of the\n> code that they cannot remove this section to simplify the code. We would\n> need a more complicated comment, something like this:\n\nThat suggests two things.\n\n - Perhaps quickfetch() is misnamed. It is to ensure \"these exist,\n and are 'connected'\"; renaming the helper to convey that would be\n sufficient to prevent future readers from removing the \"these\n exist\" check from it.\n\n - Perhaps check_connected() is also misnamed, if the above \"these\n exist, and are 'connected'\" is not a sufficient warning against\n removal of the \"these exist\" test, perhaps \"check_connected()\" is\n not telling the readers that things that are 'connected' do not\n have to exist. What does being 'connected' mean in the world\n with \"promised\" objects anyway? The designer of the feature\n should probably have a concise and clear answer.\n\n> /*\n> * Check if all wanted objects are present.\n\nHere 'wanted' means... the tip that was directly requested must\nexist, and in addition, anything that is reachable from it must\neither exist locally or available from the lazy-clone source? But\nthat is not quite it. Your definition of 'present' is fuzzy and mean\ntwo different things---for the wanted tips, they must exist. For\nthe objects that are required for these wanted tips to be well\nformed, they do not have to exist but it is OK for them to be merely\npromised.\n\nPerhaps the comment for the quickfetch() function itself should say\n\n/*\n * Ensure that the requested tips exist locally, and anything that is\n * reachable from them either exist locally or promised to be available.\n */\n\nAdding a similar comment to check_connected() function is left as an\nexercise, but I suspect it would be the latter half of the above\nsentence.\n\nIt may be worth renaming both functions for clarity, as I mentioned\nalready.\n"},{"id":"358633","messageId":"cover.1537553700.git.jonathantanmy@google.com","threadId":"49394","inReplyTo":"20180920184843.20898-1-jonathantanmy@google.com","subject":"[PATCH v2 0/2] Check presence of targets when fetching to partial clone","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-21T18:22:36Z","receivedAt":"2018-09-21T18:22:43Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"New in v2:\n - added patch to clarify in documentation what check_connected() does\n - renamed quickfetch() to check_exist_and_connected() to better reflect\n what it does\n - also updated its documentation; I avoided usage of \"wanted objects\"\n and used \"fetch targets\" instead to clarify that I'm only talking\n about the direct targets, not all objects referenced by those targets\n - in check_exist_and_connected() (previously quickfetch()), check\n existence directly regardless of whether the repository is a partial\n clone or not\n\nThis should resolve most of Junio's comments in [1], except that I chose\nnot to modify or rename check_connected(). In this current world, it is\ntrue that we usually require existence of ref targets, but that might\nnot be so in the future (having said that, I don't know of any schedules\nfor this future). Also, check_connected() is used in a few places, so\nsuch a change would cause some churn in the codebase. So I left this\nfunction alone.\n\n[1] https://public-inbox.org/git/xmqqy3bvycie.fsf@gitster-ct.c.googlers.com/\n\nJonathan Tan (2):\n connected: document connectivity in partial clones\n fetch: in partial clone, check presence of targets\n\n builtin/fetch.c | 15 +++++++++++++--\n connected.h | 6 +++---\n t/t5616-partial-clone.sh | 17 +++++++++++++++++\n 3 files changed, 33 insertions(+), 5 deletions(-)\n\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358634","messageId":"14153be920095f5fc8c11da33d5d27469e3a0f39.1537553700.git.jonathantanmy@google.com","threadId":"49394","inReplyTo":"cover.1537553700.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] connected: document connectivity in partial clones","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-21T18:22:37Z","receivedAt":"2018-09-21T18:22:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In acb0c57260 (\"fetch: support filters\", 2017-12-08), check_connected()\nwas extended to allow objects to either be promised to be available (if\nthe repository is a partial clone) or to be present; previously, this\nfunction required the latter. However, this change was not reflected in\nthe documentation of that function. Update the documentation\naccordingly.\n\nSigned-off-by: Jonathan Tan \n---\n connected.h | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/connected.h b/connected.h\nindex e4c961817..8d5a6b3ad 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -51,9 +51,9 @@ struct check_connected_options {\n #define CHECK_CONNECTED_INIT { 0 }\n \n /*\n- * Make sure that our object store has all the commits necessary to\n- * connect the ancestry chain to some of our existing refs, and all\n- * the trees and blobs that these commits use.\n+ * Make sure that all given objects and all objects reachable from them\n+ * either exist in our object store or (if the repository is a partial\n+ * clone) are promised to be available.\n *\n * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n *\n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358635","messageId":"f575afa9393c4262853547ba536dc90f9511da5c.1537553700.git.jonathantanmy@google.com","threadId":"49394","inReplyTo":"cover.1537553700.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] fetch: in partial clone, check presence of targets","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-09-21T18:22:38Z","receivedAt":"2018-09-21T18:22:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When fetching an object that is known as a promisor object to the local\nrepository, the connectivity check in quickfetch() in builtin/fetch.c\nsucceeds, causing object transfer to be bypassed. However, this should\nnot happen if that object is merely promised and not actually present.\n\nBecause this happens, when a user invokes \"git fetch origin \" on\nthe command-line, the object may not actually be fetched even\nthough the command returns an exit code of 0. This is a similar issue\n(but with a different cause) to the one fixed by a0c9016abd\n(\"upload-pack: send refs' objects despite \"filter\"\", 2018-07-09).\n\nTherefore, update quickfetch() to also directly check for the presence\nof all objects to be fetched. Its documentation and name are also\nupdated to better reflect what it does.\n\nSigned-off-by: Jonathan Tan \n---\n builtin/fetch.c | 15 +++++++++++++--\n t/t5616-partial-clone.sh | 17 +++++++++++++++++\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 61bec5d21..b9e74c129 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -924,10 +924,11 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n * everything we are going to fetch already exists and is connected\n * locally.\n */\n-static int quickfetch(struct ref *ref_map)\n+static int check_exist_and_connected(struct ref *ref_map)\n {\n \tstruct ref *rm = ref_map;\n \tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n+\tstruct ref *r;\n \n \t/*\n \t * If we are deepening a shallow clone we already have these\n@@ -938,13 +939,23 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (deepen)\n \t\treturn -1;\n+\n+\t/*\n+\t * check_connected() allows objects to merely be promised, but\n+\t * we need all direct targets to exist.\n+\t */\n+\tfor (r = rm; r; r = r->next) {\n+\t\tif (!has_object_file(&r->old_oid))\n+\t\t\treturn -1;\n+\t}\n+\n \topt.quiet = 1;\n \treturn check_connected(iterate_ref_map, &rm, &opt);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n {\n-\tint ret = quickfetch(ref_map);\n+\tint ret = check_exist_and_connected(ref_map);\n \tif (ret)\n \t\tret = transport_fetch_refs(transport, ref_map);\n \tif (!ret)\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex bbbe7537d..359d27d02 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -170,6 +170,23 @@ test_expect_success 'partial clone fetches blobs pointed to by refs even if norm\n \tgit -C dst fsck\n '\n \n+test_expect_success 'fetch what is specified on CLI even if already promised' '\n+\trm -rf src dst.git &&\n+\tgit init src &&\n+\ttest_commit -C src foo &&\n+\ttest_config -C src uploadpack.allowfilter 1 &&\n+\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n+\n+\tgit hash-object --stdin blob &&\n+\n+\tgit clone --bare --filter=blob:none \"file://$(pwd)/src\" dst.git &&\n+\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_before &&\n+\tgrep \"?$(cat blob)\" missing_before &&\n+\tgit -C dst.git fetch origin $(cat blob) &&\n+\tgit -C dst.git rev-list --objects --quiet --missing=print HEAD >missing_after &&\n+\t! grep \"?$(cat blob)\" missing_after\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.19.0.444.g18242da7ef-goog\n\n"},{"id":"358645","messageId":"xmqqmusavc1h.fsf@gitster-ct.c.googlers.com","threadId":"49394","inReplyTo":"14153be920095f5fc8c11da33d5d27469e3a0f39.1537553700.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 1/2] connected: document connectivity in partial clones","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-09-21T20:19:22Z","receivedAt":"2018-09-21T20:19:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> In acb0c57260 (\"fetch: support filters\", 2017-12-08), check_connected()\n> was extended to allow objects to either be promised to be available (if\n> the repository is a partial clone) or to be present; previously, this\n> function required the latter. However, this change was not reflected in\n> the documentation of that function. Update the documentation\n> accordingly.\n>\n> Signed-off-by: Jonathan Tan \n> ---\n> connected.h | 6 +++---\n> 1 file changed, 3 insertions(+), 3 deletions(-)\n\nVery much makes sense. I think this is sufficient clarification to\nallay your earlier worry of having to have a huge in-code comment to\nprevent the \"must exist\" loop from getting removed.\n\n> diff --git a/connected.h b/connected.h\n> index e4c961817..8d5a6b3ad 100644\n> --- a/connected.h\n> +++ b/connected.h\n> @@ -51,9 +51,9 @@ struct check_connected_options {\n> #define CHECK_CONNECTED_INIT { 0 }\n> \n> /*\n> - * Make sure that our object store has all the commits necessary to\n> - * connect the ancestry chain to some of our existing refs, and all\n> - * the trees and blobs that these commits use.\n> + * Make sure that all given objects and all objects reachable from them\n> + * either exist in our object store or (if the repository is a partial\n> + * clone) are promised to be available.\n> *\n> * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n> *\n"}]}