{"thread":{"id":"48790","subject":"[PATCH] fetch: when deepening, check connectivity fully","startedAt":"2018-06-27T17:32:44Z","lastAt":"2018-06-29T22:30:33Z","messageCount":8,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"351125","messageId":"20180627173237.25620-1-jonathantanmy@google.com","threadId":"48790","inReplyTo":null,"subject":"[PATCH] fetch: when deepening, check connectivity fully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-27T17:32:37Z","receivedAt":"2018-06-27T17:32:44Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"Do not stop at ancestors of our refs when deepening during fetching.\nThis is because when performing such a fetch, shallow entries may have\nbeen updated; the client can reasonably assume that the existing refs\nare connected up to the old shallow points, but not the new.\n\nThis was noticed when a user tried to deepen an existing repository by\nfetching with --no-shallow from a server that did not send all necessary\nobjects - the connectivity check as run by \"git fetch\" succeeded, but a\nsubsequent \"git fsck\" failed.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n builtin/fetch.c          |  5 ++++-\n connected.c              |  6 ++++--\n connected.h              |  7 +++++++\n t/t5537-fetch-shallow.sh | 23 +++++++++++++++++++++++\n 4 files changed, 38 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ea5b9669ad..2cfd13342f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -780,6 +780,7 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \tconst char *filename = dry_run ? \"/dev/null\" : git_path_fetch_head();\n \tint want_status;\n \tint summary_width = transport_summary_width(ref_map);\n+\tstruct check_connected_options opt = CHECK_CONNECTED_INIT;\n \n \tfp = fopen(filename, \"a\");\n \tif (!fp)\n@@ -791,7 +792,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \t\turl = xstrdup(\"foreign\");\n \n \trm = ref_map;\n-\tif (check_connected(iterate_ref_map, &rm, NULL)) {\n+\tif (deepen)\n+\t\topt.is_deepening_fetch = 1;\n+\tif (check_connected(iterate_ref_map, &rm, &opt)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\ndiff --git a/connected.c b/connected.c\nindex 91feb78815..1bba888eff 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -58,8 +58,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n \targv_array_push(&rev_list.args, \"--stdin\");\n \tif (repository_format_partial_clone)\n \t\targv_array_push(&rev_list.args, \"--exclude-promisor-objects\");\n-\targv_array_push(&rev_list.args, \"--not\");\n-\targv_array_push(&rev_list.args, \"--all\");\n+\tif (!opt->is_deepening_fetch) {\n+\t\targv_array_push(&rev_list.args, \"--not\");\n+\t\targv_array_push(&rev_list.args, \"--all\");\n+\t}\n \targv_array_push(&rev_list.args, \"--quiet\");\n \tif (opt->progress)\n \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\ndiff --git a/connected.h b/connected.h\nindex a53f03a61a..322dc76372 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -38,6 +38,13 @@ struct check_connected_options {\n \t * Insert these variables into the environment of the child process.\n \t */\n \tconst char **env;\n+\n+\t/*\n+\t * If non-zero, check the ancestry chain completely, not stopping at\n+\t * any existing ref. This is necessary when deepening existing refs\n+\t * during a fetch.\n+\t */\n+\tunsigned is_deepening_fetch : 1;\n };\n \n #define CHECK_CONNECTED_INIT { 0 }\ndiff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh\nindex df8d2f095a..ac4beac96b 100755\n--- a/t/t5537-fetch-shallow.sh\n+++ b/t/t5537-fetch-shallow.sh\n@@ -186,4 +186,27 @@ EOF\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'shallow fetches check connectivity without stopping at existing refs' '\n+\tcp -R .git server.git &&\n+\n+\t# Normally, the connectivity check stops at ancestors of existing refs.\n+\tgit init client &&\n+\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server.git\" &&\n+\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n+\tgrep -e \"--not --all\" rev-list-command &&\n+\n+\t# But it does not for a shallow fetch...\n+\trm -rf client trace &&\n+\tgit init client &&\n+\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --depth=1 \"$(pwd)/server.git\" &&\n+\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n+\t! grep -e \"--not --all\" rev-list-command &&\n+\n+\t# ...and when deepening.\n+\trm trace &&\n+\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --unshallow \"$(pwd)/server.git\" &&\n+\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n+\t! grep -e \"--not --all\" rev-list-command\n+'\n+\n test_done\n-- \n2.18.0.rc2.346.g013aa6912e-goog\n\n"},{"id":"351157","messageId":"xmqqin64dmui.fsf@gitster-ct.c.googlers.com","threadId":"48790","inReplyTo":"20180627173237.25620-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-27T19:57:09Z","receivedAt":"2018-06-27T19:57:16Z","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> Do not stop at ancestors of our refs when deepening during fetching.\n> This is because when performing such a fetch, shallow entries may have\n> been updated; the client can reasonably assume that the existing refs\n> are connected up to the old shallow points, but not the new.\n\nOK.\n\n> diff --git a/connected.c b/connected.c\n> index 91feb78815..1bba888eff 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -58,8 +58,10 @@ int check_connected(oid_iterate_fn fn, void *cb_data,\n>  \targv_array_push(&rev_list.args, \"--stdin\");\n>  \tif (repository_format_partial_clone)\n>  \t\targv_array_push(&rev_list.args, \"--exclude-promisor-objects\");\n> -\targv_array_push(&rev_list.args, \"--not\");\n> -\targv_array_push(&rev_list.args, \"--all\");\n> +\tif (!opt->is_deepening_fetch) {\n> +\t\targv_array_push(&rev_list.args, \"--not\");\n> +\t\targv_array_push(&rev_list.args, \"--all\");\n> +\t}\n>  \targv_array_push(&rev_list.args, \"--quiet\");\n>  \tif (opt->progress)\n>  \t\targv_array_pushf(&rev_list.args, \"--progress=%s\",\n\nHmph, remind me how old and/or new shallow cut-off points affect\nthis traversal?  In order to verify that everything above the new\ncut-off points, opt->shallow_file should be pointing at the new\ncut-off points, then we do the full sweep (without --not --all) to\nensure we won't find missing or broken objects but still stop at the\nnew cut-off points, and then only after check_connected() passes,\nupdate the shallow file to make new shallow cut-off points active\n(and if the traversal fails, then we do nto install the new shallow\ncut-off points)?\n\nIf so, that sounds sensible.  By not having \"--not --all\", we\npotentially would do a full sweep, but if we are really deepening to\nthe root commits, then we do need one full sweep anyway (as these\ndeepest parts of the history were previously hidden by the shallow\ncutoff points), and if we are only deepening the history by a bit,\neven if we do not have \"--not --all\", we would hit the updated\nshallow cutoff points (which may be at older parts of the history)\nand stop, and for correctness we need to visit there anyway, so I\nthink we are not being overly pessimistic with this change, either.\n\n"},{"id":"351159","messageId":"xmqqa7rgdlwf.fsf@gitster-ct.c.googlers.com","threadId":"48790","inReplyTo":"20180627173237.25620-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-27T20:17:36Z","receivedAt":"2018-06-27T20:17:41Z","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> +test_expect_success 'shallow fetches check connectivity without stopping at existing refs' '\n> +\tcp -R .git server.git &&\n> +\n> +\t# Normally, the connectivity check stops at ancestors of existing refs.\n> +\tgit init client &&\n> +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server.git\" &&\n> +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> +\tgrep -e \"--not --all\" rev-list-command &&\n> +\n> +\t# But it does not for a shallow fetch...\n> +\trm -rf client trace &&\n> +\tgit init client &&\n> +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --depth=1 \"$(pwd)/server.git\" &&\n> +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> +\t! grep -e \"--not --all\" rev-list-command &&\n> +\n> +\t# ...and when deepening.\n> +\trm trace &&\n> +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --unshallow \"$(pwd)/server.git\" &&\n> +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> +\t! grep -e \"--not --all\" rev-list-command\n> +'\n\nHmph, don't we quote these in the trace output, requiring us to grep\nfor \"'--not' '--all'\" or somesuch?  \n\nI do not think of a better way to do the above without a huge effort\noffhand, and the approach taken by the above may be the best we\ncould do, but it looks like quite a brittle test that knows too much\nabout the current implementation.  \"rev-list $new_commits --not\n--all\" is a so very common and useful pattern that it is not all\nthat implausible that we may want to come up with a new option to do\nso, or more likely we may want to do that with an in-process API\nwithout spawning an external rev-list (hence making it impossible to\nobserve via GIT_TRACE).\n"},{"id":"351176","messageId":"20180627224033.150025-1-jonathantanmy@google.com","threadId":"48790","inReplyTo":"xmqqin64dmui.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-27T22:40:33Z","receivedAt":"2018-06-27T22:40:40Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Hmph, remind me how old and/or new shallow cut-off points affect\n> this traversal?  In order to verify that everything above the new\n> cut-off points, opt->shallow_file should be pointing at the new\n> cut-off points, then we do the full sweep (without --not --all) to\n> ensure we won't find missing or broken objects but still stop at the\n> new cut-off points, and then only after check_connected() passes,\n> update the shallow file to make new shallow cut-off points active\n> (and if the traversal fails, then we do nto install the new shallow\n> cut-off points)?\n\nThat is the way it should work, but after thinking about it once more, I\nrealize that it isn't.\n\nopt->shallow_file is not set to anything. And fetch-pack updates the\nshallow file by itself (at least, that is my understanding of\nupdate_shallow() in fetch-pack.c) before fetch calls check_connected(),\nmeaning that if check_connected() fails, there is still no rollback of\nthe shallow file.\n\nSo to directly answer your first question, only the new shallow cut-off\npoints affect this traversal, and not the old ones.\n\n> If so, that sounds sensible.  By not having \"--not --all\", we\n> potentially would do a full sweep, but if we are really deepening to\n> the root commits, then we do need one full sweep anyway (as these\n> deepest parts of the history were previously hidden by the shallow\n> cutoff points), and if we are only deepening the history by a bit,\n> even if we do not have \"--not --all\", we would hit the updated\n> shallow cutoff points (which may be at older parts of the history)\n> and stop, and for correctness we need to visit there anyway, so I\n> think we are not being overly pessimistic with this change, either.\n\nYes, this analysis makes sense.\n"},{"id":"351177","messageId":"20180627225105.155996-1-jonathantanmy@google.com","threadId":"48790","inReplyTo":"xmqqa7rgdlwf.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-27T22:51:05Z","receivedAt":"2018-06-27T22:51:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > +test_expect_success 'shallow fetches check connectivity without stopping at existing refs' '\n> > +\tcp -R .git server.git &&\n> > +\n> > +\t# Normally, the connectivity check stops at ancestors of existing refs.\n> > +\tgit init client &&\n> > +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch \"$(pwd)/server.git\" &&\n> > +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> > +\tgrep -e \"--not --all\" rev-list-command &&\n> > +\n> > +\t# But it does not for a shallow fetch...\n> > +\trm -rf client trace &&\n> > +\tgit init client &&\n> > +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --depth=1 \"$(pwd)/server.git\" &&\n> > +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> > +\t! grep -e \"--not --all\" rev-list-command &&\n> > +\n> > +\t# ...and when deepening.\n> > +\trm trace &&\n> > +\tGIT_TRACE=\"$(pwd)/trace\" git -C client fetch --unshallow \"$(pwd)/server.git\" &&\n> > +\tgrep \"run_command: git rev-list\" trace >rev-list-command &&\n> > +\t! grep -e \"--not --all\" rev-list-command\n> > +'\n> \n> Hmph, don't we quote these in the trace output, requiring us to grep\n> for \"'--not' '--all'\" or somesuch?  \n\nI thought so too, but this was changed in commit 1fbdab21bb (\"trace:\navoid unnecessary quoting\", 2018-01-16).\n\n> I do not think of a better way to do the above without a huge effort\n> offhand, and the approach taken by the above may be the best we\n> could do, but it looks like quite a brittle test that knows too much\n> about the current implementation.  \"rev-list $new_commits --not\n> --all\" is a so very common and useful pattern that it is not all\n> that implausible that we may want to come up with a new option to do\n> so, or more likely we may want to do that with an in-process API\n> without spawning an external rev-list (hence making it impossible to\n> observe via GIT_TRACE).\n\nI agree. The best way to do it would probably be to intercept the fetch\nresponse and substitute an empty packfile for the packfile returned by\nthe fetch, like the one-time-sed mechanism [1], but I think that it is\noutside the scope of this patch.\n\n[1] https://public-inbox.org/git/afe5d7d3f876893fdad318665805df1e056717c6.1485381677.git.jonathantanmy@google.com/\n"},{"id":"351178","messageId":"xmqqwoujdejl.fsf@gitster-ct.c.googlers.com","threadId":"48790","inReplyTo":"20180627224033.150025-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-27T22:56:30Z","receivedAt":"2018-06-27T22:56: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>> Hmph, remind me how old and/or new shallow cut-off points affect\n>> this traversal?  In order to verify that everything above the new\n>> cut-off points, opt->shallow_file should be pointing at the new\n>> cut-off points, then we do the full sweep (without --not --all) to\n>> ensure we won't find missing or broken objects but still stop at the\n>> new cut-off points, and then only after check_connected() passes,\n>> update the shallow file to make new shallow cut-off points active\n>> (and if the traversal fails, then we do nto install the new shallow\n>> cut-off points)?\n>\n> That is the way it should work, but after thinking about it once more, I\n> realize that it isn't.\n>\n> opt->shallow_file is not set to anything. And fetch-pack updates the\n> shallow file by itself (at least, that is my understanding of\n> update_shallow() in fetch-pack.c) before fetch calls check_connected(),\n> meaning that if check_connected() fails, there is still no rollback of\n> the shallow file.\n\nOuch.  We need to fix that; otherwise, a broken server will keep\ngiving you a corrupt repository even with this fix, no?\n\n"},{"id":"351179","messageId":"xmqqsh57degj.fsf@gitster-ct.c.googlers.com","threadId":"48790","inReplyTo":"20180627225105.155996-1-jonathantanmy@google.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-06-27T22:58:20Z","receivedAt":"2018-06-27T22:58:25Z","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>> Hmph, don't we quote these in the trace output, requiring us to grep\n>> for \"'--not' '--all'\" or somesuch?  \n>\n> I thought so too, but this was changed in commit 1fbdab21bb (\"trace:\n> avoid unnecessary quoting\", 2018-01-16).\n\nYup; fortunately or unfortunately, that and the \"partial clone\"\nstuff are among the differences between v2.16 track and newer, so we\ncan just forget about v2.16.x and lower and target this fix to\nmaint-2.17 and above.\n\n"},{"id":"351391","messageId":"20180629223024.62002-1-jonathantanmy@google.com","threadId":"48790","inReplyTo":"xmqqwoujdejl.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] fetch: when deepening, check connectivity fully","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-06-29T22:30:24Z","receivedAt":"2018-06-29T22:30:33Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"> > That is the way it should work, but after thinking about it once more, I\n> > realize that it isn't.\n> >\n> > opt->shallow_file is not set to anything. And fetch-pack updates the\n> > shallow file by itself (at least, that is my understanding of\n> > update_shallow() in fetch-pack.c) before fetch calls check_connected(),\n> > meaning that if check_connected() fails, there is still no rollback of\n> > the shallow file.\n> \n> Ouch.  We need to fix that; otherwise, a broken server will keep\n> giving you a corrupt repository even with this fix, no?\n\nYes, that is true - the repository will be left in a corrupt state.\n\nI did some more investigation, and usually things are OK because\nunpack-trees runs a fsck_walk on all the objects it unpacks (with\n--shallow-file appropriately set). Things are bad only if the packfile\nis empty (or, presumably, if the packfile only has unrelated objects).\n\nI managed to use the one-time-sed mechanism to craft a response that\ntriggers this case. This both enables me to avoid the brittle check of\n\"rev-list\" appearing in the GIT_TRACE output (as discussed in [1]), and\nto verify that a rollback of the shallow file works, once such a patch\nis written. I'll look into writing such a patch, so feel free to hold\noff on this until that patch is done.\n\n[1] https://public-inbox.org/git/20180627225105.155996-1-jonathantanmy@google.com/\n"}]}