{"thread":{"id":"33340","subject":"[PATCH 0/4] check_everything_connected replacement","startedAt":"2013-03-31T11:09:04Z","lastAt":"2013-05-26T01:01:33Z","messageCount":35,"participants":["Nguyễn Thái Ngọc Duy","Junio C Hamano","Duy Nguyen","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"212724","messageId":"1364728148-7537-1-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":null,"subject":"[PATCH 0/4] check_everything_connected replacement","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T11:09:04Z","receivedAt":"2013-03-31T11:09:04Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"My investigation in lowering connectivity check cost in git-clone [1]\nled me to try 'index-pack --strict' code. Without calling fsck_object(),\n--strict seems to be cheaper than check_everything_connected() while\naccomplishing the same thing (imo).\n\nThe first patch is a bug fix running git-clone --depth with\nfetch.fsckObjects on. The second is \"fix while i'm there\". The third\nintroduces check_everything_connected alternative. The fourth makes\nuse of it.\n\nThe last use of check_everything_connected after this series is\nfetch.c:quickfetch(), which I think is unnecessary. It can only catch\nerrors if we have incomplete object islands in repo, which could\nhappen before this series. After this series, incomplete object\nislands can't enter the repository, at least via git transport. So\nmaybe we should remove that check_everything_connected too (maybe\nafter a few years, enough time for the laziest user to run fsck/repack\nonce).\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/219602\n\nNguyễn Thái Ngọc Duy (4):\n fetch-pack: save shallow file before fetching the pack\n index-pack: remove dead code (it should never happen)\n index-pack, unpack-objects: add --not-so-strict for connectivity check\n Use --not-so-strict on all pack transfer for connectivity check\n\n builtin/fetch.c | 6 ----\n builtin/index-pack.c | 10 ++++--\n builtin/receive-pack.c | 22 +++----------\n builtin/unpack-objects.c | 9 ++++--\n fetch-pack.c | 72 +++++++++++++++++++++++------------------\n t/t5500-fetch-pack.sh | 7 ++++\n t/t5504-fetch-receive-strict.sh | 2 +-\n 7 files changed, 66 insertions(+), 62 deletions(-)\n\n-- \n1.8.2.83.gc99314b\n"},{"id":"212725","messageId":"1364728148-7537-2-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1364728148-7537-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/4] fetch-pack: save shallow file before fetching the pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T11:09:05Z","receivedAt":"2013-03-31T11:09:05Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"index-pack --strict looks up and follows parent commits. If shallow\ninformation is not ready by the time index-pack is run, index-pack may\nbe lead to non-existent objects. Make fetch-pack save shallow file to\ndisk before invoking index-pack.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n fetch-pack.c | 70 ++++++++++++++++++++++++++++-----------------------\n t/t5500-fetch-pack.sh | 7 ++++++\n 2 files changed, 45 insertions(+), 32 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex cef8fde..1f9c5ba 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -779,11 +779,44 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n \treturn strcmp(a->name, b->name);\n }\n \n+static void flush_shallow_to_disk(struct stat *st)\n+{\n+\tstatic struct lock_file lock;\n+\tstruct cache_time mtime;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tchar *shallow = git_path(\"shallow\");\n+\tint fd;\n+\n+\tmtime.sec = st->st_mtime;\n+\tmtime.nsec = ST_MTIME_NSEC(*st);\n+\tif (stat(shallow, st)) {\n+\t\tif (mtime.sec)\n+\t\t\tdie(\"shallow file was removed during fetch\");\n+\t} else if (st->st_mtime != mtime.sec\n+#ifdef USE_NSEC\n+\t\t || ST_MTIME_NSEC(*st) != mtime.nsec\n+#endif\n+\t\t )\n+\t\tdie(\"shallow file was changed during fetch\");\n+\n+\tfd = hold_lock_file_for_update(&lock, shallow,\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (!write_shallow_commits(&sb, 0)\n+\t || write_in_full(fd, sb.buf, sb.len) != sb.len) {\n+\t\tunlink_or_warn(shallow);\n+\t\trollback_lock_file(&lock);\n+\t} else {\n+\t\tcommit_lock_file(&lock);\n+\t}\n+\tstrbuf_release(&sb);\n+}\n+\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n \t\t\t\t struct ref **sought, int nr_sought,\n-\t\t\t\t char **pack_lockfile)\n+\t\t\t\t char **pack_lockfile,\n+\t\t\t\t struct stat *shallow_st)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n@@ -858,6 +891,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tif (args->stateless_rpc)\n \t\tpacket_flush(fd[1]);\n+\tif (args->depth > 0)\n+\t\tflush_shallow_to_disk(shallow_st);\n \tif (get_pack(args, fd, pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \n@@ -952,38 +987,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\tpacket_flush(fd[1]);\n \t\tdie(\"no matching remote head\");\n \t}\n-\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n \n-\tif (args->depth > 0) {\n-\t\tstatic struct lock_file lock;\n-\t\tstruct cache_time mtime;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tchar *shallow = git_path(\"shallow\");\n-\t\tint fd;\n-\n-\t\tmtime.sec = st.st_mtime;\n-\t\tmtime.nsec = ST_MTIME_NSEC(st);\n-\t\tif (stat(shallow, &st)) {\n-\t\t\tif (mtime.sec)\n-\t\t\t\tdie(\"shallow file was removed during fetch\");\n-\t\t} else if (st.st_mtime != mtime.sec\n-#ifdef USE_NSEC\n-\t\t\t\t|| ST_MTIME_NSEC(st) != mtime.nsec\n-#endif\n-\t\t\t )\n-\t\t\tdie(\"shallow file was changed during fetch\");\n-\n-\t\tfd = hold_lock_file_for_update(&lock, shallow,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n-\t\tif (!write_shallow_commits(&sb, 0)\n-\t\t || write_in_full(fd, sb.buf, sb.len) != sb.len) {\n-\t\t\tunlink_or_warn(shallow);\n-\t\t\trollback_lock_file(&lock);\n-\t\t} else {\n-\t\t\tcommit_lock_file(&lock);\n-\t\t}\n-\t\tstrbuf_release(&sb);\n-\t}\n+\tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought,\n+\t\t\t\tpack_lockfile, &st);\n \n \treprepare_packed_git();\n \treturn ref_cpy;\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d574085..557b073 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '\n \ttest \"`git --git-dir=shallow0/.git rev-list --count HEAD`\" = 1\n '\n \n+test_expect_success 'clone shallow depth 1 with fsck' '\n+\tgit config --global fetch.fsckobjects true &&\n+\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0fsck &&\n+\ttest \"`git --git-dir=shallow0fsck/.git rev-list --count HEAD`\" = 1 &&\n+\tgit config --global --unset fetch.fsckobjects\n+'\n+\n test_expect_success 'clone shallow' '\n \tgit clone --no-single-branch --depth 2 \"file://$(pwd)/.\" shallow\n '\n-- \n1.8.2.83.gc99314b\n"},{"id":"212727","messageId":"1364728148-7537-3-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1364728148-7537-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/4] index-pack: remove dead code (it should never happen)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T11:09:06Z","receivedAt":"2013-03-31T11:09:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex ef62124..fdac7c7 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -735,8 +735,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tint eaten;\n \t\t\tvoid *buf = (void *) data;\n \n-\t\t\tif (!buf)\n-\t\t\t\tbuf = new_data = get_data_from_pack(obj_entry);\n+\t\t\tassert(data && \"data can only be NULL for large blobs\");\n \n \t\t\t/*\n \t\t\t * we do not need to free the memory here, as the\n-- \n1.8.2.83.gc99314b\n"},{"id":"212726","messageId":"1364728148-7537-4-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1364728148-7537-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/4] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T11:09:07Z","receivedAt":"2013-03-31T11:09:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"--not-so-strict only checks if all links from objects in the pack\npoint to real objects (either in current repo, or from the pack\nitself). It's like check_everything_connected() except that:\n\n - it does not follow DAG in order\n - it can detect incomplete object islands\n - it seems to be faster than \"rev-list --objects --all\"\n\nOn my box, \"rev-list --objects --all\" takes 34 seconds. index-pack takes\n\n215.25user 8.42system 1:32.31elapsed 242%CPU (0avgtext+0avgdata 1357328maxresident)k\n0inputs+1421016outputs (0major+1222987minor)pagefaults 0swaps\n\nAnd index-pack --not-so-strict takes\n\npack 96a4e3befa40bf38eddc2d7c99246a59af4ad55d\n229.75user 11.31system 1:42.50elapsed 235%CPU (0avgtext+0avgdata 1876816maxresident)k\n0inputs+1421016outputs (0major+1307989minor)pagefaults 0swaps\n\nThe overhead is about 10 seconds, just 1/3 of rev-list, which makes it\nin a better position to replace check_everything_connected(). If this\nholds true for general case, it could reduce fetch time by a little bit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 7 ++++++-\n builtin/unpack-objects.c | 9 +++++++--\n 2 files changed, 13 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex fdac7c7..3cded32 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -77,6 +77,7 @@ static int nr_threads;\n \n static int from_stdin;\n static int strict;\n+static int do_fsck_object;\n static int verbose;\n \n static struct progress *progress;\n@@ -744,7 +745,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tobj = parse_object_buffer(sha1, type, size, buf, &eaten);\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n-\t\t\tif (fsck_object(obj, 1, fsck_error_function))\n+\t\t\tif (do_fsck_object &&\n+\t\t\t fsck_object(obj, 1, fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n@@ -1491,6 +1493,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--strict\")) {\n \t\t\t\tstrict = 1;\n+\t\t\t\tdo_fsck_object = 1;\n+\t\t\t} else if (!strcmp(arg, \"--not-so-strict\")) {\n+\t\t\t\tstrict = 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/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 2217d7b..dd0518b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -12,7 +12,7 @@\n #include \"decorate.h\"\n #include \"fsck.h\"\n \n-static int dry_run, quiet, recover, has_errors, strict;\n+static int dry_run, quiet, recover, has_errors, strict, do_fsck_object;\n static const char unpack_usage[] = \"git unpack-objects [-n] [-q] [-r] [--strict] < pack-file\";\n \n /* We always read in 4kB chunks. */\n@@ -198,7 +198,7 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tif (do_fsck_object && fsck_object(obj, 1, fsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n@@ -520,6 +520,11 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--strict\")) {\n+\t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tstrict = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!strcmp(arg, \"--not-so-strict\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.8.2.83.gc99314b\n"},{"id":"212728","messageId":"1364728148-7537-5-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1364728148-7537-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/4] Use --not-so-strict on all pack transfer for connectivity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-03-31T11:09:08Z","receivedAt":"2013-03-31T11:09:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This replaces check_everything_connected() with --not-so-strict, which\naccomplishes the same thing and is generally cheaper.\n\nThis also forces connectivity check on \"git clone\".\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/fetch.c | 6 ------\n builtin/receive-pack.c | 22 ++++------------------\n fetch-pack.c | 2 ++\n t/t5504-fetch-receive-strict.sh | 2 +-\n 4 files changed, 7 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4b6b1df..d9f970f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -400,12 +400,6 @@ static int store_updated_refs(const char *raw_url, const char *remote_name,\n \telse\n \t\turl = xstrdup(\"foreign\");\n \n-\trm = ref_map;\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm)) {\n-\t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n-\t\tgoto abort;\n-\t}\n-\n \t/*\n \t * The first pass writes objects to be merged and then the\n \t * second pass writes the rest, in order to allow using\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 62ba6e7..07abb14 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -663,19 +663,6 @@ static int command_singleton_iterator(void *cb_data, unsigned char sha1[20])\n \treturn 0;\n }\n \n-static void set_connectivity_errors(struct command *commands)\n-{\n-\tstruct command *cmd;\n-\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\tcontinue;\n-\t\tcmd->error_string = \"missing necessary objects\";\n-\t}\n-}\n-\n static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])\n {\n \tstruct command **cmd_list = cb_data;\n@@ -718,11 +705,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \t\treturn;\n \t}\n \n-\tcmd = commands;\n-\tif (check_everything_connected(iterate_receive_command_list,\n-\t\t\t\t 0, &cmd))\n-\t\tset_connectivity_errors(commands);\n-\n \treject_updates_to_hidden(commands);\n \n \tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n@@ -843,6 +825,8 @@ static const char *unpack(int err_fd)\n \t\t\tunpacker[i++] = \"-q\";\n \t\tif (fsck_objects)\n \t\t\tunpacker[i++] = \"--strict\";\n+\t\telse\n+\t\t\tunpacker[i++] = \"--not-so-strict\";\n \t\tunpacker[i++] = hdr_arg;\n \t\tunpacker[i++] = NULL;\n \t\tmemset(&child, 0, sizeof(child));\n@@ -868,6 +852,8 @@ static const char *unpack(int err_fd)\n \t\tkeeper[i++] = \"--stdin\";\n \t\tif (fsck_objects)\n \t\t\tkeeper[i++] = \"--strict\";\n+\t\telse\n+\t\t\tkeeper[i++] = \"--not-so-strict\";\n \t\tkeeper[i++] = \"--fix-thin\";\n \t\tkeeper[i++] = hdr_arg;\n \t\tkeeper[i++] = keep_arg;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1f9c5ba..ae20ae5 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -754,6 +754,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t ? transfer_fsck_objects\n \t : 0)\n \t\t*av++ = \"--strict\";\n+\telse\n+\t\t*av++ = \"--not-so-strict\";\n \t*av++ = NULL;\n \n \tcmd.in = demux.out;\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 69ee13c..14d2935 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -60,7 +60,7 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \n cat >exp < writes:\n\n> The last use of check_everything_connected after this series is\n> fetch.c:quickfetch(), which I think is unnecessary. It can only catch\n> errors if we have incomplete object islands in repo, which could\n> happen before this series.\n\nA typical source of object islands the quickfetch() was designed to\nprotect against is the HTTP walker. I do not think your patch adds\nany safety in that area (nor it should).\n\n> After this series, incomplete object\n> islands can't enter the repository, at least via git transport.\n\nExactly.\n"},{"id":"212798","messageId":"7v6206l2t6.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"1364728148-7537-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 1/4] fetch-pack: save shallow file before fetching the pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-04-01T14:53:57Z","receivedAt":"2013-04-01T14:53:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> index-pack --strict looks up and follows parent commits. If shallow\n> information is not ready by the time index-pack is run, index-pack may\n> be lead to non-existent objects. Make fetch-pack save shallow file to\n> disk before invoking index-pack.\n\nI think the problem the patch is trying to solve _is_ real, but I\nwonder if this is a correct approach to solve it. What happens if\nwe die in the middle after writing the updated shallow file\nprematurely?\n\nPerhaps the index-pack (or any \"Git\" process in general) needs to\nlearn a way to use an alternate shallow file, instead of always\nusing a hard-coded git_path(\"shallow\"), so that you can flush an\nupdated one to a to-be-the-next-shallow-file, run fetch-pack with\nthat alternate shallow file, and rename it to the final name at the\nend after everything goes well, or something?\n"},{"id":"213227","messageId":"CACsJy8CEEoL7LxDNzD=dwp_8qxeHgDr+fuswXrRcD32k9zfT5w@mail.gmail.com","threadId":"33340","inReplyTo":"7v6206l2t6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/4] fetch-pack: save shallow file before fetching the pack","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-04-05T02:11:14Z","receivedAt":"2013-04-05T02:11:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Apr 2, 2013 at 1:53 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> index-pack --strict looks up and follows parent commits. If shallow\n>> information is not ready by the time index-pack is run, index-pack may\n>> be lead to non-existent objects. Make fetch-pack save shallow file to\n>> disk before invoking index-pack.\n>\n> I think the problem the patch is trying to solve _is_ real, but I\n> wonder if this is a correct approach to solve it. What happens if\n> we die in the middle after writing the updated shallow file\n> prematurely?\n>\n> Perhaps the index-pack (or any \"Git\" process in general) needs to\n> learn a way to use an alternate shallow file, instead of always\n> using a hard-coded git_path(\"shallow\"), so that you can flush an\n> updated one to a to-be-the-next-shallow-file, run fetch-pack with\n> that alternate shallow file, and rename it to the final name at the\n> end after everything goes well, or something?\n\nYeah. I was focused on the clone case so it didn't matter but this\nchange applies to git-fetch as well. Will add new GIT_SHALLOW_FILE\nenvironment variable.\n--\nDuy\n"},{"id":"216133","messageId":"1367405974-22190-1-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1364728148-7537-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 0/5] check_everything_connected replacement","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:29Z","receivedAt":"2013-05-01T10:59:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This series attempts to reduce the cost of check_everything_connected\non the clone case where index-pack is run. For example, cloning\nfile://../linux-2.6.git:\n \n before after\nreal 4m23.664s 3m47.280s\nuser 4m55.613s 4m39.530s\nsys 0m14.805s 0m17.728s\n\nThe first three patches are improvements or fixes and I think they're\ngood to go (if no bugs are found). The last two introduce\n--not-so-strict as check_everything_connected replacement and use it\nwhere applicable and are debatable (I guess).\n\nThe reroll fixes shallow file update issue Junio pointed out\npreviously. It also does not remove check_everything_connected unless\nit knows that index-pack or unpack-objects are run with\n--not-so-strict.\n\nNguyễn Thái Ngọc Duy (5):\n clone: let the user know when check_everything_connected is run\n fetch-pack: prepare updated shallow file before fetching the pack\n index-pack: remove dead code (it should never happen)\n index-pack, unpack-objects: add --not-so-strict for connectivity check\n Use --not-so-strict on all pack transfer for connectivity check\n\n Documentation/git-index-pack.txt | 3 ++\n Documentation/git-unpack-objects.txt | 4 +++\n builtin/clone.c | 18 +++++++---\n builtin/fetch.c | 11 +++---\n builtin/index-pack.c | 10 ++++--\n builtin/receive-pack.c | 51 +++------------------------\n builtin/unpack-objects.c | 9 +++--\n commit.h | 1 +\n fetch-pack.c | 67 ++++++++++++++++++------------------\n fetch-pack.h | 3 +-\n shallow.c | 30 ++++++++++++++--\n t/t5500-fetch-pack.sh | 7 ++++\n t/t5504-fetch-receive-strict.sh | 2 +-\n transport.c | 2 ++\n transport.h | 1 +\n 15 files changed, 120 insertions(+), 99 deletions(-)\n\n-- \n1.8.2.83.gc99314b\n"},{"id":"216134","messageId":"1367405974-22190-2-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/5] clone: let the user know when check_everything_connected is run","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:30Z","receivedAt":"2013-05-01T10:59:30Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"check_everything_connected could take a long time, especially in the\nclone case where the whole DAG is traversed. The user deserves to know\nwhat's going on.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 035ab64..dad4265 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -546,8 +546,12 @@ static void update_remote_refs(const struct ref *refs,\n {\n \tconst struct ref *rm = mapped_refs;\n \n+\tif (0 <= option_verbosity)\n+\t\tprintf(_(\"Checking connectivity... \"));\n \tif (check_everything_connected(iterate_ref_map, 0, &rm))\n \t\tdie(_(\"remote did not send all necessary objects\"));\n+\tif (0 <= option_verbosity)\n+\t\tprintf(_(\"done\\n\"));\n \n \tif (refs) {\n \t\twrite_remote_refs(mapped_refs);\n-- \n1.8.2.83.gc99314b\n"},{"id":"216135","messageId":"1367405974-22190-3-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/5] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:31Z","receivedAt":"2013-05-01T10:59:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"index-pack --strict looks up and follows parent commits. If shallow\ninformation is not ready by the time index-pack is run, index-pack may\nbe lead to non-existent objects. Make fetch-pack save shallow file to\ndisk before invoking index-pack.\n\nA new environment variable GIT_SHALLOW_FILE is used to inform\nindex-pack to use an alternate shallow file. This variable is for\ninternal use and thus not advertised in git.txt.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n commit.h | 1 +\n fetch-pack.c | 65 ++++++++++++++++++++++++---------------------------\n shallow.c | 30 ++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 7 ++++++\n 4 files changed, 67 insertions(+), 36 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 67bd509..0cef659 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -176,6 +176,7 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);\n extern int is_repository_shallow(void);\n extern struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\tint depth, int shallow_flag, int not_shallow_flag);\n+extern void check_shallow_file_for_update(void);\n \n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit *);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f156dd4..17cfa88 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -20,6 +20,7 @@ static int no_done;\n static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n+static struct lock_file shallow_lock;\n \n #define COMPLETE\t(1U << 0)\n #define COMMON\t\t(1U << 1)\n@@ -779,6 +780,26 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n \treturn strcmp(a->name, b->name);\n }\n \n+static void setup_alternate_shallow(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n+\n+\tcheck_shallow_file_for_update();\n+\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (write_shallow_commits(&sb, 0)) {\n+\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\t\tdie_errno(\"failed to write to %s\", shallow_lock.filename);\n+\t\tif (setenv(\"GIT_SHALLOW_FILE\", shallow_lock.filename, 1))\n+\t\t\tdie_errno(\"failed to set GIT_SHALLOW_FILE\");\n+\t} else {\n+\t\tif (setenv(\"GIT_SHALLOW_FILE\", \"\", 1))\n+\t\t\tdie_errno(\"failed to set GIT_SHALLOW_FILE\");\n+\t}\n+\tstrbuf_release(&sb);\n+}\n+\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n@@ -858,6 +879,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tif (args->stateless_rpc)\n \t\tpacket_flush(fd[1]);\n+\tif (args->depth > 0)\n+\t\tsetup_alternate_shallow();\n \tif (get_pack(args, fd, pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \n@@ -936,15 +959,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t struct ref **sought, int nr_sought,\n \t\t char **pack_lockfile)\n {\n-\tstruct stat st;\n \tstruct ref *ref_cpy;\n \n \tfetch_pack_setup();\n-\tif (args->depth > 0) {\n-\t\tif (stat(git_path(\"shallow\"), &st))\n-\t\t\tst.st_mtime = 0;\n-\t}\n-\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n@@ -955,34 +972,14 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n \n \tif (args->depth > 0) {\n-\t\tstatic struct lock_file lock;\n-\t\tstruct cache_time mtime;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tchar *shallow = git_path(\"shallow\");\n-\t\tint fd;\n-\n-\t\tmtime.sec = st.st_mtime;\n-\t\tmtime.nsec = ST_MTIME_NSEC(st);\n-\t\tif (stat(shallow, &st)) {\n-\t\t\tif (mtime.sec)\n-\t\t\t\tdie(\"shallow file was removed during fetch\");\n-\t\t} else if (st.st_mtime != mtime.sec\n-#ifdef USE_NSEC\n-\t\t\t\t|| ST_MTIME_NSEC(st) != mtime.nsec\n-#endif\n-\t\t\t )\n-\t\t\tdie(\"shallow file was changed during fetch\");\n-\n-\t\tfd = hold_lock_file_for_update(&lock, shallow,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n-\t\tif (!write_shallow_commits(&sb, 0)\n-\t\t || write_in_full(fd, sb.buf, sb.len) != sb.len) {\n-\t\t\tunlink_or_warn(shallow);\n-\t\t\trollback_lock_file(&lock);\n-\t\t} else {\n-\t\t\tcommit_lock_file(&lock);\n-\t\t}\n-\t\tstrbuf_release(&sb);\n+\t\tstruct stat st;\n+\t\tif (!fstat(shallow_lock.fd, &st) &&\n+\t\t st.st_size == 0) {\n+\t\t\tunlink_or_warn(git_path(\"shallow\"));\n+\t\t\trollback_lock_file(&shallow_lock);\n+\t\t} else\n+\t\t\tcommit_lock_file(&shallow_lock);\n+\t\tunsetenv(\"GIT_SHALLOW_FILE\");\n \t}\n \n \treprepare_packed_git();\ndiff --git a/shallow.c b/shallow.c\nindex 6be915f..9c03806 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -3,6 +3,7 @@\n #include \"tag.h\"\n \n static int is_shallow = -1;\n+static struct stat shallow_stat;\n \n int register_shallow(const unsigned char *sha1)\n {\n@@ -21,12 +22,15 @@ int is_repository_shallow(void)\n {\n \tFILE *fp;\n \tchar buf[1024];\n+\tconst char *path;\n \n \tif (is_shallow >= 0)\n \t\treturn is_shallow;\n \n-\tfp = fopen(git_path(\"shallow\"), \"r\");\n-\tif (!fp) {\n+\tpath = getenv(\"GIT_SHALLOW_FILE\");\n+\tif (!path)\n+\t\tpath = git_path(\"shallow\");\n+\tif (stat(path, &shallow_stat) || (fp = fopen(path, \"r\")) == NULL) {\n \t\tis_shallow = 0;\n \t\treturn is_shallow;\n \t}\n@@ -108,3 +112,25 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \n \treturn result;\n }\n+\n+void check_shallow_file_for_update(void)\n+{\n+\tstruct stat st;\n+\n+\tif (getenv(\"GIT_SHALLOW_FILE\"))\n+\t\tdie(\"GIT_SHALLOW_FILE should not be manually set\");\n+\n+\tif (!is_shallow)\n+\t\treturn;\n+\telse if (is_shallow == -1)\n+\t\tdie(\"BUG: shallow must be initialized by now\");\n+\n+\tif (stat(git_path(\"shallow\"), &st))\n+\t\tdie(\"shallow file was removed during fetch\");\n+\telse if (st.st_mtime != shallow_stat.st_mtime\n+#ifdef USE_NSEC\n+\t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n+#endif\n+\t\t )\n+\t\tdie(\"shallow file was changed during fetch\");\n+}\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d574085..557b073 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '\n \ttest \"`git --git-dir=shallow0/.git rev-list --count HEAD`\" = 1\n '\n \n+test_expect_success 'clone shallow depth 1 with fsck' '\n+\tgit config --global fetch.fsckobjects true &&\n+\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0fsck &&\n+\ttest \"`git --git-dir=shallow0fsck/.git rev-list --count HEAD`\" = 1 &&\n+\tgit config --global --unset fetch.fsckobjects\n+'\n+\n test_expect_success 'clone shallow' '\n \tgit clone --no-single-branch --depth 2 \"file://$(pwd)/.\" shallow\n '\n-- \n1.8.2.83.gc99314b\n"},{"id":"216136","messageId":"1367405974-22190-4-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/5] index-pack: remove dead code (it should never happen)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:32Z","receivedAt":"2013-05-01T10:59:32Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 79dfe47..1fd56d9 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -747,8 +747,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tint eaten;\n \t\t\tvoid *buf = (void *) data;\n \n-\t\t\tif (!buf)\n-\t\t\t\tbuf = new_data = get_data_from_pack(obj_entry);\n+\t\t\tassert(data && \"data can only be NULL for large blobs\");\n \n \t\t\t/*\n \t\t\t * we do not need to free the memory here, as the\n-- \n1.8.2.83.gc99314b\n"},{"id":"216137","messageId":"1367405974-22190-5-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:33Z","receivedAt":"2013-05-01T10:59:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"--not-so-strict only checks if all links from objects in the pack\npoint to real objects (either in current repo, or from the pack\nitself). It's like check_everything_connected() except that:\n\n - it does not follow DAG in order\n - it can detect incomplete object islands\n - it seems to be faster than \"rev-list --objects --all\"\n\nOn my box, \"rev-list --objects --all\" takes 34 seconds. index-pack takes\n\n215.25user 8.42system 1:32.31elapsed 242%CPU (0avgtext+0avgdata 1357328maxresident)k\n0inputs+1421016outputs (0major+1222987minor)pagefaults 0swaps\n\nAnd index-pack --not-so-strict takes\n\npack 96a4e3befa40bf38eddc2d7c99246a59af4ad55d\n229.75user 11.31system 1:42.50elapsed 235%CPU (0avgtext+0avgdata 1876816maxresident)k\n0inputs+1421016outputs (0major+1307989minor)pagefaults 0swaps\n\nThe overhead is about 10 seconds, just 1/3 of rev-list, which makes it\nin a better position to replace check_everything_connected(). If this\nholds true for general case, it could reduce fetch time by a little bit.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-index-pack.txt | 3 +++\n Documentation/git-unpack-objects.txt | 4 ++++\n builtin/index-pack.c | 7 ++++++-\n builtin/unpack-objects.c | 9 +++++++--\n 4 files changed, 20 insertions(+), 3 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex bde8eec..51af7be 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -74,6 +74,9 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--not-so-strict::\n+\tDie if the pack contains broken links. For internal use only.\n+\n --threads=::\n \tSpecifies the number of threads to spawn when resolving\n \tdeltas. This requires that index-pack be compiled with\ndiff --git a/Documentation/git-unpack-objects.txt b/Documentation/git-unpack-objects.txt\nindex ff23494..14b6018 100644\n--- a/Documentation/git-unpack-objects.txt\n+++ b/Documentation/git-unpack-objects.txt\n@@ -44,6 +44,10 @@ OPTIONS\n --strict::\n \tDon't write objects with broken content or links.\n \n+--not-so-strict::\n+\tDon't write objects with broken links. For internal\n+\tuse only.\n+\n GIT\n ---\n Part of the linkgit:git[1] suite\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1fd56d9..5d28a1b 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -77,6 +77,7 @@ static int nr_threads;\n \n static int from_stdin;\n static int strict;\n+static int do_fsck_object;\n static int verbose;\n static int show_stat;\n \n@@ -756,7 +757,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tobj = parse_object_buffer(sha1, type, size, buf, &eaten);\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n-\t\t\tif (fsck_object(obj, 1, fsck_error_function))\n+\t\t\tif (do_fsck_object &&\n+\t\t\t fsck_object(obj, 1, fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n@@ -1511,6 +1513,9 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--strict\")) {\n \t\t\t\tstrict = 1;\n+\t\t\t\tdo_fsck_object = 1;\n+\t\t\t} else if (!strcmp(arg, \"--not-so-strict\")) {\n+\t\t\t\tstrict = 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/builtin/unpack-objects.c b/builtin/unpack-objects.c\nindex 2217d7b..dd0518b 100644\n--- a/builtin/unpack-objects.c\n+++ b/builtin/unpack-objects.c\n@@ -12,7 +12,7 @@\n #include \"decorate.h\"\n #include \"fsck.h\"\n \n-static int dry_run, quiet, recover, has_errors, strict;\n+static int dry_run, quiet, recover, has_errors, strict, do_fsck_object;\n static const char unpack_usage[] = \"git unpack-objects [-n] [-q] [-r] [--strict] < pack-file\";\n \n /* We always read in 4kB chunks. */\n@@ -198,7 +198,7 @@ static int check_object(struct object *obj, int type, void *data)\n \t\treturn 0;\n \t}\n \n-\tif (fsck_object(obj, 1, fsck_error_function))\n+\tif (do_fsck_object && fsck_object(obj, 1, fsck_error_function))\n \t\tdie(\"Error in object\");\n \tif (fsck_walk(obj, check_object, NULL))\n \t\tdie(\"Error on reachable objects of %s\", sha1_to_hex(obj->sha1));\n@@ -520,6 +520,11 @@ int cmd_unpack_objects(int argc, const char **argv, const char *prefix)\n \t\t\t\tcontinue;\n \t\t\t}\n \t\t\tif (!strcmp(arg, \"--strict\")) {\n+\t\t\t\tdo_fsck_object = 1;\n+\t\t\t\tstrict = 1;\n+\t\t\t\tcontinue;\n+\t\t\t}\n+\t\t\tif (!strcmp(arg, \"--not-so-strict\")) {\n \t\t\t\tstrict = 1;\n \t\t\t\tcontinue;\n \t\t\t}\n-- \n1.8.2.83.gc99314b\n"},{"id":"216138","messageId":"1367405974-22190-6-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 5/5] Use --not-so-strict on all pack transfer for connectivity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-01T10:59:34Z","receivedAt":"2013-05-01T10:59:34Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This replaces check_everything_connected() with --not-so-strict, which\naccomplishes the same thing and is generally cheaper when index-pack\nor unpack-objects are used. All other cases fall back to\ncheck_everything_connected.\n\nThis could help reduce the impact of check_everything_connected() on\npack transport. For example, cloning file://../linux-2.6.git\n\n before after\nreal 4m23.664s 3m47.280s\nuser 4m55.613s 4m39.530s\nsys 0m14.805s 0m17.728s\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 22 ++++++++++--------\n builtin/fetch.c | 11 +++++----\n builtin/receive-pack.c | 51 ++++-------------------------------------\n fetch-pack.c | 2 ++\n fetch-pack.h | 3 ++-\n t/t5504-fetch-receive-strict.sh | 2 +-\n transport.c | 2 ++\n transport.h | 1 +\n 8 files changed, 32 insertions(+), 62 deletions(-)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dad4265..1cd37c1 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -497,6 +497,8 @@ static void write_remote_refs(const struct ref *local_refs)\n \tfor (r = local_refs; r; r = r->next) {\n \t\tif (!r->peer_ref)\n \t\t\tcontinue;\n+\t\tif (!has_sha1_file(r->old_sha1))\n+\t\t\tdie(_(\"remote did not send all necessary objects\"));\n \t\tadd_packed_ref(r->peer_ref->name, r->old_sha1);\n \t}\n \n@@ -544,15 +546,6 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t const char *branch_top,\n \t\t\t const char *msg)\n {\n-\tconst struct ref *rm = mapped_refs;\n-\n-\tif (0 <= option_verbosity)\n-\t\tprintf(_(\"Checking connectivity... \"));\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n-\t\tdie(_(\"remote did not send all necessary objects\"));\n-\tif (0 <= option_verbosity)\n-\t\tprintf(_(\"done\\n\"));\n-\n \tif (refs) {\n \t\twrite_remote_refs(mapped_refs);\n \t\tif (option_single_branch)\n@@ -953,6 +946,17 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \telse if (refs && complete_refs_before_fetch)\n \t\ttransport_fetch_refs(transport, mapped_refs);\n \n+\tif (!transport->smart_options ||\n+\t !transport->smart_options->connected) {\n+\t\tconst struct ref *rm = mapped_refs;\n+\n+\t\tif (0 <= option_verbosity)\n+\t\t\tprintf(_(\"Checking connectivity... \"));\n+\t\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n+\t\t\tdie(_(\"remote did not send all necessary objects\"));\n+\t\tif (0 <= option_verbosity)\n+\t\t\tprintf(_(\"done\\n\"));\n+\t}\n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n \t\t\t branch_top.buf, reflog_msg.buf);\n \ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 4b6b1df..e013d3f 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -380,7 +380,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 need_check)\n {\n \tFILE *fp;\n \tstruct commit *commit;\n@@ -401,7 +401,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 (need_check &&\n+\t check_everything_connected(iterate_ref_map, 0, &rm)) {\n \t\trc = error(_(\"%s did not send all necessary objects\\n\"), url);\n \t\tgoto abort;\n \t}\n@@ -538,8 +539,10 @@ 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->smart_options ||\n+\t\t\t\t\t !transport->smart_options->connected);\n \ttransport_unlock_pack(transport);\n \treturn ret;\n }\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e3eb5fc..2157c1a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -651,48 +651,6 @@ static void check_aliased_updates(struct command *commands)\n \tstring_list_clear(&ref_list, 0);\n }\n \n-static int command_singleton_iterator(void *cb_data, unsigned char sha1[20])\n-{\n-\tstruct command **cmd_list = cb_data;\n-\tstruct command *cmd = *cmd_list;\n-\n-\tif (!cmd || is_null_sha1(cmd->new_sha1))\n-\t\treturn -1; /* end of list */\n-\t*cmd_list = NULL; /* this returns only one */\n-\thashcpy(sha1, cmd->new_sha1);\n-\treturn 0;\n-}\n-\n-static void set_connectivity_errors(struct command *commands)\n-{\n-\tstruct command *cmd;\n-\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\tcontinue;\n-\t\tcmd->error_string = \"missing necessary objects\";\n-\t}\n-}\n-\n-static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])\n-{\n-\tstruct command **cmd_list = cb_data;\n-\tstruct command *cmd = *cmd_list;\n-\n-\twhile (cmd) {\n-\t\tif (!is_null_sha1(cmd->new_sha1)) {\n-\t\t\thashcpy(sha1, cmd->new_sha1);\n-\t\t\t*cmd_list = cmd->next;\n-\t\t\treturn 0;\n-\t\t}\n-\t\tcmd = cmd->next;\n-\t}\n-\t*cmd_list = NULL;\n-\treturn -1; /* end of list */\n-}\n-\n static void reject_updates_to_hidden(struct command *commands)\n {\n \tstruct command *cmd;\n@@ -718,11 +676,6 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \t\treturn;\n \t}\n \n-\tcmd = commands;\n-\tif (check_everything_connected(iterate_receive_command_list,\n-\t\t\t\t 0, &cmd))\n-\t\tset_connectivity_errors(commands);\n-\n \treject_updates_to_hidden(commands);\n \n \tif (run_receive_hook(commands, \"pre-receive\", 0)) {\n@@ -844,6 +797,8 @@ static const char *unpack(int err_fd)\n \t\t\tunpacker[i++] = \"-q\";\n \t\tif (fsck_objects)\n \t\t\tunpacker[i++] = \"--strict\";\n+\t\telse\n+\t\t\tunpacker[i++] = \"--not-so-strict\";\n \t\tunpacker[i++] = hdr_arg;\n \t\tunpacker[i++] = NULL;\n \t\tmemset(&child, 0, sizeof(child));\n@@ -869,6 +824,8 @@ static const char *unpack(int err_fd)\n \t\tkeeper[i++] = \"--stdin\";\n \t\tif (fsck_objects)\n \t\t\tkeeper[i++] = \"--strict\";\n+\t\telse\n+\t\t\tkeeper[i++] = \"--not-so-strict\";\n \t\tkeeper[i++] = \"--fix-thin\";\n \t\tkeeper[i++] = hdr_arg;\n \t\tkeeper[i++] = keep_arg;\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 17cfa88..cdd9e2d 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -755,6 +755,8 @@ static int get_pack(struct fetch_pack_args *args,\n \t ? transfer_fsck_objects\n \t : 0)\n \t\t*av++ = \"--strict\";\n+\telse if (args->check_connectivity)\n+\t\t*av++ = \"--not-so-strict\";\n \t*av++ = NULL;\n \n \tcmd.in = demux.out;\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex dc5266c..824136d 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -16,7 +16,8 @@ struct fetch_pack_args {\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\tcheck_connectivity:1;\n };\n \n /*\ndiff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh\nindex 69ee13c..14d2935 100755\n--- a/t/t5504-fetch-receive-strict.sh\n+++ b/t/t5504-fetch-receive-strict.sh\n@@ -60,7 +60,7 @@ test_expect_success 'fetch with transfer.fsckobjects' '\n \n cat >exp <verbose < 0);\n \targs.no_progress = !transport->progress;\n \targs.depth = data->options.depth;\n+\targs.check_connectivity = 1;\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -551,6 +552,7 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs = NULL;\n \tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n+\tdata->options.connected = 1;\n \n \tfree_refs(refs_tmp);\n \ndiff --git a/transport.h b/transport.h\nindex fcb1d25..e127224 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -8,6 +8,7 @@ struct git_transport_options {\n \tunsigned thin : 1;\n \tunsigned keep : 1;\n \tunsigned followtags : 1;\n+\tunsigned connected : 1;\n \tint depth;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n-- \n1.8.2.83.gc99314b\n"},{"id":"216190","messageId":"7v61z25tuf.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"1367405974-22190-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/5] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T20:27:20Z","receivedAt":"2013-05-01T20:27:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> index-pack --strict looks up and follows parent commits. If shallow\n> information is not ready by the time index-pack is run, index-pack may\n> be lead to non-existent objects. Make fetch-pack save shallow file to\n> disk before invoking index-pack.\n>\n> A new environment variable GIT_SHALLOW_FILE is used to inform\n> index-pack to use an alternate shallow file. This variable is for\n> internal use and thus not advertised in git.txt.\n\nThe idea to tell the index-pack to use a different shallow file is\nsound, but is the environment variable the best way to go? I am\nmostly worried about it being an implicit \"apply everywhere\"\nmechanism and while I do not particularly like the idea of doing\neverything in the same process as the top-level while dealing with\nsubmodules, I am reasonably sure people would want to \"clone\"\nrecursively inside the same top-level process in the future, and\nthis new environment variable adds one more thing to be cleansed\nwhen crossing a repository boundary.\n\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> commit.h | 1 +\n> fetch-pack.c | 65 ++++++++++++++++++++++++---------------------------\n> shallow.c | 30 ++++++++++++++++++++++--\n> t/t5500-fetch-pack.sh | 7 ++++++\n> 4 files changed, 67 insertions(+), 36 deletions(-)\n>\n> diff --git a/commit.h b/commit.h\n> index 67bd509..0cef659 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -176,6 +176,7 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);\n> extern int is_repository_shallow(void);\n> extern struct commit_list *get_shallow_commits(struct object_array *heads,\n> \t\tint depth, int shallow_flag, int not_shallow_flag);\n> +extern void check_shallow_file_for_update(void);\n> \n> int is_descendant_of(struct commit *, struct commit_list *);\n> int in_merge_bases(struct commit *, struct commit *);\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f156dd4..17cfa88 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -20,6 +20,7 @@ static int no_done;\n> static int fetch_fsck_objects = -1;\n> static int transfer_fsck_objects = -1;\n> static int agent_supported;\n> +static struct lock_file shallow_lock;\n> \n> #define COMPLETE\t(1U << 0)\n> #define COMMON\t\t(1U << 1)\n> @@ -779,6 +780,26 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n> \treturn strcmp(a->name, b->name);\n> }\n> \n> +static void setup_alternate_shallow(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tint fd;\n> +\n> +\tcheck_shallow_file_for_update();\n> +\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n> +\t\t\t\t LOCK_DIE_ON_ERROR);\n> +\tif (write_shallow_commits(&sb, 0)) {\n> +\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n> +\t\t\tdie_errno(\"failed to write to %s\", shallow_lock.filename);\n> +\t\tif (setenv(\"GIT_SHALLOW_FILE\", shallow_lock.filename, 1))\n> +\t\t\tdie_errno(\"failed to set GIT_SHALLOW_FILE\");\n> +\t} else {\n> +\t\tif (setenv(\"GIT_SHALLOW_FILE\", \"\", 1))\n> +\t\t\tdie_errno(\"failed to set GIT_SHALLOW_FILE\");\n> +\t}\n> +\tstrbuf_release(&sb);\n> +}\n> +\n> static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \t\t\t\t int fd[2],\n> \t\t\t\t const struct ref *orig_ref,\n> @@ -858,6 +879,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \n> \tif (args->stateless_rpc)\n> \t\tpacket_flush(fd[1]);\n> +\tif (args->depth > 0)\n> +\t\tsetup_alternate_shallow();\n> \tif (get_pack(args, fd, pack_lockfile))\n> \t\tdie(\"git fetch-pack: fetch failed.\");\n> \n> @@ -936,15 +959,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n> \t\t struct ref **sought, int nr_sought,\n> \t\t char **pack_lockfile)\n> {\n> -\tstruct stat st;\n> \tstruct ref *ref_cpy;\n> \n> \tfetch_pack_setup();\n> -\tif (args->depth > 0) {\n> -\t\tif (stat(git_path(\"shallow\"), &st))\n> -\t\t\tst.st_mtime = 0;\n> -\t}\n> -\n> \tif (nr_sought)\n> \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n> \n> @@ -955,34 +972,14 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n> \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n> \n> \tif (args->depth > 0) {\n> -\t\tstatic struct lock_file lock;\n> -\t\tstruct cache_time mtime;\n> -\t\tstruct strbuf sb = STRBUF_INIT;\n> -\t\tchar *shallow = git_path(\"shallow\");\n> -\t\tint fd;\n> -\n> -\t\tmtime.sec = st.st_mtime;\n> -\t\tmtime.nsec = ST_MTIME_NSEC(st);\n> -\t\tif (stat(shallow, &st)) {\n> -\t\t\tif (mtime.sec)\n> -\t\t\t\tdie(\"shallow file was removed during fetch\");\n> -\t\t} else if (st.st_mtime != mtime.sec\n> -#ifdef USE_NSEC\n> -\t\t\t\t|| ST_MTIME_NSEC(st) != mtime.nsec\n> -#endif\n> -\t\t\t )\n> -\t\t\tdie(\"shallow file was changed during fetch\");\n> -\n> -\t\tfd = hold_lock_file_for_update(&lock, shallow,\n> -\t\t\t\t\t LOCK_DIE_ON_ERROR);\n> -\t\tif (!write_shallow_commits(&sb, 0)\n> -\t\t || write_in_full(fd, sb.buf, sb.len) != sb.len) {\n> -\t\t\tunlink_or_warn(shallow);\n> -\t\t\trollback_lock_file(&lock);\n> -\t\t} else {\n> -\t\t\tcommit_lock_file(&lock);\n> -\t\t}\n> -\t\tstrbuf_release(&sb);\n> +\t\tstruct stat st;\n> +\t\tif (!fstat(shallow_lock.fd, &st) &&\n> +\t\t st.st_size == 0) {\n> +\t\t\tunlink_or_warn(git_path(\"shallow\"));\n> +\t\t\trollback_lock_file(&shallow_lock);\n> +\t\t} else\n> +\t\t\tcommit_lock_file(&shallow_lock);\n> +\t\tunsetenv(\"GIT_SHALLOW_FILE\");\n> \t}\n> \n> \treprepare_packed_git();\n> diff --git a/shallow.c b/shallow.c\n> index 6be915f..9c03806 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -3,6 +3,7 @@\n> #include \"tag.h\"\n> \n> static int is_shallow = -1;\n> +static struct stat shallow_stat;\n> \n> int register_shallow(const unsigned char *sha1)\n> {\n> @@ -21,12 +22,15 @@ int is_repository_shallow(void)\n> {\n> \tFILE *fp;\n> \tchar buf[1024];\n> +\tconst char *path;\n> \n> \tif (is_shallow >= 0)\n> \t\treturn is_shallow;\n> \n> -\tfp = fopen(git_path(\"shallow\"), \"r\");\n> -\tif (!fp) {\n> +\tpath = getenv(\"GIT_SHALLOW_FILE\");\n> +\tif (!path)\n> +\t\tpath = git_path(\"shallow\");\n> +\tif (stat(path, &shallow_stat) || (fp = fopen(path, \"r\")) == NULL) {\n> \t\tis_shallow = 0;\n> \t\treturn is_shallow;\n> \t}\n> @@ -108,3 +112,25 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \n> \treturn result;\n> }\n> +\n> +void check_shallow_file_for_update(void)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (getenv(\"GIT_SHALLOW_FILE\"))\n> +\t\tdie(\"GIT_SHALLOW_FILE should not be manually set\");\n> +\n> +\tif (!is_shallow)\n> +\t\treturn;\n> +\telse if (is_shallow == -1)\n> +\t\tdie(\"BUG: shallow must be initialized by now\");\n> +\n> +\tif (stat(git_path(\"shallow\"), &st))\n> +\t\tdie(\"shallow file was removed during fetch\");\n> +\telse if (st.st_mtime != shallow_stat.st_mtime\n> +#ifdef USE_NSEC\n> +\t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n> +#endif\n> +\t\t )\n> +\t\tdie(\"shallow file was changed during fetch\");\n> +}\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d574085..557b073 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '\n> \ttest \"`git --git-dir=shallow0/.git rev-list --count HEAD`\" = 1\n> '\n> \n> +test_expect_success 'clone shallow depth 1 with fsck' '\n> +\tgit config --global fetch.fsckobjects true &&\n> +\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0fsck &&\n> +\ttest \"`git --git-dir=shallow0fsck/.git rev-list --count HEAD`\" = 1 &&\n> +\tgit config --global --unset fetch.fsckobjects\n> +'\n> +\n> test_expect_success 'clone shallow' '\n> \tgit clone --no-single-branch --depth 2 \"file://$(pwd)/.\" shallow\n> '\n"},{"id":"216220","messageId":"7vvc722s0h.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"1367405974-22190-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-01T23:35:10Z","receivedAt":"2013-05-01T23:35:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> --not-so-strict only checks if all links from objects in the pack\n> point to real objects (either in current repo, or from the pack\n> itself). It's like check_everything_connected() except that:\n>\n> - it does not follow DAG in order\n> - it can detect incomplete object islands\n\nCould you clarify what this second point means?\n\n\"rev-list --objects --all --not $this $that\" does not detect\n\"islands\" but checking with the updated index-pack does?\n\n> - it seems to be faster than \"rev-list --objects --all\"\n\nMore important is that it makes sure that it is safe to update our\nrefs to the new value, just like the check this attempts to replace.\nIf that is not the case, the speed does not matter.\n\nI am guessing that the code assumes that we are updating our refs to\nobjects that are in the pack that we are looking at, and I can see\nhow the new check in sha1_object() may detect an object that points\nat another object that is missing. But that assumption (which I\nthink is correct) is probably the most important thing to say in the\nlog message.\n\n> +--not-so-strict::\n\nPerhaps \"--check-connectivity\" is a better name than this?\n"},{"id":"216239","messageId":"CACsJy8A2dYssdV7JHutYKgo-nZswBAuedXoJ=aygrVSR=JeTrw@mail.gmail.com","threadId":"33340","inReplyTo":"7vvc722s0h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-02T09:53:16Z","receivedAt":"2013-05-02T09:53:16Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 2, 2013 at 6:35 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> --not-so-strict only checks if all links from objects in the pack\n>> point to real objects (either in current repo, or from the pack\n>> itself). It's like check_everything_connected() except that:\n>>\n>> - it does not follow DAG in order\n>> - it can detect incomplete object islands\n>\n> Could you clarify what this second point means?\n>\n> \"rev-list --objects --all --not $this $that\" does not detect\n> \"islands\" but checking with the updated index-pack does?\n\nObject islands (in the new pack) by definition are not connected to\nthe main DAG and so invisible to/unreachable from rev-list. index-pack\nexamines all objects in the pack and checks links of each object. With\nthis approach, islands are no different than reachable objects.\n\n>> - it seems to be faster than \"rev-list --objects --all\"\n>\n> More important is that it makes sure that it is safe to update our\n> refs to the new value, just like the check this attempts to replace.\n> If that is not the case, the speed does not matter.\n>\n> I am guessing that the code assumes that we are updating our refs to\n> objects that are in the pack that we are looking at, and I can see\n> how the new check in sha1_object() may detect an object that points\n> at another object that is missing. But that assumption (which I\n> think is correct) is probably the most important thing to say in the\n> log message.\n\nYes, we need to make sure the new value of our refs are existing\nobjects. But it does not need to be in the new pack. After index-pack\nis run, we're guaranteed that all objects in repo are connected and\nany of them could be new ref. This is also why I add has_sha1_file()\nin clone.c. I forget if I have checked for similar call in fetch.c and\nreceive-pack.c. Will look again (and update log message)\n\n>> +--not-so-strict::\n>\n> Perhaps \"--check-connectivity\" is a better name than this?\n\nDefinitely.\n--\nDuy\n"},{"id":"216240","messageId":"CACsJy8D8DT4P2gGp-Z3v_mi_9k-uKiYEnwJ3rGPHkCMGZu3_FA@mail.gmail.com","threadId":"33340","inReplyTo":"7v61z25tuf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 2/5] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-02T10:04:38Z","receivedAt":"2013-05-02T10:04:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 2, 2013 at 3:27 AM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> index-pack --strict looks up and follows parent commits. If shallow\n>> information is not ready by the time index-pack is run, index-pack may\n>> be lead to non-existent objects. Make fetch-pack save shallow file to\n>> disk before invoking index-pack.\n>>\n>> A new environment variable GIT_SHALLOW_FILE is used to inform\n>> index-pack to use an alternate shallow file. This variable is for\n>> internal use and thus not advertised in git.txt.\n>\n> The idea to tell the index-pack to use a different shallow file is\n> sound, but is the environment variable the best way to go? I am\n> mostly worried about it being an implicit \"apply everywhere\"\n> mechanism and while I do not particularly like the idea of doing\n> everything in the same process as the top-level while dealing with\n> submodules, I am reasonably sure people would want to \"clone\"\n> recursively inside the same top-level process in the future, and\n> this new environment variable adds one more thing to be cleansed\n> when crossing a repository boundary.\n\nFirst of all, fetch does check that GIT_SHALLOW_FILE must not be set\nbefore it updates shallow file, so if fetch is run recursively, the\ninner one will get caught. And GIT_SHALLOW_FILE is only set for a\nperiod of time when pack is received. Unless somebody launches fetch\ninside index-pack, we should be safe.\n\nThat said, passing this info via a --shallow-file is not hard to do\n(so I will likely do it). But I wonder how submodule code handles the\ncase where the user set GIT_OBJECT_DIRECTORY or similar variables.\nGIT_WORK_TREE for example could be set internally by git and\npropagated down to submodule subprocesses..\n--\nDuy\n"},{"id":"216271","messageId":"7vvc711h4m.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"CACsJy8A2dYssdV7JHutYKgo-nZswBAuedXoJ=aygrVSR=JeTrw@mail.gmail.com","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-02T16:27:53Z","receivedAt":"2013-05-02T16:27:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> On Thu, May 2, 2013 at 6:35 AM, Junio C Hamano wrote:\n>> Nguyễn Thái Ngọc Duy writes:\n>>\n>>> --not-so-strict only checks if all links from objects in the pack\n>>> point to real objects (either in current repo, or from the pack\n>>> itself). It's like check_everything_connected() except that:\n>>>\n>>> - it does not follow DAG in order\n>>> - it can detect incomplete object islands\n>>\n>> Could you clarify what this second point means?\n>>\n>> \"rev-list --objects --all --not $this $that\" does not detect\n>> \"islands\" but checking with the updated index-pack does?\n>\n> Object islands (in the new pack) by definition are not connected to\n> the main DAG and so invisible to/unreachable from rev-list. index-pack\n> examines all objects in the pack and checks links of each object. With\n> this approach, islands are no different than reachable objects.\n\nOK, so if you are fetching an updated tip of the main history, and a\nnew tip of a history that is disjoint. If we imagine that my public\nrepository just added the 'todo' branch and you are fecting them for\nthe first time. The history of 'todo' branch is an island that is\nnot connected anywhere from your refs namespace yet. In order to\nensure that updating the tip of fetched 'todo' is safe, you would\nneed to verify the island is free of dangling pointers and the only\nthing you need to be sure is the tip of 'todo' is _in_ that island.\n\n>> I am guessing that the code assumes that we are updating our refs to\n>> objects that are in the pack that we are looking at, and I can see\n>> how the new check in sha1_object() may detect an object that points\n>> at another object that is missing. But that assumption (which I\n>> think is correct) is probably the most important thing to say in the\n>> log message.\n>\n> Yes, we need to make sure the new value of our refs are existing\n> objects. But it does not need to be in the new pack.\n\nIt is a bit more tricky than that. A malicious (or simply buggy)\nother side can send a subset of my 'todo' branch, which is an island\nthat is free of dangling pointers (think: 'rev-list --objects\ntodo~8'). Further imagine that you earlier attempted a fetch of the\nsame history from me over a commit walker and you happen to have\npartial history near the tip of 'todo' but not connected to the\nisland. sha1_object() will find it, but that does not say anything\nuseful. The tip _must_ appear in the island for your check to yield\na usable result, no?\n\nThe existing \"everything connected\" was designed to protect against\nthat kind of breakage as well.\n\nI might be reading your change incorrectly, but I am not sure how\nthe new code protects against such a breakage.\n\n> After index-pack\n> is run, we're guaranteed that all objects in repo are connected and\n> any of them could be new ref. This is also why I add has_sha1_file()\n> in clone.c.\n"},{"id":"216320","messageId":"CACsJy8AXe-5o7EyEp_aFB=+Ny8GoqrObxzwbAhGD4w9h7Jhmog@mail.gmail.com","threadId":"33340","inReplyTo":"7vvc711h4m.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T02:29:06Z","receivedAt":"2013-05-03T02:29:06Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 3, 2013 at 2:27 AM, Junio C Hamano wrote:\n>> Object islands (in the new pack) by definition are not connected to\n>> the main DAG and so invisible to/unreachable from rev-list. index-pack\n>> examines all objects in the pack and checks links of each object. With\n>> this approach, islands are no different than reachable objects.\n>\n> OK, so if you are fetching an updated tip of the main history, and a\n> new tip of a history that is disjoint. If we imagine that my public\n> repository just added the 'todo' branch and you are fecting them for\n> the first time. The history of 'todo' branch is an island that is\n> not connected anywhere from your refs namespace yet. In order to\n> ensure that updating the tip of fetched 'todo' is safe, you would\n> need to verify the island is free of dangling pointers and the only\n> thing you need to be sure is the tip of 'todo' is _in_ that island.\n\nWhy tip must be in that island or any other islands? There is no way\nwe know which island (or the main DAG) should be connected to any\ntips. It should be enough that the tip in question exists and do not\ncontain any dangling pointers.\n\n>>> I am guessing that the code assumes that we are updating our refs to\n>>> objects that are in the pack that we are looking at, and I can see\n>>> how the new check in sha1_object() may detect an object that points\n>>> at another object that is missing. But that assumption (which I\n>>> think is correct) is probably the most important thing to say in the\n>>> log message.\n>>\n>> Yes, we need to make sure the new value of our refs are existing\n>> objects. But it does not need to be in the new pack.\n>\n> It is a bit more tricky than that. A malicious (or simply buggy)\n> other side can send a subset of my 'todo' branch, which is an island\n> that is free of dangling pointers (think: 'rev-list --objects\n> todo~8'). Further imagine that you earlier attempted a fetch of the\n> same history from me over a commit walker and you happen to have\n> partial history near the tip of 'todo' but not connected to the\n> island. sha1_object() will find it, but that does not say anything\n> useful. The tip _must_ appear in the island for your check to yield\n> a usable result, no?\n\nWhat do you mean by \"partial history\"? Do we have dangling pointers\nafter doing that commit walker?\n\n> The existing \"everything connected\" was designed to protect against\n> that kind of breakage as well.\n>\n> I might be reading your change incorrectly, but I am not sure how\n> the new code protects against such a breakage.\n>\n>> After index-pack\n>> is run, we're guaranteed that all objects in repo are connected and\n>> any of them could be new ref. This is also why I add has_sha1_file()\n>> in clone.c.\n-- \nDuy\n"},{"id":"216337","messageId":"7vk3ngzi6o.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"CACsJy8AXe-5o7EyEp_aFB=+Ny8GoqrObxzwbAhGD4w9h7Jhmog@mail.gmail.com","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T06:33:19Z","receivedAt":"2013-05-03T06:33:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Duy Nguyen writes:\n\n> What do you mean by \"partial history\"? Do we have dangling pointers\n> after doing that commit walker?\n\n\"^C\" will leave the objects and it is safe because it will not\nupdate refs.\n\nBut your code that does not verify the full connectivity from such\nan object (that is outside the transferred pack) to the rest of the\nhistory will then make the resulting repository broken, if you\nupdate your ref to point at such an object, no? Ading a single\nhas_sha1_file() only verifies that single object, not the history\nbehind it.\n"},{"id":"216342","messageId":"7vbo8szh4y.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"7vk3ngzi6o.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T06:55:57Z","receivedAt":"2013-05-03T06:55:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Duy Nguyen writes:\n>\n>> What do you mean by \"partial history\"? Do we have dangling pointers\n>> after doing that commit walker?\n>\n> \"^C\" will leave the objects and it is safe because it will not\n> update refs.\n>\n> But your code that does not verify the full connectivity from such\n> an object (that is outside the transferred pack) to the rest of the\n> history will then make the resulting repository broken, if you\n> update your ref to point at such an object, no? Ading a single\n> has_sha1_file() only verifies that single object, not the history\n> behind it.\n\nLet's illustrate. Imagine your project as a whole has this history:\n\n---A---B---C---D---E\n\nWhen you cloned, it only had up to A, so that is what you have.\nThen you try http walker, which slurps E, wants to go to D and dig\ndown, but after fetching E, some trees and blobs in E, and fetching\nD, but before completing D' trees and blobs, your ISP cuts you off.\n\nYou have these in your object store:\n\n---A D---E\n\nbut your ref is pointing at A, because we promise that we make sure\nfull connectivity before we update the ref, and even if you have\ncommits D and E, the associated trees, blobs, and commits behind\nthis subpart of the history are missing.\n\nNow you try to fetch from another mirror over the pack transport.\nYou advertise that you have A (but you do not advertise E, because\nyour refs do not point at it), and you expect all objects that are\nlisted in \"rev-list --objects A..E\" to be gien to you.\n\nUnfortunately, the mirror is broken, in that it only packs the\nobjects that appear in \"rev-list --objects A..B\", but still tells\nyou that it is sending objects to complete history leading to E.\n\nYour object store would have objects like this after this transfer:\n\n---A---B D---E\n\nYou may still have commits D and E unpruned, but are missing C, and\ntrees and blobs that are needed in D or E.\n\nYou have to walk the history from E and list the necessary objects\nyourself, running has_sha1_file() on all of them, to prove that you\nhave everything needed, and the only things you can trust are your\nrefs (in this case, A).\n\nIf you verify that all the objects in the transferred pack are\ncomplete, and also verify that the object the transfer proposes to\nupdate your ref to is _in_ that pack, then you can detect a mirror\nthat is broken and only sends objects for A..B, but that does not\nactually solve the issue. Imagine another broken mirror that\ninstead sends objects for E. E, its trees and all its blobs may be\nin the pack and the only outgoing link from that pack may be a\npointer out of E pointing at D. You happen to _have_ it in your\nobject store, but that does not mean you can update your ref to E\n(remember, you do not have trees and blobs to complete D, or the\nhistory behind it).\n\nThe current check is implemented in the way we currently do it,\n_not_ because we were stupid and did not realize the optimization\npossibility (in other words, what your patch proposes is not new),\nbut because we rejected this approach because it was not safe.\n"},{"id":"216343","messageId":"20130503070938.GA5239@duynguyen-vnpc.dek-tpc.internal","threadId":"33340","inReplyTo":"7vbo8szh4y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T07:09:38Z","receivedAt":"2013-05-03T07:09:38Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, May 02, 2013 at 11:55:57PM -0700, Junio C Hamano wrote:\n> Let's illustrate. Imagine your project as a whole has this history:\n\n[snip]\n\nOK I agree my approach is flawed. But if all these are met, we can be\nsure the new refs are good, correct?\n\n - all refs point to an object in the pack\n - there are no dangling pointers in any object in the pack\n - all objects in the pack does not point to any objects outside the\n pack\n\nWe can check all these for the clone case with the patch below, even\nif somehow we get some bad objects in the cloned repository before the\nnew pack arrives.\n\n-- 8< --\nSubject: [PATCH] clone: open a shortcut for connectivity check\n\nIn order to make sure the cloned repository is good, we run \"rev-list\n--objects --not --all $new_refs\" on the repository. This is expensive\non large repositories. This patch attempts to mitigate the impact in\nthis special case.\n\nIn the \"good\" clone case, we only have one pack. If all of the\nfollowing are met, we can be sure that all objects reachable from the\nnew refs exist, which is the intention of running \"rev-list ...\":\n\n - all refs point to an object in the pack\n - there are no dangling pointers in any object in the pack\n - all objects in the pack does not point to objects outside the pack\n\nThe second and third checks can be done with the help of index-pack as\na slight modification of --strict check (which introduces a new\ncondition for the shortcut: pack transfer must be used and the number\nof objects large enough to call index-pack). The first is checked in\ncheck_everything_connected after we get an \"ok\" from index-pack.\n\n\"index-pack + new checks\" is still faster than \"index-pack + rev-list\"\ncurrently, which is the whole point of this patch. If any of the\nconditions fails, we fall back to good old and expensive \"rev-list\n..\". In that case it's even more expensive because we have to pay for\nthe new checks in index-pack.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-index-pack.txt | 3 +++\n builtin/clone.c | 11 ++++++++---\n builtin/index-pack.c | 35 ++++++++++++++++++++++++++---------\n connected.c | 34 +++++++++++++++++++++++++++++++++-\n connected.h | 5 +++++\n fetch-pack.c | 11 ++++++++++-\n fetch-pack.h | 4 +++-\n transport.c | 4 ++++\n transport.h | 2 ++\n 9 files changed, 94 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex bde8eec..7a4e055 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -74,6 +74,9 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--check-self-contained-and-connected::\n+\tDie if the pack contains broken links. For internal use only.\n+\n --threads=::\n \tSpecifies the number of threads to spawn when resolving\n \tdeltas. This requires that index-pack be compiled with\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dad4265..427bec4 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -542,13 +542,15 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t const struct ref *mapped_refs,\n \t\t\t const struct ref *remote_head_points_at,\n \t\t\t const char *branch_top,\n-\t\t\t const char *msg)\n+\t\t\t const char *msg,\n+\t\t\t struct transport *transport)\n {\n \tconst struct ref *rm = mapped_refs;\n \n \tif (0 <= option_verbosity)\n \t\tprintf(_(\"Checking connectivity... \"));\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n+\tif (check_everything_connected_with_transport(iterate_ref_map,\n+\t\t\t\t\t\t 0, &rm, transport))\n \t\tdie(_(\"remote did not send all necessary objects\"));\n \tif (0 <= option_verbosity)\n \t\tprintf(_(\"done\\n\"));\n@@ -893,6 +895,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (option_upload_pack)\n \t\t\ttransport_set_option(transport, TRANS_OPT_UPLOADPACK,\n \t\t\t\t\t option_upload_pack);\n+\n+\t\tif (transport->smart_options)\n+\t\t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \t}\n \n \trefs = transport_get_remote_refs(transport);\n@@ -954,7 +959,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\ttransport_fetch_refs(transport, mapped_refs);\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n-\t\t\t branch_top.buf, reflog_msg.buf);\n+\t\t\t branch_top.buf, reflog_msg.buf, transport);\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 1fd56d9..6aa7bb6 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -77,8 +77,10 @@ static int nr_threads;\n \n static int from_stdin;\n static int strict;\n+static int do_fsck_object;\n static int verbose;\n static int show_stat;\n+static int check_self_contained_and_connected;\n \n static struct progress *progress;\n \n@@ -187,13 +189,13 @@ static int mark_link(struct object *obj, int type, void *data)\n \n /* The content of each linked object must have been checked\n or it must be already present in the object database */\n-static void check_object(struct object *obj)\n+static unsigned check_object(struct object *obj)\n {\n \tif (!obj)\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!(obj->flags & FLAG_LINK))\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!(obj->flags & FLAG_CHECKED)) {\n \t\tunsigned long size;\n@@ -201,17 +203,20 @@ static void check_object(struct object *obj)\n \t\tif (type != obj->type || type <= 0)\n \t\t\tdie(_(\"object of unexpected type\"));\n \t\tobj->flags |= FLAG_CHECKED;\n-\t\treturn;\n+\t\treturn 1;\n \t}\n+\n+\treturn 0;\n }\n \n-static void check_objects(void)\n+static unsigned check_objects(void)\n {\n-\tunsigned i, max;\n+\tunsigned i, max, foreign_nr;\n \n \tmax = get_max_object_index();\n \tfor (i = 0; i < max; i++)\n-\t\tcheck_object(get_indexed_object(i));\n+\t\tforeign_nr += check_object(get_indexed_object(i));\n+\treturn foreign_nr;\n }\n \n \n@@ -756,7 +761,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tobj = parse_object_buffer(sha1, type, size, buf, &eaten);\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n-\t\t\tif (fsck_object(obj, 1, fsck_error_function))\n+\t\t\tif (do_fsck_object &&\n+\t\t\t fsck_object(obj, 1, fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n@@ -1490,6 +1496,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tstruct pack_idx_entry **idx_objects;\n \tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n+\tunsigned foreign_nr = 1;\t/* zero is a \"good\" value, assume bad */\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(index_pack_usage);\n@@ -1511,6 +1518,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--strict\")) {\n \t\t\t\tstrict = 1;\n+\t\t\t\tdo_fsck_object = 1;\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, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\n@@ -1624,7 +1635,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tconclude_pack(fix_thin_pack, curr_pack, pack_sha1);\n \tfree(deltas);\n \tif (strict)\n-\t\tcheck_objects();\n+\t\tforeign_nr = check_objects();\n \n \tif (show_stat)\n \t\tshow_pack_info(stat_only);\n@@ -1650,5 +1661,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (index_name == NULL)\n \t\tfree((void *) curr_index);\n \n+\t/*\n+\t * Let the caller know this pack is not self contained\n+\t */\n+\tif (check_self_contained_and_connected && foreign_nr)\n+\t\treturn 1;\n+\n \treturn 0;\n }\ndiff --git a/connected.c b/connected.c\nindex 1e89c1c..fae8d64 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -2,7 +2,12 @@\n #include \"run-command.h\"\n #include \"sigchain.h\"\n #include \"connected.h\"\n+#include \"transport.h\"\n \n+int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+{\n+\treturn check_everything_connected_with_transport(fn, quiet, cb_data, NULL);\n+}\n /*\n * If we feed all the commits we want to verify to this command\n *\n@@ -14,7 +19,10 @@\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_with_transport(sha1_iterate_fn fn,\n+\t\t\t\t\t int quiet,\n+\t\t\t\t\t void *cb_data,\n+\t\t\t\t\t struct transport *transport)\n {\n \tstruct child_process rev_list;\n \tconst char *argv[] = {\"rev-list\", \"--objects\",\n@@ -22,10 +30,23 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \tchar commit[41];\n \tunsigned char sha1[20];\n \tint err = 0;\n+\tstruct packed_git *new_pack = NULL;\n \n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n+\tif (transport && transport->smart_options &&\n+\t transport->smart_options->self_contained_and_connected &&\n+\t transport->pack_lockfile &&\n+\t !suffixcmp(transport->pack_lockfile, \".keep\")) {\n+\t\tstruct strbuf idx_file = STRBUF_INIT;\n+\t\tstrbuf_addstr(&idx_file, transport->pack_lockfile);\n+\t\tstrbuf_setlen(&idx_file, idx_file.len - 5); /* \".keep\" */\n+\t\tstrbuf_addstr(&idx_file, \".idx\");\n+\t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n+\t\tstrbuf_release(&idx_file);\n+\t}\n+\n \tif (quiet)\n \t\targv[5] = \"--quiet\";\n \n@@ -42,6 +63,17 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \n \tcommit[40] = '\\n';\n \tdo {\n+\t\t/*\n+\t\t * If index-pack already checked that:\n+\t\t * - there are no dangling pointers in the new pack\n+\t\t * - the pack is self contained\n+\t\t * Then if the updated ref is in the new pack, then we\n+\t\t * are sure the ref is good and not sending it to\n+\t\t * rev-list for verification.\n+\t\t */\n+\t\tif (new_pack && find_pack_entry_one(sha1, new_pack))\n+\t\t\tcontinue;\n+\n \t\tmemcpy(commit, sha1_to_hex(sha1), 40);\n \t\tif (write_in_full(rev_list.in, commit, 41) < 0) {\n \t\t\tif (errno != EPIPE && errno != EINVAL)\ndiff --git a/connected.h b/connected.h\nindex 7e4585a..0b060b7 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -1,6 +1,8 @@\n #ifndef CONNECTED_H\n #define CONNECTED_H\n \n+struct transport;\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@@ -16,5 +18,8 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\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_with_transport(sha1_iterate_fn, int quiet,\n+\t\t\t\t\t\t void *cb_data,\n+\t\t\t\t\t\t struct transport *transport);\n \n #endif /* CONNECTED_H */\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 17cfa88..75c6f05 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -690,6 +690,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tconst char **av;\n \tint do_keep = args->keep_pack;\n \tstruct child_process cmd;\n+\tint ret;\n \n \tmemset(&demux, 0, sizeof(demux));\n \tif (use_sideband) {\n@@ -741,11 +742,14 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t\tstrcpy(keep_arg + s, \"localhost\");\n \t\t\t*av++ = keep_arg;\n \t\t}\n+\t\tif (args->check_self_contained_and_connected)\n+\t\t\t*av++ = \"--check-self-contained-and-connected\";\n \t}\n \telse {\n \t\t*av++ = \"unpack-objects\";\n \t\tif (args->quiet || args->no_progress)\n \t\t\t*av++ = \"-q\";\n+\t\targs->check_self_contained_and_connected = 0;\n \t}\n \tif (*hdr_arg)\n \t\t*av++ = hdr_arg;\n@@ -766,7 +770,12 @@ static int get_pack(struct fetch_pack_args *args,\n \t\tclose(cmd.out);\n \t}\n \n-\tif (finish_command(&cmd))\n+\tret = finish_command(&cmd);\n+\tif (!ret || (args->check_self_contained_and_connected && ret == 1))\n+\t\targs->self_contained_and_connected =\n+\t\t\targs->check_self_contained_and_connected &&\n+\t\t\tret == 0;\n+\telse\n \t\tdie(\"%s failed\", argv[0]);\n \tif (use_sideband && finish_async(&demux))\n \t\tdie(\"error in sideband demultiplexer\");\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex dc5266c..40f08ba 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -16,7 +16,9 @@ struct fetch_pack_args {\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\tcheck_self_contained_and_connected:1,\n+\t\tself_contained_and_connected:1;\n };\n \n /*\ndiff --git a/transport.c b/transport.c\nindex ba5d8af..359a671 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -534,6 +534,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.quiet = (transport->verbose < 0);\n \targs.no_progress = !transport->progress;\n \targs.depth = data->options.depth;\n+\targs.check_self_contained_and_connected =\n+\t\tdata->options.check_self_contained_and_connected;\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -551,6 +553,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs = NULL;\n \tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n+\tdata->options.self_contained_and_connected =\n+\t\targs.self_contained_and_connected;\n \n \tfree_refs(refs_tmp);\n \ndiff --git a/transport.h b/transport.h\nindex fcb1d25..4edebc5 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -8,6 +8,8 @@ struct git_transport_options {\n \tunsigned thin : 1;\n \tunsigned keep : 1;\n \tunsigned followtags : 1;\n+\tunsigned check_self_contained_and_connected : 1;\n+\tunsigned self_contained_and_connected : 1;\n \tint depth;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n-- \n1.8.2.82.gc24b958\n-- 8< --\n"},{"id":"216344","messageId":"CAPig+cRRHhiKwc5bPd36xWv-43JoEVU4OaqwxvKMWh0Bv35dOQ@mail.gmail.com","threadId":"33340","inReplyTo":"20130503070938.GA5239@duynguyen-vnpc.dek-tpc.internal","subject":"Re: [PATCH v2 4/5] index-pack, unpack-objects: add --not-so-strict for connectivity check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-03T08:16:46Z","receivedAt":"2013-05-03T08:16:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 3, 2013 at 3:09 AM, Duy Nguyen wrote:\n> Subject: [PATCH] clone: open a shortcut for connectivity check\n>\n> In order to make sure the cloned repository is good, we run \"rev-list\n> --objects --not --all $new_refs\" on the repository. This is expensive\n> on large repositories. This patch attempts to mitigate the impact in\n> this special case.\n>\n> In the \"good\" clone case, we only have one pack. If all of the\n> following are met, we can be sure that all objects reachable from the\n> new refs exist, which is the intention of running \"rev-list ...\":\n>\n> - all refs point to an object in the pack\n> - there are no dangling pointers in any object in the pack\n> - all objects in the pack does not point to objects outside the pack\n\n\"no objects in the pack point to...\"\n\n>\n> The second and third checks can be done with the help of index-pack as\n> a slight modification of --strict check (which introduces a new\n> condition for the shortcut: pack transfer must be used and the number\n> of objects large enough to call index-pack). The first is checked in\n> check_everything_connected after we get an \"ok\" from index-pack.\n>\n> \"index-pack + new checks\" is still faster than \"index-pack + rev-list\"\n> currently, which is the whole point of this patch. If any of the\n> conditions fails, we fall back to good old and expensive \"rev-list\n> ..\". In that case it's even more expensive because we have to pay for\n> the new checks in index-pack.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"216351","messageId":"1367584514-19806-1-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367405974-22190-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 0/4] check_everything_connected replacement","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T12:35:10Z","receivedAt":"2013-05-03T12:35:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"v3 is more like check_everything_connected's special case for clone\nbecause check_everything_connected is not really replaced.\nGIT_SHALLOW_FILE in 2/4 is now replaced by --shallow-file to avoid\nunintended propagation to child processes.\n\nNguyễn Thái Ngọc Duy (4):\n clone: let the user know when check_everything_connected is run\n fetch-pack: prepare updated shallow file before fetching the pack\n index-pack: remove dead code (it should never happen)\n clone: open a shortcut for connectivity check\n\n Documentation/git-index-pack.txt | 3 ++\n builtin/clone.c | 15 ++++++--\n builtin/index-pack.c | 38 +++++++++++++------\n commit.h | 2 +\n connected.c | 34 ++++++++++++++++-\n connected.h | 5 +++\n fetch-pack.c | 80 ++++++++++++++++++++++------------------\n fetch-pack.h | 4 +-\n git.c | 5 +++\n shallow.c | 45 +++++++++++++++++++++-\n t/t5500-fetch-pack.sh | 7 ++++\n transport.c | 4 ++\n transport.h | 2 +\n 13 files changed, 190 insertions(+), 54 deletions(-)\n\n-- \n1.8.2.83.gc99314b\n"},{"id":"216352","messageId":"1367584514-19806-2-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 1/4] clone: let the user know when check_everything_connected is run","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T12:35:11Z","receivedAt":"2013-05-03T12:35:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"check_everything_connected could take a long time, especially in the\nclone case where the whole DAG is traversed. The user deserves to know\nwhat's going on.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/clone.c | 4 ++++\n 1 file changed, 4 insertions(+)\n\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex 035ab64..dad4265 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -546,8 +546,12 @@ static void update_remote_refs(const struct ref *refs,\n {\n \tconst struct ref *rm = mapped_refs;\n \n+\tif (0 <= option_verbosity)\n+\t\tprintf(_(\"Checking connectivity... \"));\n \tif (check_everything_connected(iterate_ref_map, 0, &rm))\n \t\tdie(_(\"remote did not send all necessary objects\"));\n+\tif (0 <= option_verbosity)\n+\t\tprintf(_(\"done\\n\"));\n \n \tif (refs) {\n \t\twrite_remote_refs(mapped_refs);\n-- \n1.8.2.83.gc99314b\n"},{"id":"216353","messageId":"1367584514-19806-3-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 2/4] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T12:35:12Z","receivedAt":"2013-05-03T12:35:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"index-pack --strict looks up and follows parent commits. If shallow\ninformation is not ready by the time index-pack is run, index-pack may\nbe lead to non-existent objects. Make fetch-pack save shallow file to\ndisk before invoking index-pack.\n\ngit learns new global option --shallow-file to pass on the alternate\nshallow file path. Undocumented (and not even support --shallow-file=\nsyntax) because it's unlikely to be used again elsewhere.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n commit.h | 2 ++\n fetch-pack.c | 69 +++++++++++++++++++++++++--------------------------\n git.c | 5 ++++\n shallow.c | 45 +++++++++++++++++++++++++++++++--\n t/t5500-fetch-pack.sh | 7 ++++++\n 5 files changed, 91 insertions(+), 37 deletions(-)\n\ndiff --git a/commit.h b/commit.h\nindex 67bd509..6e9c7cd 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -176,6 +176,8 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);\n extern int is_repository_shallow(void);\n extern struct commit_list *get_shallow_commits(struct object_array *heads,\n \t\tint depth, int shallow_flag, int not_shallow_flag);\n+extern void check_shallow_file_for_update(void);\n+extern void set_alternate_shallow_file(const char *path);\n \n int is_descendant_of(struct commit *, struct commit_list *);\n int in_merge_bases(struct commit *, struct commit *);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f156dd4..1ca4f6b 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -20,6 +20,8 @@ static int no_done;\n static int fetch_fsck_objects = -1;\n static int transfer_fsck_objects = -1;\n static int agent_supported;\n+static struct lock_file shallow_lock;\n+static const char *alternate_shallow_file;\n \n #define COMPLETE\t(1U << 0)\n #define COMMON\t\t(1U << 1)\n@@ -683,7 +685,7 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t int xd[2], char **pack_lockfile)\n {\n \tstruct async demux;\n-\tconst char *argv[20];\n+\tconst char *argv[22];\n \tchar keep_arg[256];\n \tchar hdr_arg[256];\n \tconst char **av;\n@@ -724,6 +726,11 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\tdo_keep = 1;\n \t}\n \n+\tif (alternate_shallow_file) {\n+\t\t*av++ = \"--shallow-file\";\n+\t\t*av++ = alternate_shallow_file;\n+\t}\n+\n \tif (do_keep) {\n \t\tif (pack_lockfile)\n \t\t\tcmd.out = -1;\n@@ -779,6 +786,23 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n \treturn strcmp(a->name, b->name);\n }\n \n+static void setup_alternate_shallow(void)\n+{\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tint fd;\n+\n+\tcheck_shallow_file_for_update();\n+\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n+\t\t\t\t LOCK_DIE_ON_ERROR);\n+\tif (write_shallow_commits(&sb, 0)) {\n+\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n+\t\t\tdie_errno(\"failed to write to %s\", shallow_lock.filename);\n+\t\talternate_shallow_file = shallow_lock.filename;\n+\t} else\n+\t\talternate_shallow_file = \"\";\n+\tstrbuf_release(&sb);\n+}\n+\n static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\t\t\t int fd[2],\n \t\t\t\t const struct ref *orig_ref,\n@@ -858,6 +882,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \n \tif (args->stateless_rpc)\n \t\tpacket_flush(fd[1]);\n+\tif (args->depth > 0)\n+\t\tsetup_alternate_shallow();\n \tif (get_pack(args, fd, pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \n@@ -936,15 +962,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t struct ref **sought, int nr_sought,\n \t\t char **pack_lockfile)\n {\n-\tstruct stat st;\n \tstruct ref *ref_cpy;\n \n \tfetch_pack_setup();\n-\tif (args->depth > 0) {\n-\t\tif (stat(git_path(\"shallow\"), &st))\n-\t\t\tst.st_mtime = 0;\n-\t}\n-\n \tif (nr_sought)\n \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n \n@@ -955,34 +975,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n \n \tif (args->depth > 0) {\n-\t\tstatic struct lock_file lock;\n-\t\tstruct cache_time mtime;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\t\tchar *shallow = git_path(\"shallow\");\n-\t\tint fd;\n-\n-\t\tmtime.sec = st.st_mtime;\n-\t\tmtime.nsec = ST_MTIME_NSEC(st);\n-\t\tif (stat(shallow, &st)) {\n-\t\t\tif (mtime.sec)\n-\t\t\t\tdie(\"shallow file was removed during fetch\");\n-\t\t} else if (st.st_mtime != mtime.sec\n-#ifdef USE_NSEC\n-\t\t\t\t|| ST_MTIME_NSEC(st) != mtime.nsec\n-#endif\n-\t\t\t )\n-\t\t\tdie(\"shallow file was changed during fetch\");\n-\n-\t\tfd = hold_lock_file_for_update(&lock, shallow,\n-\t\t\t\t\t LOCK_DIE_ON_ERROR);\n-\t\tif (!write_shallow_commits(&sb, 0)\n-\t\t || write_in_full(fd, sb.buf, sb.len) != sb.len) {\n-\t\t\tunlink_or_warn(shallow);\n-\t\t\trollback_lock_file(&lock);\n-\t\t} else {\n-\t\t\tcommit_lock_file(&lock);\n-\t\t}\n-\t\tstrbuf_release(&sb);\n+\t\tstruct stat st;\n+\t\tif (!fstat(shallow_lock.fd, &st) &&\n+\t\t st.st_size == 0) {\n+\t\t\tunlink_or_warn(git_path(\"shallow\"));\n+\t\t\trollback_lock_file(&shallow_lock);\n+\t\t} else\n+\t\t\tcommit_lock_file(&shallow_lock);\n \t}\n \n \treprepare_packed_git();\ndiff --git a/git.c b/git.c\nindex 1ada169..6450a38 100644\n--- a/git.c\n+++ b/git.c\n@@ -4,6 +4,7 @@\n #include \"help.h\"\n #include \"quote.h\"\n #include \"run-command.h\"\n+#include \"commit.h\"\n \n const char git_usage_string[] =\n \t\"git [--version] [--help] [-c name=value]\\n\"\n@@ -146,6 +147,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n \t\t\tsetenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, \"0\", 1);\n \t\t\tif (envchanged)\n \t\t\t\t*envchanged = 1;\n+\t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n+\t\t\t(*argv)++;\n+\t\t\t(*argc)--;\n+\t\t\tset_alternate_shallow_file((*argv)[0]);\n \t\t} else {\n \t\t\tfprintf(stderr, \"Unknown option: %s\\n\", cmd);\n \t\t\tusage(git_usage_string);\ndiff --git a/shallow.c b/shallow.c\nindex 6be915f..bdae988 100644\n--- a/shallow.c\n+++ b/shallow.c\n@@ -3,6 +3,16 @@\n #include \"tag.h\"\n \n static int is_shallow = -1;\n+static struct stat shallow_stat;\n+static char *alternate_shallow_file;\n+\n+void set_alternate_shallow_file(const char *path)\n+{\n+\tif (is_shallow != -1)\n+\t\tdie(\"BUG: is_repository_shallow must not be called before set_alternate_shallow_file\");\n+\tfree(alternate_shallow_file);\n+\talternate_shallow_file = path ? xstrdup(path) : NULL;\n+}\n \n int register_shallow(const unsigned char *sha1)\n {\n@@ -21,12 +31,21 @@ int is_repository_shallow(void)\n {\n \tFILE *fp;\n \tchar buf[1024];\n+\tconst char *path = alternate_shallow_file;\n \n \tif (is_shallow >= 0)\n \t\treturn is_shallow;\n \n-\tfp = fopen(git_path(\"shallow\"), \"r\");\n-\tif (!fp) {\n+\tif (!path)\n+\t\tpath = git_path(\"shallow\");\n+\t/*\n+\t * fetch-pack set '--shallow-file \"\"' as an indicator that no\n+\t * shallow file should be used. We could just open it and it\n+\t * will likely fail. But let's do an explicit check instead.\n+\t */\n+\tif (!*path ||\n+\t stat(path, &shallow_stat) ||\n+\t (fp = fopen(path, \"r\")) == NULL) {\n \t\tis_shallow = 0;\n \t\treturn is_shallow;\n \t}\n@@ -108,3 +127,25 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n \n \treturn result;\n }\n+\n+void check_shallow_file_for_update(void)\n+{\n+\tstruct stat st;\n+\n+\tif (getenv(\"GIT_SHALLOW_FILE\"))\n+\t\tdie(\"GIT_SHALLOW_FILE should not be manually set\");\n+\n+\tif (!is_shallow)\n+\t\treturn;\n+\telse if (is_shallow == -1)\n+\t\tdie(\"BUG: shallow must be initialized by now\");\n+\n+\tif (stat(git_path(\"shallow\"), &st))\n+\t\tdie(\"shallow file was removed during fetch\");\n+\telse if (st.st_mtime != shallow_stat.st_mtime\n+#ifdef USE_NSEC\n+\t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n+#endif\n+\t\t )\n+\t\tdie(\"shallow file was changed during fetch\");\n+}\ndiff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\nindex d574085..557b073 100755\n--- a/t/t5500-fetch-pack.sh\n+++ b/t/t5500-fetch-pack.sh\n@@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '\n \ttest \"`git --git-dir=shallow0/.git rev-list --count HEAD`\" = 1\n '\n \n+test_expect_success 'clone shallow depth 1 with fsck' '\n+\tgit config --global fetch.fsckobjects true &&\n+\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0fsck &&\n+\ttest \"`git --git-dir=shallow0fsck/.git rev-list --count HEAD`\" = 1 &&\n+\tgit config --global --unset fetch.fsckobjects\n+'\n+\n test_expect_success 'clone shallow' '\n \tgit clone --no-single-branch --depth 2 \"file://$(pwd)/.\" shallow\n '\n-- \n1.8.2.83.gc99314b\n"},{"id":"216354","messageId":"1367584514-19806-4-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 3/4] index-pack: remove dead code (it should never happen)","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T12:35:13Z","receivedAt":"2013-05-03T12:35:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/index-pack.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 79dfe47..f52a04f 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -747,8 +747,7 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tint eaten;\n \t\t\tvoid *buf = (void *) data;\n \n-\t\t\tif (!buf)\n-\t\t\t\tbuf = new_data = get_data_from_pack(obj_entry);\n+\t\t\tassert(data && \"data can only be NULL for large _blobs_\");\n \n \t\t\t/*\n \t\t\t * we do not need to free the memory here, as the\n-- \n1.8.2.83.gc99314b\n"},{"id":"216355","messageId":"1367584514-19806-5-git-send-email-pclouds@gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v3 4/4] clone: open a shortcut for connectivity check","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-03T12:35:14Z","receivedAt":"2013-05-03T12:35:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"In order to make sure the cloned repository is good, we run \"rev-list\n--objects --not --all $new_refs\" on the repository. This is expensive\non large repositories. This patch attempts to mitigate the impact in\nthis special case.\n\nIn the \"good\" clone case, we only have one pack. If all of the\nfollowing are met, we can be sure that all objects reachable from the\nnew refs exist, which is the intention of running \"rev-list ...\":\n\n - all refs point to an object in the pack\n - there are no dangling pointers in any object in the pack\n - no objects in the pack point to objects outside the pack\n\nThe second and third checks can be done with the help of index-pack as\na slight variation of --strict check (which introduces a new condition\nfor the shortcut: pack transfer must be used and the number of objects\nlarge enough to call index-pack). The first is checked in\ncheck_everything_connected after we get an \"ok\" from index-pack.\n\n\"index-pack + new checks\" is still faster than the current \"index-pack\n+ rev-list\", which is the whole point of this patch. If any of the\nconditions fails, we fall back to the good old but expensive \"rev-list\n..\". In that case it's even more expensive because we have to pay for\nthe new checks in index-pack. But that should only happen when the\nother side is either buggy or malicious.\n\nCloning linux-2.6 over file://\n\n before after\nreal 3m25.693s 2m53.050s\nuser 5m2.037s 4m42.396s\nsys 0m13.750s 0m16.574s\n\nA more realistic test with ssh:// over wireless\n\n before after\nreal 11m26.629s 10m4.213s\nuser 5m43.196s 5m19.444s\nsys 0m35.812s 0m37.630s\n\nThis shortcut is not applied to shallow clones, partly because shallow\nclones should have no more objects than a usual fetch and the cost of\nrev-list is acceptable, partly to avoid dealing with corner cases when\ngrafting is involved.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n Documentation/git-index-pack.txt | 3 +++\n builtin/clone.c | 11 ++++++++---\n builtin/index-pack.c | 35 ++++++++++++++++++++++++++---------\n connected.c | 34 +++++++++++++++++++++++++++++++++-\n connected.h | 5 +++++\n fetch-pack.c | 11 ++++++++++-\n fetch-pack.h | 4 +++-\n transport.c | 4 ++++\n transport.h | 2 ++\n 9 files changed, 94 insertions(+), 15 deletions(-)\n\ndiff --git a/Documentation/git-index-pack.txt b/Documentation/git-index-pack.txt\nindex bde8eec..7a4e055 100644\n--- a/Documentation/git-index-pack.txt\n+++ b/Documentation/git-index-pack.txt\n@@ -74,6 +74,9 @@ OPTIONS\n --strict::\n \tDie, if the pack contains broken objects or links.\n \n+--check-self-contained-and-connected::\n+\tDie if the pack contains broken links. For internal use only.\n+\n --threads=::\n \tSpecifies the number of threads to spawn when resolving\n \tdeltas. This requires that index-pack be compiled with\ndiff --git a/builtin/clone.c b/builtin/clone.c\nindex dad4265..069e81e 100644\n--- a/builtin/clone.c\n+++ b/builtin/clone.c\n@@ -542,13 +542,15 @@ static void update_remote_refs(const struct ref *refs,\n \t\t\t const struct ref *mapped_refs,\n \t\t\t const struct ref *remote_head_points_at,\n \t\t\t const char *branch_top,\n-\t\t\t const char *msg)\n+\t\t\t const char *msg,\n+\t\t\t struct transport *transport)\n {\n \tconst struct ref *rm = mapped_refs;\n \n \tif (0 <= option_verbosity)\n \t\tprintf(_(\"Checking connectivity... \"));\n-\tif (check_everything_connected(iterate_ref_map, 0, &rm))\n+\tif (check_everything_connected_with_transport(iterate_ref_map,\n+\t\t\t\t\t\t 0, &rm, transport))\n \t\tdie(_(\"remote did not send all necessary objects\"));\n \tif (0 <= option_verbosity)\n \t\tprintf(_(\"done\\n\"));\n@@ -893,6 +895,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\tif (option_upload_pack)\n \t\t\ttransport_set_option(transport, TRANS_OPT_UPLOADPACK,\n \t\t\t\t\t option_upload_pack);\n+\n+\t\tif (transport->smart_options && !option_depth)\n+\t\t\ttransport->smart_options->check_self_contained_and_connected = 1;\n \t}\n \n \trefs = transport_get_remote_refs(transport);\n@@ -954,7 +959,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)\n \t\ttransport_fetch_refs(transport, mapped_refs);\n \n \tupdate_remote_refs(refs, mapped_refs, remote_head_points_at,\n-\t\t\t branch_top.buf, reflog_msg.buf);\n+\t\t\t branch_top.buf, reflog_msg.buf, transport);\n \n \tupdate_head(our_head_points_at, remote_head, reflog_msg.buf);\n \ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex f52a04f..9c1cfac 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -77,8 +77,10 @@ static int nr_threads;\n \n static int from_stdin;\n static int strict;\n+static int do_fsck_object;\n static int verbose;\n static int show_stat;\n+static int check_self_contained_and_connected;\n \n static struct progress *progress;\n \n@@ -187,13 +189,13 @@ static int mark_link(struct object *obj, int type, void *data)\n \n /* The content of each linked object must have been checked\n or it must be already present in the object database */\n-static void check_object(struct object *obj)\n+static unsigned check_object(struct object *obj)\n {\n \tif (!obj)\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!(obj->flags & FLAG_LINK))\n-\t\treturn;\n+\t\treturn 0;\n \n \tif (!(obj->flags & FLAG_CHECKED)) {\n \t\tunsigned long size;\n@@ -201,17 +203,20 @@ static void check_object(struct object *obj)\n \t\tif (type != obj->type || type <= 0)\n \t\t\tdie(_(\"object of unexpected type\"));\n \t\tobj->flags |= FLAG_CHECKED;\n-\t\treturn;\n+\t\treturn 1;\n \t}\n+\n+\treturn 0;\n }\n \n-static void check_objects(void)\n+static unsigned check_objects(void)\n {\n-\tunsigned i, max;\n+\tunsigned i, max, foreign_nr = 0;\n \n \tmax = get_max_object_index();\n \tfor (i = 0; i < max; i++)\n-\t\tcheck_object(get_indexed_object(i));\n+\t\tforeign_nr += check_object(get_indexed_object(i));\n+\treturn foreign_nr;\n }\n \n \n@@ -756,7 +761,8 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\tobj = parse_object_buffer(sha1, type, size, buf, &eaten);\n \t\t\tif (!obj)\n \t\t\t\tdie(_(\"invalid %s\"), typename(type));\n-\t\t\tif (fsck_object(obj, 1, fsck_error_function))\n+\t\t\tif (do_fsck_object &&\n+\t\t\t fsck_object(obj, 1, fsck_error_function))\n \t\t\t\tdie(_(\"Error in object\"));\n \t\t\tif (fsck_walk(obj, mark_link, NULL))\n \t\t\t\tdie(_(\"Not all child objects of %s are reachable\"), sha1_to_hex(obj->sha1));\n@@ -1490,6 +1496,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tstruct pack_idx_entry **idx_objects;\n \tstruct pack_idx_option opts;\n \tunsigned char pack_sha1[20];\n+\tunsigned foreign_nr = 1;\t/* zero is a \"good\" value, assume bad */\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(index_pack_usage);\n@@ -1511,6 +1518,10 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\tfix_thin_pack = 1;\n \t\t\t} else if (!strcmp(arg, \"--strict\")) {\n \t\t\t\tstrict = 1;\n+\t\t\t\tdo_fsck_object = 1;\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, \"--verify\")) {\n \t\t\t\tverify = 1;\n \t\t\t} else if (!strcmp(arg, \"--verify-stat\")) {\n@@ -1624,7 +1635,7 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tconclude_pack(fix_thin_pack, curr_pack, pack_sha1);\n \tfree(deltas);\n \tif (strict)\n-\t\tcheck_objects();\n+\t\tforeign_nr = check_objects();\n \n \tif (show_stat)\n \t\tshow_pack_info(stat_only);\n@@ -1650,5 +1661,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \tif (index_name == NULL)\n \t\tfree((void *) curr_index);\n \n+\t/*\n+\t * Let the caller know this pack is not self contained\n+\t */\n+\tif (check_self_contained_and_connected && foreign_nr)\n+\t\treturn 1;\n+\n \treturn 0;\n }\ndiff --git a/connected.c b/connected.c\nindex 1e89c1c..fae8d64 100644\n--- a/connected.c\n+++ b/connected.c\n@@ -2,7 +2,12 @@\n #include \"run-command.h\"\n #include \"sigchain.h\"\n #include \"connected.h\"\n+#include \"transport.h\"\n \n+int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n+{\n+\treturn check_everything_connected_with_transport(fn, quiet, cb_data, NULL);\n+}\n /*\n * If we feed all the commits we want to verify to this command\n *\n@@ -14,7 +19,10 @@\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_with_transport(sha1_iterate_fn fn,\n+\t\t\t\t\t int quiet,\n+\t\t\t\t\t void *cb_data,\n+\t\t\t\t\t struct transport *transport)\n {\n \tstruct child_process rev_list;\n \tconst char *argv[] = {\"rev-list\", \"--objects\",\n@@ -22,10 +30,23 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \tchar commit[41];\n \tunsigned char sha1[20];\n \tint err = 0;\n+\tstruct packed_git *new_pack = NULL;\n \n \tif (fn(cb_data, sha1))\n \t\treturn err;\n \n+\tif (transport && transport->smart_options &&\n+\t transport->smart_options->self_contained_and_connected &&\n+\t transport->pack_lockfile &&\n+\t !suffixcmp(transport->pack_lockfile, \".keep\")) {\n+\t\tstruct strbuf idx_file = STRBUF_INIT;\n+\t\tstrbuf_addstr(&idx_file, transport->pack_lockfile);\n+\t\tstrbuf_setlen(&idx_file, idx_file.len - 5); /* \".keep\" */\n+\t\tstrbuf_addstr(&idx_file, \".idx\");\n+\t\tnew_pack = add_packed_git(idx_file.buf, idx_file.len, 1);\n+\t\tstrbuf_release(&idx_file);\n+\t}\n+\n \tif (quiet)\n \t\targv[5] = \"--quiet\";\n \n@@ -42,6 +63,17 @@ int check_everything_connected(sha1_iterate_fn fn, int quiet, void *cb_data)\n \n \tcommit[40] = '\\n';\n \tdo {\n+\t\t/*\n+\t\t * If index-pack already checked that:\n+\t\t * - there are no dangling pointers in the new pack\n+\t\t * - the pack is self contained\n+\t\t * Then if the updated ref is in the new pack, then we\n+\t\t * are sure the ref is good and not sending it to\n+\t\t * rev-list for verification.\n+\t\t */\n+\t\tif (new_pack && find_pack_entry_one(sha1, new_pack))\n+\t\t\tcontinue;\n+\n \t\tmemcpy(commit, sha1_to_hex(sha1), 40);\n \t\tif (write_in_full(rev_list.in, commit, 41) < 0) {\n \t\t\tif (errno != EPIPE && errno != EINVAL)\ndiff --git a/connected.h b/connected.h\nindex 7e4585a..0b060b7 100644\n--- a/connected.h\n+++ b/connected.h\n@@ -1,6 +1,8 @@\n #ifndef CONNECTED_H\n #define CONNECTED_H\n \n+struct transport;\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@@ -16,5 +18,8 @@ typedef int (*sha1_iterate_fn)(void *, unsigned char [20]);\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_with_transport(sha1_iterate_fn, int quiet,\n+\t\t\t\t\t\t void *cb_data,\n+\t\t\t\t\t\t struct transport *transport);\n \n #endif /* CONNECTED_H */\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex 1ca4f6b..6b7e701 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -691,6 +691,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tconst char **av;\n \tint do_keep = args->keep_pack;\n \tstruct child_process cmd;\n+\tint ret;\n \n \tmemset(&demux, 0, sizeof(demux));\n \tif (use_sideband) {\n@@ -747,11 +748,14 @@ static int get_pack(struct fetch_pack_args *args,\n \t\t\t\tstrcpy(keep_arg + s, \"localhost\");\n \t\t\t*av++ = keep_arg;\n \t\t}\n+\t\tif (args->check_self_contained_and_connected)\n+\t\t\t*av++ = \"--check-self-contained-and-connected\";\n \t}\n \telse {\n \t\t*av++ = \"unpack-objects\";\n \t\tif (args->quiet || args->no_progress)\n \t\t\t*av++ = \"-q\";\n+\t\targs->check_self_contained_and_connected = 0;\n \t}\n \tif (*hdr_arg)\n \t\t*av++ = hdr_arg;\n@@ -772,7 +776,12 @@ static int get_pack(struct fetch_pack_args *args,\n \t\tclose(cmd.out);\n \t}\n \n-\tif (finish_command(&cmd))\n+\tret = finish_command(&cmd);\n+\tif (!ret || (args->check_self_contained_and_connected && ret == 1))\n+\t\targs->self_contained_and_connected =\n+\t\t\targs->check_self_contained_and_connected &&\n+\t\t\tret == 0;\n+\telse\n \t\tdie(\"%s failed\", argv[0]);\n \tif (use_sideband && finish_async(&demux))\n \t\tdie(\"error in sideband demultiplexer\");\ndiff --git a/fetch-pack.h b/fetch-pack.h\nindex dc5266c..40f08ba 100644\n--- a/fetch-pack.h\n+++ b/fetch-pack.h\n@@ -16,7 +16,9 @@ struct fetch_pack_args {\n \t\tverbose:1,\n \t\tno_progress:1,\n \t\tinclude_tag:1,\n-\t\tstateless_rpc:1;\n+\t\tstateless_rpc:1,\n+\t\tcheck_self_contained_and_connected:1,\n+\t\tself_contained_and_connected:1;\n };\n \n /*\ndiff --git a/transport.c b/transport.c\nindex ba5d8af..359a671 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -534,6 +534,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \targs.quiet = (transport->verbose < 0);\n \targs.no_progress = !transport->progress;\n \targs.depth = data->options.depth;\n+\targs.check_self_contained_and_connected =\n+\t\tdata->options.check_self_contained_and_connected;\n \n \tif (!data->got_remote_heads) {\n \t\tconnect_setup(transport, 0, 0);\n@@ -551,6 +553,8 @@ static int fetch_refs_via_pack(struct transport *transport,\n \t\trefs = NULL;\n \tdata->conn = NULL;\n \tdata->got_remote_heads = 0;\n+\tdata->options.self_contained_and_connected =\n+\t\targs.self_contained_and_connected;\n \n \tfree_refs(refs_tmp);\n \ndiff --git a/transport.h b/transport.h\nindex fcb1d25..4edebc5 100644\n--- a/transport.h\n+++ b/transport.h\n@@ -8,6 +8,8 @@ struct git_transport_options {\n \tunsigned thin : 1;\n \tunsigned keep : 1;\n \tunsigned followtags : 1;\n+\tunsigned check_self_contained_and_connected : 1;\n+\tunsigned self_contained_and_connected : 1;\n \tint depth;\n \tconst char *uploadpack;\n \tconst char *receivepack;\n-- \n1.8.2.83.gc99314b\n"},{"id":"216356","messageId":"CAPig+cR=Vqj_XjsyHg649Dpjh_VHd8NRU8TMvjFM1w_Au1_-=g@mail.gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 2/4] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-03T12:37:40Z","receivedAt":"2013-05-03T12:37:40Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 3, 2013 at 8:35 AM, Nguyễn Thái Ngọc Duy wrote:\n> index-pack --strict looks up and follows parent commits. If shallow\n> information is not ready by the time index-pack is run, index-pack may\n> be lead to non-existent objects. Make fetch-pack save shallow file to\n\ns/lead/led/\n\n> disk before invoking index-pack.\n>\n> git learns new global option --shallow-file to pass on the alternate\n> shallow file path. Undocumented (and not even support --shallow-file=\n> syntax) because it's unlikely to be used again elsewhere.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"216357","messageId":"CAPig+cSvoahDBxg+SzEMPSbNUXfi18Q=iKmRTuznmz6qz6V-6g@mail.gmail.com","threadId":"33340","inReplyTo":"1367584514-19806-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 4/4] clone: open a shortcut for connectivity check","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-05-03T12:41:44Z","receivedAt":"2013-05-03T12:41:44Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Fri, May 3, 2013 at 8:35 AM, Nguyễn Thái Ngọc Duy wrote:\n> In order to make sure the cloned repository is good, we run \"rev-list\n> --objects --not --all $new_refs\" on the repository. This is expensive\n> on large repositories. This patch attempts to mitigate the impact in\n> this special case.\n>\n> In the \"good\" clone case, we only have one pack. If all of the\n> following are met, we can be sure that all objects reachable from the\n> new refs exist, which is the intention of running \"rev-list ...\":\n>\n> - all refs point to an object in the pack\n> - there are no dangling pointers in any object in the pack\n> - no objects in the pack point to objects outside the pack\n>\n> The second and third checks can be done with the help of index-pack as\n> a slight variation of --strict check (which introduces a new condition\n> for the shortcut: pack transfer must be used and the number of objects\n> large enough to call index-pack). The first is checked in\n> check_everything_connected after we get an \"ok\" from index-pack.\n>\n> \"index-pack + new checks\" is still faster than the current \"index-pack\n> + rev-list\", which is the whole point of this patch. If any of the\n> conditions fails, we fall back to the good old but expensive \"rev-list\n\ns/fails/fail/\n\n> ..\". In that case it's even more expensive because we have to pay for\n> the new checks in index-pack. But that should only happen when the\n> other side is either buggy or malicious.\n>\n> Cloning linux-2.6 over file://\n>\n> before after\n> real 3m25.693s 2m53.050s\n> user 5m2.037s 4m42.396s\n> sys 0m13.750s 0m16.574s\n>\n> A more realistic test with ssh:// over wireless\n>\n> before after\n> real 11m26.629s 10m4.213s\n> user 5m43.196s 5m19.444s\n> sys 0m35.812s 0m37.630s\n>\n> This shortcut is not applied to shallow clones, partly because shallow\n> clones should have no more objects than a usual fetch and the cost of\n> rev-list is acceptable, partly to avoid dealing with corner cases when\n> grafting is involved.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n"},{"id":"216361","messageId":"7vwqrgxcoc.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"1367584514-19806-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 4/4] clone: open a shortcut for connectivity check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-03T16:15:15Z","receivedAt":"2013-05-03T16:15:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> In order to make sure the cloned repository is good, we run \"rev-list\n> --objects --not --all $new_refs\" on the repository. This is expensive\n> on large repositories. This patch attempts to mitigate the impact in\n> this special case.\n>\n> In the \"good\" clone case, we only have one pack.\n\nIf \"On large repositories\" is the focus, we need to take into\naccount the fact that pack.packSizeLimit can split and store the\nincoming packstream to multiple packs, so \"only have one pack\" is\nmisleading.\n\nI think you can still do the same trick even when we split the pack\nas index-pack will keep track of the objects it saw in the same\nincoming pack stream (but I am writing this from memory without\nlooking at the original code you are touching, so please double\ncheck).\n\n> If all of the\n> following are met, we can be sure that all objects reachable from the\n> new refs exist, which is the intention of running \"rev-list ...\":\n>\n> - all refs point to an object in the pack\n> - there are no dangling pointers in any object in the pack\n> - no objects in the pack point to objects outside the pack\n>\n> The second and third checks can be done with the help of index-pack as\n> a slight variation of --strict check (which introduces a new condition\n> for the shortcut: pack transfer must be used and the number of objects\n> large enough to call index-pack). The first is checked in\n> check_everything_connected after we get an \"ok\" from index-pack.\n>\n> \"index-pack + new checks\" is still faster than the current \"index-pack\n> + rev-list\", which is the whole point of this patch. If any of the\n\nDoes the same check apply if we end up on the unpack-objects\ncodepath?\n\n> This shortcut is not applied to shallow clones, partly because shallow\n> clones should have no more objects than a usual fetch and the cost of\n> rev-list is acceptable, partly to avoid dealing with corner cases when\n> grafting is involved.\n"},{"id":"216396","messageId":"CACsJy8BSMkFGQcHxFD3wTn_PpC=qEFJ+vR_XygndzVnprye3Aw@mail.gmail.com","threadId":"33340","inReplyTo":"7vwqrgxcoc.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 4/4] clone: open a shortcut for connectivity check","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-04T01:10:55Z","receivedAt":"2013-05-04T01:10:55Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, May 3, 2013 at 11:15 PM, Junio C Hamano wrote:\n> Nguyễn Thái Ngọc Duy writes:\n>\n>> In order to make sure the cloned repository is good, we run \"rev-list\n>> --objects --not --all $new_refs\" on the repository. This is expensive\n>> on large repositories. This patch attempts to mitigate the impact in\n>> this special case.\n>>\n>> In the \"good\" clone case, we only have one pack.\n>\n> If \"On large repositories\" is the focus, we need to take into\n> account the fact that pack.packSizeLimit can split and store the\n> incoming packstream to multiple packs, so \"only have one pack\" is\n> misleading.\n\nI only had a quick look. But I don't think index-pack respects\npackSizeLimit. pack-objects does but only when --stdout is not used,\nwhich is not the case for pack transfer.\n\n> I think you can still do the same trick even when we split the pack\n> as index-pack will keep track of the objects it saw in the same\n> incoming pack stream (but I am writing this from memory without\n> looking at the original code you are touching, so please double\n> check).\n\nYeah. As long we have only one incoming stream, we can still do the\nsame verification.\n\n>> \"index-pack + new checks\" is still faster than the current \"index-pack\n>> + rev-list\", which is the whole point of this patch. If any of the\n>\n> Does the same check apply if we end up on the unpack-objects\n> codepath?\n\nNo. unpack-objects does not do this and check_everything_connected\nshould invoke rev-list like before.\n--\nDuy\n"},{"id":"216625","messageId":"7va9o6lr0h.fsf@alter.siamese.dyndns.org","threadId":"33340","inReplyTo":"1367584514-19806-3-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v3 2/4] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-05-07T15:59:58Z","receivedAt":"2013-05-07T15:59:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyễn Thái Ngọc Duy writes:\n\n> index-pack --strict looks up and follows parent commits. If shallow\n> information is not ready by the time index-pack is run, index-pack may\n> be lead to non-existent objects. Make fetch-pack save shallow file to\n> disk before invoking index-pack.\n>\n> git learns new global option --shallow-file to pass on the alternate\n> shallow file path. Undocumented (and not even support --shallow-file=\n> syntax) because it's unlikely to be used again elsewhere.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n> commit.h | 2 ++\n> fetch-pack.c | 69 +++++++++++++++++++++++++--------------------------\n> git.c | 5 ++++\n> shallow.c | 45 +++++++++++++++++++++++++++++++--\n> t/t5500-fetch-pack.sh | 7 ++++++\n> 5 files changed, 91 insertions(+), 37 deletions(-)\n>\n> diff --git a/commit.h b/commit.h\n> index 67bd509..6e9c7cd 100644\n> --- a/commit.h\n> +++ b/commit.h\n> @@ -176,6 +176,8 @@ extern int for_each_commit_graft(each_commit_graft_fn, void *);\n> extern int is_repository_shallow(void);\n> extern struct commit_list *get_shallow_commits(struct object_array *heads,\n> \t\tint depth, int shallow_flag, int not_shallow_flag);\n> +extern void check_shallow_file_for_update(void);\n> +extern void set_alternate_shallow_file(const char *path);\n> \n> int is_descendant_of(struct commit *, struct commit_list *);\n> int in_merge_bases(struct commit *, struct commit *);\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f156dd4..1ca4f6b 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -20,6 +20,8 @@ static int no_done;\n> static int fetch_fsck_objects = -1;\n> static int transfer_fsck_objects = -1;\n> static int agent_supported;\n> +static struct lock_file shallow_lock;\n> +static const char *alternate_shallow_file;\n> \n> #define COMPLETE\t(1U << 0)\n> #define COMMON\t\t(1U << 1)\n> @@ -683,7 +685,7 @@ static int get_pack(struct fetch_pack_args *args,\n> \t\t int xd[2], char **pack_lockfile)\n> {\n> \tstruct async demux;\n> -\tconst char *argv[20];\n> +\tconst char *argv[22];\n> \tchar keep_arg[256];\n> \tchar hdr_arg[256];\n> \tconst char **av;\n> @@ -724,6 +726,11 @@ static int get_pack(struct fetch_pack_args *args,\n> \t\t\tdo_keep = 1;\n> \t}\n> \n> +\tif (alternate_shallow_file) {\n> +\t\t*av++ = \"--shallow-file\";\n> +\t\t*av++ = alternate_shallow_file;\n> +\t}\n> +\n> \tif (do_keep) {\n> \t\tif (pack_lockfile)\n> \t\t\tcmd.out = -1;\n> @@ -779,6 +786,23 @@ static int cmp_ref_by_name(const void *a_, const void *b_)\n> \treturn strcmp(a->name, b->name);\n> }\n> \n> +static void setup_alternate_shallow(void)\n> +{\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tint fd;\n> +\n> +\tcheck_shallow_file_for_update();\n> +\tfd = hold_lock_file_for_update(&shallow_lock, git_path(\"shallow\"),\n> +\t\t\t\t LOCK_DIE_ON_ERROR);\n> +\tif (write_shallow_commits(&sb, 0)) {\n> +\t\tif (write_in_full(fd, sb.buf, sb.len) != sb.len)\n> +\t\t\tdie_errno(\"failed to write to %s\", shallow_lock.filename);\n> +\t\talternate_shallow_file = shallow_lock.filename;\n> +\t} else\n> +\t\talternate_shallow_file = \"\";\n\nThis \"empty string, not NULL\" trick needs to be commented here to\nmatch the other place that uses this as a cue to do things\ndifferently.\n\n> +\tstrbuf_release(&sb);\n> +}\n> +\n> static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \t\t\t\t int fd[2],\n> \t\t\t\t const struct ref *orig_ref,\n> @@ -858,6 +882,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n> \n> \tif (args->stateless_rpc)\n> \t\tpacket_flush(fd[1]);\n> +\tif (args->depth > 0)\n> +\t\tsetup_alternate_shallow();\n> \tif (get_pack(args, fd, pack_lockfile))\n> \t\tdie(\"git fetch-pack: fetch failed.\");\n> \n> @@ -936,15 +962,9 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n> \t\t struct ref **sought, int nr_sought,\n> \t\t char **pack_lockfile)\n> {\n> \tstruct ref *ref_cpy;\n> \n> \tfetch_pack_setup();\n> \tif (nr_sought)\n> \t\tnr_sought = remove_duplicates_in_refs(sought, nr_sought);\n> \n> @@ -955,34 +975,13 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n> \tref_cpy = do_fetch_pack(args, fd, ref, sought, nr_sought, pack_lockfile);\n> \n> \tif (args->depth > 0) {\n> +\t\tstruct stat st;\n> +\t\tif (!fstat(shallow_lock.fd, &st) &&\n> +\t\t st.st_size == 0) {\n> +\t\t\tunlink_or_warn(git_path(\"shallow\"));\n\nAre we unlinking the right file here?\n\n> +\t\t\trollback_lock_file(&shallow_lock);\n> +\t\t} else\n> +\t\t\tcommit_lock_file(&shallow_lock);\n> \t}\n> \n> \treprepare_packed_git();\n> diff --git a/git.c b/git.c\n> index 1ada169..6450a38 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -4,6 +4,7 @@\n> #include \"help.h\"\n> #include \"quote.h\"\n> #include \"run-command.h\"\n> +#include \"commit.h\"\n> \n> const char git_usage_string[] =\n> \t\"git [--version] [--help] [-c name=value]\\n\"\n> @@ -146,6 +147,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)\n> \t\t\tsetenv(GIT_LITERAL_PATHSPECS_ENVIRONMENT, \"0\", 1);\n> \t\t\tif (envchanged)\n> \t\t\t\t*envchanged = 1;\n> +\t\t} else if (!strcmp(cmd, \"--shallow-file\")) {\n> +\t\t\t(*argv)++;\n> +\t\t\t(*argc)--;\n> +\t\t\tset_alternate_shallow_file((*argv)[0]);\n\nShould this count as \"envchanged\"?\n\n> diff --git a/shallow.c b/shallow.c\n> index 6be915f..bdae988 100644\n> --- a/shallow.c\n> +++ b/shallow.c\n> @@ -3,6 +3,16 @@\n> #include \"tag.h\"\n> \n> static int is_shallow = -1;\n> +static struct stat shallow_stat;\n> +static char *alternate_shallow_file;\n> +\n> +void set_alternate_shallow_file(const char *path)\n> +{\n> +\tif (is_shallow != -1)\n> +\t\tdie(\"BUG: is_repository_shallow must not be called before set_alternate_shallow_file\");\n> +\tfree(alternate_shallow_file);\n> +\talternate_shallow_file = path ? xstrdup(path) : NULL;\n> +}\n> \n> int register_shallow(const unsigned char *sha1)\n> {\n> @@ -21,12 +31,21 @@ int is_repository_shallow(void)\n> {\n> \tFILE *fp;\n> \tchar buf[1024];\n> +\tconst char *path = alternate_shallow_file;\n> \n> \tif (is_shallow >= 0)\n> \t\treturn is_shallow;\n> \n> -\tfp = fopen(git_path(\"shallow\"), \"r\");\n> -\tif (!fp) {\n> +\tif (!path)\n> +\t\tpath = git_path(\"shallow\");\n> +\t/*\n> +\t * fetch-pack set '--shallow-file \"\"' as an indicator that no\n\ns/set/&s/?\n\n> +\t * shallow file should be used. We could just open it and it\n> +\t * will likely fail. But let's do an explicit check instead.\n> +\t */\n> +\tif (!*path ||\n> +\t stat(path, &shallow_stat) ||\n> +\t (fp = fopen(path, \"r\")) == NULL) {\n> \t\tis_shallow = 0;\n> \t\treturn is_shallow;\n> \t}\n> @@ -108,3 +127,25 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,\n> \n> \treturn result;\n> }\n> +\n> +void check_shallow_file_for_update(void)\n> +{\n> +\tstruct stat st;\n> +\n> +\tif (getenv(\"GIT_SHALLOW_FILE\"))\n> +\t\tdie(\"GIT_SHALLOW_FILE should not be manually set\");\n\nForgot to remove this?\n\n> +\tif (!is_shallow)\n> +\t\treturn;\n> +\telse if (is_shallow == -1)\n> +\t\tdie(\"BUG: shallow must be initialized by now\");\n> +\n> +\tif (stat(git_path(\"shallow\"), &st))\n> +\t\tdie(\"shallow file was removed during fetch\");\n> +\telse if (st.st_mtime != shallow_stat.st_mtime\n> +#ifdef USE_NSEC\n> +\t\t || ST_MTIME_NSEC(st) != ST_MTIME_NSEC(shallow_stat)\n> +#endif\n> +\t\t )\n> +\t\tdie(\"shallow file was changed during fetch\");\n> +}\n> diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh\n> index d574085..557b073 100755\n> --- a/t/t5500-fetch-pack.sh\n> +++ b/t/t5500-fetch-pack.sh\n> @@ -135,6 +135,13 @@ test_expect_success 'clone shallow depth 1' '\n> \ttest \"`git --git-dir=shallow0/.git rev-list --count HEAD`\" = 1\n> '\n> \n> +test_expect_success 'clone shallow depth 1 with fsck' '\n> +\tgit config --global fetch.fsckobjects true &&\n> +\tgit clone --no-single-branch --depth 1 \"file://$(pwd)/.\" shallow0fsck &&\n> +\ttest \"`git --git-dir=shallow0fsck/.git rev-list --count HEAD`\" = 1 &&\n> +\tgit config --global --unset fetch.fsckobjects\n> +'\n> +\n> test_expect_success 'clone shallow' '\n> \tgit clone --no-single-branch --depth 2 \"file://$(pwd)/.\" shallow\n> '\n"},{"id":"218550","messageId":"CACsJy8BFbDA1MitnktL6tNy59sWMw4vnsZ0wN6UfDfY_oAKh+w@mail.gmail.com","threadId":"33340","inReplyTo":"7va9o6lr0h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/4] fetch-pack: prepare updated shallow file before fetching the pack","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-05-26T01:01:33Z","receivedAt":"2013-05-26T01:01:33Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, May 7, 2013 at 10:59 PM, Junio C Hamano wrote:\n>> if (args->depth > 0) {\n>> + struct stat st;\n>> + if (!fstat(shallow_lock.fd, &st) &&\n>> + st.st_size == 0) {\n>> + unlink_or_warn(git_path(\"shallow\"));\n>\n> Are we unlinking the right file here?\n\nYes we are. when st.st_size is 0, the new shallow file is empty (i.e.\n--unshallow), so we want to remove the old file. I should check\n\"!*alternate_shallow_file\" here instead of based on st.st_size.\n--\nDuy\n"}]}