{"thread":{"id":"57975","subject":"[PATCH] connected: distinguish local/remote bad objects","startedAt":"2022-06-08T21:05:47Z","lastAt":"2022-06-17T20:03:07Z","messageCount":9,"participants":["Jonathan Tan","Junio C Hamano","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"456899","messageId":"20220608210537.185094-1-jonathantanmy@google.com","threadId":"57975","inReplyTo":null,"subject":"[PATCH] connected: distinguish local/remote bad objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-08T21:05:37Z","receivedAt":"2022-06-08T21:05:47Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the connectivity check after a fetch fails, an error message\n\"<remote> did not send all necessary objects\" is printed. That error\nmessage is printed regardless of the reason of failure: in particular,\nthat message may be printed if the connectivity check fails because a\nlocal object is missing. (The connectivity check reads local objects too\nbecause it compares the set of objects that the remote claims to send\nagainst the set of objects that our refs directly or indirectly\nreference.)\n\nThe connectivity check passes, to \"git rev-list\", remote objects\ndirectly and local objects through \"--not\". And internally, the latter\nare marked with the UNINTERESTING flag. When reading a commit during the\ncommit walk, we know whether the commit came from an UNINTERESTING\ncommit or not. Therefore, use this flag to produce a clearer error\nmessage when a bad object is read.\n\nThis necessitates changes in revision.c which is used by components\nother than the connectivity check and may have different meanings for\nobjects passe with and without \"--not\", so guard the extra diagnostics\nbehind a CLI argument.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nWe noticed, at $DAYJOB, some of these messages that were likely caused\nby missing objects in the local repository instead, so here is a patch\nthat will make it easier to diagnose such issues.\n---\n builtin/fetch.c              |  2 +-\n connected.c                  |  1 +\n revision.c                   | 16 ++++++++++++--\n revision.h                   |  3 +++\n t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++\n 5 files changed, 62 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae..6f43b2bf8d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1133,7 +1133,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n-\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\t\trc = error(_(\"connectivity check failed for %s\\n\"), url);\n \t\t\tgoto abort;\n \t\t}\n \t}\ndiff --git a/connected.c b/connected.c\nindex ed3025e7a2..ea773f25db 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -94,6 +94,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \t\tstrvec_push(&rev_list.args, opt->shallow_file);\n \t}\n \tstrvec_push(&rev_list.args,\"rev-list\");\n+\tstrvec_push(&rev_list.args, \"--detailed-bad-object\");\n \tstrvec_push(&rev_list.args, \"--objects\");\n \tstrvec_push(&rev_list.args, \"--stdin\");\n \tif (has_promisor_remote())\ndiff --git a/revision.c b/revision.c\nindex 090a967bf4..777e762373 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -367,6 +367,16 @@ void add_head_to_pending(struct rev_info *revs)\n \tadd_pending_object(revs, obj, \"HEAD\");\n }\n \n+static void NORETURN bad_object(struct rev_info *revs, const char *name,\n+\t\t\t\tunsigned int flags)\n+{\n+\tif (!revs->detailed_bad_object)\n+\t\tdie(\"bad object %s\", name);\n+\tif (flags & UNINTERESTING)\n+\t\tdie(\"bad object %s (from local object store)\", name);\n+\tdie(\"bad object %s (from remote)\", name);\n+}\n+\n static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\t\t    const struct object_id *oid,\n \t\t\t\t    unsigned int flags)\n@@ -390,7 +400,7 @@ static struct object *get_reference(struct rev_info *revs, const char *name,\n \t\t\treturn object;\n \t\tif (revs->exclude_promisor_objects && is_promisor_object(oid))\n \t\t\treturn NULL;\n-\t\tdie(\"bad object %s\", name);\n+\t\tbad_object(revs, name, flags);\n \t}\n \tobject->flags |= flags;\n \treturn object;\n@@ -426,7 +436,7 @@ static struct commit *handle_commit(struct rev_info *revs,\n \t\t\tif (revs->exclude_promisor_objects &&\n \t\t\t    is_promisor_object(&tag->tagged->oid))\n \t\t\t\treturn NULL;\n-\t\t\tdie(\"bad object %s\", oid_to_hex(&tag->tagged->oid));\n+\t\t\tbad_object(revs, oid_to_hex(&tag->tagged->oid), flags);\n \t\t}\n \t\tobject->flags |= flags;\n \t\t/*\n@@ -2537,6 +2547,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\tif (fetch_if_missing)\n \t\t\tBUG(\"exclude_promisor_objects can only be used when fetch_if_missing is 0\");\n \t\trevs->exclude_promisor_objects = 1;\n+\t} else if (!strcmp(arg, \"--detailed-bad-object\")) {\n+\t\trevs->detailed_bad_object = 1;\n \t} else {\n \t\tint opts = diff_opt_parse(&revs->diffopt, argv, argc, revs->prefix);\n \t\tif (!opts)\ndiff --git a/revision.h b/revision.h\nindex e80c148b19..7f685dd5bb 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -328,6 +328,9 @@ struct rev_info {\n \n \t/* Location where temporary objects for remerge-diff are written. */\n \tstruct tmp_objdir *remerge_objdir;\n+\n+\t/* Error reporting info */\n+\tunsigned detailed_bad_object : 1;\n };\n \n int ref_excluded(struct string_list *, const char *path);\ndiff --git a/t/t5518-fetch-exit-status.sh b/t/t5518-fetch-exit-status.sh\nindex 5c4ac2556e..f1adac1dd6 100755\n--- a/t/t5518-fetch-exit-status.sh\n+++ b/t/t5518-fetch-exit-status.sh\n@@ -37,4 +37,47 @@ test_expect_success 'forced update' '\n \n '\n \n+. \"$TEST_DIRECTORY\"/lib-httpd.sh\n+start_httpd\n+\n+test_expect_success 'connectivity check failure due to missing local object' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\ttest_when_finished \"rm -rf \\\"$SERVER\\\" client\" &&\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\n+\ttest_create_repo client &&\n+\ttest_commit -C client bar &&\n+\n+\t# Simulate missing client objects.\n+\trm -rf client/.git/objects/* &&\n+\ttest_must_fail git -C client fetch $HTTPD_URL/smart/server 2>err &&\n+\tgrep \"(from local object store)\" err &&\n+\t! grep \"(from remote)\" err &&\n+\tgrep \"error: connectivity check failed for\" err\n+'\n+\n+test_expect_success 'connectivity check failure due to missing remote object' '\n+\tSERVER=\"$HTTPD_DOCUMENT_ROOT_PATH/server\" &&\n+\ttest_when_finished \"rm -rf \\\"$SERVER\\\" client\" &&\n+\ttest_create_repo \"$SERVER\" &&\n+\ttest_commit -C \"$SERVER\" foo &&\n+\tgit -C \"$SERVER\" config uploadpack.allowRefInWant true &&\n+\tSERVER_HEAD=$(git -C \"$SERVER\" rev-parse HEAD) &&\n+\tSERVER_FAKE_HEAD=$(echo $SERVER_HEAD | tr \"0123456789abcdef\" \"123456789abcdef0\") &&\n+\n+\ttest_create_repo client &&\n+\n+\t# Make the server claim that it has $SERVER_FAKE_HEAD as\n+\t# refs/heads/main. The server still sends $SERVER_HEAD in the packfile,\n+\t# so the client will see $SERVER_FAKE_HEAD as missing.\n+\techo \"s=$SERVER_HEAD refs/heads/main=$SERVER_FAKE_HEAD refs/heads/main= if /wanted-refs/../packfile/\" >\"$HTTPD_ROOT_PATH/one-time-perl\" &&\n+\n+\ttest_must_fail git -C client fetch $HTTPD_URL/one_time_perl/server refs/heads/main 2>err &&\n+\tgrep \"(from remote)\" err &&\n+\t! grep \"(from local object store)\" err &&\n+\tgrep \"error: connectivity check failed for\" err\n+'\n+\n test_done\n+\n-- \n2.36.1.255.ge46751e96f-goog\n\n"},{"id":"456903","messageId":"xmqqh74upyz3.fsf@gitster.g","threadId":"57975","inReplyTo":"20220608210537.185094-1-jonathantanmy@google.com","subject":"Re: [PATCH] connected: distinguish local/remote bad objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-08T22:33:20Z","receivedAt":"2022-06-08T22:33:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index ac29c2b1ae..6f43b2bf8d 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1133,7 +1133,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n>  \n>  \t\trm = ref_map;\n>  \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n> -\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n> +\t\t\trc = error(_(\"connectivity check failed for %s\\n\"), url);\n>  \t\t\tgoto abort;\n>  \t\t}\n>  \t}\n\nClever.\n\nI was wondering how you are going to deal with the different exit\ncondition from the rev-list that is only expressed with the two\ndifferent messages.  You _could_ grep for \"from local\" vs \"from\nremote\", but you just let the human user who is staring at the error\nmessage to read it, and stop mentioning the details in this message.\n\nOK.\n\n> diff --git a/connected.c b/connected.c\n> index ed3025e7a2..ea773f25db 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -94,6 +94,7 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \t\tstrvec_push(&rev_list.args, opt->shallow_file);\n>  \t}\n>  \tstrvec_push(&rev_list.args,\"rev-list\");\n> +\tstrvec_push(&rev_list.args, \"--detailed-bad-object\");\n>  \tstrvec_push(&rev_list.args, \"--objects\");\n>  \tstrvec_push(&rev_list.args, \"--stdin\");\n>  \tif (has_promisor_remote())\n> diff --git a/revision.c b/revision.c\n> index 090a967bf4..777e762373 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -367,6 +367,16 @@ void add_head_to_pending(struct rev_info *revs)\n>  \tadd_pending_object(revs, obj, \"HEAD\");\n>  }\n>  \n> +static void NORETURN bad_object(struct rev_info *revs, const char *name,\n> +\t\t\t\tunsigned int flags)\n> +{\n> +\tif (!revs->detailed_bad_object)\n> +\t\tdie(\"bad object %s\", name);\n> +\tif (flags & UNINTERESTING)\n> +\t\tdie(\"bad object %s (from local object store)\", name);\n> +\tdie(\"bad object %s (from remote)\", name);\n> +}\n\nIf the missing object you found is reachable from existing tips\n(i.e. local aka UNINTERESTING) and also from what they should have\nsent (i.e. remote tips), when we discover that the object does not\nexist locally (but we can have an in-core shell object whose object\nname is already known because child objects that are closer to the\ntips than the missing object do exist and point at it), does this\nnew heuristic work reliably?  \n\nDo we always die and report bad_object() with UNINTERESTING in the\nthe flags variable, or only when we are lucky?\n\nIOW, if our current branch pionts at A, while the other side says\nthey are updating it to B,\n\n    X-----o-----A\n     \\\n      x---B\n\nwe try to traverse \"rev-list ^A B\" and make sure everything exists.\nIf we find objects 'o' missing, it is clear that it was something we\nwere supposed to have on the local side even before we started the\nfetch.  But if 'X' is missing, by the time we notice and report a\nmissing object, do we always reliably know that it ought to be\nreachable from both?  Or do we have 50/50 chance that the traversal\ncomes from 'o' earlier than from 'x' (in which case X is found to be\nmissing when we try to see who is the parent of 'o', and flags have\nUNINTERESTING bit), or later than from 'x' (in which case X is found\nwhen trying to see who the parents of 'x' is, and we may not know\nand we may not bother to find out if X is also a parent of 'o',\nhence we'd still say 'You do not have X, and we were looking at 'x',\nwhich we got from the other end, so they were supposed to have sent\nit', which would be a misdiagnosis)?\n\nThanks.\n"},{"id":"456950","messageId":"xmqqwndpn5dz.fsf@gitster.g","threadId":"57975","inReplyTo":"20220608210537.185094-1-jonathantanmy@google.com","subject":"Re: [PATCH] connected: distinguish local/remote bad objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-09T16:55:20Z","receivedAt":"2022-06-09T16:55:35Z","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>  builtin/fetch.c              |  2 +-\n>  connected.c                  |  1 +\n>  revision.c                   | 16 ++++++++++++--\n>  revision.h                   |  3 +++\n>  t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++\n>  5 files changed, 62 insertions(+), 3 deletions(-)\n\nThis seems to break linux-leaks CI job by making 5518, which was\nmarked in some topic in flight to expect to be leak-free, fail.\n\nBecause of the way linux-leaks test framework is done, it is not\neasy to tell if the code changes essential to this topic introduced\nnew leaks, in which case we would want to fix that.\n\nNote that this may not the fault of the code changes in this patch.\nIf the tests added by the patch started using git commands that are\nknown to leak (i.e. not ready to be subjected to the \"leaks\" test)\nin order to prepare the scenario or to inspect the result, even if\nthe code changes in this topic did not introduce any leak, we can\nsee the same breakage in linux-leaks CI job.  An easy way out would\nbe to disable leak-check CI for the entire 5518, but that is not\nvery satisfactory, as the earlier part of that script should still\nbe leak-free.  Another way out might be to add these two tests in a\nnew script, which is not marked as not-leaking.  After all, what the\nnew topic adds is not about exit status but how that exit status\ncomes about, so it might not be a bad idea even without the CI leak\nstuff anyway.\n\nÆvar, does the internal state used for revision walking count as\nleaking when it is still held by the time we hit die() in\nbad_object(), or anything on stack when we die() are still reachable\nand won't be reported as a failure?\n\nThanks.\n"},{"id":"456951","messageId":"20220609171708.346901-1-jonathantanmy@google.com","threadId":"57975","inReplyTo":"xmqqwndpn5dz.fsf@gitster.g","subject":"Re: [PATCH] connected: distinguish local/remote bad objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-09T17:17:08Z","receivedAt":"2022-06-09T17:17:16Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> >  builtin/fetch.c              |  2 +-\n> >  connected.c                  |  1 +\n> >  revision.c                   | 16 ++++++++++++--\n> >  revision.h                   |  3 +++\n> >  t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++\n> >  5 files changed, 62 insertions(+), 3 deletions(-)\n> \n> This seems to break linux-leaks CI job by making 5518, which was\n> marked in some topic in flight to expect to be leak-free, fail.\n> \n> Because of the way linux-leaks test framework is done, it is not\n> easy to tell if the code changes essential to this topic introduced\n> new leaks, in which case we would want to fix that.\n\nThanks. I'll recheck this once I make a new version.\n"},{"id":"456952","messageId":"20220609171759.347681-1-jonathantanmy@google.com","threadId":"57975","inReplyTo":"xmqqh74upyz3.fsf@gitster.g","subject":"Re: [PATCH] connected: distinguish local/remote bad objects","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-09T17:17:58Z","receivedAt":"2022-06-09T17:18:08Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> If the missing object you found is reachable from existing tips\n> (i.e. local aka UNINTERESTING) and also from what they should have\n> sent (i.e. remote tips), when we discover that the object does not\n> exist locally (but we can have an in-core shell object whose object\n> name is already known because child objects that are closer to the\n> tips than the missing object do exist and point at it), does this\n> new heuristic work reliably?  \n> \n> Do we always die and report bad_object() with UNINTERESTING in the\n> the flags variable, or only when we are lucky?\n> \n> IOW, if our current branch pionts at A, while the other side says\n> they are updating it to B,\n> \n>     X-----o-----A\n>      \\\n>       x---B\n> \n> we try to traverse \"rev-list ^A B\" and make sure everything exists.\n> If we find objects 'o' missing, it is clear that it was something we\n> were supposed to have on the local side even before we started the\n> fetch.  But if 'X' is missing, by the time we notice and report a\n> missing object, do we always reliably know that it ought to be\n> reachable from both?  Or do we have 50/50 chance that the traversal\n> comes from 'o' earlier than from 'x' (in which case X is found to be\n> missing when we try to see who is the parent of 'o', and flags have\n> UNINTERESTING bit), or later than from 'x' (in which case X is found\n> when trying to see who the parents of 'x' is, and we may not know\n> and we may not bother to find out if X is also a parent of 'o',\n> hence we'd still say 'You do not have X, and we were looking at 'x',\n> which we got from the other end, so they were supposed to have sent\n> it', which would be a misdiagnosis)?\n> \n> Thanks.\n\nAh, good catch. I'll take a look into fixing this. (A misdiagnosis would\ndefeat the purpose of this patch, yes.)\n"},{"id":"456955","messageId":"220609.86zgilsofm.gmgdl@evledraar.gmail.com","threadId":"57975","inReplyTo":"xmqqwndpn5dz.fsf@gitster.g","subject":"Re: [PATCH] connected: distinguish local/remote bad objects","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2022-06-09T18:00:05Z","receivedAt":"2022-06-09T18:05:09Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Thu, Jun 09 2022, Junio C Hamano wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n>\n>>  builtin/fetch.c              |  2 +-\n>>  connected.c                  |  1 +\n>>  revision.c                   | 16 ++++++++++++--\n>>  revision.h                   |  3 +++\n>>  t/t5518-fetch-exit-status.sh | 43 ++++++++++++++++++++++++++++++++++++\n>>  5 files changed, 62 insertions(+), 3 deletions(-)\n>\n> This seems to break linux-leaks CI job by making 5518, which was\n> marked in some topic in flight to expect to be leak-free, fail.\n>\n> Because of the way linux-leaks test framework is done, it is not\n> easy to tell if the code changes essential to this topic introduced\n> new leaks, in which case we would want to fix that.\n\nI think this is just an existing leak that happens to be exposed by a\nnew (in this file) test, i.e. transport_get() leaks via an xmalloc() for\ntransport_helper.\n\n> Note that this may not the fault of the code changes in this patch.\n> If the tests added by the patch started using git commands that are\n> known to leak (i.e. not ready to be subjected to the \"leaks\" test)\n> in order to prepare the scenario or to inspect the result, even if\n> the code changes in this topic did not introduce any leak, we can\n> see the same breakage in linux-leaks CI job.  An easy way out would\n> be to disable leak-check CI for the entire 5518, but that is not\n> very satisfactory, as the earlier part of that script should still\n> be leak-free.\n\nI think doing that would be fine in this case. It will get easier to fix\nleaks now that \"struct rev_info\" is out of the way (and I've got a lot\nof pending patches), but I can always loop back & re-mark this\nparticular test as leak-free at some future date.\n\n> Another way out might be to add these two tests in a\n> new script, which is not marked as not-leaking.  After all, what the\n> new topic adds is not about exit status but how that exit status\n> comes about, so it might not be a bad idea even without the CI leak\n> stuff anyway.\n\nYeah, that sounds especially good in this case, as if we can't run httpd\nwe'll print a meaningful \"skip\" message in that case. See 0a2bfccb9c8\n(t0051: use \"skip_all\" under !MINGW in single-test file, 2022-02-04)\n\n> Ævar, does the internal state used for revision walking count as\n> leaking when it is still held by the time we hit die() in\n> bad_object(), or anything on stack when we die() are still reachable\n> and won't be reported as a failure?\n\nNo, but in this case the variable containing the leaked data isn't in\nscope by the time we exit, i.e. it was used by fetch_one() which had it\nmalloc'd, but the struct it lived in went away, and now we're exiting\nfrom cmd_fetch() etc.\n"},{"id":"457041","messageId":"20220610195247.1177549-1-jonathantanmy@google.com","threadId":"57975","inReplyTo":"20220608210537.185094-1-jonathantanmy@google.com","subject":"[PATCH v2] fetch,fetch-pack: clarify connectivity check error","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-10T19:52:47Z","receivedAt":"2022-06-10T19:52:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When the connectivity check after a fetch fails, an error message\n\"<remote> did not send all necessary objects\" is printed. That error\nmessage is printed regardless of the reason of failure: in particular,\nthat message may be printed if the connectivity check fails because a\nlocal object is missing. (The connectivity check reads local objects too\nbecause it compares the set of objects that the remote claims to send\nagainst the set of objects that our refs directly or indirectly\nreference.)\n\nSince it is not necessarily true that the remote did not send all\nnecessary objects, update the error message to be something that more\ncorrectly reflects the situation.\n\nThe updated error message is admittedly vague and one alternative\nsolution would be to update the revision walking code to be able to more\nprecisely specify if a bad object is supposed to be available locally or\nsupposed to have been sent by the remote. That would likely require, in\nthe revision walking code, to delay parsing any object until all its\nchildren has been parsed, which seems to be a significant undertaking.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\nAs Junio said in [1], my original v1 code doesn't work when an object is\nreferenced both by a local object and a remote one. I tried looking into\nmaking it work but it looks difficult.\n\nSo here is a patch that just changes the error message to a vaguer but\nhopefully more correct one. I wonder what the community thinks of it -\nI think it is more correct (and means that we do not need to say \"it's\nnot the remote fault despite what the error message says\") but at the\nsame time, many people are already used to this message (and a search\nonline reveals several web sites that talk about this).\n\n[1] https://lore.kernel.org/git/xmqqh74upyz3.fsf@gitster.g/\n---\n builtin/fetch.c | 2 +-\n fetch-pack.c    | 2 +-\n 2 files changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ac29c2b1ae..15737ca293 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1133,7 +1133,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \n \t\trm = ref_map;\n \t\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n-\t\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n+\t\t\trc = error(_(\"connectivity check failed after fetching from %s\\n\"), url);\n \t\t\tgoto abort;\n \t\t}\n \t}\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex cb6647d657..91269008cc 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -2021,7 +2021,7 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tif (args->deepen)\n \t\t\topt.is_deepening_fetch = 1;\n \t\tif (check_connected(iterate_ref_map, &iterator, &opt)) {\n-\t\t\terror(_(\"remote did not send all necessary objects\"));\n+\t\t\terror(_(\"connectivity check failed after fetching from remote\"));\n \t\t\tfree_refs(ref_cpy);\n \t\t\tref_cpy = NULL;\n \t\t\trollback_shallow_file(the_repository, &shallow_lock);\n-- \n2.36.1.476.g0c4daa206d-goog\n\n"},{"id":"457042","messageId":"xmqqr13wclll.fsf@gitster.g","threadId":"57975","inReplyTo":"20220610195247.1177549-1-jonathantanmy@google.com","subject":"Re: [PATCH v2] fetch,fetch-pack: clarify connectivity check error","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2022-06-10T20:25:10Z","receivedAt":"2022-06-10T20:27:33Z","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 the connectivity check after a fetch fails, an error message\n> \"<remote> did not send all necessary objects\" is printed. That error\n> message is printed regardless of the reason of failure: in particular,\n> that message may be printed if the connectivity check fails because a\n> local object is missing. (The connectivity check reads local objects too\n> because it compares the set of objects that the remote claims to send\n> against the set of objects that our refs directly or indirectly\n> reference.)\n>\n> Since it is not necessarily true that the remote did not send all\n> necessary objects, update the error message to be something that more\n> correctly reflects the situation.\n>\n> The updated error message is admittedly vague and one alternative\n> solution would be to update the revision walking code to be able to more\n> precisely specify if a bad object is supposed to be available locally or\n> supposed to have been sent by the remote. That would likely require, in\n> the revision walking code, to delay parsing any object until all its\n> children has been parsed, which seems to be a significant undertaking.\n>\n\nWhile that is all true, I do not think the posted patch as-is is a\ngood idea.  After all, object missing on our side locally before you\nstart \"git fetch\" is a local repository corruption, which ought to\nbe a much rarer event than a still in development and in flux server\nend sending nonsense packfiles, no?\n\nAt least, \"they didn't do X\" would give the user somewhere to start\ninvestigation (e.g. complain to the server folks about the error,\nstating where they started from and what they tried to fetch).  The\nnew message may be playing it \"safe\" by not saying anything you are\nnot absolutely sure about, but that is much less useful to the users\nwho got the message.\n\nNow, when check_connected() fails early in store_updated_refs(), we\nhaven't updated the remote-tracking refs to point at the tips we\nobtained, have we?\n\nOne thing we could do is to run \"git fsck\" to see if the local\nhistory is broken.  We may have added unreferenced objects that have\ncome from the remote end, but having dangling or unreachable objects\nis not an error, while it is a sure sign of repository corruption if\nthere are missing objects.  Because we haven't update any of the\nrefs (local or remote-tracking), we still should be able to see if\nthe problem is on our end at this point\n\nIf \"o\" or \"X\" (in the illustrated history in my review of your v1)\nis found to be missing by \"git fsck\" (or \"git fsck A\"), then you\nknow that the server end is not the culprit (or at least may not be\nthe sole culprit) for this connectivity check failure.  If \"git\nfsck\" reports no missing objects, then what was found to be missing\nis not \"X\" but is \"x\" or \"B\", and we are more certain that the\nremote end is more likely to be problematic.\n\nAs it is on the error codepath, we could even automatically run fsck\nto figure out if we have objects missing after giving an initial\ndiagnosis, but at least it would be easy to give an advice at this\npoint in the code to run fsck after seeing a failure from this\nfetch.\n\nAs you have two places that show the error, perhaps the first step\nis to introduce a helper function:\n\n\tint diagnose_check_connected_error(const char *url)\n\t{\n\t\treturn error(_(\"%s may not have sent necessary objects\"),\n                \t\turl ? url : \"remote\");\n\t}\n\nand call it instead of calling error() in the places you touched in\nv2.\n\nAs a follow up step, then, we could do something like\n\n\tint diagnose_check_connected_error(const char *url)\n\t{\n+\t\tstatic const char advice[] =\n+\t\t\tN_(\"run 'git fsck' to see if your repository \"\n+\t\t\t\"is missing objects; if it passes, the problem is \"\n+\t\t\t\"on the remote end\");\n+\n+\t\tadvise_if_enabled(ADVICE_CONNECTIVETY_FAILURE, advice);\n\t\treturn error(_(\"%s may not have sent necessary\tobjects\"),\n                \t\turl ? url : \"remote\");\n\t}\n\nor if we are more ambitious, actually run \"git fsck\" via the run_command() \nAPI for the user.\n"},{"id":"457460","messageId":"20220617200301.2700839-1-jonathantanmy@google.com","threadId":"57975","inReplyTo":"xmqqr13wclll.fsf@gitster.g","subject":"Re: [PATCH v2] fetch,fetch-pack: clarify connectivity check error","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2022-06-17T20:03:01Z","receivedAt":"2022-06-17T20:03:07Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> writes:\n> While that is all true, I do not think the posted patch as-is is a\n> good idea.  After all, object missing on our side locally before you\n> start \"git fetch\" is a local repository corruption, which ought to\n> be a much rarer event than a still in development and in flux server\n> end sending nonsense packfiles, no?\n> \n> At least, \"they didn't do X\" would give the user somewhere to start\n> investigation (e.g. complain to the server folks about the error,\n> stating where they started from and what they tried to fetch).  The\n> new message may be playing it \"safe\" by not saying anything you are\n> not absolutely sure about, but that is much less useful to the users\n> who got the message.\n\nHmm...that's true. Let me try to understand the revision walking code a\nbit more, and if by then I still don't end up changing the revision walk\nto avoid prematurely parsing a parent, I'll make a patch with your\nsuggestion.\n"}]}