{"thread":{"id":"42120","subject":"[PATCH] fast-import: implement --min-pack-size parameter","startedAt":"2016-04-23T02:42:25Z","lastAt":"2016-04-25T21:17:28Z","messageCount":6,"participants":["Eric Wong","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"284218","messageId":"20160423024225.GA4293@dcvr.yhbt.net","threadId":"42120","inReplyTo":null,"subject":"[PATCH] fast-import: implement --min-pack-size parameter","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-23T02:42:25Z","receivedAt":"2016-04-23T02:42:25Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"With many incremental imports, small packs become highly\ninefficient due to the need to readdir scan and load many\nindices to locate even a single object.  Frequent repacking and\nconsolidation may be prohibitively expensive in terms of disk\nI/O, especially in large repositories where the initial packs\nwere aggressively optimized and marked with .keep files.\n\nIn those cases, users may be better served with loose objects\nand relying on \"git gc --auto\".\n\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n  There should be a matching config file directive, but I'm\n  not sure how/if it should affect other commands.  So I'm\n  not sure if it should be \"pack.packSizeMin\" or\n  \"fastimport.packSizeMin\" or something else.\n\n  To further reduce disk I/O, the fsync_or_die call in\n  fixup_pack_header_footer could probably be moved out of that\n  function and become the fphf caller's responsibility.\n\n Documentation/git-fast-import.txt   |  9 ++++++++\n fast-import.c                       | 30 ++++++++++++++++++++++++++\n t/t9302-fast-import-min-packsize.sh | 42 +++++++++++++++++++++++++++++++++++++\n 3 files changed, 81 insertions(+)\n create mode 100755 t/t9302-fast-import-min-packsize.sh\n\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 66910aa..8c0ac94 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -136,6 +136,15 @@ Performance and Compression Tuning\n \tMaximum size of each output packfile.\n \tThe default is unlimited.\n \n+--min-pack-size=<n>::\n+\tMininum size of an output packfile, packfiles smaller\n+\tthan this threshold are unpacked into loose objects and\n+\tthe pack is discarded.  This is useful when performing\n+\tsmall, incremental imports as loose objects and relying\n+\ton `git gc --auto` may be more efficient than generating\n+\tmany tiny packs.\n+\tThe default is to always preserve the pack and never\n+\tgenerate loose objects.\n \n Performance\n -----------\ndiff --git a/fast-import.c b/fast-import.c\nindex 9fc7093..a00bee5 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -166,6 +166,7 @@ Format of STDIN stream:\n #include \"quote.h\"\n #include \"exec_cmd.h\"\n #include \"dir.h\"\n+#include \"run-command.h\"\n \n #define PACK_ID_BITS 16\n #define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)\n@@ -282,6 +283,7 @@ struct recent_command {\n /* Configured limits on output */\n static unsigned long max_depth = 10;\n static off_t max_packsize;\n+static off_t min_packsize;\n static int force_update;\n static int pack_compression_level = Z_DEFAULT_COMPRESSION;\n static int pack_compression_seen;\n@@ -950,6 +952,22 @@ static void unkeep_all_packs(void)\n \t}\n }\n \n+static int loosen_small_pack(const struct packed_git *p)\n+{\n+\tstruct child_process unpack = CHILD_PROCESS_INIT;\n+\n+\tif (lseek(p->pack_fd, 0, SEEK_SET) < 0)\n+\t\tdie_errno(\"Failed seeking to start of '%s'\", p->pack_name);\n+\n+\tunpack.in = p->pack_fd;\n+\tunpack.git_cmd = 1;\n+\tunpack.stdout_to_stderr = 1;\n+\targv_array_push(&unpack.args, \"unpack-objects\");\n+\targv_array_push(&unpack.args, \"-q\");\n+\n+\treturn run_command(&unpack);\n+}\n+\n static void end_packfile(void)\n {\n \tstatic int running;\n@@ -972,6 +990,12 @@ static void end_packfile(void)\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n \t\t\t\t    pack_data->pack_name, object_count,\n \t\t\t\t    cur_pack_sha1, pack_size);\n+\n+\t\tif (pack_size < min_packsize) {\n+\t\t\tif (loosen_small_pack(pack_data) == 0)\n+\t\t\t\tgoto discard_pack;\n+\t\t}\n+\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n \n@@ -1002,6 +1026,7 @@ static void end_packfile(void)\n \t\tpack_id++;\n \t}\n \telse {\n+discard_pack:\n \t\tclose(pack_data->pack_fd);\n \t\tunlink_or_warn(pack_data->pack_name);\n \t}\n@@ -3237,6 +3262,11 @@ static int parse_one_option(const char *option)\n \t\t\tv = 1024 * 1024;\n \t\t}\n \t\tmax_packsize = v;\n+\t} else if (skip_prefix(option, \"min-pack-size=\", &option)) {\n+\t\tunsigned long v;\n+\t\tif (!git_parse_ulong(option, &v))\n+\t\t\treturn 0;\n+\t\tmin_packsize = v;\n \t} else if (skip_prefix(option, \"big-file-threshold=\", &option)) {\n \t\tunsigned long v;\n \t\tif (!git_parse_ulong(option, &v))\ndiff --git a/t/t9302-fast-import-min-packsize.sh b/t/t9302-fast-import-min-packsize.sh\nnew file mode 100755\nindex 0000000..7dcdccc\n--- /dev/null\n+++ b/t/t9302-fast-import-min-packsize.sh\n@@ -0,0 +1,42 @@\n+#!/bin/sh\n+test_description='test git fast-import min-packsize'\n+. ./test-lib.sh\n+\n+test_expect_success 'create loose objects on import' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tinitial\n+\tCOMMIT\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit fast-import --done --min-pack-size=1g <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 0\n+'\n+\n+test_expect_success 'bigger packs are preserved' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tincremental should create a pack\n+\tCOMMIT\n+\tfrom refs/heads/master^0\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit fast-import --done --min-pack-size=10 <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 2\n+'\n+\n+test_done\n-- \nEW\n"},{"id":"284219","messageId":"20160423031313.GA15043@sigill.intra.peff.net","threadId":"42120","inReplyTo":"20160423024225.GA4293@dcvr.yhbt.net","subject":"Re: [PATCH] fast-import: implement --min-pack-size parameter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-04-23T03:13:13Z","receivedAt":"2016-04-23T03:13:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 23, 2016 at 02:42:25AM +0000, Eric Wong wrote:\n\n> With many incremental imports, small packs become highly\n> inefficient due to the need to readdir scan and load many\n> indices to locate even a single object.  Frequent repacking and\n> consolidation may be prohibitively expensive in terms of disk\n> I/O, especially in large repositories where the initial packs\n> were aggressively optimized and marked with .keep files.\n> \n> In those cases, users may be better served with loose objects\n> and relying on \"git gc --auto\".\n> \n> Signed-off-by: Eric Wong <normalperson@yhbt.net>\n> ---\n>   There should be a matching config file directive, but I'm\n>   not sure how/if it should affect other commands.  So I'm\n>   not sure if it should be \"pack.packSizeMin\" or\n>   \"fastimport.packSizeMin\" or something else.\n\nThis same concept exists for pushing/fetching, but there we measure it\nnot in bytes but by the number of objects. Which is probably a better\nmeasure. A single 10MB blob is better as a loose object than as a pack,\nbut a thousand 10KB blobs should be a pack.\n\nThere we have fetch.unpackLimit and receive.unpackLimit for the two\noperations, plus transfer.unpackLimit to control both of them. This\ndoesn't necessarily need to be tied to that config, but you could\ncertainly consider it in the same boat. It's a way of transferring a\nload of objects into the repository.\n\nSo it would make some sense to me to have fastimport.unpackLimit,\nfalling back to transfer.unpackLimit.\n\n-Peff\n"},{"id":"284234","messageId":"20160424043223.GA27609@dcvr.yhbt.net","threadId":"42120","inReplyTo":"20160423031313.GA15043@sigill.intra.peff.net","subject":"[PATCH v2] fast-import: implement unpack limit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-24T04:32:23Z","receivedAt":"2016-04-24T04:32:23Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Jeff King <peff@peff.net> wrote:\n> There we have fetch.unpackLimit and receive.unpackLimit for the two\n> operations, plus transfer.unpackLimit to control both of them. This\n> doesn't necessarily need to be tied to that config, but you could\n> certainly consider it in the same boat. It's a way of transferring a\n> load of objects into the repository.\n\nAh, thanks, I've always overlooked those :x  But it makes sense\nto me this way.\n\n> So it would make some sense to me to have fastimport.unpackLimit,\n> falling back to transfer.unpackLimit.\n\nDone below, purely as a config option with no CLI switch.\nI'm hoping the change in default behavior is acceptable\nto match behavior of the other \"transfer\" mechanisms.\nI needed to adjust the t9300 test, though.\n\n-----------------------------8<-----------------------------\nSubject: [PATCH] fast-import: implement unpack limit\n\nWith many incremental imports, small packs become highly\ninefficient due to the need to readdir scan and load many\nindices to locate even a single object.  Frequent repacking and\nconsolidation may be prohibitively expensive in terms of disk\nI/O, especially in large repositories where the initial packs\nwere aggressively optimized and marked with .keep files.\n\nIn those cases, users may be better served with loose objects\nand relying on \"git gc --auto\".\n\nThis changes the default behavior of fast-import for small\nimports found in test cases, so adjustments to t9300 were\nnecessary.\n\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n Documentation/config.txt            |  9 +++++++\n Documentation/git-fast-import.txt   |  2 ++\n fast-import.c                       | 31 ++++++++++++++++++++++++\n t/t9300-fast-import.sh              |  2 ++\n t/t9302-fast-import-unpack-limit.sh | 48 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 92 insertions(+)\n create mode 100755 t/t9302-fast-import-unpack-limit.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 42d2b50..3d8bc97 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1154,6 +1154,15 @@ difftool.<tool>.cmd::\n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n \n+fastimport.unpackLimit::\n+\tIf the number of objects imported by linkgit:git-fast-import[1]\n+\tis below this limit, then the objects will be unpacked into\n+\tloose object files.  However if the number of imported objects\n+\tequals or exceeds this limit then the pack will be stored as a\n+\tpack.  Storing the pack from a fast-import can make the import\n+\toperation complete faster, especially on slow filesystems.  If\n+\tnot set, the value of `transfer.unpackLimit` is used instead.\n+\n fetch.recurseSubmodules::\n \tThis option can be either set to a boolean value or to 'on-demand'.\n \tSetting it to a boolean changes the behavior of fetch and pull to\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 66910aa..644df99 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -136,6 +136,8 @@ Performance and Compression Tuning\n \tMaximum size of each output packfile.\n \tThe default is unlimited.\n \n+fastimport.unpackLimit::\n+\tSee linkgit:git-config[1]\n \n Performance\n -----------\ndiff --git a/fast-import.c b/fast-import.c\nindex 9fc7093..381d3a0 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -166,6 +166,7 @@ Format of STDIN stream:\n #include \"quote.h\"\n #include \"exec_cmd.h\"\n #include \"dir.h\"\n+#include \"run-command.h\"\n \n #define PACK_ID_BITS 16\n #define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)\n@@ -282,6 +283,7 @@ struct recent_command {\n /* Configured limits on output */\n static unsigned long max_depth = 10;\n static off_t max_packsize;\n+static int unpack_limit = 100;\n static int force_update;\n static int pack_compression_level = Z_DEFAULT_COMPRESSION;\n static int pack_compression_seen;\n@@ -950,6 +952,22 @@ static void unkeep_all_packs(void)\n \t}\n }\n \n+static int loosen_small_pack(const struct packed_git *p)\n+{\n+\tstruct child_process unpack = CHILD_PROCESS_INIT;\n+\n+\tif (lseek(p->pack_fd, 0, SEEK_SET) < 0)\n+\t\tdie_errno(\"Failed seeking to start of '%s'\", p->pack_name);\n+\n+\tunpack.in = p->pack_fd;\n+\tunpack.git_cmd = 1;\n+\tunpack.stdout_to_stderr = 1;\n+\targv_array_push(&unpack.args, \"unpack-objects\");\n+\targv_array_push(&unpack.args, \"-q\");\n+\n+\treturn run_command(&unpack);\n+}\n+\n static void end_packfile(void)\n {\n \tstatic int running;\n@@ -972,6 +990,12 @@ static void end_packfile(void)\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n \t\t\t\t    pack_data->pack_name, object_count,\n \t\t\t\t    cur_pack_sha1, pack_size);\n+\n+\t\tif (object_count <= unpack_limit) {\n+\t\t\tif (loosen_small_pack(pack_data) == 0)\n+\t\t\t\tgoto discard_pack;\n+\t\t}\n+\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n \n@@ -1002,6 +1026,7 @@ static void end_packfile(void)\n \t\tpack_id++;\n \t}\n \telse {\n+discard_pack:\n \t\tclose(pack_data->pack_fd);\n \t\tunlink_or_warn(pack_data->pack_name);\n \t}\n@@ -3317,6 +3342,7 @@ static void parse_option(const char *option)\n static void git_pack_config(void)\n {\n \tint indexversion_value;\n+\tint limit;\n \tunsigned long packsizelimit_value;\n \n \tif (!git_config_get_ulong(\"pack.depth\", &max_depth)) {\n@@ -3341,6 +3367,11 @@ static void git_pack_config(void)\n \tif (!git_config_get_ulong(\"pack.packsizelimit\", &packsizelimit_value))\n \t\tmax_packsize = packsizelimit_value;\n \n+\tif (!git_config_get_int(\"fastimport.unpacklimit\", &limit))\n+\t\tunpack_limit = limit;\n+\telse if (!git_config_get_int(\"transfer.unpacklimit\", &limit))\n+\t\tunpack_limit = limit;\n+\n \tgit_config(git_default_config, NULL);\n }\n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 25bb60b..e6a2b8a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -52,6 +52,7 @@ echo \"$@\"'\n ###\n \n test_expect_success 'empty stream succeeds' '\n+\tgit config fastimport.unpackLimit 0 &&\n \tgit fast-import </dev/null\n '\n \n@@ -2675,6 +2676,7 @@ test_expect_success 'R: blob bigger than threshold' '\n \techo >>input &&\n \n \ttest_create_repo R &&\n+\tgit --git-dir=R/.git config fastimport.unpackLimit 0 &&\n \tgit --git-dir=R/.git fast-import --big-file-threshold=1 <input\n '\n \ndiff --git a/t/t9302-fast-import-unpack-limit.sh b/t/t9302-fast-import-unpack-limit.sh\nnew file mode 100755\nindex 0000000..0f686d2\n--- /dev/null\n+++ b/t/t9302-fast-import-unpack-limit.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+test_description='test git fast-import unpack limit'\n+. ./test-lib.sh\n+\n+test_expect_success 'create loose objects on import' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tinitial\n+\tCOMMIT\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit -c fastimport.unpackLimit=2 fast-import --done <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 0\n+'\n+\n+test_expect_success 'bigger packs are preserved' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tincremental should create a pack\n+\tCOMMIT\n+\tfrom refs/heads/master^0\n+\n+\tcommit refs/heads/branch\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tbranch\n+\tCOMMIT\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit -c fastimport.unpackLimit=2 fast-import --done <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 2\n+'\n+\n+test_done\n-- \nEW\n"},{"id":"284355","messageId":"xmqqpoten7db.fsf@gitster.mtv.corp.google.com","threadId":"42120","inReplyTo":"20160424043223.GA27609@dcvr.yhbt.net","subject":"Re: [PATCH v2] fast-import: implement unpack limit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-24T19:18:08Z","receivedAt":"2016-04-24T19:18:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong <normalperson@yhbt.net> writes:\n\n> +static int loosen_small_pack(const struct packed_git *p)\n> +{\n> +\tstruct child_process unpack = CHILD_PROCESS_INIT;\n> +\n> +\tif (lseek(p->pack_fd, 0, SEEK_SET) < 0)\n> +\t\tdie_errno(\"Failed seeking to start of '%s'\", p->pack_name);\n> +\n> +\tunpack.in = p->pack_fd;\n> +\tunpack.git_cmd = 1;\n> +\tunpack.stdout_to_stderr = 1;\n> +\targv_array_push(&unpack.args, \"unpack-objects\");\n> +\targv_array_push(&unpack.args, \"-q\");\n> +\n> +\treturn run_command(&unpack);\n> +}\n\nSo you have fd open to a *.pack file you have been writing, you are\ngoing to close and discard it after you return from here, so you\njust seek the fd to the beginning and hand it to unpack-objects.\n\nAnd this works if you haven't finalized the *.pack file with the\ncorresponding *.idx (otherwise unpack-objects would silently ignore\nobjects found in the *.idx file); and obviously you haven't and you\nare not going to create *.idx file for this.\n\nLooks good.  I haven't thought if \"-q\" is appropriate or not though.\n\n> @@ -972,6 +990,12 @@ static void end_packfile(void)\n>  \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n>  \t\t\t\t    pack_data->pack_name, object_count,\n>  \t\t\t\t    cur_pack_sha1, pack_size);\n> +\n> +\t\tif (object_count <= unpack_limit) {\n> +\t\t\tif (loosen_small_pack(pack_data) == 0)\n> +\t\t\t\tgoto discard_pack;\n> +\t\t}\n\n\"if (!loosen_small_pack(pack_data))\" would be more idiomatic, but\nthe logic is very clear here.  We haven't created the idx, we skip\nthe part that creates the idx and instead jump directly to the part\nthat closes and unlinks it.\n\nI like this change.  Thanks.\n\n>  \t\tclose(pack_data->pack_fd);\n>  \t\tidx_name = keep_pack(create_index());\n>  \n> @@ -1002,6 +1026,7 @@ static void end_packfile(void)\n>  \t\tpack_id++;\n>  \t}\n>  \telse {\n> +discard_pack:\n>  \t\tclose(pack_data->pack_fd);\n>  \t\tunlink_or_warn(pack_data->pack_name);\n>  \t}\n"},{"id":"284365","messageId":"20160424203650.GA32458@dcvr.yhbt.net","threadId":"42120","inReplyTo":"xmqqpoten7db.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2] fast-import: implement unpack limit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-24T20:36:50Z","receivedAt":"2016-04-24T20:36:50Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano <gitster@pobox.com> wrote:\n> Eric Wong <normalperson@yhbt.net> writes:\n> > +\targv_array_push(&unpack.args, \"unpack-objects\");\n> > +\targv_array_push(&unpack.args, \"-q\");\n> > +\n> > +\treturn run_command(&unpack);\n \n> Looks good.  I haven't thought if \"-q\" is appropriate or not though.\n\nOops, I think tying it to the existing --quiet option in PATCH v3\nwould be good.\n\n> > @@ -972,6 +990,12 @@ static void end_packfile(void)\n> >  \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n> >  \t\t\t\t    pack_data->pack_name, object_count,\n> >  \t\t\t\t    cur_pack_sha1, pack_size);\n> > +\n> > +\t\tif (object_count <= unpack_limit) {\n> > +\t\t\tif (loosen_small_pack(pack_data) == 0)\n> > +\t\t\t\tgoto discard_pack;\n> > +\t\t}\n> \n> \"if (!loosen_small_pack(pack_data))\" would be more idiomatic, but\n> the logic is very clear here.  We haven't created the idx, we skip\n> the part that creates the idx and instead jump directly to the part\n> that closes and unlinks it.\n\nI was on the fence about \"!\" vs \"== 0\" vs something else, too;\nand I get thrown off by things like \"!strcmp\" in C all the time.\nI can change it to \"if (!loosen_small_pack(pack_data))\" in v3\n(probably in a day or so in case there's further comments)\n"},{"id":"284485","messageId":"20160425211727.GA17750@dcvr.yhbt.net","threadId":"42120","inReplyTo":"20160424203650.GA32458@dcvr.yhbt.net","subject":"[PATCH v3] fast-import: implement unpack limit","fromName":"Eric Wong","fromEmail":"normalperson@yhbt.net","sentAt":"2016-04-25T21:17:28Z","receivedAt":"2016-04-25T21:17:28Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"With many incremental imports, small packs become highly\ninefficient due to the need to readdir scan and load many\nindices to locate even a single object.  Frequent repacking and\nconsolidation may be prohibitively expensive in terms of disk\nI/O, especially in large repositories where the initial packs\nwere aggressively optimized and marked with .keep files.\n\nIn those cases, users may be better served with loose objects\nand relying on \"git gc --auto\".\n\nThis changes the default behavior of fast-import for small\nimports found in test cases, so adjustments to t9300 were\nnecessary.\n\nSigned-off-by: Eric Wong <normalperson@yhbt.net>\n---\n  v2 changes: implemented as a git-config directive\n  v3 changes:\n  * honor --quiet for fast-import as -q for unpack-objects\n  * minor style adjustment when calling loosen_small_pack\n\n Documentation/config.txt            |  9 +++++++\n Documentation/git-fast-import.txt   |  2 ++\n fast-import.c                       | 32 +++++++++++++++++++++++++\n t/t9300-fast-import.sh              |  2 ++\n t/t9302-fast-import-unpack-limit.sh | 48 +++++++++++++++++++++++++++++++++++++\n 5 files changed, 93 insertions(+)\n create mode 100755 t/t9302-fast-import-unpack-limit.sh\n\ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 42d2b50..3d8bc97 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1154,6 +1154,15 @@ difftool.<tool>.cmd::\n difftool.prompt::\n \tPrompt before each invocation of the diff tool.\n \n+fastimport.unpackLimit::\n+\tIf the number of objects imported by linkgit:git-fast-import[1]\n+\tis below this limit, then the objects will be unpacked into\n+\tloose object files.  However if the number of imported objects\n+\tequals or exceeds this limit then the pack will be stored as a\n+\tpack.  Storing the pack from a fast-import can make the import\n+\toperation complete faster, especially on slow filesystems.  If\n+\tnot set, the value of `transfer.unpackLimit` is used instead.\n+\n fetch.recurseSubmodules::\n \tThis option can be either set to a boolean value or to 'on-demand'.\n \tSetting it to a boolean changes the behavior of fetch and pull to\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 66910aa..644df99 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -136,6 +136,8 @@ Performance and Compression Tuning\n \tMaximum size of each output packfile.\n \tThe default is unlimited.\n \n+fastimport.unpackLimit::\n+\tSee linkgit:git-config[1]\n \n Performance\n -----------\ndiff --git a/fast-import.c b/fast-import.c\nindex 9fc7093..4fb464c 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -166,6 +166,7 @@ Format of STDIN stream:\n #include \"quote.h\"\n #include \"exec_cmd.h\"\n #include \"dir.h\"\n+#include \"run-command.h\"\n \n #define PACK_ID_BITS 16\n #define MAX_PACK_ID ((1<<PACK_ID_BITS)-1)\n@@ -282,6 +283,7 @@ struct recent_command {\n /* Configured limits on output */\n static unsigned long max_depth = 10;\n static off_t max_packsize;\n+static int unpack_limit = 100;\n static int force_update;\n static int pack_compression_level = Z_DEFAULT_COMPRESSION;\n static int pack_compression_seen;\n@@ -950,6 +952,23 @@ static void unkeep_all_packs(void)\n \t}\n }\n \n+static int loosen_small_pack(const struct packed_git *p)\n+{\n+\tstruct child_process unpack = CHILD_PROCESS_INIT;\n+\n+\tif (lseek(p->pack_fd, 0, SEEK_SET) < 0)\n+\t\tdie_errno(\"Failed seeking to start of '%s'\", p->pack_name);\n+\n+\tunpack.in = p->pack_fd;\n+\tunpack.git_cmd = 1;\n+\tunpack.stdout_to_stderr = 1;\n+\targv_array_push(&unpack.args, \"unpack-objects\");\n+\tif (!show_stats)\n+\t\targv_array_push(&unpack.args, \"-q\");\n+\n+\treturn run_command(&unpack);\n+}\n+\n static void end_packfile(void)\n {\n \tstatic int running;\n@@ -972,6 +991,12 @@ static void end_packfile(void)\n \t\tfixup_pack_header_footer(pack_data->pack_fd, pack_data->sha1,\n \t\t\t\t    pack_data->pack_name, object_count,\n \t\t\t\t    cur_pack_sha1, pack_size);\n+\n+\t\tif (object_count <= unpack_limit) {\n+\t\t\tif (!loosen_small_pack(pack_data))\n+\t\t\t\tgoto discard_pack;\n+\t\t}\n+\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n \n@@ -1002,6 +1027,7 @@ static void end_packfile(void)\n \t\tpack_id++;\n \t}\n \telse {\n+discard_pack:\n \t\tclose(pack_data->pack_fd);\n \t\tunlink_or_warn(pack_data->pack_name);\n \t}\n@@ -3317,6 +3343,7 @@ static void parse_option(const char *option)\n static void git_pack_config(void)\n {\n \tint indexversion_value;\n+\tint limit;\n \tunsigned long packsizelimit_value;\n \n \tif (!git_config_get_ulong(\"pack.depth\", &max_depth)) {\n@@ -3341,6 +3368,11 @@ static void git_pack_config(void)\n \tif (!git_config_get_ulong(\"pack.packsizelimit\", &packsizelimit_value))\n \t\tmax_packsize = packsizelimit_value;\n \n+\tif (!git_config_get_int(\"fastimport.unpacklimit\", &limit))\n+\t\tunpack_limit = limit;\n+\telse if (!git_config_get_int(\"transfer.unpacklimit\", &limit))\n+\t\tunpack_limit = limit;\n+\n \tgit_config(git_default_config, NULL);\n }\n \ndiff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh\nindex 25bb60b..e6a2b8a 100755\n--- a/t/t9300-fast-import.sh\n+++ b/t/t9300-fast-import.sh\n@@ -52,6 +52,7 @@ echo \"$@\"'\n ###\n \n test_expect_success 'empty stream succeeds' '\n+\tgit config fastimport.unpackLimit 0 &&\n \tgit fast-import </dev/null\n '\n \n@@ -2675,6 +2676,7 @@ test_expect_success 'R: blob bigger than threshold' '\n \techo >>input &&\n \n \ttest_create_repo R &&\n+\tgit --git-dir=R/.git config fastimport.unpackLimit 0 &&\n \tgit --git-dir=R/.git fast-import --big-file-threshold=1 <input\n '\n \ndiff --git a/t/t9302-fast-import-unpack-limit.sh b/t/t9302-fast-import-unpack-limit.sh\nnew file mode 100755\nindex 0000000..0f686d2\n--- /dev/null\n+++ b/t/t9302-fast-import-unpack-limit.sh\n@@ -0,0 +1,48 @@\n+#!/bin/sh\n+test_description='test git fast-import unpack limit'\n+. ./test-lib.sh\n+\n+test_expect_success 'create loose objects on import' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tinitial\n+\tCOMMIT\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit -c fastimport.unpackLimit=2 fast-import --done <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 0\n+'\n+\n+test_expect_success 'bigger packs are preserved' '\n+\ttest_tick &&\n+\tcat >input <<-INPUT_END &&\n+\tcommit refs/heads/master\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tincremental should create a pack\n+\tCOMMIT\n+\tfrom refs/heads/master^0\n+\n+\tcommit refs/heads/branch\n+\tcommitter $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE\n+\tdata <<COMMIT\n+\tbranch\n+\tCOMMIT\n+\n+\tdone\n+\tINPUT_END\n+\n+\tgit -c fastimport.unpackLimit=2 fast-import --done <input &&\n+\tgit fsck --no-progress &&\n+\ttest $(find .git/objects/?? -type f | wc -l) -eq 2 &&\n+\ttest $(find .git/objects/pack -type f | wc -l) -eq 2\n+'\n+\n+test_done\n-- \nEW\n"}]}