{"thread":{"id":"48043","subject":"[PATCH 0/2] Make partial clone/fetch work when transfer.fsckobjects=1","startedAt":"2018-03-14T18:42:58Z","lastAt":"2018-03-15T17:15:20Z","messageCount":6,"participants":["Jonathan Tan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"341684","messageId":"cover.1521052825.git.jonathantanmy@google.com","threadId":"48043","inReplyTo":null,"subject":"[PATCH 0/2] Make partial clone/fetch work when transfer.fsckobjects=1","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-03-14T18:42:39Z","receivedAt":"2018-03-14T18:42:58Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"One of my colleagues noticed that we obtain a \"fatal: did not receive\nexpected object\" error when partial-cloning (that is, with --filter set)\nif transfer.fsckobjects is true. Here's a fix for that.\n\nJonathan Tan (2):\n  index-pack: support checking objects but not links\n  fetch-pack: do not check links for partial fetch\n\n Documentation/git-index-pack.txt |  3 +++\n builtin/index-pack.c             |  6 ++++--\n fetch-pack.c                     | 13 +++++++++++--\n t/t5302-pack-index.sh            |  5 +++++\n t/t5616-partial-clone.sh         | 11 +++++++++++\n 5 files changed, 34 insertions(+), 4 deletions(-)\n\n-- \n2.16.2.520.gd0db9edba.dirty\n\n"},{"id":"341685","messageId":"0ec687d21f69270a07f4bc86dcae6bb40f899755.1521052825.git.jonathantanmy@google.com","threadId":"48043","inReplyTo":"cover.1521052825.git.jonathantanmy@google.com","subject":"[PATCH 1/2] index-pack: support checking objects but not links","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-03-14T18:42:40Z","receivedAt":"2018-03-14T18:43:01Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"The index-pack command currently supports the\n--check-self-contained-and-connected argument, for internal use only,\nthat instructs it to only check for broken links and not broken objects.\nFor partial clones, we need the inverse, so add a --fsck-objects\nargument that checks for broken objects and not broken links, also for\ninternal use only.\n\nThis will be used by fetch-pack in a subsequent patch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n Documentation/git-index-pack.txt | 3 +++\n builtin/index-pack.c             | 6 ++++--\n t/t5302-pack-index.sh            | 5 +++++\n 3 files changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex 1b4b65d66..138edb47b 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -77,6 +77,9 @@ OPTIONS\n --check-self-contained-and-connected::\n \tDie if the pack contains broken links. For internal use only.\n \n+--fsck-objects::\n+\tDie if the pack contains broken objects. For internal use only.\n+\n --threads=<n>::\n \tSpecifies the number of threads to spawn when resolving\n \tdeltas. This requires that index-pack be compiled with\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 59878e70b..f46cb5967 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -827,7 +827,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\tfree(has_data);\n \t}\n \n-\tif (strict) {\n+\tif (strict || do_fsck_object) {\n \t\tread_lock();\n \t\tif (type == OBJ_BLOB) {\n \t\t\tstruct blob *blob = lookup_blob(oid);\n@@ -853,7 +853,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tif (do_fsck_object &&\n \t\t\t    fsck_object(obj, buf, size, &fsck_options))\n \t\t\t\tdie(_(\"Error in object\"));\n-\t\t\tif (fsck_walk(obj, NULL, &fsck_options))\n+\t\t\tif (strict && fsck_walk(obj, NULL, &fsck_options))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), oid_to_hex(&obj->oid));\n \n \t\t\tif (obj->type == OBJ_TREE) {\n@@ -1688,6 +1688,8 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t} else if (!strcmp(arg, \"--check-self-contained-and-connected\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcheck_self_contained_and_connected = 1;\n+\t\t\t} else if (!strcmp(arg, \"--fsck-objects\")) {\n+\t\t\t\tdo_fsck_object = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\ndiff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh\nindex c2fc584da..d695a6082 100755\n--- a/t/t5302-pack-index.sh\n+++ b/t/t5302-pack-index.sh\n@@ -262,4 +262,9 @@ EOF\n     grep \"^warning:.* expected .tagger. line\" err\n '\n \n+test_expect_success 'index-pack --fsck-objects also warns upon missing tagger in tag' '\n+    git index-pack --fsck-objects tag-test-${pack1}.pack 2>err &&\n+    grep \"^warning:.* expected .tagger. line\" err\n+'\n+\n test_done\n-- \n2.16.2.520.gd0db9edba.dirty\n\n"},{"id":"341686","messageId":"c2f59866a1b1dc5f11713e1922eac2521ef98b07.1521052825.git.jonathantanmy@google.com","threadId":"48043","inReplyTo":"cover.1521052825.git.jonathantanmy@google.com","subject":"[PATCH 2/2] fetch-pack: do not check links for partial fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-03-14T18:42:41Z","receivedAt":"2018-03-14T18:43:02Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"When doing a partial clone or fetch with transfer.fsckobjects=1, use the\n--fsck-objects instead of the --strict flag when invoking index-pack so\nthat links are not checked, only objects. This is because incomplete\nlinks are expected when doing a partial clone or fetch.\n\nSigned-off-by: Jonathan Tan <jonathantanmy@google.com>\n---\n fetch-pack.c             | 13 +++++++++++--\n t/t5616-partial-clone.sh | 11 +++++++++++\n 2 files changed, 22 insertions(+), 2 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex d97461296..1d6117565 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -886,8 +886,17 @@ static int get_pack(struct fetch_pack_args *args,\n \t    ? fetch_fsck_objects\n \t    : transfer_fsck_objects >= 0\n \t    ? transfer_fsck_objects\n-\t    : 0)\n-\t\targv_array_push(&cmd.args, \"--strict\");\n+\t    : 0) {\n+\t\tif (args->from_promisor)\n+\t\t\t/*\n+\t\t\t * We cannot use --strict in index-pack because it\n+\t\t\t * checks both broken objects and links, but we only\n+\t\t\t * want to check for broken objects.\n+\t\t\t */\n+\t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n+\t\telse\n+\t\t\targv_array_push(&cmd.args, \"--strict\");\n+\t}\n \n \tcmd.in = demux.out;\n \tcmd.git_cmd = 1;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 29d863118..cee556536 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -143,4 +143,15 @@ test_expect_success 'manual prefetch of missing objects' '\n \ttest_line_count = 0 observed.oids\n '\n \n+test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '\n+\tgit init src &&\n+\ttest_commit -C src x &&\n+\ttest_config -C src uploadpack.allowfilter 1 &&\n+\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n+\n+\tGIT_TRACE=\"$(pwd)/trace\" git -c transfer.fsckobjects=1 \\\n+\t\tclone --filter=\"blob:none\" \"file://$(pwd)/src\" dst &&\n+\tgrep \"git index-pack.*--fsck-objects\" trace\n+'\n+\n test_done\n-- \n2.16.2.520.gd0db9edba.dirty\n\n"},{"id":"341718","messageId":"xmqqwoyexqy4.fsf@gitster-ct.c.googlers.com","threadId":"48043","inReplyTo":"c2f59866a1b1dc5f11713e1922eac2521ef98b07.1521052825.git.jonathantanmy@google.com","subject":"Re: [PATCH 2/2] fetch-pack: do not check links for partial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-14T21:55:31Z","receivedAt":"2018-03-14T21:55:40Z","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 doing a partial clone or fetch with transfer.fsckobjects=1, use the\n> --fsck-objects instead of the --strict flag when invoking index-pack so\n> that links are not checked, only objects. This is because incomplete\n> links are expected when doing a partial clone or fetch.\n\nIt is expected that _some_ links are missing, but this makes me\nwonder if we can do better than disabling the connectivity check\naltogether.  Does \"git fetch\" lack sufficient information to attempt\nthe connectivity check, and when (and only when) it hits a broken\nlink, see if that is because the connectivity check traversal is\ncrossing a \"partial\" fetch boundary, or something along that line?\n\n>\n> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>\n> ---\n>  fetch-pack.c             | 13 +++++++++++--\n>  t/t5616-partial-clone.sh | 11 +++++++++++\n>  2 files changed, 22 insertions(+), 2 deletions(-)\n>\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index d97461296..1d6117565 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -886,8 +886,17 @@ static int get_pack(struct fetch_pack_args *args,\n>  \t    ? fetch_fsck_objects\n>  \t    : transfer_fsck_objects >= 0\n>  \t    ? transfer_fsck_objects\n> -\t    : 0)\n> -\t\targv_array_push(&cmd.args, \"--strict\");\n> +\t    : 0) {\n> +\t\tif (args->from_promisor)\n> +\t\t\t/*\n> +\t\t\t * We cannot use --strict in index-pack because it\n> +\t\t\t * checks both broken objects and links, but we only\n> +\t\t\t * want to check for broken objects.\n> +\t\t\t */\n> +\t\t\targv_array_push(&cmd.args, \"--fsck-objects\");\n> +\t\telse\n> +\t\t\targv_array_push(&cmd.args, \"--strict\");\n> +\t}\n>  \n>  \tcmd.in = demux.out;\n>  \tcmd.git_cmd = 1;\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> index 29d863118..cee556536 100755\n> --- a/t/t5616-partial-clone.sh\n> +++ b/t/t5616-partial-clone.sh\n> @@ -143,4 +143,15 @@ test_expect_success 'manual prefetch of missing objects' '\n>  \ttest_line_count = 0 observed.oids\n>  '\n>  \n> +test_expect_success 'partial clone with transfer.fsckobjects=1 uses index-pack --fsck-objects' '\n> +\tgit init src &&\n> +\ttest_commit -C src x &&\n> +\ttest_config -C src uploadpack.allowfilter 1 &&\n> +\ttest_config -C src uploadpack.allowanysha1inwant 1 &&\n> +\n> +\tGIT_TRACE=\"$(pwd)/trace\" git -c transfer.fsckobjects=1 \\\n> +\t\tclone --filter=\"blob:none\" \"file://$(pwd)/src\" dst &&\n> +\tgrep \"git index-pack.*--fsck-objects\" trace\n> +'\n> +\n>  test_done\n"},{"id":"341724","messageId":"20180314152429.df14adfb49b028e2e2a9001a@google.com","threadId":"48043","inReplyTo":"xmqqwoyexqy4.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 2/2] fetch-pack: do not check links for partial fetch","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2018-03-14T22:24:29Z","receivedAt":"2018-03-14T22:24:37Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On Wed, 14 Mar 2018 14:55:31 -0700\nJunio C Hamano <gitster@pobox.com> wrote:\n\n> Jonathan Tan <jonathantanmy@google.com> writes:\n> \n> > When doing a partial clone or fetch with transfer.fsckobjects=1, use the\n> > --fsck-objects instead of the --strict flag when invoking index-pack so\n> > that links are not checked, only objects. This is because incomplete\n> > links are expected when doing a partial clone or fetch.\n> \n> It is expected that _some_ links are missing, but this makes me\n> wonder if we can do better than disabling the connectivity check\n> altogether.  Does \"git fetch\" lack sufficient information to attempt\n> the connectivity check, and when (and only when) it hits a broken\n> link, see if that is because the connectivity check traversal is\n> crossing a \"partial\" fetch boundary, or something along that line?\n\nOur only definition (currently) for the \"partial\" fetch boundary is\nwhether an object in a promisor packfile (a packfile obtained from the\npromisor remote) references it, so I think that checking for crossing a\n\"partial\" fetch boundary does not add anything. This is because by that\ndefinition, any missing links observed from objects newly fetched from\nthe promisor remote cross a \"partial\" fetch boundary (since all objects\nfetched in this way \"promise\" all objects that they refer to).\n\nBut it is true that we might be able to do better in checking, for\nexample, that a packfile fetched using a blob size limit contains all\nreferenced trees (that is, only blobs are allowed to be missing).\n"},{"id":"341772","messageId":"xmqqo9jp5kgv.fsf@gitster-ct.c.googlers.com","threadId":"48043","inReplyTo":"20180314152429.df14adfb49b028e2e2a9001a@google.com","subject":"Re: [PATCH 2/2] fetch-pack: do not check links for partial fetch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-03-15T17:15:12Z","receivedAt":"2018-03-15T17:15:20Z","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> Our only definition (currently) for the \"partial\" fetch boundary is\n> whether an object in a promisor packfile (a packfile obtained from the\n> promisor remote) references it, so I think that checking for crossing a\n> \"partial\" fetch boundary does not add anything.\n\nAh, that's a perfect answer. When we find a link that leads to a\nmissing object in a pack after receiving it from a remote repository\nthat gives us promises, the missing object must be available from\nthe remote repository by definition.  Makes perfect sense ;-)\n\nThanks.\n"}]}