{"thread":{"id":"34826","subject":"[PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","startedAt":"2013-09-02T03:05:07Z","lastAt":"2013-09-08T06:35:34Z","messageCount":12,"participants":["Nguyễn Thái Ngọc Duy","Eric Sunshine","Jeff King","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"226492","messageId":"1378091107-31682-1-git-send-email-pclouds@gmail.com","threadId":"34826","inReplyTo":null,"subject":"[PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-02T03:05:07Z","receivedAt":"2013-09-02T03:05:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current code peaks into the transfered pack's header, if the number of\nobjects is under a limit, unpack-objects is called to handle the rest,\notherwise index-pack is. This patch makes fetch-pack use index-pack\nunconditionally, then turn objects loose and remove the pack at the\nend. unpack-objects is deprecated and may be removed in future.\n\nThere are a few reasons for this:\n\n - down to two code paths to maintain regarding pack reading\n   (sha1_file.c and index-pack.c). When .pack v4 comes, we don't need\n   to duplicate work in index-pack and unpack-objects.\n\n - the number of objects should not be the only indicator for\n   unpacking. If there are a few large objects in the pack, the pack\n   should be kept anyway. Keeping the pack lets us examine the whole\n   pack later and make a better decision.\n\n - by going through index-pack first, then unpack, we pay extra cost\n   for completing a thin pack into a full one. But compared to fetch's\n   total time, it should not be noticeable because unpack-objects is\n   only called when the pack contains a small number of objects.\n\n - unpack-objects does not support streaming large blobs. Granted\n   force_object_loose(), which is used by this patch, does not either.\n   But it'll be easier to do so because sha1_file.c interface supports\n   streaming.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n builtin/receive-pack.c | 120 +++++++++++++------------------------------------\n cache.h                |   1 +\n fetch-pack.c           |  77 +++++++++++--------------------\n sha1_file.c            |  70 ++++++++++++++++++++++++++++-\n 4 files changed, 128 insertions(+), 140 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e3eb5fc..6eb4ffb 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -792,105 +792,49 @@ static struct command *read_head_info(void)\n \treturn commands;\n }\n \n-static const char *parse_pack_header(struct pack_header *hdr)\n-{\n-\tswitch (read_pack_header(0, hdr)) {\n-\tcase PH_ERROR_EOF:\n-\t\treturn \"eof before pack header was fully read\";\n-\n-\tcase PH_ERROR_PACK_SIGNATURE:\n-\t\treturn \"protocol error (pack signature mismatch detected)\";\n-\n-\tcase PH_ERROR_PROTOCOL:\n-\t\treturn \"protocol error (pack version unsupported)\";\n-\n-\tdefault:\n-\t\treturn \"unknown error in parse_pack_header\";\n-\n-\tcase 0:\n-\t\treturn NULL;\n-\t}\n-}\n-\n static const char *pack_lockfile;\n \n static const char *unpack(int err_fd)\n {\n-\tstruct pack_header hdr;\n-\tconst char *hdr_err;\n-\tchar hdr_arg[38];\n \tint fsck_objects = (receive_fsck_objects >= 0\n \t\t\t    ? receive_fsck_objects\n \t\t\t    : transfer_fsck_objects >= 0\n \t\t\t    ? transfer_fsck_objects\n \t\t\t    : 0);\n \n-\thdr_err = parse_pack_header(&hdr);\n-\tif (hdr_err) {\n-\t\tif (err_fd > 0)\n-\t\t\tclose(err_fd);\n-\t\treturn hdr_err;\n-\t}\n-\tsnprintf(hdr_arg, sizeof(hdr_arg),\n-\t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n-\t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n-\n-\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n-\t\tint code, i = 0;\n-\t\tstruct child_process child;\n-\t\tconst char *unpacker[5];\n-\t\tunpacker[i++] = \"unpack-objects\";\n-\t\tif (quiet)\n-\t\t\tunpacker[i++] = \"-q\";\n-\t\tif (fsck_objects)\n-\t\t\tunpacker[i++] = \"--strict\";\n-\t\tunpacker[i++] = hdr_arg;\n-\t\tunpacker[i++] = NULL;\n-\t\tmemset(&child, 0, sizeof(child));\n-\t\tchild.argv = unpacker;\n-\t\tchild.no_stdout = 1;\n-\t\tchild.err = err_fd;\n-\t\tchild.git_cmd = 1;\n-\t\tcode = run_command(&child);\n-\t\tif (!code)\n-\t\t\treturn NULL;\n-\t\treturn \"unpack-objects abnormal exit\";\n-\t} else {\n-\t\tconst char *keeper[7];\n-\t\tint s, status, i = 0;\n-\t\tchar keep_arg[256];\n-\t\tstruct child_process ip;\n-\n-\t\ts = sprintf(keep_arg, \"--keep=receive-pack %\"PRIuMAX\" on \", (uintmax_t) getpid());\n-\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n-\t\t\tstrcpy(keep_arg + s, \"localhost\");\n-\n-\t\tkeeper[i++] = \"index-pack\";\n-\t\tkeeper[i++] = \"--stdin\";\n-\t\tif (fsck_objects)\n-\t\t\tkeeper[i++] = \"--strict\";\n-\t\tkeeper[i++] = \"--fix-thin\";\n-\t\tkeeper[i++] = hdr_arg;\n-\t\tkeeper[i++] = keep_arg;\n-\t\tkeeper[i++] = NULL;\n-\t\tmemset(&ip, 0, sizeof(ip));\n-\t\tip.argv = keeper;\n-\t\tip.out = -1;\n-\t\tip.err = err_fd;\n-\t\tip.git_cmd = 1;\n-\t\tstatus = start_command(&ip);\n-\t\tif (status) {\n-\t\t\treturn \"index-pack fork failed\";\n-\t\t}\n-\t\tpack_lockfile = index_pack_lockfile(ip.out);\n-\t\tclose(ip.out);\n-\t\tstatus = finish_command(&ip);\n-\t\tif (!status) {\n-\t\t\treprepare_packed_git();\n-\t\t\treturn NULL;\n-\t\t}\n+\tconst char *keeper[7];\n+\tint s, status, i = 0;\n+\tchar keep_arg[256];\n+\tstruct child_process ip;\n+\n+\ts = sprintf(keep_arg, \"--keep=receive-pack %\"PRIuMAX\" on \", (uintmax_t) getpid());\n+\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n+\t\tstrcpy(keep_arg + s, \"localhost\");\n+\n+\tkeeper[i++] = \"index-pack\";\n+\tkeeper[i++] = \"--stdin\";\n+\tif (fsck_objects)\n+\t\tkeeper[i++] = \"--strict\";\n+\tkeeper[i++] = \"--fix-thin\";\n+\tkeeper[i++] = keep_arg;\n+\tkeeper[i++] = NULL;\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = keeper;\n+\tip.out = -1;\n+\tip.err = err_fd;\n+\tip.git_cmd = 1;\n+\tstatus = start_command(&ip);\n+\tif (status) {\n+\t\treturn \"index-pack fork failed\";\n+\t}\n+\tpack_lockfile = index_pack_lockfile(ip.out);\n+\tclose(ip.out);\n+\tstatus = finish_command(&ip);\n+\tif (status)\n \t\treturn \"index-pack abnormal exit\";\n-\t}\n+\tif (!maybe_unpack_objects(pack_lockfile, unpack_limit))\n+\t\tpack_lockfile = NULL;\n+\treturn NULL;\n }\n \n static const char *unpack_with_sideband(void)\ndiff --git a/cache.h b/cache.h\nindex 85b544f..0fff958 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -789,6 +789,7 @@ extern int hash_sha1_file(const void *buf, unsigned long len, const char *type,\n extern int write_sha1_file(const void *buf, unsigned long len, const char *type, unsigned char *return_sha1);\n extern int pretend_sha1_file(void *, unsigned long, enum object_type, unsigned char *);\n extern int force_object_loose(const unsigned char *sha1, time_t mtime);\n+extern int maybe_unpack_objects(const char *pack_lockfile, int limit);\n extern void *map_sha1_file(const unsigned char *sha1, unsigned long *size);\n extern int unpack_sha1_header(git_zstream *stream, unsigned char *map, unsigned long mapsize, void *buffer, unsigned long bufsiz);\n extern int parse_sha1_header(const char *hdr, unsigned long *sizep);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f5d99c1..9c81a15 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -687,9 +687,7 @@ static int get_pack(struct fetch_pack_args *args,\n \tstruct async demux;\n \tconst char *argv[22];\n \tchar keep_arg[256];\n-\tchar hdr_arg[256];\n \tconst char **av;\n-\tint do_keep = args->keep_pack;\n \tstruct child_process cmd;\n \tint ret;\n \n@@ -711,54 +709,29 @@ static int get_pack(struct fetch_pack_args *args,\n \n \tmemset(&cmd, 0, sizeof(cmd));\n \tcmd.argv = argv;\n+\tcmd.out = -1;\n \tav = argv;\n-\t*hdr_arg = 0;\n-\tif (!args->keep_pack && unpack_limit) {\n-\t\tstruct pack_header header;\n-\n-\t\tif (read_pack_header(demux.out, &header))\n-\t\t\tdie(\"protocol error: bad pack header\");\n-\t\tsnprintf(hdr_arg, sizeof(hdr_arg),\n-\t\t\t \"--pack_header=%\"PRIu32\",%\"PRIu32,\n-\t\t\t ntohl(header.hdr_version), ntohl(header.hdr_entries));\n-\t\tif (ntohl(header.hdr_entries) < unpack_limit)\n-\t\t\tdo_keep = 0;\n-\t\telse\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-\t\t*av++ = \"index-pack\";\n-\t\t*av++ = \"--stdin\";\n-\t\tif (!args->quiet && !args->no_progress)\n-\t\t\t*av++ = \"-v\";\n-\t\tif (args->use_thin_pack)\n-\t\t\t*av++ = \"--fix-thin\";\n-\t\tif (args->lock_pack || unpack_limit) {\n-\t\t\tint s = sprintf(keep_arg,\n-\t\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on \", (uintmax_t) getpid());\n-\t\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\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+\t*av++ = \"index-pack\";\n+\t*av++ = \"--stdin\";\n+\tif (!args->quiet && !args->no_progress)\n+\t\t*av++ = \"-v\";\n+\tif (args->use_thin_pack)\n+\t\t*av++ = \"--fix-thin\";\n+\tif (args->lock_pack || unpack_limit) {\n+\t\tint s = sprintf(keep_arg,\n+\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on \", (uintmax_t) getpid());\n+\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n+\t\t\tstrcpy(keep_arg + s, \"localhost\");\n+\t\t*av++ = keep_arg;\n+\t}\n+\tif (args->check_self_contained_and_connected)\n+\t\t*av++ = \"--check-self-contained-and-connected\";\n \tif (fetch_fsck_objects >= 0\n \t    ? fetch_fsck_objects\n \t    : transfer_fsck_objects >= 0\n@@ -771,10 +744,8 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tif (do_keep && pack_lockfile) {\n-\t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n-\t\tclose(cmd.out);\n-\t}\n+\t*pack_lockfile = index_pack_lockfile(cmd.out);\n+\tclose(cmd.out);\n \n \tret = finish_command(&cmd);\n \tif (!ret || (args->check_self_contained_and_connected && ret == 1))\n@@ -820,11 +791,12 @@ 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_p)\n {\n \tstruct ref *ref = copy_ref_list(orig_ref);\n \tunsigned char sha1[20];\n \tconst char *agent_feature;\n+\tchar *pack_lockfile = NULL;\n \tint agent_len;\n \n \tsort_ref_list(&ref, ref_compare_name);\n@@ -899,9 +871,15 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,\n \t\tsetup_alternate_shallow();\n \telse\n \t\talternate_shallow_file = NULL;\n-\tif (get_pack(args, fd, pack_lockfile))\n+\tif (get_pack(args, fd, &pack_lockfile))\n \t\tdie(\"git fetch-pack: fetch failed.\");\n \n+\tif (!maybe_unpack_objects(pack_lockfile,\n+\t\t\t\t  args->keep_pack ? 0 : unpack_limit))\n+\t\tpack_lockfile = NULL;\n+\tif (pack_lockfile_p)\n+\t\t*pack_lockfile_p = pack_lockfile;\n+\n  all_done:\n \treturn ref;\n }\n@@ -997,6 +975,5 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\tcommit_lock_file(&shallow_lock);\n \t}\n \n-\treprepare_packed_git();\n \treturn ref_cpy;\n }\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 8e27db1..a0cdeae 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -61,6 +61,8 @@ static struct cached_object empty_tree = {\n };\n \n static struct packed_git *last_found_pack;\n+/* temporarily skip one pack in find_pack_entry() */\n+static struct packed_git *skip_this_pack;\n \n static struct cached_object *find_cached_object(const unsigned char *sha1)\n {\n@@ -2376,11 +2378,15 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tif (last_found_pack && fill_pack_entry(sha1, e, last_found_pack))\n+\tif (last_found_pack &&\n+\t    last_found_pack != skip_this_pack &&\n+\t    fill_pack_entry(sha1, e, last_found_pack))\n \t\treturn 1;\n \n \tfor (p = packed_git; p; p = p->next) {\n-\t\tif (p == last_found_pack || !fill_pack_entry(sha1, e, p))\n+\t\tif (p == last_found_pack ||\n+\t\t    p == skip_this_pack ||\n+\t\t    !fill_pack_entry(sha1, e, p))\n \t\t\tcontinue;\n \n \t\tlast_found_pack = p;\n@@ -3133,3 +3139,63 @@ void assert_sha1_type(const unsigned char *sha1, enum object_type expect)\n \t\tdie(\"%s is not a valid '%s' object\", sha1_to_hex(sha1),\n \t\t    typename(expect));\n }\n+\n+static int has_sha1_except_in(struct packed_git *p,\n+\t\t\t      const unsigned char *sha1)\n+{\n+\tint has;\n+\tskip_this_pack = p;\n+\thas = has_sha1_file(sha1);\n+\tskip_this_pack = NULL;\n+\treturn has;\n+}\n+\n+/*\n+ * Unpack objects if the number of objects in the pack is lower than\n+ * specified limit. Otherwise make sure the new pack is registered\n+ * (including when pack_lockfile is NULL). Return 0 is the pack is\n+ * removed.\n+ */\n+int maybe_unpack_objects(const char *pack_lockfile, int limit)\n+{\n+\tconst char *ext[] = { \".pack\", \".idx\", \".keep\", NULL };\n+\tstruct packed_git *p;\n+\tchar *path;\n+\tint len, ret = 0;\n+\tuint32_t i;\n+\n+\treprepare_packed_git();\n+\tif (!pack_lockfile)\n+\t\treturn -1;\n+\n+\t/* must be enough for any extensions in ext[] */\n+\tpath = xstrdup(pack_lockfile);\n+\tlen = strlen(pack_lockfile) - strlen(\".keep\");\n+\tstrcpy(path + len, \".pack\");\n+\tfor (p = packed_git; p; p = p->next)\n+\t\tif (!strcmp(p->pack_name, path))\n+\t\t\tbreak;\n+\tif (!p || open_pack_index(p))\n+\t\tdie(\"unable to find pack %s\", path);\n+\n+\tif (p->num_objects >= limit) {\n+\t\tfree(path);\n+\t\treturn -1;\n+\t}\n+\n+\tfor (i = 0; i < p->num_objects; i++) {\n+\t\tconst unsigned char *sha1 = nth_packed_object_sha1(p, i);\n+\t\tif (!has_sha1_except_in(p, sha1)) /* skip --fix-thin objects */\n+\t\t\tret |= force_object_loose(sha1, p->mtime);\n+\t}\n+\n+\tif (!ret) {\n+\t\tfree_pack_by_name(p->pack_name);\n+\t\tfor (i = 0; ext[i]; i++) {\n+\t\t\tstrcpy(path + len, ext[i]);\n+\t\t\tunlink_or_warn(path);\n+\t\t}\n+\t}\n+\tfree(path);\n+\treturn ret;\n+}\n-- \n1.8.2.83.gc99314b\n"},{"id":"226493","messageId":"CAPig+cTscSov3kHX6Mj_LpK1CeH+x4NTexdWZD42NT-KfohvrA@mail.gmail.com","threadId":"34826","inReplyTo":"1378091107-31682-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-09-02T04:38:59Z","receivedAt":"2013-09-02T04:38:59Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Sep 1, 2013 at 11:05 PM, Nguyễn Thái Ngọc Duy <pclouds@gmail.com> wrote:\n> Current code peaks into the transfered pack's header, if the number of\n\ns/peaks/peeks/\n\n> objects is under a limit, unpack-objects is called to handle the rest,\n> otherwise index-pack is. This patch makes fetch-pack use index-pack\n> unconditionally, then turn objects loose and remove the pack at the\n> end. unpack-objects is deprecated and may be removed in future.\n>\n> There are a few reasons for this:\n>\n>  - down to two code paths to maintain regarding pack reading\n>    (sha1_file.c and index-pack.c). When .pack v4 comes, we don't need\n>    to duplicate work in index-pack and unpack-objects.\n>\n>  - the number of objects should not be the only indicator for\n>    unpacking. If there are a few large objects in the pack, the pack\n>    should be kept anyway. Keeping the pack lets us examine the whole\n>    pack later and make a better decision.\n>\n>  - by going through index-pack first, then unpack, we pay extra cost\n>    for completing a thin pack into a full one. But compared to fetch's\n>    total time, it should not be noticeable because unpack-objects is\n>    only called when the pack contains a small number of objects.\n>\n>  - unpack-objects does not support streaming large blobs. Granted\n>    force_object_loose(), which is used by this patch, does not either.\n>    But it'll be easier to do so because sha1_file.c interface supports\n>    streaming.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n"},{"id":"226581","messageId":"20130903064938.GB3608@sigill.intra.peff.net","threadId":"34826","inReplyTo":"1378091107-31682-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T06:49:38Z","receivedAt":"2013-09-03T06:49:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 02, 2013 at 10:05:07AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> Current code peaks into the transfered pack's header, if the number of\n> objects is under a limit, unpack-objects is called to handle the rest,\n> otherwise index-pack is. This patch makes fetch-pack use index-pack\n> unconditionally, then turn objects loose and remove the pack at the\n> end. unpack-objects is deprecated and may be removed in future.\n\nI do like consolidating the object-receiving code paths, but there is a\ndownside to this strategy: we increase the I/O in cases where we end up\nunpacking, as we spool the tmpfile to disk, and then force objects loose\n(whereas with the current code, unpack-objects reads straight from the\nnetwork into loose objects). I think that is what you're saying here:\n\n>  - by going through index-pack first, then unpack, we pay extra cost\n>    for completing a thin pack into a full one. But compared to fetch's\n>    total time, it should not be noticeable because unpack-objects is\n>    only called when the pack contains a small number of objects.\n\n...but the cost is paid by total pack size, not number of objects. So if\nI am pushing up a commit with a large uncompressible blob, I've\neffectively doubled my disk I/O. It would make more sense to me for\nindex-pack to learn command-line options specifying the limits, and then\nto operate on the pack as it streams in. E.g., to decide after seeing\nthe header to unpack rather than index, or to drop large blobs from the\npack (and put them in their own pack directly) as we are streaming into\nit (we do not know the blob size ahead of time, but we can make a good\nguess if it has a large on-disk size in the pack).\n\n-Peff\n"},{"id":"226613","messageId":"CACsJy8ButV8Y6Rm=F8dLTw3NvRCJZ1RgJ5f-591ASbOqfDRRTQ@mail.gmail.com","threadId":"34826","inReplyTo":"20130903064938.GB3608@sigill.intra.peff.net","subject":"Re: [PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-03T11:56:23Z","receivedAt":"2013-09-03T11:56:23Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Tue, Sep 3, 2013 at 1:49 PM, Jeff King <peff@peff.net> wrote:\n>>  - by going through index-pack first, then unpack, we pay extra cost\n>>    for completing a thin pack into a full one. But compared to fetch's\n>>    total time, it should not be noticeable because unpack-objects is\n>>    only called when the pack contains a small number of objects.\n>\n> ...but the cost is paid by total pack size, not number of objects. So if\n> I am pushing up a commit with a large uncompressible blob, I've\n> effectively doubled my disk I/O. It would make more sense to me for\n> index-pack to learn command-line options specifying the limits, and then\n> to operate on the pack as it streams in. E.g., to decide after seeing\n> the header to unpack rather than index, or to drop large blobs from the\n> pack (and put them in their own pack directly) as we are streaming into\n> it (we do not know the blob size ahead of time, but we can make a good\n> guess if it has a large on-disk size in the pack).\n\nYeah letting index-pack do the work was my backup plan :) I think if\nthere is a big blob in the pack, then the pack should not be unpacked\nat all. If you store big blobs in a separate pack you already pay the\nthe lookup cost of one more pack in find_pack_entry(), why go through\nthe process of unpacking? index-pack still has the advantage of\nstreaming though. Will rework.\n-- \nDuy\n"},{"id":"226642","messageId":"20130903172514.GB1050@sigill.intra.peff.net","threadId":"34826","inReplyTo":"CACsJy8ButV8Y6Rm=F8dLTw3NvRCJZ1RgJ5f-591ASbOqfDRRTQ@mail.gmail.com","subject":"Re: [PATCH] {fetch,receive}-pack: drop unpack-objects, delay loosing objects until the end","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-03T17:25:14Z","receivedAt":"2013-09-03T17:25:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 03, 2013 at 06:56:23PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > ...but the cost is paid by total pack size, not number of objects. So if\n> > I am pushing up a commit with a large uncompressible blob, I've\n> > effectively doubled my disk I/O. It would make more sense to me for\n> > index-pack to learn command-line options specifying the limits, and then\n> > to operate on the pack as it streams in. E.g., to decide after seeing\n> > the header to unpack rather than index, or to drop large blobs from the\n> > pack (and put them in their own pack directly) as we are streaming into\n> > it (we do not know the blob size ahead of time, but we can make a good\n> > guess if it has a large on-disk size in the pack).\n> \n> Yeah letting index-pack do the work was my backup plan :) I think if\n> there is a big blob in the pack, then the pack should not be unpacked\n> at all. If you store big blobs in a separate pack you already pay the\n> the lookup cost of one more pack in find_pack_entry(), why go through\n> the process of unpacking? index-pack still has the advantage of\n> streaming though. Will rework.\n\nIn general, our large-blob strategy is to push them out to their own\npack so that we do not incur the I/O overhead of rewriting them whenever\nwe repack. But the flipside is that we have to pay the cost of an extra\n.idx open and lookup for each such object. In the longer term, I think\nit might make sense to be able to generate a multi-pack .idx for such a\ncase (or even to simply store the large blobs in a special area indexed\nby the object sha1, as we do for loose objects).\n\nBut that is all orthogonal to your patch. I think as long as we are\nmoving towards \"index-pack makes the decisions while it processes the\npack\" we are going in a good direction. Even if we do not implement all\nof the decisions immediately, it leaves room for doing so later without\nloss of efficiency.\n\n-Peff\n"},{"id":"226899","messageId":"1378428363-14086-1-git-send-email-pclouds@gmail.com","threadId":"34826","inReplyTo":"1378091107-31682-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-06T00:46:01Z","receivedAt":"2013-09-06T00:46:01Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"If the number of objects in the given pack is less than the limit, all\nobjects in the pack will be unpacked, and the pack will not be created\nif it's streamed in. It's intended to replace unpack-objects.\n\nUnlike unpack-objects, this operation requires writing the stream to\ndisk for delta resolution. Base objects are not appended to the\ntemporary pack though. Given that unpack limit is usually small, the\nI/O overhead should be unnoticeable.\n\nWhen large blobs are present in the stream, things will be\ndifferent. Right now, those blobs are uncompressed in memory for\nsimplicity. But in future bulk-checkin should be used for streaming\nlarge blobs to a new pack (and not written to the temp pack to reduce\nI/O overhead).\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n I had something that could unpack without writing to temp pack file\n but I scraped it and chose this way because it follows closely how\n index-pack works. It's a good thing imo because .pack v4 is coming\n and I don't know how v4 may impact this unpack code path. Once things\n are settled, we can revisit and open a separate code path if it's\n still a good idea.\n\n builtin/index-pack.c | 136 +++++++++++++++++++++++++++++++++++++++++++++------\n 1 file changed, 122 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/index-pack.c b/builtin/index-pack.c\nindex 9c1cfac..a5b69e4 100644\n--- a/builtin/index-pack.c\n+++ b/builtin/index-pack.c\n@@ -81,6 +81,7 @@ static int do_fsck_object;\n static int verbose;\n static int show_stat;\n static int check_self_contained_and_connected;\n+static int unpack;\n \n static struct progress *progress;\n \n@@ -93,6 +94,21 @@ static git_SHA_CTX input_ctx;\n static uint32_t input_crc32;\n static int input_fd, output_fd, pack_fd;\n \n+/*\n+ * When unpacking under --strict mode, objects whose reachability are\n+ * suspect are kept in core without getting written in the object\n+ * store.\n+ */\n+struct obj_buffer {\n+\tchar *buffer;\n+\tunsigned long size;\n+\tenum object_type type;\n+\tunsigned char sha1[20];\n+};\n+\n+struct obj_buffer *obj_buffers;\n+int nr_object_buffers;\n+\n #ifndef NO_PTHREADS\n \n static struct thread_local *thread_data;\n@@ -209,6 +225,23 @@ static unsigned check_object(struct object *obj)\n \treturn 0;\n }\n \n+static void write_cached_objects(void)\n+{\n+\tunsigned char parano_sha1[20];\n+\tint i;\n+\tfor (i = 0; i < nr_object_buffers; i++) {\n+\t\tstruct obj_buffer *obj_buf = obj_buffers + i;\n+\t\tif (write_sha1_file(obj_buf->buffer, obj_buf->size,\n+\t\t\t\t    typename(obj_buf->type), parano_sha1) < 0)\n+\t\t\tdie(_(\"failed to write object %s\"),\n+\t\t\t    sha1_to_hex(obj_buf->sha1));\n+\t\tif (hashcmp(obj_buf->sha1, parano_sha1))\n+\t\t\tdie(_(\"confused by unstable object source data for %s\"),\n+\t\t\t    sha1_to_hex(obj_buf->sha1));\n+\t\tfree(obj_buf->buffer);\n+\t}\n+}\n+\n static unsigned check_objects(void)\n {\n \tunsigned i, max, foreign_nr = 0;\n@@ -424,7 +457,18 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size,\n \t\tgit_SHA1_Update(&c, hdr, hdrlen);\n \t} else\n \t\tsha1 = NULL;\n-\tif (type == OBJ_BLOB && size > big_file_threshold)\n+\tif (type == OBJ_BLOB && size > big_file_threshold &&\n+\t    /*\n+\t     * In future we should use bulk-checkin instead of forcing\n+\t     * large blobs in core like this.\n+\t     *\n+\t     * If we do so, in --stdin mode, we might also want to\n+\t     * update flush() to do seek() instead of write() on large\n+\t     * blobs to reduce I/O and disk consumption (because we\n+\t     * need the temp pack with other objects at correct\n+\t     * position for delta resolution)\n+\t     */\n+\t    !unpack)\n \t\tbuf = fixed_buf;\n \telse\n \t\tbuf = xmalloc(size);\n@@ -700,17 +744,18 @@ static int check_collison(struct object_entry *entry)\n \treturn 0;\n }\n \n-static void sha1_object(const void *data, struct object_entry *obj_entry,\n-\t\t\tunsigned long size, enum object_type type,\n-\t\t\tconst unsigned char *sha1)\n+static void *sha1_object(void *data, struct object_entry *obj_entry,\n+\t\t\t unsigned long size, enum object_type type,\n+\t\t\t const unsigned char *sha1)\n {\n \tvoid *new_data = NULL;\n \tint collision_test_needed;\n+\tint queued = 0;\n \n \tassert(data || obj_entry);\n \n \tread_lock();\n-\tcollision_test_needed = has_sha1_file(sha1);\n+\tcollision_test_needed = unpack ? 0 : has_sha1_file(sha1);\n \tread_unlock();\n \n \tif (collision_test_needed && !data) {\n@@ -776,11 +821,36 @@ static void sha1_object(const void *data, struct object_entry *obj_entry,\n \t\t\t\tcommit->buffer = NULL;\n \t\t\t}\n \t\t\tobj->flags |= FLAG_CHECKED;\n+\t\t\tif (unpack) {\n+\t\t\t\tobj_buffers[nr_object_buffers].buffer = data;\n+\t\t\t\tobj_buffers[nr_object_buffers].size = size;\n+\t\t\t\tobj_buffers[nr_object_buffers].type = type;\n+\t\t\t\thashcpy(obj_buffers[nr_object_buffers].sha1, sha1);\n+\t\t\t\tnr_object_buffers++;\n+\t\t\t\tdata = NULL;\n+\t\t\t\tqueued = 1;\n+\t\t\t}\n \t\t}\n \t\tread_unlock();\n \t}\n \n+\tif (unpack && !queued) {\n+\t\tunsigned char parano_sha1[20];\n+\n+\t\tread_lock(); /* because write_sha1_file calls has_sha1_file */\n+\t\tif (write_sha1_file(data, size, typename(type),\n+\t\t\t\t    parano_sha1))\n+\t\t\tdie(_(\"failed to unpack object at offset %lu\"),\n+\t\t\t    obj_entry->idx.offset);\n+\t\tread_unlock();\n+\n+\t\tif (hashcmp(sha1, parano_sha1))\n+\t\t\tdie(_(\"confused by unstable object source data for %s\"),\n+\t\t\t    sha1_to_hex(sha1));\n+\t}\n+\n \tfree(new_data);\n+\treturn data == new_data ? NULL : data;\n }\n \n /*\n@@ -1021,7 +1091,8 @@ static void parse_pack_objects(unsigned char *sha1)\n \t\t\tobj->real_type = OBJ_BAD;\n \t\t\tnr_delays++;\n \t\t} else\n-\t\t\tsha1_object(data, NULL, obj->size, obj->type, obj->idx.sha1);\n+\t\t\tdata = sha1_object(data, NULL, obj->size,\n+\t\t\t\t\t   obj->type, obj->idx.sha1);\n \t\tfree(data);\n \t\tdisplay_progress(progress, i+1);\n \t}\n@@ -1188,6 +1259,18 @@ static struct object_entry *append_obj_to_pack(struct sha1file *f,\n \tunsigned long s = size;\n \tint n = 0;\n \tunsigned char c = (type << 4) | (s & 15);\n+\tif (unpack) {\n+\t\t/*\n+\t\t * Just enough info to make find_unresolved_deltas()\n+\t\t * happy. We do not need to actually append the object\n+\t\t * in unpack case.\n+\t\t */\n+\t\tobj[0].size = size;\n+\t\tobj[0].type = type;\n+\t\tobj[0].real_type = type;\n+\t\thashcpy(obj->idx.sha1, sha1);\n+\t\treturn obj;\n+\t}\n \ts >>= 4;\n \twhile (s) {\n \t\theader[n++] = c | 0x80;\n@@ -1277,7 +1360,11 @@ static void final(const char *final_pack_name, const char *curr_pack_name,\n \t\terr = close(output_fd);\n \t\tif (err)\n \t\t\tdie_errno(_(\"error while closing pack file\"));\n+\t\tif (unpack)\n+\t\t\tunlink(curr_pack_name);\n \t}\n+\tif (unpack)\n+\t\treturn;\n \n \tif (keep_msg) {\n \t\tint keep_fd, keep_msg_len = strlen(keep_msg);\n@@ -1489,14 +1576,14 @@ static void show_pack_info(int stat_only)\n int cmd_index_pack(int argc, const char **argv, const char *prefix)\n {\n \tint i, fix_thin_pack = 0, verify = 0, stat_only = 0;\n-\tconst char *curr_pack, *curr_index;\n+\tconst char *curr_pack, *curr_index = NULL;\n \tconst char *index_name = NULL, *pack_name = NULL;\n \tconst char *keep_name = NULL, *keep_msg = NULL;\n \tchar *index_name_buf = NULL, *keep_name_buf = NULL;\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+\tint unpack_limit = 0;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage(index_pack_usage);\n@@ -1574,6 +1661,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \t\t\t\t\topts.off32_limit = strtoul(c+1, &c, 0);\n \t\t\t\tif (*c || opts.off32_limit & 0x80000000)\n \t\t\t\t\tdie(_(\"bad %s\"), arg);\n+\t\t\t} else if (!prefixcmp(arg, \"--unpack-limit=\")) {\n+\t\t\t\tchar *end;\n+\t\t\t\tunpack_limit = strtoul(arg+15, &end, 0);\n+\t\t\t\tif (!arg[15] || *end || unpack_limit < 0)\n+\t\t\t\t\tusage(index_pack_usage);\n \t\t\t} else\n \t\t\t\tusage(index_pack_usage);\n \t\t\tcontinue;\n@@ -1628,23 +1720,39 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)\n \n \tcurr_pack = open_pack_file(pack_name);\n \tparse_pack_header();\n+\n+\tif (nr_objects < unpack_limit) {\n+\t\tunpack = 1;\n+\t\tif (strict)\n+\t\t\tobj_buffers = xcalloc(nr_objects, sizeof(*obj_buffers));\n+\t}\n+\n \tobjects = xcalloc(nr_objects + 1, sizeof(struct object_entry));\n \tdeltas = xcalloc(nr_objects, sizeof(struct delta_entry));\n \tparse_pack_objects(pack_sha1);\n \tresolve_deltas();\n \tconclude_pack(fix_thin_pack, curr_pack, pack_sha1);\n \tfree(deltas);\n-\tif (strict)\n+\tif (strict) {\n \t\tforeign_nr = check_objects();\n+\t\tif (unpack) {\n+\t\t\twrite_cached_objects();\n+\t\t\tfree(obj_buffers);\n+\t\t}\n+\t}\n \n \tif (show_stat)\n \t\tshow_pack_info(stat_only);\n \n-\tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n-\tfor (i = 0; i < nr_objects; i++)\n-\t\tidx_objects[i] = &objects[i].idx;\n-\tcurr_index = write_idx_file(index_name, idx_objects, nr_objects, &opts, pack_sha1);\n-\tfree(idx_objects);\n+\tif (!unpack) {\n+\t\tstruct pack_idx_entry **idx_objects;\n+\t\tidx_objects = xmalloc((nr_objects) * sizeof(struct pack_idx_entry *));\n+\t\tfor (i = 0; i < nr_objects; i++)\n+\t\t\tidx_objects[i] = &objects[i].idx;\n+\t\tcurr_index = write_idx_file(index_name, idx_objects,\n+\t\t\t\t\t    nr_objects, &opts, pack_sha1);\n+\t\tfree(idx_objects);\n+\t}\n \n \tif (!verify)\n \t\tfinal(pack_name, curr_pack,\n-- \n1.8.2.83.gc99314b\n"},{"id":"226900","messageId":"1378428363-14086-2-git-send-email-pclouds@gmail.com","threadId":"34826","inReplyTo":"1378428363-14086-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 2/3] fetch-pack: use index-pack --unpack-limit instead of unpack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-06T00:46:02Z","receivedAt":"2013-09-06T00:46:02Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Current code peeks into the transfered pack's header, if the number of\nobjects is under a limit, unpack-objects is called to handle the rest,\notherwise index-pack is. This patch makes index-pack handle both\ncases. After the next patch, unpack-objects will no longer be used. It\nmay be removed in future.\n\nNow we only have two code paths to maintain regarding pack reading\n(sha1_file.c and index-pack.c). When .pack v4 comes, we don't need to\nduplicate work in index-pack and unpack-objects.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>\n---\n fetch-pack.c | 86 +++++++++++++++++++++++++-----------------------------------\n 1 file changed, 35 insertions(+), 51 deletions(-)\n\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f5d99c1..44d029f 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -682,16 +682,16 @@ static int sideband_demux(int in, int out, void *data)\n }\n \n static int get_pack(struct fetch_pack_args *args,\n-\t\t    int xd[2], char **pack_lockfile)\n+\t\t    int xd[2], char **pack_lockfile_p)\n {\n \tstruct async demux;\n-\tconst char *argv[22];\n+\tconst char *argv[23];\n \tchar keep_arg[256];\n-\tchar hdr_arg[256];\n+\tchar unpack_limit_arg[256];\n \tconst char **av;\n-\tint do_keep = args->keep_pack;\n \tstruct child_process cmd;\n \tint ret;\n+\tchar *pack_lockfile;\n \n \tmemset(&demux, 0, sizeof(demux));\n \tif (use_sideband) {\n@@ -711,54 +711,33 @@ static int get_pack(struct fetch_pack_args *args,\n \n \tmemset(&cmd, 0, sizeof(cmd));\n \tcmd.argv = argv;\n+\tcmd.out = -1;\n \tav = argv;\n-\t*hdr_arg = 0;\n-\tif (!args->keep_pack && unpack_limit) {\n-\t\tstruct pack_header header;\n-\n-\t\tif (read_pack_header(demux.out, &header))\n-\t\t\tdie(\"protocol error: bad pack header\");\n-\t\tsnprintf(hdr_arg, sizeof(hdr_arg),\n-\t\t\t \"--pack_header=%\"PRIu32\",%\"PRIu32,\n-\t\t\t ntohl(header.hdr_version), ntohl(header.hdr_entries));\n-\t\tif (ntohl(header.hdr_entries) < unpack_limit)\n-\t\t\tdo_keep = 0;\n-\t\telse\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-\t\t*av++ = \"index-pack\";\n-\t\t*av++ = \"--stdin\";\n-\t\tif (!args->quiet && !args->no_progress)\n-\t\t\t*av++ = \"-v\";\n-\t\tif (args->use_thin_pack)\n-\t\t\t*av++ = \"--fix-thin\";\n-\t\tif (args->lock_pack || unpack_limit) {\n-\t\t\tint s = sprintf(keep_arg,\n-\t\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on \", (uintmax_t) getpid());\n-\t\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\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+\t*av++ = \"index-pack\";\n+\t*av++ = \"--stdin\";\n+\tif (!args->quiet && !args->no_progress)\n+\t\t*av++ = \"-v\";\n+\tif (args->use_thin_pack)\n+\t\t*av++ = \"--fix-thin\";\n+\tif (args->lock_pack || unpack_limit) {\n+\t\tint s = sprintf(keep_arg,\n+\t\t\t\t\"--keep=fetch-pack %\"PRIuMAX \" on \", (uintmax_t) getpid());\n+\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n+\t\t\tstrcpy(keep_arg + s, \"localhost\");\n+\t\t*av++ = keep_arg;\n+\t}\n+\tif (!args->keep_pack) {\n+\t\tsprintf(unpack_limit_arg, \"--unpack-limit=%u\", unpack_limit);\n+\t\t*av++ = unpack_limit_arg;\n+\t}\n+\tif (args->check_self_contained_and_connected)\n+\t\t*av++ = \"--check-self-contained-and-connected\";\n \tif (fetch_fsck_objects >= 0\n \t    ? fetch_fsck_objects\n \t    : transfer_fsck_objects >= 0\n@@ -771,20 +750,26 @@ static int get_pack(struct fetch_pack_args *args,\n \tcmd.git_cmd = 1;\n \tif (start_command(&cmd))\n \t\tdie(\"fetch-pack: unable to fork off %s\", argv[0]);\n-\tif (do_keep && pack_lockfile) {\n-\t\t*pack_lockfile = index_pack_lockfile(cmd.out);\n-\t\tclose(cmd.out);\n-\t}\n-\n+\tpack_lockfile = index_pack_lockfile(cmd.out);\n+\tclose(cmd.out);\n \tret = finish_command(&cmd);\n+\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+\n \tif (use_sideband && finish_async(&demux))\n \t\tdie(\"error in sideband demultiplexer\");\n+\n+\tif (pack_lockfile)\n+\t\treprepare_packed_git();\n+\n+\tif (pack_lockfile_p)\n+\t\t*pack_lockfile_p = pack_lockfile;\n+\n \treturn 0;\n }\n \n@@ -997,6 +982,5 @@ struct ref *fetch_pack(struct fetch_pack_args *args,\n \t\t\tcommit_lock_file(&shallow_lock);\n \t}\n \n-\treprepare_packed_git();\n \treturn ref_cpy;\n }\n-- \n1.8.2.83.gc99314b\n"},{"id":"226901","messageId":"1378428363-14086-3-git-send-email-pclouds@gmail.com","threadId":"34826","inReplyTo":"1378428363-14086-1-git-send-email-pclouds@gmail.com","subject":"[PATCH v2 3/3] receive-pack: use index-pack --unpack-limit instead of unpack-objects","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-06T00:46:03Z","receivedAt":"2013-09-06T00:46:03Z","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 <pclouds@gmail.com>\n---\n builtin/receive-pack.c | 122 ++++++++++++++-----------------------------------\n 1 file changed, 34 insertions(+), 88 deletions(-)\n\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex e3eb5fc..0a84a61 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -792,105 +792,51 @@ static struct command *read_head_info(void)\n \treturn commands;\n }\n \n-static const char *parse_pack_header(struct pack_header *hdr)\n-{\n-\tswitch (read_pack_header(0, hdr)) {\n-\tcase PH_ERROR_EOF:\n-\t\treturn \"eof before pack header was fully read\";\n-\n-\tcase PH_ERROR_PACK_SIGNATURE:\n-\t\treturn \"protocol error (pack signature mismatch detected)\";\n-\n-\tcase PH_ERROR_PROTOCOL:\n-\t\treturn \"protocol error (pack version unsupported)\";\n-\n-\tdefault:\n-\t\treturn \"unknown error in parse_pack_header\";\n-\n-\tcase 0:\n-\t\treturn NULL;\n-\t}\n-}\n-\n static const char *pack_lockfile;\n \n static const char *unpack(int err_fd)\n {\n-\tstruct pack_header hdr;\n-\tconst char *hdr_err;\n-\tchar hdr_arg[38];\n \tint fsck_objects = (receive_fsck_objects >= 0\n \t\t\t    ? receive_fsck_objects\n \t\t\t    : transfer_fsck_objects >= 0\n \t\t\t    ? transfer_fsck_objects\n \t\t\t    : 0);\n \n-\thdr_err = parse_pack_header(&hdr);\n-\tif (hdr_err) {\n-\t\tif (err_fd > 0)\n-\t\t\tclose(err_fd);\n-\t\treturn hdr_err;\n-\t}\n-\tsnprintf(hdr_arg, sizeof(hdr_arg),\n-\t\t\t\"--pack_header=%\"PRIu32\",%\"PRIu32,\n-\t\t\tntohl(hdr.hdr_version), ntohl(hdr.hdr_entries));\n-\n-\tif (ntohl(hdr.hdr_entries) < unpack_limit) {\n-\t\tint code, i = 0;\n-\t\tstruct child_process child;\n-\t\tconst char *unpacker[5];\n-\t\tunpacker[i++] = \"unpack-objects\";\n-\t\tif (quiet)\n-\t\t\tunpacker[i++] = \"-q\";\n-\t\tif (fsck_objects)\n-\t\t\tunpacker[i++] = \"--strict\";\n-\t\tunpacker[i++] = hdr_arg;\n-\t\tunpacker[i++] = NULL;\n-\t\tmemset(&child, 0, sizeof(child));\n-\t\tchild.argv = unpacker;\n-\t\tchild.no_stdout = 1;\n-\t\tchild.err = err_fd;\n-\t\tchild.git_cmd = 1;\n-\t\tcode = run_command(&child);\n-\t\tif (!code)\n-\t\t\treturn NULL;\n-\t\treturn \"unpack-objects abnormal exit\";\n-\t} else {\n-\t\tconst char *keeper[7];\n-\t\tint s, status, i = 0;\n-\t\tchar keep_arg[256];\n-\t\tstruct child_process ip;\n-\n-\t\ts = sprintf(keep_arg, \"--keep=receive-pack %\"PRIuMAX\" on \", (uintmax_t) getpid());\n-\t\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n-\t\t\tstrcpy(keep_arg + s, \"localhost\");\n-\n-\t\tkeeper[i++] = \"index-pack\";\n-\t\tkeeper[i++] = \"--stdin\";\n-\t\tif (fsck_objects)\n-\t\t\tkeeper[i++] = \"--strict\";\n-\t\tkeeper[i++] = \"--fix-thin\";\n-\t\tkeeper[i++] = hdr_arg;\n-\t\tkeeper[i++] = keep_arg;\n-\t\tkeeper[i++] = NULL;\n-\t\tmemset(&ip, 0, sizeof(ip));\n-\t\tip.argv = keeper;\n-\t\tip.out = -1;\n-\t\tip.err = err_fd;\n-\t\tip.git_cmd = 1;\n-\t\tstatus = start_command(&ip);\n-\t\tif (status) {\n-\t\t\treturn \"index-pack fork failed\";\n-\t\t}\n-\t\tpack_lockfile = index_pack_lockfile(ip.out);\n-\t\tclose(ip.out);\n-\t\tstatus = finish_command(&ip);\n-\t\tif (!status) {\n-\t\t\treprepare_packed_git();\n-\t\t\treturn NULL;\n-\t\t}\n+\tconst char *keeper[7];\n+\tint s, status, i = 0;\n+\tchar keep_arg[256];\n+\tchar unpack_limit_arg[256];\n+\tstruct child_process ip;\n+\n+\ts = sprintf(keep_arg, \"--keep=receive-pack %\"PRIuMAX\" on \", (uintmax_t) getpid());\n+\tif (gethostname(keep_arg + s, sizeof(keep_arg) - s))\n+\t\tstrcpy(keep_arg + s, \"localhost\");\n+\tsprintf(unpack_limit_arg, \"--unpack-limit=%u\", unpack_limit);\n+\n+\tkeeper[i++] = \"index-pack\";\n+\tkeeper[i++] = \"--stdin\";\n+\tif (fsck_objects)\n+\t\tkeeper[i++] = \"--strict\";\n+\tkeeper[i++] = \"--fix-thin\";\n+\tkeeper[i++] = keep_arg;\n+\tkeeper[i++] = unpack_limit_arg;\n+\tkeeper[i++] = NULL;\n+\tmemset(&ip, 0, sizeof(ip));\n+\tip.argv = keeper;\n+\tip.out = -1;\n+\tip.err = err_fd;\n+\tip.git_cmd = 1;\n+\tstatus = start_command(&ip);\n+\tif (status)\n+\t\treturn \"index-pack fork failed\";\n+\tpack_lockfile = index_pack_lockfile(ip.out);\n+\tclose(ip.out);\n+\tstatus = finish_command(&ip);\n+\tif (status)\n \t\treturn \"index-pack abnormal exit\";\n-\t}\n+\tif (pack_lockfile)\n+\t\treprepare_packed_git();\n+\treturn NULL;\n }\n \n static const char *unpack_with_sideband(void)\n-- \n1.8.2.83.gc99314b\n"},{"id":"227065","messageId":"20130908044417.GD14019@sigill.intra.peff.net","threadId":"34826","inReplyTo":"1378428363-14086-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T04:44:17Z","receivedAt":"2013-09-08T04:44:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2013 at 07:46:01AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> ---\n>  I had something that could unpack without writing to temp pack file\n>  but I scraped it and chose this way because it follows closely how\n>  index-pack works. It's a good thing imo because .pack v4 is coming\n>  and I don't know how v4 may impact this unpack code path. Once things\n>  are settled, we can revisit and open a separate code path if it's\n>  still a good idea.\n\n>From a cursory read, this seems fine. If it were done in complete\nisolation, I'd say it was a slight regression, just because we are doing\nmore I/O for the unpack case, and it is not really saving us any code\n(it is not like we can throw away unpack-objects, as I think we would\nwant to keep it as a last resort for getting data out of malformed or\notherwise non-indexable packs).\n\nBut I can also see it making pack v4 handling easier. So it would make\nsense to me to put it at the start of a series adding pack v4 indexing.\nBy the end of the series you would be able to see the benefits of the\nreduced code complexity. Until then, it is a \"probably this will help\nlater\" change.\n\n-Peff\n"},{"id":"227066","messageId":"20130908044531.GE14019@sigill.intra.peff.net","threadId":"34826","inReplyTo":"1378428363-14086-2-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH v2 2/3] fetch-pack: use index-pack --unpack-limit instead of unpack-objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T04:45:32Z","receivedAt":"2013-09-08T04:45:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 06, 2013 at 07:46:02AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n>  static int get_pack(struct fetch_pack_args *args,\n> -\t\t    int xd[2], char **pack_lockfile)\n> +\t\t    int xd[2], char **pack_lockfile_p)\n>  {\n>  \tstruct async demux;\n> -\tconst char *argv[22];\n> +\tconst char *argv[23];\n>  \tchar keep_arg[256];\n> -\tchar hdr_arg[256];\n> +\tchar unpack_limit_arg[256];\n\nPerhaps it is time to convert this to use argv_array? I think you could\neven get rid of the fixed-size buffers, too, with argv_array_pushf().\n\n-Peff\n"},{"id":"227079","messageId":"CACsJy8DeJnwJ2hxbKs+ks3Rp6h+uedFA+qt8JTTwyen8sF7XMw@mail.gmail.com","threadId":"34826","inReplyTo":"20130908044417.GD14019@sigill.intra.peff.net","subject":"Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2013-09-08T06:28:47Z","receivedAt":"2013-09-08T06:28:47Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sun, Sep 8, 2013 at 11:44 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Sep 06, 2013 at 07:46:01AM +0700, Nguyen Thai Ngoc Duy wrote:\n>\n>> ---\n>>  I had something that could unpack without writing to temp pack file\n>>  but I scraped it and chose this way because it follows closely how\n>>  index-pack works. It's a good thing imo because .pack v4 is coming\n>>  and I don't know how v4 may impact this unpack code path. Once things\n>>  are settled, we can revisit and open a separate code path if it's\n>>  still a good idea.\n>\n> From a cursory read, this seems fine. If it were done in complete\n> isolation, I'd say it was a slight regression, just because we are doing\n> more I/O for the unpack case, and it is not really saving us any code\n> (it is not like we can throw away unpack-objects, as I think we would\n> want to keep it as a last resort for getting data out of malformed or\n> otherwise non-indexable packs).\n\nI can see unpack-objects is more tolerable on corrupt/incomplete\npacks. If index-pack finds something wrong, it aborts the whole\nprocess. I think we can make index-pack stop at the first bad object,\nadjust nr_objects, and try to recover as much as possible out of the\ngood part. Any other reasons to keep unpack-objects (because I still\nwant to kill it)?\n\n> But I can also see it making pack v4 handling easier. So it would make\n> sense to me to put it at the start of a series adding pack v4 indexing.\n> By the end of the series you would be able to see the benefits of the\n> reduced code complexity. Until then, it is a \"probably this will help\n> later\" change.\n\nAgreed.\n-- \nDuy\n"},{"id":"227080","messageId":"20130908063533.GA22949@sigill.intra.peff.net","threadId":"34826","inReplyTo":"CACsJy8DeJnwJ2hxbKs+ks3Rp6h+uedFA+qt8JTTwyen8sF7XMw@mail.gmail.com","subject":"Re: [PATCH v2 1/3] index-pack: add --unpack-limit to unpack objects","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-09-08T06:35:34Z","receivedAt":"2013-09-08T06:35:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 08, 2013 at 01:28:47PM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > From a cursory read, this seems fine. If it were done in complete\n> > isolation, I'd say it was a slight regression, just because we are doing\n> > more I/O for the unpack case, and it is not really saving us any code\n> > (it is not like we can throw away unpack-objects, as I think we would\n> > want to keep it as a last resort for getting data out of malformed or\n> > otherwise non-indexable packs).\n> \n> I can see unpack-objects is more tolerable on corrupt/incomplete\n> packs. If index-pack finds something wrong, it aborts the whole\n> process. I think we can make index-pack stop at the first bad object,\n> adjust nr_objects, and try to recover as much as possible out of the\n> good part. Any other reasons to keep unpack-objects (because I still\n> want to kill it)?\n\nNo, I don't think there is another reason. The command name may hang\naround for historical reasons, but we can always make it an alias for\n\"index-pack --unpack-limit=0\" or whatever.\n\nI do not think index-pack even needs to be particularly clever about\ntrying to recover. It is mainly that we may do some extra sanity checks\nwhen writing the index (e.g., the recent discussion on avoiding\nduplicates in the index), that do not even come up when simply exploding\nthe pack in a linear fashion. But I don't think there is any fundamental\nreason why index-pack could not learn to be as robust when operating in\nunpack mode.\n\nAs an aside, you cannot just drop the nr_objects that index-pack's\ngenerated index says it contains. Packfile readers double-check that the\n.idx and .pack agree on the number of objects (I tried it as a method\nfor working around duplicate entries :) ).\n\n-Peff\n"}]}