{"thread":{"id":"58522","subject":"[PATCH 0/2] Fail early when partial clone prefetch fails","startedAt":"2022-09-27T22:12:40Z","lastAt":"2022-10-05T18:09:06Z","messageCount":15,"participants":["Jonathan Tan","Glen Choo","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"463772","messageId":"cover.1664316642.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":null,"subject":"[PATCH 0/2] Fail early when partial clone prefetch fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-27T22:12:28Z","receivedAt":"2022-09-27T22:12:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"This fixes something we noticed at $DAYJOB, when a partial clone\nprefetch intermittently fails but all subsequent fetches work. More\ndetails are in patch 2's commit message.\n\nJonathan Tan (2):\n promisor-remote: remove a return value\n promisor-remote: die upon failing fetch\n\n object-file.c | 4 ----\n promisor-remote.c | 23 ++++++++++++++---------\n promisor-remote.h | 11 +++++------\n 3 files changed, 19 insertions(+), 19 deletions(-)\n\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463773","messageId":"7cef8088cebe368e66237837eacec71381182ec1.1664316642.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":"cover.1664316642.git.jonathantanmy@google.com","subject":"[PATCH 1/2] promisor-remote: remove a return value","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-27T22:12:29Z","receivedAt":"2022-09-27T22:12:45Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"No caller of promisor_remote_get_direct() is checking its return value,\nso remove it.\n\nSigned-off-by: Jonathan Tan \n---\n promisor-remote.c | 12 ++++--------\n promisor-remote.h | 11 +++++------\n 2 files changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 68f46f5ec7..8b4d650b4c 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -230,18 +230,17 @@ static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n }\n \n-int promisor_remote_get_direct(struct repository *repo,\n-\t\t\t const struct object_id *oids,\n-\t\t\t int oid_nr)\n+void promisor_remote_get_direct(struct repository *repo,\n+\t\t\t\tconst struct object_id *oids,\n+\t\t\t\tint oid_nr)\n {\n \tstruct promisor_remote *r;\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n-\tint res = -1;\n \n \tif (oid_nr == 0)\n-\t\treturn 0;\n+\t\treturn;\n \n \tpromisor_remote_init(repo);\n \n@@ -256,12 +255,9 @@ int promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\t\tres = 0;\n \t\tbreak;\n \t}\n \n \tif (to_free)\n \t\tfree(remaining_oids);\n-\n-\treturn res;\n }\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex edc45ab0f5..df36eb08ef 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -39,13 +39,12 @@ static inline int has_promisor_remote(void)\n \n /*\n * Fetches all requested objects from all promisor remotes, trying them one at\n- * a time until all objects are fetched. Returns 0 upon success, and non-zero\n- * otherwise.\n+ * a time until all objects are fetched.\n *\n- * If oid_nr is 0, this function returns 0 (success) immediately.\n+ * If oid_nr is 0, this function returns immediately.\n */\n-int promisor_remote_get_direct(struct repository *repo,\n-\t\t\t const struct object_id *oids,\n-\t\t\t int oid_nr);\n+void promisor_remote_get_direct(struct repository *repo,\n+\t\t\t\tconst struct object_id *oids,\n+\t\t\t\tint oid_nr);\n \n #endif /* PROMISOR_REMOTE_H */\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463774","messageId":"7f6412eb8ce0c47a7645b89fab171a212353f8b2.1664316642.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":"cover.1664316642.git.jonathantanmy@google.com","subject":"[PATCH 2/2] promisor-remote: die upon failing fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-27T22:12:30Z","receivedAt":"2022-09-27T22:12:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, an attempt to read a missing object results in an\nattempt to fetch that single object. In order to avoid multiple\nsequential fetches, which would occur when multiple objects are missing\n(which is the typical case), some commands have been taught to prefetch\nin a batch: such a command would, in a partial clone, notice that\nseveral objects that it will eventually need are missing, and call\npromisor_remote_get_direct() with all such objects at once.\n\nWhen this batch prefetch fails, these commands fall back to the\nsequential fetches. But at $DAYJOB we have noticed that this results in\na bad user experience: a command would take unexpectedly long to finish\nif the batch prefetch would fail for some intermittent reason, but all\nsubsequent fetches would work. It would be a better user experience for\nsuch a command would just fail.\n\nTherefore, make it a fatal error if the prefetch fails and at least one\nobject being fetched is known to be a promisor object. (The latter\ncriterion is to make sure that we are not misleading the user that such\nan object would be present from the promisor remote. For example, a\nmissing object may be a result of repository corruption and not because\nit is expectedly missing due to the repository being a partial clone.)\n\nSigned-off-by: Jonathan Tan \n---\n object-file.c | 4 ----\n promisor-remote.c | 11 ++++++++++-\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 5b270f046d..5e30960234 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1599,10 +1599,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n \t\t !already_retried &&\n \t\t !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {\n-\t\t\t/*\n-\t\t\t * TODO Investigate checking promisor_remote_get_direct()\n-\t\t\t * TODO return value and stopping on error here.\n-\t\t\t */\n \t\t\tpromisor_remote_get_direct(r, real, 1);\n \t\t\talready_retried = 1;\n \t\t\tcontinue;\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8b4d650b4c..faa7612941 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -4,6 +4,7 @@\n #include \"config.h\"\n #include \"transport.h\"\n #include \"strvec.h\"\n+#include \"packfile.h\"\n \n struct promisor_remote_config {\n \tstruct promisor_remote *promisors;\n@@ -238,6 +239,7 @@ void promisor_remote_get_direct(struct repository *repo,\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n+\tint i;\n \n \tif (oid_nr == 0)\n \t\treturn;\n@@ -255,9 +257,16 @@ void promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\t\tbreak;\n+\t\tgoto all_fetched;\n+\t}\n+\n+\tfor (i = 0; i < remaining_nr; i++) {\n+\t\tif (is_promisor_object(&remaining_oids[i]))\n+\t\t\tdie(_(\"could not fetch %s from promisor remote\"),\n+\t\t\t oid_to_hex(&remaining_oids[i]));\n \t}\n \n+all_fetched:\n \tif (to_free)\n \t\tfree(remaining_oids);\n }\n-- \n2.37.3.998.g577e59143f-goog\n\n"},{"id":"463864","messageId":"kl6l8rm3mm2x.fsf@chooglen-macbookpro.roam.corp.google.com","threadId":"58522","inReplyTo":"7f6412eb8ce0c47a7645b89fab171a212353f8b2.1664316642.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] promisor-remote: die upon failing fetch","fromName":"Glen Choo","fromEmail":"chooglen@google.com","sentAt":"2022-09-28T17:43:18Z","receivedAt":"2022-09-28T17:43:34Z","isPatch":true,"sender":{"key":"glencbz@gmail.com","avatar":"https://avatars.githubusercontent.com/u/58092771?v=4"},"body":"No real comments on the code - I'm not familiar enough with it, but it\nseems really simple anyway.\n\nJonathan Tan writes:\n\n> When this batch prefetch fails, these commands fall back to the\n> sequential fetches. But at $DAYJOB we have noticed that this results in\n> a bad user experience: a command would take unexpectedly long to finish\n> if the batch prefetch would fail for some intermittent reason, but all\n> subsequent fetches would work. It would be a better user experience for\n> such a command would just fail.\n\nI'm not certain that this fail-fast approach is always a better user\nexperience:\n\n- I could imagine that for a small-enough set of objects (say, a very\n restrictive set of sparse specifications), one-by-one fetching would be\n good enough.\n- Even if one-by-one fetching isn't fast, I'd imagine that each\n individual fetch is more likely to succeed than a batch prefetch, and\n as a user, I would prefer to ^C an operation that takes longer than I\n expected than to have retry the repeatedly.\n\nHere are some other arguments that you didn't mention, but I find more\nconvincing:\n\n- Running prefetch in a non-interactive process (e.g. running a job in\n CI) and the user would prefer to fail fast than to have the job run\n longer than expected, e.g. they could script retries manually\n (although, maybe we should do that ourselves).\n\n- Fetching might be subject to a quota, which will be exhausted by\n one-by-one fetching.\n\nAs such, it _might_ make sense to make this behavior configurable, since\nwe may sometimes want it and sometimes not.\n"},{"id":"463915","messageId":"xmqqmtainfw6.fsf@gitster.g","threadId":"58522","inReplyTo":"7cef8088cebe368e66237837eacec71381182ec1.1664316642.git.jonathantanmy@google.com","subject":"Re: [PATCH 1/2] promisor-remote: remove a return value","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T19:23:53Z","receivedAt":"2022-09-29T19:23:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> No caller of promisor_remote_get_direct() is checking its return value,\n> so remove it.\n\nThe above is a good explanation why this change will not hurt the\ncurrent users, but is it a good thing to do in the first place?\n\nIsn't it an indication that all the existing callers are sloppy? Or\ndoes it mean that the value returned from this function is not a\nuseful signal to the caller?\n\nLooking at the implementation, the function seems to want to\nindicate if the fetching succeeded (with 0) or failed (with -1).\nWhat is curious is that the function seems to consider it a success\neven if some requested objects were never downloaded after\nconsulting all the promisor remotes. There is no way for the caller\nto obtain the list of objects it wanted to have but the function\nfailed to deliver, either (other than obviously redoing what\npromisor-remote.c::remove_fetched_oids() does itself).\n\nAre these what makes the returned value from the function useless\nand the callers ignore it? If these are fixed, would it make the\nindication of error more useful?\n\nLooking at some of the existing callers:\n\n * builtin/index-pack.c calls it to fill the \"gap\" and after giving\n the function a chance to download \"missing\" objects, goes on its\n prosessing as if nothing has happened. We will properly die if\n any object we need is still missing, so unless we are interested\n in failing early, checking the return value of the function does\n not help all that much.\n\n * The caller in builtin/pack-objects.c is similar. We have a list\n of bunch of objects we want to pack, some of which may be\n missing. We ask all missing ones from the list in bulk before\n doing anything, but we rely on the regular codepath to notice if\n the promisor remote did not give us necessary objects.\n\nI didn't look at others, but the above two are probably OK, unless\nwe want to get a signal to be cautious about poorly managed promisor\nremotes. Not distinguishing the reason why we do not have objects\nbetween a corrupt repository and a promisor remote that breaks its\nearlier promises makes it impossible to say \"hey, that promisor\nremote is failing its expectation, perhaps we should wean us off\nfrom it\".\n"},{"id":"463917","messageId":"xmqqh70qndk1.fsf@gitster.g","threadId":"58522","inReplyTo":"7f6412eb8ce0c47a7645b89fab171a212353f8b2.1664316642.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] promisor-remote: die upon failing fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T20:14:22Z","receivedAt":"2022-09-29T20:14:29Z","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/object-file.c b/object-file.c\n> index 5b270f046d..5e30960234 100644\n> --- a/object-file.c\n> +++ b/object-file.c\n> @@ -1599,10 +1599,6 @@ static int do_oid_object_info_extended(struct repository *r,\n> \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n> \t\t !already_retried &&\n> \t\t !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {\n> -\t\t\t/*\n> -\t\t\t * TODO Investigate checking promisor_remote_get_direct()\n> -\t\t\t * TODO return value and stopping on error here.\n> -\t\t\t */\n> \t\t\tpromisor_remote_get_direct(r, real, 1);\n> \t\t\talready_retried = 1;\n> \t\t\tcontinue;\n> diff --git a/promisor-remote.c b/promisor-remote.c\n> index 8b4d650b4c..faa7612941 100644\n> --- a/promisor-remote.c\n> +++ b/promisor-remote.c\n> @@ -4,6 +4,7 @@\n> #include \"config.h\"\n> #include \"transport.h\"\n> #include \"strvec.h\"\n> +#include \"packfile.h\"\n> \n> struct promisor_remote_config {\n> \tstruct promisor_remote *promisors;\n> @@ -238,6 +239,7 @@ void promisor_remote_get_direct(struct repository *repo,\n> \tstruct object_id *remaining_oids = (struct object_id *)oids;\n> \tint remaining_nr = oid_nr;\n> \tint to_free = 0;\n> +\tint i;\n> \n> \tif (oid_nr == 0)\n> \t\treturn;\n> @@ -255,9 +257,16 @@ void promisor_remote_get_direct(struct repository *repo,\n> \t\t\t\tcontinue;\n> \t\t\t}\n> \t\t}\n> -\t\tbreak;\n> +\t\tgoto all_fetched;\n\nOK. So when fetch_object() says it got everything we asked it to\ngive, there is no behaviour change. But if invocations of\nfetch_objects() on all promisor remotes end unsuccessfully, we see\nif some are supposed to be available from the promisor remote and\ndie.\n\nAn obvious alternative would be to do without the previous step, and\ninstead tighten the callers to react to error return, and then from\nthe new code, return a different error return value to allow the\ncaller to tell what kind of breakage it got. But this extra die()\nwith more sloppy callers would probably work just fine in practice.\n\n> +\t}\n> +\n> +\tfor (i = 0; i < remaining_nr; i++) {\n> +\t\tif (is_promisor_object(&remaining_oids[i]))\n> +\t\t\tdie(_(\"could not fetch %s from promisor remote\"),\n> +\t\t\t oid_to_hex(&remaining_oids[i]));\n> \t}\n> \n> +all_fetched:\n> \tif (to_free)\n> \t\tfree(remaining_oids);\n> }\n"},{"id":"463918","messageId":"xmqqczbendil.fsf@gitster.g","threadId":"58522","inReplyTo":"cover.1664316642.git.jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Fail early when partial clone prefetch fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-29T20:15:14Z","receivedAt":"2022-09-29T20:15:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> This fixes something we noticed at $DAYJOB, when a partial clone\n> prefetch intermittently fails but all subsequent fetches work. More\n> details are in patch 2's commit message.\n>\n> Jonathan Tan (2):\n> promisor-remote: remove a return value\n> promisor-remote: die upon failing fetch\n>\n> object-file.c | 4 ----\n> promisor-remote.c | 23 ++++++++++++++---------\n> promisor-remote.h | 11 +++++------\n> 3 files changed, 19 insertions(+), 19 deletions(-)\n\nNo changes to t/ directory?\n"},{"id":"464024","messageId":"20220930215000.2079107-1-jonathantanmy@google.com","threadId":"58522","inReplyTo":"xmqqczbendil.fsf@gitster.g","subject":"Re: [PATCH 0/2] Fail early when partial clone prefetch fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-30T21:50:00Z","receivedAt":"2022-09-30T21:50:15Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> No changes to t/ directory?\n\nHmm...any test you would like to see in particular? Here, we just fail\nwith a specific error message when any sort of fetching from the\npromisor remote due to a lookup of a missing object.\n"},{"id":"464025","messageId":"20220930220235.2080522-1-jonathantanmy@google.com","threadId":"58522","inReplyTo":"xmqqmtainfw6.fsf@gitster.g","subject":"Re: [PATCH 1/2] promisor-remote: remove a return value","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-30T22:02:35Z","receivedAt":"2022-09-30T22:02:42Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano writes:\n> Jonathan Tan writes:\n> \n> > No caller of promisor_remote_get_direct() is checking its return value,\n> > so remove it.\n> \n> The above is a good explanation why this change will not hurt the\n> current users, but is it a good thing to do in the first place?\n> \n> Isn't it an indication that all the existing callers are sloppy? Or\n> does it mean that the value returned from this function is not a\n> useful signal to the caller?\n> \n> Looking at the implementation, the function seems to want to\n> indicate if the fetching succeeded (with 0) or failed (with -1).\n> What is curious is that the function seems to consider it a success\n> even if some requested objects were never downloaded after\n> consulting all the promisor remotes. There is no way for the caller\n> to obtain the list of objects it wanted to have but the function\n> failed to deliver, either (other than obviously redoing what\n> promisor-remote.c::remove_fetched_oids() does itself).\n> \n> Are these what makes the returned value from the function useless\n> and the callers ignore it? If these are fixed, would it make the\n> indication of error more useful?\n> \n> Looking at some of the existing callers:\n> \n> * builtin/index-pack.c calls it to fill the \"gap\" and after giving\n> the function a chance to download \"missing\" objects, goes on its\n> prosessing as if nothing has happened. We will properly die if\n> any object we need is still missing, so unless we are interested\n> in failing early, checking the return value of the function does\n> not help all that much.\n> \n> * The caller in builtin/pack-objects.c is similar. We have a list\n> of bunch of objects we want to pack, some of which may be\n> missing. We ask all missing ones from the list in bulk before\n> doing anything, but we rely on the regular codepath to notice if\n> the promisor remote did not give us necessary objects.\n> \n> I didn't look at others, but the above two are probably OK, unless\n> we want to get a signal to be cautious about poorly managed promisor\n> remotes. \n\nTrue - in all cases, we either fall back to a code path where we try to\nread a single object (if promisor_remote_get_direct() was called by a\nprefetch) or the same code path that is taken when a non-partial-clone\ntries to read a missing object. So it's not fully clear what's going on,\nbut it is safe. I should have added a note about this in the commit\nmessage and will do so in the next version.\n\n> Not distinguishing the reason why we do not have objects\n> between a corrupt repository and a promisor remote that breaks its\n> earlier promises makes it impossible to say \"hey, that promisor\n> remote is failing its expectation, perhaps we should wean us off\n> from it\".\n\nI'll add a note that in a subsequent patch, we will add a message that\ncan indeed distinguish this case.\n"},{"id":"464028","messageId":"20220930221016.2081461-1-jonathantanmy@google.com","threadId":"58522","inReplyTo":"kl6l8rm3mm2x.fsf@chooglen-macbookpro.roam.corp.google.com","subject":"Re: [PATCH 2/2] promisor-remote: die upon failing fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-09-30T22:10:16Z","receivedAt":"2022-09-30T22:10:29Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Glen Choo writes:\n> I'm not certain that this fail-fast approach is always a better user\n> experience:\n> \n> - I could imagine that for a small-enough set of objects (say, a very\n> restrictive set of sparse specifications), one-by-one fetching would be\n> good enough.\n\nI think that in this case, if you couldn't fetch a small set, you\nwouldn't be able to fetch a single object too.\n\n> - Even if one-by-one fetching isn't fast, I'd imagine that each\n> individual fetch is more likely to succeed than a batch prefetch, and\n> as a user, I would prefer to ^C an operation that takes longer than I\n> expected than to have retry the repeatedly.\n\nHmm...but when you ^C, you have to retry it too right?\n\n> Here are some other arguments that you didn't mention, but I find more\n> convincing:\n> \n> - Running prefetch in a non-interactive process (e.g. running a job in\n> CI) and the user would prefer to fail fast than to have the job run\n> longer than expected, e.g. they could script retries manually\n> (although, maybe we should do that ourselves).\n\nThat's true.\n\n> - Fetching might be subject to a quota, which will be exhausted by\n> one-by-one fetching.\n\nI'll add a note that lengthy execution can be bad for quota reasons as\nwell (in addition to others).\n\n> As such, it _might_ make sense to make this behavior configurable, since\n> we may sometimes want it and sometimes not.\n\nI don't think there is a compelling reason to fallback to single object\nfetching (which is there not because it is useful, but just so that Git\ncommands that haven't been updated to prefetch will still function), so\nI'd rather not add an option for this.\n"},{"id":"464029","messageId":"xmqqmtag7bhu.fsf@gitster.g","threadId":"58522","inReplyTo":"20220930215000.2079107-1-jonathantanmy@google.com","subject":"Re: [PATCH 0/2] Fail early when partial clone prefetch fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-09-30T22:17:49Z","receivedAt":"2022-09-30T22:17:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Junio C Hamano writes:\n>> No changes to t/ directory?\n>\n> Hmm...any test you would like to see in particular? Here, we just fail\n> with a specific error message when any sort of fetching from the\n> promisor remote due to a lookup of a missing object.\n\nI presume that we already have a test that interact with a promisor\nremote that does not keep its promise? If we don't then adding one\nor two would help. If we do, then how the command fails would\nprobably be different (observable via trace2) with your \"early\nbreak\" change, no?\n"},{"id":"464209","messageId":"cover.1664917853.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":"cover.1664316642.git.jonathantanmy@google.com","subject":"[PATCH v2 0/2] Fail early when partial clone prefetch fails","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-10-04T21:13:39Z","receivedAt":"2022-10-04T21:13:52Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Thanks everyone for your comments. I don't think that there is a test \nfor when a lazy fetch fails, for some reason, so I added one.\n\nJonathan Tan (2):\n promisor-remote: remove a return value\n promisor-remote: die upon failing fetch\n\n object-file.c | 4 ----\n promisor-remote.c | 23 ++++++++++++++---------\n promisor-remote.h | 11 +++++------\n t/t0410-partial-clone.sh | 14 ++++++++++++++\n 4 files changed, 33 insertions(+), 19 deletions(-)\n\nRange-diff against v1:\n1: fdb35bc9a6 ! 1: 207332c2df promisor-remote: remove a return value\n @@ Commit message\n No caller of promisor_remote_get_direct() is checking its return value,\n so remove it.\n \n + Not checking the return value means that the user would not know\n + whether the failure of reading an object is due to the promisor remote\n + not supplying the object or because of local repository corruption, but\n + this will be fixed in a subsequent patch.\n +\n Signed-off-by: Jonathan Tan \n Signed-off-by: Junio C Hamano \n \n2: 06a649b01a ! 2: 9bbf130c2c promisor-remote: die upon failing fetch\n @@ Commit message\n When this batch prefetch fails, these commands fall back to the\n sequential fetches. But at $DAYJOB we have noticed that this results in\n a bad user experience: a command would take unexpectedly long to finish\n - if the batch prefetch would fail for some intermittent reason, but all\n - subsequent fetches would work. It would be a better user experience for\n - such a command would just fail.\n + (and possibly use up a lot of bandwidth) if the batch prefetch would\n + fail for some intermittent reason, but all subsequent fetches would\n + work. It would be a better user experience for such a command would\n + just fail.\n \n Therefore, make it a fatal error if the prefetch fails and at least one\n object being fetched is known to be a promisor object. (The latter\n @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,\n \tif (to_free)\n \t\tfree(remaining_oids);\n }\n +\n + ## t/t0410-partial-clone.sh ##\n +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects' '\n + \tgrep \"$HASH\" out\n + '\n + \n ++test_expect_success 'fetching of a promised object that promisor remote no longer has' '\n ++\trm -f err &&\n ++\ttest_create_repo unreliable-server &&\n ++\tgit -C unreliable-server config uploadpack.allowanysha1inwant 1 &&\n ++\tgit -C unreliable-server config uploadpack.allowfilter 1 &&\n ++\ttest_commit -C unreliable-server foo &&\n ++\n ++\tgit clone --filter=blob:none --no-checkout \"file://$(pwd)/unreliable-server\" unreliable-client &&\n ++\n ++\trm -rf unreliable-server/.git/objects/* &&\n ++\ttest_must_fail git -C unreliable-client checkout HEAD 2>err &&\n ++\tgrep \"could not fetch.*from promisor remote\" err\n ++'\n ++\n + test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n + \t# ref-in-want requires protocol version 2\n + \tgit -C server config protocol.version 2 &&\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464210","messageId":"207332c2dff047866a298ba7f72f043a009702f3.1664917853.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":"cover.1664917853.git.jonathantanmy@google.com","subject":"[PATCH v2 1/2] promisor-remote: remove a return value","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-10-04T21:13:40Z","receivedAt":"2022-10-04T21:13:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"No caller of promisor_remote_get_direct() is checking its return value,\nso remove it.\n\nNot checking the return value means that the user would not know\nwhether the failure of reading an object is due to the promisor remote\nnot supplying the object or because of local repository corruption, but\nthis will be fixed in a subsequent patch.\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n promisor-remote.c | 12 ++++--------\n promisor-remote.h | 11 +++++------\n 2 files changed, 9 insertions(+), 14 deletions(-)\n\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 68f46f5ec7..8b4d650b4c 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -230,18 +230,17 @@ static int remove_fetched_oids(struct repository *repo,\n \treturn remaining_nr;\n }\n \n-int promisor_remote_get_direct(struct repository *repo,\n-\t\t\t const struct object_id *oids,\n-\t\t\t int oid_nr)\n+void promisor_remote_get_direct(struct repository *repo,\n+\t\t\t\tconst struct object_id *oids,\n+\t\t\t\tint oid_nr)\n {\n \tstruct promisor_remote *r;\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n-\tint res = -1;\n \n \tif (oid_nr == 0)\n-\t\treturn 0;\n+\t\treturn;\n \n \tpromisor_remote_init(repo);\n \n@@ -256,12 +255,9 @@ int promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\t\tres = 0;\n \t\tbreak;\n \t}\n \n \tif (to_free)\n \t\tfree(remaining_oids);\n-\n-\treturn res;\n }\ndiff --git a/promisor-remote.h b/promisor-remote.h\nindex edc45ab0f5..df36eb08ef 100644\n--- a/promisor-remote.h\n+++ b/promisor-remote.h\n@@ -39,13 +39,12 @@ static inline int has_promisor_remote(void)\n \n /*\n * Fetches all requested objects from all promisor remotes, trying them one at\n- * a time until all objects are fetched. Returns 0 upon success, and non-zero\n- * otherwise.\n+ * a time until all objects are fetched.\n *\n- * If oid_nr is 0, this function returns 0 (success) immediately.\n+ * If oid_nr is 0, this function returns immediately.\n */\n-int promisor_remote_get_direct(struct repository *repo,\n-\t\t\t const struct object_id *oids,\n-\t\t\t int oid_nr);\n+void promisor_remote_get_direct(struct repository *repo,\n+\t\t\t\tconst struct object_id *oids,\n+\t\t\t\tint oid_nr);\n \n #endif /* PROMISOR_REMOTE_H */\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464211","messageId":"9bbf130c2c3d589d15811d9f83d88ec97643c1f6.1664917853.git.jonathantanmy@google.com","threadId":"58522","inReplyTo":"cover.1664917853.git.jonathantanmy@google.com","subject":"[PATCH v2 2/2] promisor-remote: die upon failing fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-10-04T21:13:41Z","receivedAt":"2022-10-04T21:14:00Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"In a partial clone, an attempt to read a missing object results in an\nattempt to fetch that single object. In order to avoid multiple\nsequential fetches, which would occur when multiple objects are missing\n(which is the typical case), some commands have been taught to prefetch\nin a batch: such a command would, in a partial clone, notice that\nseveral objects that it will eventually need are missing, and call\npromisor_remote_get_direct() with all such objects at once.\n\nWhen this batch prefetch fails, these commands fall back to the\nsequential fetches. But at $DAYJOB we have noticed that this results in\na bad user experience: a command would take unexpectedly long to finish\n(and possibly use up a lot of bandwidth) if the batch prefetch would\nfail for some intermittent reason, but all subsequent fetches would\nwork. It would be a better user experience for such a command would\njust fail.\n\nTherefore, make it a fatal error if the prefetch fails and at least one\nobject being fetched is known to be a promisor object. (The latter\ncriterion is to make sure that we are not misleading the user that such\nan object would be present from the promisor remote. For example, a\nmissing object may be a result of repository corruption and not because\nit is expectedly missing due to the repository being a partial clone.)\n\nSigned-off-by: Jonathan Tan \nSigned-off-by: Junio C Hamano \n---\n object-file.c | 4 ----\n promisor-remote.c | 11 ++++++++++-\n t/t0410-partial-clone.sh | 14 ++++++++++++++\n 3 files changed, 24 insertions(+), 5 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex 6c8e3b1660..a5e0160d28 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -1599,10 +1599,6 @@ static int do_oid_object_info_extended(struct repository *r,\n \t\tif (fetch_if_missing && repo_has_promisor_remote(r) &&\n \t\t !already_retried &&\n \t\t !(flags & OBJECT_INFO_SKIP_FETCH_OBJECT)) {\n-\t\t\t/*\n-\t\t\t * TODO Investigate checking promisor_remote_get_direct()\n-\t\t\t * TODO return value and stopping on error here.\n-\t\t\t */\n \t\t\tpromisor_remote_get_direct(r, real, 1);\n \t\t\talready_retried = 1;\n \t\t\tcontinue;\ndiff --git a/promisor-remote.c b/promisor-remote.c\nindex 8b4d650b4c..faa7612941 100644\n--- a/promisor-remote.c\n+++ b/promisor-remote.c\n@@ -4,6 +4,7 @@\n #include \"config.h\"\n #include \"transport.h\"\n #include \"strvec.h\"\n+#include \"packfile.h\"\n \n struct promisor_remote_config {\n \tstruct promisor_remote *promisors;\n@@ -238,6 +239,7 @@ void promisor_remote_get_direct(struct repository *repo,\n \tstruct object_id *remaining_oids = (struct object_id *)oids;\n \tint remaining_nr = oid_nr;\n \tint to_free = 0;\n+\tint i;\n \n \tif (oid_nr == 0)\n \t\treturn;\n@@ -255,9 +257,16 @@ void promisor_remote_get_direct(struct repository *repo,\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t}\n-\t\tbreak;\n+\t\tgoto all_fetched;\n+\t}\n+\n+\tfor (i = 0; i < remaining_nr; i++) {\n+\t\tif (is_promisor_object(&remaining_oids[i]))\n+\t\t\tdie(_(\"could not fetch %s from promisor remote\"),\n+\t\t\t oid_to_hex(&remaining_oids[i]));\n \t}\n \n+all_fetched:\n \tif (to_free)\n \t\tfree(remaining_oids);\n }\ndiff --git a/t/t0410-partial-clone.sh b/t/t0410-partial-clone.sh\nindex 1e864cf317..5b7bee888d 100755\n--- a/t/t0410-partial-clone.sh\n+++ b/t/t0410-partial-clone.sh\n@@ -215,6 +215,20 @@ test_expect_success 'fetching of missing objects' '\n \tgrep \"$HASH\" out\n '\n \n+test_expect_success 'fetching of a promised object that promisor remote no longer has' '\n+\trm -f err &&\n+\ttest_create_repo unreliable-server &&\n+\tgit -C unreliable-server config uploadpack.allowanysha1inwant 1 &&\n+\tgit -C unreliable-server config uploadpack.allowfilter 1 &&\n+\ttest_commit -C unreliable-server foo &&\n+\n+\tgit clone --filter=blob:none --no-checkout \"file://$(pwd)/unreliable-server\" unreliable-client &&\n+\n+\trm -rf unreliable-server/.git/objects/* &&\n+\ttest_must_fail git -C unreliable-client checkout HEAD 2>err &&\n+\tgrep \"could not fetch.*from promisor remote\" err\n+'\n+\n test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n \t# ref-in-want requires protocol version 2\n \tgit -C server config protocol.version 2 &&\n-- \n2.38.0.rc1.362.ged0d419d3c-goog\n\n"},{"id":"464250","messageId":"xmqqy1tup2gy.fsf@gitster.g","threadId":"58522","inReplyTo":"cover.1664917853.git.jonathantanmy@google.com","subject":"Re: [PATCH v2 0/2] Fail early when partial clone prefetch fails","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-10-05T18:09:01Z","receivedAt":"2022-10-05T18:09:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Tan writes:\n\n> Thanks everyone for your comments. I don't think that there is a test \n> for when a lazy fetch fails, for some reason, so I added one.\n>\n> Jonathan Tan (2):\n> promisor-remote: remove a return value\n> promisor-remote: die upon failing fetch\n>\n> object-file.c | 4 ----\n> promisor-remote.c | 23 ++++++++++++++---------\n> promisor-remote.h | 11 +++++------\n> t/t0410-partial-clone.sh | 14 ++++++++++++++\n> 4 files changed, 33 insertions(+), 19 deletions(-)\n\nThanks. Will replace.\n\nI think the C/H code was good already in the first round, so\nhopefully this can go 'next' early in this cycle.\n\n>\n> Range-diff against v1:\n> 1: fdb35bc9a6 ! 1: 207332c2df promisor-remote: remove a return value\n> @@ Commit message\n> No caller of promisor_remote_get_direct() is checking its return value,\n> so remove it.\n> \n> + Not checking the return value means that the user would not know\n> + whether the failure of reading an object is due to the promisor remote\n> + not supplying the object or because of local repository corruption, but\n> + this will be fixed in a subsequent patch.\n> +\n> Signed-off-by: Jonathan Tan \n> Signed-off-by: Junio C Hamano \n> \n> 2: 06a649b01a ! 2: 9bbf130c2c promisor-remote: die upon failing fetch\n> @@ Commit message\n> When this batch prefetch fails, these commands fall back to the\n> sequential fetches. But at $DAYJOB we have noticed that this results in\n> a bad user experience: a command would take unexpectedly long to finish\n> - if the batch prefetch would fail for some intermittent reason, but all\n> - subsequent fetches would work. It would be a better user experience for\n> - such a command would just fail.\n> + (and possibly use up a lot of bandwidth) if the batch prefetch would\n> + fail for some intermittent reason, but all subsequent fetches would\n> + work. It would be a better user experience for such a command would\n> + just fail.\n> \n> Therefore, make it a fatal error if the prefetch fails and at least one\n> object being fetched is known to be a promisor object. (The latter\n> @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo,\n> \tif (to_free)\n> \t\tfree(remaining_oids);\n> }\n> +\n> + ## t/t0410-partial-clone.sh ##\n> +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects' '\n> + \tgrep \"$HASH\" out\n> + '\n> + \n> ++test_expect_success 'fetching of a promised object that promisor remote no longer has' '\n> ++\trm -f err &&\n> ++\ttest_create_repo unreliable-server &&\n> ++\tgit -C unreliable-server config uploadpack.allowanysha1inwant 1 &&\n> ++\tgit -C unreliable-server config uploadpack.allowfilter 1 &&\n> ++\ttest_commit -C unreliable-server foo &&\n> ++\n> ++\tgit clone --filter=blob:none --no-checkout \"file://$(pwd)/unreliable-server\" unreliable-client &&\n> ++\n> ++\trm -rf unreliable-server/.git/objects/* &&\n> ++\ttest_must_fail git -C unreliable-client checkout HEAD 2>err &&\n> ++\tgrep \"could not fetch.*from promisor remote\" err\n> ++'\n> ++\n> + test_expect_success 'fetching of missing objects works with ref-in-want enabled' '\n> + \t# ref-in-want requires protocol version 2\n> + \tgit -C server config protocol.version 2 &&\n"}]}