{"thread":{"id":"29756","subject":"[PATCH] Perform cheaper connectivity check when pack is used as medium","startedAt":"2012-02-27T15:39:51Z","lastAt":"2012-03-16T03:46:43Z","messageCount":23,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Johannes Sixt","Andreas Ericsson","Nguyen Thai Ngoc Duy"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"185519","messageId":"1330357191-32011-1-git-send-email-pclouds@gmail.com","threadId":"29756","inReplyTo":null,"subject":"[PATCH] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-27T15:39:51Z","receivedAt":"2012-02-27T15:39:51Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When we fetch or push, usually \"git rev-list --verify-objects --not\n--all --stdin\" is used to make sure that there are no gaps between\nexisting refs and new refs. --verify-objects calls parse_object(),\nwhich internally calls check_sha1_signature() to verify object content\nmatches its SHA-1 signature.\n\ncheck_sha1_signature() is an expensive operation, especially when new\nrefs are far away from existing ones because all objects in between\nare re-hashed. However, if we receive new objects by pack, we can\nskip the operation because packs themselves do not contain SHA-1\nsignatures. All signatures are recreated by index-pack's hashing the\npack, which we can trust.\n\nDetect pack transfer cases and turn --verify-objects to --objects.\n--objects is similar to --verify-objects except that it does not call\ncheck_sha1_signature().\n\nAs an (extreme) example, a repository is created with only one commit:\ne83c516 (Initial revision of \"git\", the information manager from hell\n- 2005-04-07). The rest of git.git is fetched on top. Without the\npatch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125638, done.\nremote: Compressing objects: 100% (33201/33201), done.\nremote: Total 125638 (delta 92568), reused 123517 (delta 90743)\nReceiving objects: 100% (125638/125638), 34.58 MiB | 8.07 MiB/s, done.\nResolving deltas: 100% (92568/92568), done.\nFrom file:///home/pclouds/w/git/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    1m30.972s\nuser    1m31.410s\nsys     0m1.757s\n\nWith the patch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125647, done.\nremote: Compressing objects: 100% (33209/33209), done.\nremote: Total 125647 (delta 92576), reused 123516 (delta 90744)\nReceiving objects: 100% (125647/125647), 34.58 MiB | 7.99 MiB/s, done.\nResolving deltas: 100% (92576/92576), done.\nFrom file:///home/pclouds/w/git/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    0m51.456s\nuser    0m52.737s\nsys     0m1.548s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/fetch.c        |   12 +++++++-----\n builtin/receive-pack.c |    4 ++--\n connected.c            |    5 ++++-\n connected.h            |    3 ++-\n transport.c            |    5 +++++\n transport.h            |    1 +\n 6 files changed, 21 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 65f5f9b..2b62f42 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -368,7 +368,7 @@ static int iterate_ref_map(void *cb_data, unsigned char sha1[20])\n }\n \n static int store_updated_refs(const char *raw_url, const char *remote_name,\n-\t\tstruct ref *ref_map)\n+\t\t\t      struct ref *ref_map, int pack_transport)\n {\n \tFILE *fp;\n \tstruct commit *commit;\n@@ -389,7 +389,8 @@ 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_everything_connected(iterate_ref_map, 0, &rm)) {\n+\tif (check_everything_connected(iterate_ref_map, 0,\n+\t\t\t\t       pack_transport ? 0 : 1, &rm)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\n@@ -516,7 +517,7 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (depth)\n \t\treturn -1;\n-\treturn check_everything_connected(iterate_ref_map, 1, &rm);\n+\treturn check_everything_connected(iterate_ref_map, 1, 0, &rm);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n@@ -526,8 +527,9 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\tret = transport_fetch_refs(transport, ref_map);\n \tif (!ret)\n \t\tret |= store_updated_refs(transport->url,\n-\t\t\t\ttransport->remote->name,\n-\t\t\t\tref_map);\n+\t\t\t\t\t  transport->remote->name,\n+\t\t\t\t\t  ref_map,\n+\t\t\t\t\t  is_pack_transport(transport));\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0afb8b2..5935751 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -669,7 +669,7 @@ static void set_connectivity_errors(struct command *commands)\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tstruct command *singleton = cmd;\n \t\tif (!check_everything_connected(command_singleton_iterator,\n-\t\t\t\t\t\t0, &singleton))\n+\t\t\t\t\t\t0, 0, &singleton))\n \t\t\tcontinue;\n \t\tcmd->error_string = \"missing necessary objects\";\n \t}\n@@ -705,7 +705,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcmd = commands;\n \tif (check_everything_connected(iterate_receive_command_list,\n-\t\t\t\t       0, &cmd))\n+\t\t\t\t       0, 0, &cmd))\n \t\tset_connectivity_errors(commands);\n \n \tif (run_receive_hook(commands, pre_receive_hook, 0)) {\ndiff --git a/connected.c b/connected.c\nindex d762423..6ebd330 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -14,7 +14,8 @@\n  *\n  * Returns 0 if everything is connected, non-zero otherwise.\n  */\n-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+int check_everything_connected(sha1_iterate_fn fn, int quiet,\n+\t\t\t       int strict, void *cb_data)\n {\n \tstruct child_process rev_list;\n \tconst char *argv[] = {\"rev-list\", \"--verify-objects\",\n@@ -26,6 +27,8 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n+\tif (!strict)\n+\t\targv[1] = \"--objects\";\n \tif (quiet)\n \t\targv[5] = \"--quiet\";\n \ndiff --git a/connected.h b/connected.h\nindex 7e4585a..1f191da 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -15,6 +15,7 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\n  *\n  * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n  */\n-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);\n+extern int check_everything_connected(sha1_iterate_fn, int quiet,\n+\t\t\t\t      int strict, void *cb_data);\n \n #endif /* CONNECTED_H */\ndiff --git a/transport.c b/transport.c\nindex 181f8f2..cd5e0ca 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1248,3 +1248,8 @@ void for_each_alternate_ref(alternate_ref_fn fn, void *data)\n \tcb.data = data;\n \tforeach_alt_odb(refs_from_alternate_cb, &cb);\n }\n+\n+int is_pack_transport(const struct transport *transport)\n+{\n+\treturn transport->fetch == fetch_refs_via_pack;\n+}\ndiff --git a/transport.h b/transport.h\nindex ce99ef8..7cf72ff 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -150,6 +150,7 @@ int transport_disconnect(struct transport *transport);\n char *transport_anonymize_url(const char *url);\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child);\n+int is_pack_transport(const struct transport *transport);\n \n int transport_connect(struct transport *transport, const char *name,\n \t\t      const char *exec, int fd[2]);\n-- \n1.7.8.36.g69ee2\n"},{"id":"185533","messageId":"7vmx849ma8.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"1330357191-32011-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] Perform cheaper connectivity check when pack is used as medium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-02-27T18:14:07Z","receivedAt":"2012-02-27T18:14:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Please turn the existing \"int quiet\" that carries only one bit into a flag\nword, instead of adding yet another int that is only used for one bit.\n\nOther than that, the idea/approach feels sound.\n\nThanks.\n \n"},{"id":"185621","messageId":"1330435109-4437-1-git-send-email-pclouds@gmail.com","threadId":"29756","inReplyTo":"7vmx849ma8.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-28T13:18:29Z","receivedAt":"2012-02-28T13:18:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When we fetch or push, usually \"git rev-list --verify-objects --not\n--all --stdin\" is used to make sure that there are no gaps between\nexisting refs and new refs. --verify-objects calls parse_object(),\nwhich internally calls check_sha1_signature() to verify object content\nmatches its SHA-1 signature.\n\ncheck_sha1_signature() is an expensive operation, especially when new\nrefs are far away from existing ones because all objects in between\nare re-hashed. However, if we receive new objects by pack, we can\nskip the operation because packs themselves do not contain SHA-1\nsignatures. All signatures are recreated by unpack-objects/index-pack's\nhashing objects in the pack, which we can trust.\n\nDetect pack transfer cases and turn --verify-objects to --objects.\n--objects is similar to --verify-objects except that it does not call\ncheck_sha1_signature().\n\nAs an (extreme) example, a repository is created with only one commit:\ne83c516 (Initial revision of \"git\", the information manager from hell\n- 2005-04-07). The rest of git.git is fetched on top. Without the\npatch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125638, done.\nremote: Compressing objects: 100% (33201/33201), done.\nremote: Total 125638 (delta 92568), reused 123517 (delta 90743)\nReceiving objects: 100% (125638/125638), 34.58 MiB | 8.07 MiB/s, done.\nResolving deltas: 100% (92568/92568), done.\nFrom file:///home/pclouds/w/git/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    1m30.972s\nuser    1m31.410s\nsys     0m1.757s\n\nWith the patch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125647, done.\nremote: Compressing objects: 100% (33209/33209), done.\nremote: Total 125647 (delta 92576), reused 123516 (delta 90744)\nReceiving objects: 100% (125647/125647), 34.58 MiB | 7.99 MiB/s, done.\nResolving deltas: 100% (92576/92576), done.\nFrom file:///home/pclouds/w/git/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    0m51.456s\nuser    0m52.737s\nsys     0m1.548s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n combine \"quiet\" and \"strict\" to a flag word\n\n builtin/fetch.c |   14 +++++++++-----\n connected.c     |   11 +++++++----\n connected.h     |    6 +++++-\n transport.c     |    5 +++++\n transport.h     |    1 +\n 5 files changed, 27 insertions(+), 10 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 65f5f9b..70c9ca3 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -368,7 +368,7 @@ static int iterate_ref_map(void *cb_data, unsigned char sha1[20])\n }\n \n static int store_updated_refs(const char *raw_url, const char *remote_name,\n-\t\tstruct ref *ref_map)\n+\t\t\t      struct ref *ref_map, int use_pack)\n {\n \tFILE *fp;\n \tstruct commit *commit;\n@@ -389,7 +389,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_everything_connected(iterate_ref_map, 0, &rm)) {\n+\tif (check_everything_connected(iterate_ref_map,\n+\t\t\t\t       use_pack ? 0 : CHECK_CONNECT_STRICT,\n+\t\t\t\t       &rm)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\n@@ -516,7 +518,8 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (depth)\n \t\treturn -1;\n-\treturn check_everything_connected(iterate_ref_map, 1, &rm);\n+\treturn check_everything_connected(iterate_ref_map,\n+\t\t\t\t\t  CHECK_CONNECT_QUIET, &rm);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n@@ -526,8 +529,9 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\tret = transport_fetch_refs(transport, ref_map);\n \tif (!ret)\n \t\tret |= store_updated_refs(transport->url,\n-\t\t\t\ttransport->remote->name,\n-\t\t\t\tref_map);\n+\t\t\t\t\t  transport->remote->name,\n+\t\t\t\t\t  ref_map,\n+\t\t\t\t\t  is_pack_transport(transport));\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\ndiff --git a/connected.c b/connected.c\nindex d762423..9df357d 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -14,10 +14,11 @@\n  *\n  * Returns 0 if everything is connected, non-zero otherwise.\n  */\n-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+int check_everything_connected(sha1_iterate_fn fn, unsigned int flags,\n+\t\t\t       void *cb_data)\n {\n \tstruct child_process rev_list;\n-\tconst char *argv[] = {\"rev-list\", \"--verify-objects\",\n+\tconst char *argv[] = {\"rev-list\", \"--objects\",\n \t\t\t      \"--stdin\", \"--not\", \"--all\", NULL, NULL};\n \tchar commit[41];\n \tunsigned char sha1[20];\n@@ -26,7 +27,9 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n-\tif (quiet)\n+\tif (flags & CHECK_CONNECT_STRICT)\n+\t\targv[1] = \"--verify-objects\";\n+\tif (flags & CHECK_CONNECT_QUIET)\n \t\targv[5] = \"--quiet\";\n \n \tmemset(&rev_list, 0, sizeof(rev_list));\n@@ -34,7 +37,7 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n-\trev_list.no_stderr = quiet;\n+\trev_list.no_stderr = flags & CHECK_CONNECT_QUIET;\n \tif (start_command(&rev_list))\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n \ndiff --git a/connected.h b/connected.h\nindex 7e4585a..52636b9 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -1,6 +1,9 @@\n #ifndef CONNECTED_H\n #define CONNECTED_H\n \n+#define CHECK_CONNECT_QUIET  1\n+#define CHECK_CONNECT_STRICT 2\n+\n /*\n  * Take callback data, and return next object name in the buffer.\n  * When called after returning the name for the last object, return -1\n@@ -15,6 +18,7 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\n  *\n  * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n  */\n-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);\n+extern int check_everything_connected(sha1_iterate_fn, unsigned int flags,\n+\t\t\t\t      void *cb_data);\n \n #endif /* CONNECTED_H */\ndiff --git a/transport.c b/transport.c\nindex 181f8f2..cd5e0ca 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -1248,3 +1248,8 @@ void for_each_alternate_ref(alternate_ref_fn fn, void *data)\n \tcb.data = data;\n \tforeach_alt_odb(refs_from_alternate_cb, &cb);\n }\n+\n+int is_pack_transport(const struct transport *transport)\n+{\n+\treturn transport->fetch == fetch_refs_via_pack;\n+}\ndiff --git a/transport.h b/transport.h\nindex ce99ef8..7cf72ff 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -150,6 +150,7 @@ int transport_disconnect(struct transport *transport);\n char *transport_anonymize_url(const char *url);\n void transport_take_over(struct transport *transport,\n \t\t\t struct child_process *child);\n+int is_pack_transport(const struct transport *transport);\n \n int transport_connect(struct transport *transport, const char *name,\n \t\t      const char *exec, int fd[2]);\n-- \n1.7.8.36.g69ee2\n"},{"id":"185630","messageId":"4F4CF58A.9090502@viscovery.net","threadId":"29756","inReplyTo":"1330435109-4437-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2012-02-28T15:40:58Z","receivedAt":"2012-02-28T15:40:58Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 2/28/2012 14:18, schrieb Nguyễn Thái Ngọc Duy:\n> Without the patch:\n> $ time git fetch file:///home/pclouds/w/git/.git\n> remote: Counting objects: 125638, done.\n> remote: Compressing objects: 100% (33201/33201), done.\n...\n> With the patch:\n> $ time git fetch file:///home/pclouds/w/git/.git\n> remote: Counting objects: 125647, done.\n> remote: Compressing objects: 100% (33209/33209), done.\n\nIt is a bit irritating that the number are different when they should be\nidentical...\n\n-- Hannes\n"},{"id":"185631","messageId":"4F4CF72C.6070508@op5.se","threadId":"29756","inReplyTo":"4F4CF58A.9090502@viscovery.net","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Andreas Ericsson","fromEmail":"ae@op5.se","sentAt":"2012-02-28T15:47:56Z","receivedAt":"2012-02-28T15:47:56Z","isPatch":true,"sender":{"key":"ae@op5.se","avatar":"https://gravatar.com/avatar/426e89595c75a8f5252dd0c989e5fabe5bcac616e68557427ad9aef6b0ca342a?d=mp&s=160"},"body":"On 02/28/2012 04:40 PM, Johannes Sixt wrote:\n> Am 2/28/2012 14:18, schrieb Nguyễn Thái Ngọc Duy:\n>> Without the patch:\n>> $ time git fetch file:///home/pclouds/w/git/.git\n>> remote: Counting objects: 125638, done.\n>> remote: Compressing objects: 100% (33201/33201), done.\n> ...\n>> With the patch:\n>> $ time git fetch file:///home/pclouds/w/git/.git\n>> remote: Counting objects: 125647, done.\n>> remote: Compressing objects: 100% (33209/33209), done.\n> \n> It is a bit irritating that the number are different when they should be\n> identical...\n> \n\nI found it odd as well, but since the latter shows a larger object\ncount and a shorter time, I disregarded it and considered it some\nevidence that he pushed this patch to that repo.\n\nSince commit created 6 blobs, 2 trees and 1 commit object, and the\nlatter has 9 objects more, I assume that's what happened anyways.\nAs such, I think we can live with the small discrepancy. Also note\nthat the latter post had slower transfer rate. That also skews the\ncomparison somewhat, but again it's in favour of the patch.\n\n-- \nAndreas Ericsson                   andreas.ericsson@op5.se\nOP5 AB                             www.op5.se\nTel: +46 8-230225                  Fax: +46 8-230231\n\nConsidering the successes of the wars on alcohol, poverty, drugs and\nterror, I think we should give some serious thought to declaring war\non peace.\n"},{"id":"185689","messageId":"CACsJy8Cj+F+bNOBZiZq=xJJRAwq1QebSB8o-wxtyd9YGtd_Tjg@mail.gmail.com","threadId":"29756","inReplyTo":"4F4CF58A.9090502@viscovery.net","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-02-29T01:41:33Z","receivedAt":"2012-02-29T01:41:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/2/28 Johannes Sixt <j.sixt@viscovery.net>:\n> Am 2/28/2012 14:18, schrieb Nguyễn Thái Ngọc Duy:\n>> Without the patch:\n>> $ time git fetch file:///home/pclouds/w/git/.git\n>> remote: Counting objects: 125638, done.\n>> remote: Compressing objects: 100% (33201/33201), done.\n> ...\n>> With the patch:\n>> $ time git fetch file:///home/pclouds/w/git/.git\n>> remote: Counting objects: 125647, done.\n>> remote: Compressing objects: 100% (33209/33209), done.\n>\n> It is a bit irritating that the number are different when they should be\n> identical...\n\n~/w/git is my working directory so objects may change a little bit.\nShould have fetched from a clone instead, sorry.\n-- \nDuy\n"},{"id":"185885","messageId":"7vwr73h6td.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"1330435109-4437-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T06:10:06Z","receivedAt":"2012-03-02T06:10:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> When we fetch or push, usually \"git rev-list --verify-objects --not\n> --all --stdin\" is used to make sure that there are no gaps between\n> existing refs and new refs. --verify-objects calls parse_object(),\n> which internally calls check_sha1_signature() to verify object content\n> matches its SHA-1 signature.\n>\n> check_sha1_signature() is an expensive operation, especially when new\n> refs are far away from existing ones because all objects in between\n> are re-hashed. However, if we receive new objects by pack, we can\n> skip the operation because packs themselves do not contain SHA-1\n> signatures. All signatures are recreated by unpack-objects/index-pack's\n> hashing objects in the pack, which we can trust.\n>\n> Detect pack transfer cases and turn --verify-objects to --objects.\n\nAfter thinking more about this patch, I do not think this \"optimization\"\nis correct.\n\nConsider a case where you have this history\n\n\n    ---T       o---o---o\n\nwhere 'T' is the tip of your ref (everything reachable from it is known to\nbe good), and 'o' are \"isolated islands\" commits that somehow exist in\nyour repository but are not complete for whatever reason.\n\nThe global history may look like this, where 'X' is the tip the other end\nadvertised, and '.' are commits that are new to your repository.\n\n    ---T---.---o---o---o---.---X\n\nUpon seeing 'X' advertised and noticing 'T' is the current tip, you would\nask for everything that is needed, i.e. \"rev-list --objects T..X\", to be\nsent to you.\n\nBut the other end could send all the trees and commits for 'X' and '.' but\nnothing for 'o'.  You will end up with a corrupt history but the loosened\n\"rev-list --objects T..X\" you run after the object transfer will not catch\nit.  You need --verify-objects if you want to catch this mode of attack.\n\nAdmittedly, in order to mount such an attack successfully, the attacker\nneeds to know what \"isolated islands\" you happen to have in the receiving\nrepository, which makes it much harder than a simpler attack (e.g. sending\nonly X but nothing else) that would have worked before we introduced the\nconnectivity check after object transfer.\n\nBut we need to realize that the reasoning expressed in your log message\n\"we received new objects by pack, so everything must validate fine\" is not\nvalid, and we are loosening ourselves to new attacks when we evaluate this\npatch.  This is because the completion of the history may be using objects\nthat did not come from the other side (i.e. commits 'o' and all the trees\nnecessary for them, but their blobs may be corrupt).\n"},{"id":"185922","messageId":"CACsJy8D7JB9bzOvammGAx1rW04DEUCVzvfp-_Q1KtE9G00rZVQ@mail.gmail.com","threadId":"29756","inReplyTo":"7vwr73h6td.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-02T14:05:09Z","receivedAt":"2012-03-02T14:05:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/3/2 Junio C Hamano <gitster@pobox.com>:\n> Consider a case where you have this history\n>\n>\n>    ---T       o---o---o\n>\n> where 'T' is the tip of your ref (everything reachable from it is known to\n> be good), and 'o' are \"isolated islands\" commits that somehow exist in\n> your repository but are not complete for whatever reason.\n>\n> The global history may look like this, where 'X' is the tip the other end\n> advertised, and '.' are commits that are new to your repository.\n>\n>    ---T---.---o---o---o---.---X\n>\n> Upon seeing 'X' advertised and noticing 'T' is the current tip, you would\n> ask for everything that is needed, i.e. \"rev-list --objects T..X\", to be\n> sent to you.\n>\n> But the other end could send all the trees and commits for 'X' and '.' but\n> nothing for 'o'.  You will end up with a corrupt history but the loosened\n> \"rev-list --objects T..X\" you run after the object transfer will not catch\n> it.  You need --verify-objects if you want to catch this mode of attack.\n\nOK I think I get what you are trying to say. With \"rev-list --objects\nT..X\" we could check commit connectivity from X to T fine. But some\ntrees in those 'o' commits are bad, but well-formed. As a result,\nrev-list goes well and we blindly accept bad trees/blobs. By checking\nfor tree and blob integrity, we would catch these bad guys. Is that\ncorrect?\n\nThe bad islands may be injected from somewhere and cannot be trusted\nuntil they get connectivity with the main DAG.\n\n> Admittedly, in order to mount such an attack successfully, the attacker\n> needs to know what \"isolated islands\" you happen to have in the receiving\n> repository, which makes it much harder than a simpler attack (e.g. sending\n> only X but nothing else) that would have worked before we introduced the\n> connectivity check after object transfer.\n>\n> But we need to realize that the reasoning expressed in your log message\n> \"we received new objects by pack, so everything must validate fine\" is not\n> valid, and we are loosening ourselves to new attacks when we evaluate this\n> patch.  This is because the completion of the history may be using objects\n> that did not come from the other side (i.e. commits 'o' and all the trees\n> necessary for them, but their blobs may be corrupt).\n\nNot everything is valid, then. Objects from new packs are, existing\nones may be guilty. If there is a way to mark new packs trusted, then\nwe only need to validate the other objects, which should be the\nminority or even empty unless an attack is mounted.\n-- \nDuy\n"},{"id":"185931","messageId":"7v1upagb8z.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"CACsJy8D7JB9bzOvammGAx1rW04DEUCVzvfp-_Q1KtE9G00rZVQ@mail.gmail.com","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-02T17:31:56Z","receivedAt":"2012-03-02T17:31:56Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> OK I think I get what you are trying to say.\n> ...\n\nThe attack can be even more simplified; the other side needs to know about\nonly one blob.\n\nSuppose you have a corrupt blob B that is not referenced from anything in\nyour repository. \"git fsck\" will find the corruption of that single blob,\nbut that does not make your repository corrupt, as the corrupt object is\nirrelevant to your history. The tip of your current healthy history is at\ncommit T.\n\nStarting from that state, you fetch from the other side, that has commit X\nat the tip. In this simplified scenario, X is a direct child of T.\n\nYou expect that the other side sends everything contained in X that you do\nnot have in T.  Now, the only difference X makes relative to T is that it\nadds a new file whose contents is B at the toplevel of the tree.  And the\ntransfer gives you the commit object X itself, and its toplevel tree\nobject, but it omits the blob B by malice (or mistake).\n\nYour \"rev-list --object T..X\" that is run after the transfer completes\nwill not notice that B is corrupt, because it only checks if it exists.\n\nAnd now you corrupted your repository, by making B a part of the history\nyou (incorrectly) declare complete.\n\nThe whole point of the check after the transfer is to make sure that the\ntransfer will not make a repository that was healthy into a corrupt one,\nso using --objects and not --verify-objects is a wrong \"optimization\" in\nthis case.\n\n> Not everything is valid, then. Objects from new packs are, existing\n> ones may be guilty. If there is a way to mark new packs trusted, then\n> we only need to validate the other objects, which should be the\n> minority or even empty unless an attack is mounted.\n\nYes, but how?  Running \"fsck\" on all of pre-existing objects every time\nyou fetch (or accept push) is not an answer.\n\nIf your fetch did not explode the incoming pack into pieces, a possibility\nis to still use the --verify-object codepath, but pass the identity of the\npack (e.g. struct packed_git) down the callchain so that you can avoid\nrehashing the objects that came from that single pack, but that would not\nhelp the case where you ended up calling unpack-objects.\n\nI also suspect that more than trivial amount of computation is needed to\ndetermine if a given object exists only in a single pack, so the end\nresult may not be that much cheaper than the current --verify-object code.\n"},{"id":"185972","messageId":"CACsJy8B7u-rvnFZKi4t2CoB=J3Ra8pWxK4439NTdnHQDhm2ibQ@mail.gmail.com","threadId":"29756","inReplyTo":"7v1upagb8z.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-03T03:05:27Z","receivedAt":"2012-03-03T03:05:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 3, 2012 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Not everything is valid, then. Objects from new packs are, existing\n>> ones may be guilty. If there is a way to mark new packs trusted, then\n>> we only need to validate the other objects, which should be the\n>> minority or even empty unless an attack is mounted.\n>\n> Yes, but how?  Running \"fsck\" on all of pre-existing objects every time\n> you fetch (or accept push) is not an answer.\n>\n> If your fetch did not explode the incoming pack into pieces, a possibility\n> is to still use the --verify-object codepath, but pass the identity of the\n> pack (e.g. struct packed_git) down the callchain so that you can avoid\n> rehashing the objects that came from that single pack, but that would not\n> help the case where you ended up calling unpack-objects.\n\nIt won't help the unpack-objects case. But unpack-objects is only used\nwhen the pack has less than a certain number of objects, doing heavy\ncheck in that case should not take too long. Yes, I was thinking I\nwould pass pack identity down the verify-pack callchain for index-pack\ncase.\n\n> I also suspect that more than trivial amount of computation is needed to\n> determine if a given object exists only in a single pack, so the end\n> result may not be that much cheaper than the current --verify-object code.\n\nObjects can exist in multiple packs right now if they are base\nobjects. I'm not sure why you need to check for object existence in a\nsingle pack. index-pack tries to make sure duplicate objects with the\nsame sha-1 must be identical, content-wise. Though it does not do that\nhard enough (i.e. only check with one in-repo instance of the object,\ninstead of all of them).\n-- \nDuy\n"},{"id":"185974","messageId":"7vfwdq8914.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"CACsJy8B7u-rvnFZKi4t2CoB=J3Ra8pWxK4439NTdnHQDhm2ibQ@mail.gmail.com","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-03T06:59:19Z","receivedAt":"2012-03-03T06:59:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> It won't help the unpack-objects case. But unpack-objects is only used\n> when the pack has less than a certain number of objects, doing heavy\n> check in that case should not take too long. Yes, I was thinking I\n> would pass pack identity down the verify-pack callchain for index-pack\n> case.\n\nYes, I think we are on the same track; see below.\n\n>> I also suspect that more than trivial amount of computation is needed to\n>> determine if a given object exists only in a single pack, so the end\n>> result may not be that much cheaper than the current --verify-object code.\n>\n> Objects can exist in multiple packs right now if they are base\n> objects. I'm not sure why you need to check for object existence in a\n> single pack.\n\nWhat I meant to say was not \"it is in this pack and nowhere else\", but\nabout a check like this:\n\n        static void finish_object(struct object *obj, ...)\n        {\n                struct packed_git *fetched_pack = cb_data->fetched_pack;\n\n                if (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))\n                        die(\"missing\");\n                if (!info->revs->verify_objects)\n                        return;\n\t\tif (find_pack_entry_one(obj->sha1, fetched_pack))\n                        return; /* we just fetched and ran index-pack on it */\n                if (!obj->parsed && obj->type != OBJ_COMMIT)\n                        parse_object(obj->sha1);                \n        }\n\nI think this is the kind of \"passing identity down the callchain\" both of\nus have in mind.  I was trying to say that find_pack_entry() may not be\ntrivially cheap.  But probably I am being worried too much.\n\nBut now you brought it up, I think we may also need to worry about a\ncorrupt pre-existing loose blob object.  In general, we tend to always\nfavor reading objects from packs over loose objects, but I do not know\noffhand what repacking would do when there are two places it can read the\nsame object from (it should be allowed to pick whichever is easier to\nread).\n"},{"id":"185981","messageId":"CACsJy8ANF+tqMzXrYamSHv0N==6Wc2sVbQwJO40+=f+R+HMnbA@mail.gmail.com","threadId":"29756","inReplyTo":"7vfwdq8914.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-03T10:09:05Z","receivedAt":"2012-03-03T10:09:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 3, 2012 at 1:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>>> I also suspect that more than trivial amount of computation is needed to\n>>> determine if a given object exists only in a single pack, so the end\n>>> result may not be that much cheaper than the current --verify-object code.\n>>\n>> Objects can exist in multiple packs right now if they are base\n>> objects. I'm not sure why you need to check for object existence in a\n>> single pack.\n>\n> What I meant to say was not \"it is in this pack and nowhere else\", but\n> about a check like this:\n>\n>        static void finish_object(struct object *obj, ...)\n>        {\n>                struct packed_git *fetched_pack = cb_data->fetched_pack;\n>\n>                if (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))\n>                        die(\"missing\");\n>                if (!info->revs->verify_objects)\n>                        return;\n>                if (find_pack_entry_one(obj->sha1, fetched_pack))\n>                        return; /* we just fetched and ran index-pack on it */\n>                if (!obj->parsed && obj->type != OBJ_COMMIT)\n>                        parse_object(obj->sha1);\n>        }\n>\n> I think this is the kind of \"passing identity down the callchain\" both of\n> us have in mind.  I was trying to say that find_pack_entry() may not be\n> trivially cheap.  But probably I am being worried too much.\n\nThis is after index-pack is run and .idx file created, I think\ndetermining object's storage type should be relatively cheap compared\nto rehashing. We'll know when I update the patch.\n\n> But now you brought it up, I think we may also need to worry about a\n> corrupt pre-existing loose blob object.  In general, we tend to always\n> favor reading objects from packs over loose objects, but I do not know\n> offhand what repacking would do when there are two places it can read the\n> same object from (it should be allowed to pick whichever is easier to\n> read).\n\n.. which should be pack for pack-objects/repack because they can do a\nstraight copy from pack to pack. --no-reuse-objects delegates object\nreading back to read_sha1_file(), and this one prefers packs too.\n-- \nDuy\n"},{"id":"186199","messageId":"CACsJy8A+c6t_zHziPcQZ-Nw+fm0TmaG8VmDfnoPf-DzHHm8QVA@mail.gmail.com","threadId":"29756","inReplyTo":"7vfwdq8914.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-06T11:20:21Z","receivedAt":"2012-03-06T11:20:21Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, Mar 3, 2012 at 1:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> But now you brought it up, I think we may also need to worry about a\n> corrupt pre-existing loose blob object.  In general, we tend to always\n> favor reading objects from packs over loose objects, but I do not know\n> offhand what repacking would do when there are two places it can read the\n> same object from (it should be allowed to pick whichever is easier to\n> read).\n\nCorrupt accidentally or on purpose? If some one can add a few corrupt\nloose objects to your repository, (s)he can also put a pack with a\ncorrupt index (e.g. sha-1 does not point to the content that matches\nthat sha-1) there too. If that corrupt pack is chosen for reading at\n\"repack -ad\" time, I think pack-objects would happily copy over\nwhatever the index points to. And the new index that pack-objects\ngenerates (i.e. not through index-pack) may be also corrupt.\n-- \nDuy\n"},{"id":"186221","messageId":"7vfwdlvi3d.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"CACsJy8A+c6t_zHziPcQZ-Nw+fm0TmaG8VmDfnoPf-DzHHm8QVA@mail.gmail.com","subject":"Re: [PATCH v2] Perform cheaper connectivity check when pack is used as medium","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-06T15:56:22Z","receivedAt":"2012-03-06T15:56:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> On Sat, Mar 3, 2012 at 1:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> But now you brought it up, I think we may also need to worry about a\n>> corrupt pre-existing loose blob object. In general, we tend to always\n>> favor reading objects from packs over loose objects, but I do not know\n>> offhand what repacking would do when there are two places it can read the\n>> same object from (it should be allowed to pick whichever is easier to\n>> read).\n>\n> Corrupt accidentally or on purpose?\n\nDoes not matter. The attack outlined does not require you to write a\ncorrupt one into victim's repository. You only need to _know_ one\nthat the victim happens to have.\n"},{"id":"186949","messageId":"1331736055-21019-1-git-send-email-pclouds@gmail.com","threadId":"29756","inReplyTo":"7vfwdq8914.fsf@alter.siamese.dyndns.org","subject":"[PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-14T14:40:55Z","receivedAt":"2012-03-14T14:40:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"When we fetch or push, usually \"git rev-list --verify-objects --not\n--all --stdin\" is used to make sure that all objects between existing\nrefs and new refs are good. This means no gaps in between, all objects\nare well-formed, object content agrees with its sha-1 signature.\n\nFor the last one, --verify-objects calls check_sha1_signature() via\nparse_object(). check_sha1_signature() is an expensive operation,\nespecially when new refs are far away from existing ones because all\nobjects in between are re-hashed. Because objects coming from the new\npack are already hashed by index-pack, we can trust their\nintegrity. The only objects left to check are existing ones in repo\nbut has no connection to any current refs.\n\nPass the new pack id down to--verify-objects and skip\ncheck_sha1_signature() on objects from that pack.\n\nAs an (extreme) example, a repository is created with only one commit:\ne83c516 (Initial revision of \"git\", the information manager from hell\n- 2005-04-07). The rest of git.git is fetched on top. Without the\npatch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125656, done.\nremote: Compressing objects: 100% (33280/33280), done.\nremote: Total 125656 (delta 92585), reused 123464 (delta 90682)\nReceiving objects: 100% (125656/125656), 34.60 MiB | 8.47 MiB/s, done.\nResolving deltas: 100% (92585/92585), done.\nFrom file:///home/pclouds/t/test/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    1m30.437s\nuser    1m31.338s\nsys     0m1.687s\n\nWith the patch:\n\n$ time git fetch file:///home/pclouds/w/git/.git\nremote: Counting objects: 125656, done.\nremote: Compressing objects: 100% (33280/33280), done.\nremote: Total 125656 (delta 92585), reused 123464 (delta 90682)\nReceiving objects: 100% (125656/125656), 34.60 MiB | 7.86 MiB/s, done.\nResolving deltas: 100% (92585/92585), done.\nFrom file:///home/pclouds/t/test/\n * branch            HEAD       -> FETCH_HEAD\n\nreal    0m52.182s\nuser    0m53.151s\nsys     0m1.465s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n Another try. Exclude all objects reachable from refs and the new pack\n from --verify-objects tests.\n \n I keep CHECK_CONNECT_QUIET change because it seems a good change\n anyway. transport.[ch] changes are dropped because we can detect pack\n case based on pack_lockfile.\n\n builtin/fetch.c        |   12 +++++++-----\n builtin/receive-pack.c |    7 +++----\n builtin/rev-list.c     |   20 ++++++++++++++++++--\n connected.c            |   31 +++++++++++++++++++++++--------\n connected.h            |    5 ++++-\n revision.c             |    5 +++++\n revision.h             |    3 +++\n 7 files changed, 63 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 65f5f9b..cc84d04 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -368,7 +368,7 @@ static int iterate_ref_map(void *cb_data, unsigned char sha1[20])\n }\n \n static int store_updated_refs(const char *raw_url, const char *remote_name,\n-\t\tstruct ref *ref_map)\n+\t\t\t      struct ref *ref_map, const char *pack_lockfile)\n {\n \tFILE *fp;\n \tstruct commit *commit;\n@@ -389,7 +389,7 @@ 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_everything_connected(iterate_ref_map, 0, &rm)) {\n+\tif (check_everything_connected(iterate_ref_map, 0, pack_lockfile, &rm)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\n@@ -516,7 +516,8 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (depth)\n \t\treturn -1;\n-\treturn check_everything_connected(iterate_ref_map, 1, &rm);\n+\treturn check_everything_connected(iterate_ref_map,\n+\t\t\t\t\t  CHECK_CONNECT_QUIET, NULL, &rm);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\n@@ -526,8 +527,9 @@ static int fetch_refs(struct transport *transport, struct ref *ref_map)\n \t\tret = transport_fetch_refs(transport, ref_map);\n \tif (!ret)\n \t\tret |= store_updated_refs(transport->url,\n-\t\t\t\ttransport->remote->name,\n-\t\t\t\tref_map);\n+\t\t\t\t\t  transport->remote->name,\n+\t\t\t\t\t  ref_map,\n+\t\t\t\t\t  transport->pack_lockfile);\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 0afb8b2..01d37f6 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -40,6 +40,7 @@ static int auto_gc = 1;\n static const char *head_name;\n static void *head_name_to_free;\n static int sent_capabilities;\n+static const char *pack_lockfile;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n {\n@@ -669,7 +670,7 @@ static void set_connectivity_errors(struct command *commands)\n \tfor (cmd = commands; cmd; cmd = cmd->next) {\n \t\tstruct command *singleton = cmd;\n \t\tif (!check_everything_connected(command_singleton_iterator,\n-\t\t\t\t\t\t0, &singleton))\n+\t\t\t\t\t\t0, pack_lockfile, &singleton))\n \t\t\tcontinue;\n \t\tcmd->error_string = \"missing necessary objects\";\n \t}\n@@ -705,7 +706,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcmd = commands;\n \tif (check_everything_connected(iterate_receive_command_list,\n-\t\t\t\t       0, &cmd))\n+\t\t\t\t       0, pack_lockfile, &cmd))\n \t\tset_connectivity_errors(commands);\n \n \tif (run_receive_hook(commands, pre_receive_hook, 0)) {\n@@ -797,8 +798,6 @@ static const char *parse_pack_header(struct pack_header *hdr)\n \t}\n }\n \n-static const char *pack_lockfile;\n-\n static const char *unpack(void)\n {\n \tstruct pack_header hdr;\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 4c4d404..21d714b 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -180,8 +180,24 @@ static void finish_object(struct object *obj,\n \tstruct rev_list_info *info = cb_data;\n \tif (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))\n \t\tdie(\"missing blob object '%s'\", sha1_to_hex(obj->sha1));\n-\tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n-\t\tparse_object(obj->sha1);\n+\tif (info->revs->verify_objects &&\n+\t    !obj->parsed && obj->type != OBJ_COMMIT) {\n+\t\tconst char *safe_pack = info->revs->safe_pack;\n+\t\tstruct object_info oi;\n+\t\tint safe = 0;\n+\t\tmemset(&oi, 0, sizeof(oi));\n+\t\tif (*safe_pack &&\n+\t\t    sha1_object_info_extended(obj->sha1, &oi) >= 0 &&\n+\t\t    oi.whence == OI_PACKED) {\n+\t\t\tconst char *pack = oi.u.packed.pack->pack_name;\n+\t\t\tint len = strlen(pack);\n+\t\t\tassert(strncmp(pack + len - 51, \"/pack-\", 6) == 0);\n+\t\t\tassert(strcmp(pack + len - 5, \".pack\") == 0);\n+\t\t\tsafe = !memcmp(safe_pack, pack + len - 45, 40);\n+\t\t}\n+\t\tif (!safe)\n+\t\t\tparse_object(obj->sha1);\n+\t}\n }\n \n static void show_object(struct object *obj,\ndiff --git a/connected.c b/connected.c\nindex d762423..af81049 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -14,28 +14,43 @@\n  *\n  * Returns 0 if everything is connected, non-zero otherwise.\n  */\n-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+int check_everything_connected(sha1_iterate_fn fn, unsigned int flags,\n+\t\t\t       const char *pack_lockfile, void *cb_data)\n {\n \tstruct child_process rev_list;\n-\tconst char *argv[] = {\"rev-list\", \"--verify-objects\",\n-\t\t\t      \"--stdin\", \"--not\", \"--all\", NULL, NULL};\n+\tconst char *argv[] = {\"rev-list\", \"--verify-objects\", \"--stdin\",\n+\t\t\t      \"--not\", \"--all\", NULL, NULL, NULL, NULL };\n \tchar commit[41];\n \tunsigned char sha1[20];\n-\tint err = 0;\n+\tint err = 0, ac = 5;\n+\tstruct strbuf packfile = STRBUF_INIT;\n \n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n-\tif (quiet)\n-\t\targv[5] = \"--quiet\";\n+\tif (flags & CHECK_CONNECT_QUIET)\n+\t\targv[ac++] = \"--quiet\";\n+\tif (pack_lockfile) {\n+\t\tstrbuf_addstr(&packfile, pack_lockfile);\n+\t\t/* xxx/pack-%40s.keep */\n+\t\tassert(strcmp(packfile.buf + packfile.len - 5, \".keep\") == 0);\n+\t\tassert(strncmp(packfile.buf + packfile.len - 51, \"/pack-\", 6) == 0);\n+\t\tstrbuf_setlen(&packfile, packfile.len - 5);\n+\t\tstrbuf_remove(&packfile, 0, packfile.len - 40);\n+\t\targv[ac++] = \"--safe-pack\";\n+\t\targv[ac++] = packfile.buf;\n+\t}\n+\tassert(ac < ARRAY_SIZE(argv) && argv[ac] == NULL);\n \n \tmemset(&rev_list, 0, sizeof(rev_list));\n \trev_list.argv = argv;\n \trev_list.git_cmd = 1;\n \trev_list.in = -1;\n \trev_list.no_stdout = 1;\n-\trev_list.no_stderr = quiet;\n-\tif (start_command(&rev_list))\n+\trev_list.no_stderr = flags & CHECK_CONNECT_QUIET;\n+\terr = start_command(&rev_list);\n+\tstrbuf_release(&packfile);\n+\tif (err)\n \t\treturn error(_(\"Could not run 'git rev-list'\"));\n \n \tsigchain_push(SIGPIPE, SIG_IGN);\ndiff --git a/connected.h b/connected.h\nindex 7e4585a..ed0b559 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -1,6 +1,8 @@\n #ifndef CONNECTED_H\n #define CONNECTED_H\n \n+#define CHECK_CONNECT_QUIET  1\n+\n /*\n  * Take callback data, and return next object name in the buffer.\n  * When called after returning the name for the last object, return -1\n@@ -15,6 +17,7 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\n  *\n  * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n  */\n-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);\n+extern int check_everything_connected(sha1_iterate_fn, unsigned int flags,\n+\t\t\t\t      const char *pack_lockfile, void *cb_data);\n \n #endif /* CONNECTED_H */\ndiff --git a/revision.c b/revision.c\nindex 819ff01..1c2d017 100644\n--- a/revision.c\n+++ b/revision.c\n@@ -1451,6 +1451,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n \t\trevs->tree_objects = 1;\n \t\trevs->blob_objects = 1;\n \t\trevs->verify_objects = 1;\n+\t} else if (!strcmp(arg, \"--safe-pack\")) {\n+\t\tif (strlen(argv[1]) != 40)\n+\t\t\tdie(\"--safe-pack requires an SHA-1 as pack id, not %s\", argv[1]);\n+\t\tstrcpy(revs->safe_pack, argv[1]);\n+\t\treturn 2;\n \t} else if (!strcmp(arg, \"--unpacked\")) {\n \t\trevs->unpacked = 1;\n \t} else if (!prefixcmp(arg, \"--unpacked=\")) {\ndiff --git a/revision.h b/revision.h\nindex b8e9223..2790910 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -168,6 +168,9 @@ struct rev_info {\n \tint count_left;\n \tint count_right;\n \tint count_same;\n+\n+\t/* --verify-objects */\n+\tchar safe_pack[41];\n };\n \n #define REV_TREE_SAME\t\t0\n-- \n1.7.8.36.g69ee2\n"},{"id":"186979","messageId":"7v4ntrq9z5.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"1331736055-21019-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-14T19:05:34Z","receivedAt":"2012-03-14T19:05:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:\n\n>  Another try. Exclude all objects reachable from refs and the new pack\n>  from --verify-objects tests.\n\nThanks.  Looks much nicer in general.\n\n>  I keep CHECK_CONNECT_QUIET change because it seems a good change\n>  anyway.\n\nIt appears that you lost the \"one int for a single boolean to flag\"\nconversion that was the point of CHECK_CONNECT_QUIET symbol.\n\n> diff --git a/builtin/rev-list.c b/builtin/rev-list.c\n> index 4c4d404..21d714b 100644\n> --- a/builtin/rev-list.c\n> +++ b/builtin/rev-list.c\n> @@ -180,8 +180,24 @@ static void finish_object(struct object *obj,\n>  \tstruct rev_list_info *info = cb_data;\n>  \tif (obj->type == OBJ_BLOB && !has_sha1_file(obj->sha1))\n>  \t\tdie(\"missing blob object '%s'\", sha1_to_hex(obj->sha1));\n> -\tif (info->revs->verify_objects && !obj->parsed && obj->type != OBJ_COMMIT)\n> -\t\tparse_object(obj->sha1);\n> +\tif (info->revs->verify_objects &&\n> +\t    !obj->parsed && obj->type != OBJ_COMMIT) {\n> +\t\tconst char *safe_pack = info->revs->safe_pack;\n> +\t\tstruct object_info oi;\n> +\t\tint safe = 0;\n> +\t\tmemset(&oi, 0, sizeof(oi));\n> +\t\tif (*safe_pack &&\n> +\t\t    sha1_object_info_extended(obj->sha1, &oi) >= 0 &&\n> +\t\t    oi.whence == OI_PACKED) {\n> +\t\t\tconst char *pack = oi.u.packed.pack->pack_name;\n> +\t\t\tint len = strlen(pack);\n> +\t\t\tassert(strncmp(pack + len - 51, \"/pack-\", 6) == 0);\n> +\t\t\tassert(strcmp(pack + len - 5, \".pack\") == 0);\n> +\t\t\tsafe = !memcmp(safe_pack, pack + len - 45, 40);\n> +\t\t}\n> +\t\tif (!safe)\n> +\t\t\tparse_object(obj->sha1);\n> +\t}\n>  }\n\nThis looks unnecessarily complex, and I think the complexity comes only\nbecause the new info->revs->safe_pack is a hextual packfile ID.  Wouldn't\nit make more sense to store a pointer to \"struct packed_git\" there, so\nthat oi.u.packed.pack can be compared with it?\n\nThe caller may need a new API in sha1_file.c to let it find a packed git\nwith a given packfile ID.\n\n> diff --git a/connected.c b/connected.c\n> index d762423..af81049 100644\n> --- a/connected.c\n> +++ b/connected.c\n> @@ -14,28 +14,43 @@\n>   *\n>   * Returns 0 if everything is connected, non-zero otherwise.\n>   */\n> -int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n> +int check_everything_connected(sha1_iterate_fn fn, unsigned int flags,\n> +\t\t\t       const char *pack_lockfile, void *cb_data)\n>  {\n> ...\n> +\tif (pack_lockfile) {\n> +\t\tstrbuf_addstr(&packfile, pack_lockfile);\n> +\t\t/* xxx/pack-%40s.keep */\n> +\t\tassert(strcmp(packfile.buf + packfile.len - 5, \".keep\") == 0);\n> +\t\tassert(strncmp(packfile.buf + packfile.len - 51, \"/pack-\", 6) == 0);\n> +\t\tstrbuf_setlen(&packfile, packfile.len - 5);\n> +\t\tstrbuf_remove(&packfile, 0, packfile.len - 40);\n> +\t\targv[ac++] = \"--safe-pack\";\n> +\t\targv[ac++] = packfile.buf;\n> +\t}\n\nThat looks like a very round-about way.  Why not check the \".keep\" and\n\"/pack-\" substrings in pack_lockfile (as a side effect of doing so, you\nwill learn the offset of the 40-byte substring you want to put in the\npackfile strbuf) and strbuf_add(&packfile, pack_lockfile + offset, 40)?\n\n> +\tassert(ac < ARRAY_SIZE(argv) && argv[ac] == NULL);\n\nPerhaps you want to use argv_list API instead of adding an assert() here.\n\n> diff --git a/revision.c b/revision.c\n> index 819ff01..1c2d017 100644\n> --- a/revision.c\n> +++ b/revision.c\n> @@ -1451,6 +1451,11 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg\n>  \t\trevs->tree_objects = 1;\n>  \t\trevs->blob_objects = 1;\n>  \t\trevs->verify_objects = 1;\n> +\t} else if (!strcmp(arg, \"--safe-pack\")) {\n> +\t\tif (strlen(argv[1]) != 40)\n> +\t\t\tdie(\"--safe-pack requires an SHA-1 as pack id, not %s\", argv[1]);\n> +\t\tstrcpy(revs->safe_pack, argv[1]);\n> +\t\treturn 2;\n\nThe condition for the above \"die()\" does not check if that random 40-byte\nstring is an SHA-1, let alone it names an existing packfile.  It would be\nsolved automatically if you change the type of revs.safe_pack to a pointer\nto \"struct packed_git\", though.\n"},{"id":"187058","messageId":"7vy5r1inax.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"1331736055-21019-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T21:09:26Z","receivedAt":"2012-03-15T21:09:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n\n> When we fetch or push, usually \"git rev-list --verify-objects --not\n> --all --stdin\" is used to make sure that all objects between existing\n> refs and new refs are good. This means no gaps in between, all objects\n> are well-formed, object content agrees with its sha-1 signature.\n>\n> For the last one, --verify-objects calls check_sha1_signature() via\n> parse_object(). check_sha1_signature() is an expensive operation,\n\nAfter thinking about this a bit more, I am beginning to think that the\nvalidation of object contents is unnecessary in _all_ cases that involve\n\"git fetch\".  Unpack-objects and index-pack already validate individual\nobjects, and the only thing we would want to catch are objects that we\nalready happened to have had in our repository but were unreferenced from\nour refs.  But the codepaths that write out loose objects or packfiles\nthat must have left these objects during the earlier run in our repository\nshould already have done the validation.\n\nSo the only thing that we would be catching with this extra check is a\nloose object or a packfile that was deposited outside the control of git\nin your repository.\n\nBut that is not something we should be trying to catch every time we run\nfetch---if your repository is open to be written by a random person from\nsideways bypassing git, you have a much bigger problem.  And we have the\ntool to catch that kind of breakage: \"git fsck\".\n\nSo the right solution is probably use --objects for connectivity checks as\nbefore; we could add a fetch.paranoid configuration to allow people to\nstill use it (with this patch to remove the over-pessimism from the code)\nbut only if they want to.\n\nCc'ing Shawn, as I took inspiration from him in the first place for the\nupdate to --verify-objects (no, the overly-pessimistic implementation and\nexcess overhead you solved partially with this patch is not his fault but\nis mine).\n\nI said \"partially\" above in the parentheses because we would still end up\nrevalidating all the objects in quickfetch() check when you are fetching\nfrom the repository that is your alternate, even with this patch.\n"},{"id":"187059","messageId":"7vsjh9il3l.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"7vy5r1inax.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-15T21:57:02Z","receivedAt":"2012-03-15T21:57:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> After thinking about this a bit more, I am beginning to think that the\n> validation of object contents is unnecessary in _all_ cases that involve\n> \"git fetch\". ...\n> So the right solution is probably use --objects for connectivity checks as\n> before; we could add a fetch.paranoid configuration to allow people to\n> still use it (with this patch to remove the over-pessimism from the code)\n> but only if they want to.\n\nFollowing the above train of thought, the patch to revert what 6d4bb38\n(fetch: verify we have everything we need before updating our ref,\n2011-09-01) did which caused this performance regression should look like\nthis.  \"tag --contains 6d4bb38\" tells me that this affects everything\nsince v1.7.8-rc0 so we may want to issue maintenance updates once this\nproves good in 1.7.10.\n\nNguyễn, sorry for being dense.  I think this ended up being very close to\nyour initial patch.\n\n-- >8 --\nSubject: fetch/receive: remove over-pessimistic connectivity check\n\nGit 1.7.8 introduced an object and history re-validation step after\n\"fetch\" or \"push\" causes new history to be added to a receiving\nrepository, in order to protect a malicious server or pushing client to\ncorrupt it by taking advantage of an existing corrupt object that is\nunconnected to existing history of the receiving repository.\n\nBut this is way over-pessimistic, and often adds 5x to 8x runtime overhead\nfor a little gain.  During \"fetch\" or \"receive-pack\" (the server side of\n\"push\"), unpack-objects and index-pack already validate individual objects\nthat are received, and the only thing we would want to catch are objects\nthat already happened to exist in our repository but were not referenced\nfrom our refs.  But such objects must have been written by an earlier run\nof our codepaths that write out loose objects or packfiles, and they must\nhave done the validation of individual objects when they did so.  The only\nthing left to worry about is the connectivity integrity, which can be done\nwith much cheaper \"rev-list --objects\".\n\nRemove this over-pessimistic check, while leaving the door open for later\nchanges, hinting the possibility in an in-code comment.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fetch.c |    2 +-\n connected.c     |   14 +++++++++-----\n connected.h     |    3 ++-\n 3 files changed, 12 insertions(+), 7 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 8761a33..b2f7d5b 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -505,7 +505,7 @@ static int quickfetch(struct ref *ref_map)\n \t */\n \tif (depth)\n \t\treturn -1;\n-\treturn check_everything_connected(iterate_ref_map, 1, &rm);\n+\treturn check_everything_connected(iterate_ref_map, CONN_CHECK_QUIET, &rm);\n }\n \n static int fetch_refs(struct transport *transport, struct ref *ref_map)\ndiff --git a/connected.c b/connected.c\nindex d762423..3564a9f 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -6,22 +6,26 @@\n /*\n  * If we feed all the commits we want to verify to this command\n  *\n- *  $ git rev-list --verify-objects --stdin --not --all\n+ *  $ git rev-list --objects --stdin --not --all\n  *\n  * and if it does not error out, that means everything reachable from\n- * these commits locally exists and is connected to some of our\n- * existing refs.\n+ * these commits locally exists and is connected to our existing refs.\n  *\n  * Returns 0 if everything is connected, non-zero otherwise.\n+ *\n+ * We may want to introduce CONN_CHECK_PARANOIA option the caller can\n+ * pass to use --verify-objects instead of --objects to optionally have\n+ * individual objects re-validated.\n  */\n-int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+int check_everything_connected(sha1_iterate_fn fn, unsigned flags, void *cb_data)\n {\n \tstruct child_process rev_list;\n-\tconst char *argv[] = {\"rev-list\", \"--verify-objects\",\n+\tconst char *argv[] = {\"rev-list\", \"--objects\",\n \t\t\t      \"--stdin\", \"--not\", \"--all\", NULL, NULL};\n \tchar commit[41];\n \tunsigned char sha1[20];\n \tint err = 0;\n+\tint quiet = !!(flags & CONN_CHECK_QUIET);\n \n \tif (fn(cb_data, sha1))\n \t\treturn err;\ndiff --git a/connected.h b/connected.h\nindex 7e4585a..6f17e07 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -15,6 +15,7 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\n  *\n  * Return 0 if Ok, non zero otherwise (i.e. some missing objects)\n  */\n-extern int check_everything_connected(sha1_iterate_fn, int quiet, void *cb_data);\n+extern int check_everything_connected(sha1_iterate_fn, unsigned flags, void *cb_data);\n+#define CONN_CHECK_QUIET 01\n \n #endif /* CONNECTED_H */\n"},{"id":"187069","messageId":"CACsJy8A-W97Jd+8FOk1zWbDoTPb=RwO6Ck37O73v7DRrR+M5ew@mail.gmail.com","threadId":"29756","inReplyTo":"7vy5r1inax.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-16T02:24:34Z","receivedAt":"2012-03-16T02:24:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/3/16 Junio C Hamano <gitster@pobox.com>:\n> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:\n>\n>> When we fetch or push, usually \"git rev-list --verify-objects --not\n>> --all --stdin\" is used to make sure that all objects between existing\n>> refs and new refs are good. This means no gaps in between, all objects\n>> are well-formed, object content agrees with its sha-1 signature.\n>>\n>> For the last one, --verify-objects calls check_sha1_signature() via\n>> parse_object(). check_sha1_signature() is an expensive operation,\n>\n> After thinking about this a bit more, I am beginning to think that the\n> validation of object contents is unnecessary in _all_ cases that involve\n> \"git fetch\".  Unpack-objects and index-pack already validate individual\n> objects, and the only thing we would want to catch are objects that we\n> already happened to have had in our repository but were unreferenced from\n> our refs.\n\nWhat about remote helpers? Should we declare it's remote helper\nresponsibility to validate all incoming objects?\n-- \nDuy\n"},{"id":"187070","messageId":"CACsJy8D5UNd5b=Dpt75Qrc96u6wRLffB7eGSsKqM3qQ37p4cOw@mail.gmail.com","threadId":"29756","inReplyTo":"7vsjh9il3l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-16T02:28:19Z","receivedAt":"2012-03-16T02:28:19Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2012/3/16 Junio C Hamano <gitster@pobox.com>:\n> Following the above train of thought, the patch to revert what 6d4bb38\n> (fetch: verify we have everything we need before updating our ref,\n> 2011-09-01) did which caused this performance regression should look like\n> this.  \"tag --contains 6d4bb38\" tells me that this affects everything\n> since v1.7.8-rc0 so we may want to issue maintenance updates once this\n> proves good in 1.7.10.\n>\n> Nguyễn, sorry for being dense.  I think this ended up being very close to\n> your initial patch.\n\nI have cheaper \"git fetch\" any way. That's all that counts from my\nuser point of view :)\n\nWhat about push/receive-pack? I think the same thing happens there.\n-- \nDuy\n"},{"id":"187071","messageId":"7v4ntpi54l.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"CACsJy8D5UNd5b=Dpt75Qrc96u6wRLffB7eGSsKqM3qQ37p4cOw@mail.gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-16T03:42:02Z","receivedAt":"2012-03-16T03:42:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> What about push/receive-pack? I think the same thing happens there.\n\nAs the patch goes to check_everything_connected(), it is automatically\ncovered, no?\n"},{"id":"187072","messageId":"CACsJy8DcwjgdU5j_6LMbhwFJQXg+M=C3S_-UcRBsF3WyYZ6z7A@mail.gmail.com","threadId":"29756","inReplyTo":"7v4ntpi54l.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-03-16T03:42:39Z","receivedAt":"2012-03-16T03:42:39Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Mar 16, 2012 at 10:42 AM, Junio C Hamano <gitster@pobox.com> wrote:\n> Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n>\n>> What about push/receive-pack? I think the same thing happens there.\n>\n> As the patch goes to check_everything_connected(), it is automatically\n> covered, no?\n\nYeah right, sorry stupid question.\n-- \nDuy\n"},{"id":"187073","messageId":"7vwr6lgqcc.fsf@alter.siamese.dyndns.org","threadId":"29756","inReplyTo":"CACsJy8A-W97Jd+8FOk1zWbDoTPb=RwO6Ck37O73v7DRrR+M5ew@mail.gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-16T03:46:43Z","receivedAt":"2012-03-16T03:46:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:\n\n> What about remote helpers?\n\nWhat about them?  Don't these foreign-vcs helpers generally create git\nobjects on our side based on whatever data that comes from the remote\nforeign vcs anyway?\n\n> Should we declare it's remote helper\n> responsibility to validate all incoming objects?\n\nIt is nothing new that they must not deposit broken objects in the\nrepository, so I do not think there is nothing that needs to be\nspecifically pointed out.\n"}]}