{"thread":{"id":"22514","subject":"[PATCH 2/3] pack-objects: fix pack generation when using pack_size_limit","startedAt":"2010-02-04T03:48:26Z","lastAt":"2010-02-04T21:20:33Z","messageCount":16,"participants":["Nicolas Pitre","Shawn O. Pearce","Junio C Hamano","Ilari Liusvaara"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"133579","messageId":"1265255308-20514-1-git-send-email-nico@fluxnic.net","threadId":"22514","inReplyTo":null,"subject":"[PATCH 1/3] fix multiple issues with t5300","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T03:48:26Z","receivedAt":"2010-02-04T03:48:26Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"First of all, trying to run 'git verify-pack' on packs produced by\nthe tests using pack.packSizeLimit always failed. After lots of digging\nand head scratching, it turns out that the preceeding test simulating\na SHA1 collision did leave the repository quite confused, impacting\nsubsequent tests.\n\nSo let's move that destructive test last, and add tests to run\nverify-pack on the output from those packSizeLimit tests to catch such\ngoofage.\n\nFinally, group those packSizeLimit tests together.\n\nSigned-off-by: Nicolas Pitre \n---\n t/t5300-pack-object.sh | 54 ++++++++++++++++++++++++++++++-----------------\n 1 files changed, 34 insertions(+), 20 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex e2aa254..ac81114 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -280,26 +280,8 @@ test_expect_success \\\n \n :'\n \n-test_expect_success \\\n- 'fake a SHA1 hash collision' \\\n- 'test -f\t.git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67 &&\n- cp -f\t.git/objects/9d/235ed07cd19811a6ceb342de82f190e49c9f68 \\\n-\t\t.git/objects/c8/2de19312b6c3695c0c18f70709a6c535682a67'\n-\n-test_expect_success \\\n- 'make sure index-pack detects the SHA1 collision' \\\n- 'test_must_fail git index-pack -o bad.idx test-3.pack 2>msg &&\n- grep \"SHA1 COLLISION FOUND\" msg'\n-\n-test_expect_success \\\n- 'honor pack.packSizeLimit' \\\n- 'git config pack.packSizeLimit 200 &&\n- packname_4=$(git pack-objects test-4 msg &&\n+ grep \"SHA1 COLLISION FOUND\" msg'\n+\n test_done\n-- \n1.7.0.rc1.149.g0b0b7\n"},{"id":"133578","messageId":"1265255308-20514-2-git-send-email-nico@fluxnic.net","threadId":"22514","inReplyTo":"1265255308-20514-1-git-send-email-nico@fluxnic.net","subject":"[PATCH 2/3] pack-objects: fix pack generation when using pack_size_limit","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T03:48:27Z","receivedAt":"2010-02-04T03:48:27Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Current handling of pack_size_limit is quite suboptimal. Let's consider\na list of objects to pack which contain alternatively big and small\nobjects (which pretty matches reality when big blobs are interlaced\nwith tree objects). Currently, the code simply close the pack and opens\na new one when the next object in line breaks the size limit.\n\nThe current code may degenerate into:\n\n - small tree object => store into pack #1\n - big blob object busting the pack size limit => store into pack #2\n - small blob but pack #2 is over the limit already => pack #3\n - big blob busting the size limit => pack #4\n - small tree but pack #4 is over the limit => pack #5\n - big blob => pack #6\n - small tree => pack #7\n - ... and so on.\n\nThe reality is that the content of packs 1, 3, 5 and 7 could well be\nstored more efficiently (and delta compressed) together in pack #1 if\nthe big blobs were not forcing an immediate transition to a new pack.\n\nIncidentally this can be fixed pretty easily by simply skipping over\nthose objects that are too big to fit in the current pack while trying\nthe whole list of unwritten objects, and then that list considered from\nthe beginning again when a new pack is opened. This creates much fewer\nsmallish pack files and help making more predictable test cases for the\ntest suite.\n\nThis change made one of the self sanity checks useless so it is removed\nas well. That check was rather redundant already anyway.\n\nSigned-off-by: Nicolas Pitre \n---\n builtin-pack-objects.c | 37 +++++++++++++------------------------\n 1 files changed, 13 insertions(+), 24 deletions(-)\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 4a41547..3186035 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -246,7 +246,7 @@ static unsigned long write_object(struct sha1file *f,\n \n \ttype = entry->type;\n \n-\t/* write limit if limited packsize and not first object */\n+\t/* apply size limit if limited packsize and not first object */\n \tif (!pack_size_limit || !nr_written)\n \t\tlimit = 0;\n \telse if (pack_size_limit <= write_offset)\n@@ -443,11 +443,15 @@ static int write_one(struct sha1file *f,\n \n \t/* offset is non zero if object is written already. */\n \tif (e->idx.offset || e->preferred_base)\n-\t\treturn 1;\n+\t\treturn -1;\n \n-\t/* if we are deltified, write out base object first. */\n-\tif (e->delta && !write_one(f, e->delta, offset))\n-\t\treturn 0;\n+\t/*\n+\t * If we are deltified, attempt to write out base object first.\n+\t * If that fails due to the pack size limit then the current\n+\t * object might still possibly fit undeltified within that limit.\n+\t */\n+\tif (e->delta)\n+\t write_one(f, e->delta, offset);\n \n \te->idx.offset = *offset;\n \tsize = write_object(f, e, *offset);\n@@ -501,11 +505,9 @@ static void write_pack_file(void)\n \t\tsha1write(f, &hdr, sizeof(hdr));\n \t\toffset = sizeof(hdr);\n \t\tnr_written = 0;\n-\t\tfor (; i < nr_objects; i++) {\n-\t\t\tif (!write_one(f, objects + i, &offset))\n-\t\t\t\tbreak;\n-\t\t\tdisplay_progress(progress_state, written);\n-\t\t}\n+\t\tfor (i = 0; i < nr_objects; i++)\n+\t\t\tif (write_one(f, objects + i, &offset) == 1)\n+\t\t\t\tdisplay_progress(progress_state, written);\n \n \t\t/*\n \t\t * Did we write the wrong # entries in the header?\n@@ -580,26 +582,13 @@ static void write_pack_file(void)\n \t\t\twritten_list[j]->offset = (off_t)-1;\n \t\t}\n \t\tnr_remaining -= nr_written;\n-\t} while (nr_remaining && i < nr_objects);\n+\t} while (nr_remaining);\n \n \tfree(written_list);\n \tstop_progress(&progress_state);\n \tif (written != nr_result)\n \t\tdie(\"wrote %\"PRIu32\" objects while expecting %\"PRIu32,\n \t\t\twritten, nr_result);\n-\t/*\n-\t * We have scanned through [0 ... i). Since we have written\n-\t * the correct number of objects, the remaining [i ... nr_objects)\n-\t * items must be either already written (due to out-of-order delta base)\n-\t * or a preferred base. Count those which are neither and complain if any.\n-\t */\n-\tfor (j = 0; i < nr_objects; i++) {\n-\t\tstruct object_entry *e = objects + i;\n-\t\tj += !e->idx.offset && !e->preferred_base;\n-\t}\n-\tif (j)\n-\t\tdie(\"wrote %\"PRIu32\" objects as expected but %\"PRIu32\n-\t\t\t\" unwritten\", written, j);\n }\n \n static int locate_object_entry_hash(const unsigned char *sha1)\n-- \n1.7.0.rc1.149.g0b0b7\n"},{"id":"133580","messageId":"1265255308-20514-3-git-send-email-nico@fluxnic.net","threadId":"22514","inReplyTo":"1265255308-20514-1-git-send-email-nico@fluxnic.net","subject":"[PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T03:48:28Z","receivedAt":"2010-02-04T03:48:28Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"The value passed to --max-pack-size used to count in MiB which was\ninconsistent with the corresponding configuration variable as well as\nother command arguments which are defined to count in bytes with an\noptional unit suffix. This brings --max-pack-size in line with the\nrest of Git.\n\nAlso, in order not to cause havoc with people used to the previous\nmegabyte scale, and because this is a sane thing to do anyway, a\nminimum size of 1 MiB is enforced to avoid an explosion of pack files.\n\nAdjust and extend test suite accordingly.\n\nSigned-off-by: Nicolas Pitre \n---\n Documentation/RelNotes-1.7.0.txt | 8 +++++++-\n Documentation/config.txt | 11 +++++++----\n Documentation/git-pack-objects.txt | 5 +++--\n Documentation/git-repack.txt | 8 +++++---\n builtin-pack-objects.c | 11 ++++++-----\n t/t5300-pack-object.sh | 14 ++++++++------\n 6 files changed, 36 insertions(+), 21 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt\nindex 323ae54..e66945c 100644\n--- a/Documentation/RelNotes-1.7.0.txt\n+++ b/Documentation/RelNotes-1.7.0.txt\n@@ -38,7 +38,7 @@ Notes on behaviour change\n whitespaces is reported with zero exit status when run with\n --exit-code, and there is no \"diff --git\" header for such a change.\n \n- * external diff and textconv helpers are now executed using the shell.\n+ * External diff and textconv helpers are now executed using the shell.\n This makes them consistent with other programs executed by git, and\n allows you to pass command-line parameters to the helpers. Any helper\n paths containing spaces or other metacharacters now need to be\n@@ -46,6 +46,12 @@ Notes on behaviour change\n environment, and diff.*.command and diff.*.textconv in the config\n file.\n \n+ * The --max-pack-size argument to 'git repack' and 'git pack-objects' was\n+ assuming the provided size to be expressed in MiB, unlike the\n+ corresponding config variable and other similar options accepting a size\n+ value. It is now expecting a size expressed in bytes, with a possible\n+ unit suffix of 'k', 'm', or 'g'.\n+\n Updates since v1.6.6\n --------------------\n \ndiff --git a/Documentation/config.txt b/Documentation/config.txt\nindex 36ad992..4c36aa9 100644\n--- a/Documentation/config.txt\n+++ b/Documentation/config.txt\n@@ -1368,10 +1368,13 @@ you can use linkgit:git-index-pack[1] on the *.pack file to regenerate\n the `{asterisk}.idx` file.\n \n pack.packSizeLimit::\n-\tThe default maximum size of a pack. This setting only affects\n-\tpacking to a file, i.e. the git:// protocol is unaffected. It\n-\tcan be overridden by the `\\--max-pack-size` option of\n-\tlinkgit:git-repack[1].\n+\tThe maximum size of a pack. This setting only affects\n+\tpacking to a file when repacking, i.e. the git:// protocol\n+\tis unaffected. It can be overridden by the `\\--max-pack-size`\n+\toption of linkgit:git-repack[1]. The minimum size allowed is\n+\tlimited to 1 MiB. The default is unlimited.\n+\tCommon unit suffixes of 'k', 'm', or 'g' are\n+\tsupported.\n \n pager.::\n \tAllows turning on or off pagination of the output of a\ndiff --git a/Documentation/git-pack-objects.txt b/Documentation/git-pack-objects.txt\nindex 097a147..ffd5025 100644\n--- a/Documentation/git-pack-objects.txt\n+++ b/Documentation/git-pack-objects.txt\n@@ -105,8 +105,9 @@ base-name::\n \t`--window-memory=0` makes memory usage unlimited, which is the\n \tdefault.\n \n---max-pack-size=::\n-\tMaximum size of each output packfile, expressed in MiB.\n+--max-pack-size=[N]::\n+\tMaximum size of each output pack file. The size can be suffixed with\n+\t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n \tIf specified, multiple packfiles may be created.\n \tThe default is unlimited, unless the config variable\n \t`pack.packSizeLimit` is set.\ndiff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt\nindex 538895c..e2f2fa2 100644\n--- a/Documentation/git-repack.txt\n+++ b/Documentation/git-repack.txt\n@@ -98,10 +98,12 @@ other objects in that pack they already have locally.\n \t`--window-memory=0` makes memory usage unlimited, which is the\n \tdefault.\n \n---max-pack-size=::\n-\tMaximum size of each output packfile, expressed in MiB.\n+--max-pack-size=[N]::\n+\tMaximum size of each output pack file. The size can be suffixed with\n+\t\"k\", \"m\", or \"g\". The minimum size allowed is limited to 1 MiB.\n \tIf specified, multiple packfiles may be created.\n-\tThe default is unlimited.\n+\tThe default is unlimited, unless the config variable\n+\t`pack.packSizeLimit` is set.\n \n \n Configuration\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 3186035..dcfe62a 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -77,7 +77,7 @@ static int allow_ofs_delta;\n static const char *base_name;\n static int progress = 1;\n static int window = 10;\n-static uint32_t pack_size_limit, pack_size_limit_cfg;\n+static unsigned long pack_size_limit, pack_size_limit_cfg;\n static int depth = 50;\n static int delta_search_threads;\n static int pack_to_stdout;\n@@ -2192,10 +2192,8 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \t\t\tcontinue;\n \t\t}\n \t\tif (!prefixcmp(arg, \"--max-pack-size=\")) {\n-\t\t\tchar *end;\n \t\t\tpack_size_limit_cfg = 0;\n-\t\t\tpack_size_limit = strtoul(arg+16, &end, 0) * 1024 * 1024;\n-\t\t\tif (!arg[16] || *end)\n+\t\t\tif (!git_parse_ulong(arg+16, &pack_size_limit))\n \t\t\t\tusage(pack_usage);\n \t\t\tcontinue;\n \t\t}\n@@ -2335,9 +2333,12 @@ int cmd_pack_objects(int argc, const char **argv, const char *prefix)\n \n \tif (!pack_to_stdout && !pack_size_limit)\n \t\tpack_size_limit = pack_size_limit_cfg;\n-\n \tif (pack_to_stdout && pack_size_limit)\n \t\tdie(\"--max-pack-size cannot be used to build a pack for transfer.\");\n+\tif (pack_size_limit && pack_size_limit < 1024*1024) {\n+\t\twarning(\"minimum pack size limit is 1 MiB\");\n+\t\tpack_size_limit = 1024*1024;\n+\t}\n \n \tif (!pack_to_stdout && thin)\n \t\tdie(\"--thin cannot be used to build an indexable pack.\");\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex ac81114..c80f3a3 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -16,7 +16,9 @@ test_expect_success \\\n perl -e \"print \\\"a\\\" x 4096;\" > a &&\n perl -e \"print \\\"b\\\" x 4096;\" > b &&\n perl -e \"print \\\"c\\\" x 4096;\" > c &&\n- git update-index --add a b c &&\n+ test-genrandom \"seed a\" 2097152 > a_big &&\n+ test-genrandom \"seed b\" 2097152 > b_big &&\n+ git update-index --add a a_big b b_big c &&\n cat c >d && echo foo >>d && git update-index --add d &&\n tree=`git write-tree` &&\n commit=`git commit-tree $tree wrote:\n> The value passed to --max-pack-size used to count in MiB which was\n> inconsistent with the corresponding configuration variable as well as\n> other command arguments which are defined to count in bytes with an\n> optional unit suffix. This brings --max-pack-size in line with the\n> rest of Git.\n...\n> Documentation/RelNotes-1.7.0.txt | 8 +++++++-\n> Documentation/config.txt | 11 +++++++----\n> Documentation/git-pack-objects.txt | 5 +++--\n> Documentation/git-repack.txt | 8 +++++---\n> builtin-pack-objects.c | 11 ++++++-----\n> t/t5300-pack-object.sh | 14 ++++++++------\n> 6 files changed, 36 insertions(+), 21 deletions(-)\n\nShouldn't we also change fast-import.c ?\n\n-- \nShawn.\n"},{"id":"133585","messageId":"7v7hqtty38.fsf@alter.siamese.dyndns.org","threadId":"22514","inReplyTo":"20100204040046.GR14799@spearce.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-04T04:38:03Z","receivedAt":"2010-02-04T04:38:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Shawn O. Pearce\" writes:\n\n> Nicolas Pitre wrote:\n>> The value passed to --max-pack-size used to count in MiB which was\n>> inconsistent with the corresponding configuration variable as well as\n>> other command arguments which are defined to count in bytes with an\n>> optional unit suffix. This brings --max-pack-size in line with the\n>> rest of Git.\n> ...\n>> Documentation/RelNotes-1.7.0.txt | 8 +++++++-\n>> Documentation/config.txt | 11 +++++++----\n>> Documentation/git-pack-objects.txt | 5 +++--\n>> Documentation/git-repack.txt | 8 +++++---\n>> builtin-pack-objects.c | 11 ++++++-----\n>> t/t5300-pack-object.sh | 14 ++++++++------\n>> 6 files changed, 36 insertions(+), 21 deletions(-)\n>\n> Shouldn't we also change fast-import.c ?\n\nSurely; could you do the honors? I cannot really decide how big the deal\nwould be to break backward compatibility for max-pack-size myself.\n"},{"id":"133616","messageId":"7vtytxexjl.fsf@alter.siamese.dyndns.org","threadId":"22514","inReplyTo":"7v7hqtty38.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-04T17:11:10Z","receivedAt":"2010-02-04T17:11:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> \"Shawn O. Pearce\" writes:\n> ...\n>> Shouldn't we also change fast-import.c ?\n>\n> Surely; could you do the honors? I cannot really decide how big the deal\n> would be to break backward compatibility for max-pack-size myself.\n\nI'll queue the \"fix mismerge\" one I got \"Yup, looks good to me.\" from you,\nwith this:\n\ncommit 76ea93ccb5df138eb57b2e8f2aee61dd1ca666ea\nAuthor: Junio C Hamano \nDate: Wed Feb 3 18:27:08 2010 -0800\n\n fast-import.c: Fix big-file-threshold parsing bug\n \n Manual merge made at 844ad3d (Merge branch 'sp/maint-fast-import-large-blob'\n into sp/fast-import-large-blob, 2010-02-01) did not correctly reflect the change\n of unit in which this variable's value is counted from its previous version.\n \n Now it counts in bytes, not in megabytes.\n \n Signed-off-by: Junio C Hamano \n Acked-by: Shawn O. Pearce \n\nI am debating myself if we want to have another one (see below) on top; I\nam not a heavy user of fast-import and I don't know if existing users will\nbe hurt by such a change and if so how much.\n\n-- >8 --\nSubject: [PATCH] fast-import: count --max-pack-size in bytes\n\nSimilar in spirit to 07cf0f2 (make --max-pack-size argument to 'git\npack-object' count in bytes, 2010-02-03) which made the option by the same\nname to pack-objects, this counts the pack size limit in bytes.\n\nIn order not to cause havoc with people used to the previous megabyte\nscale, and because this is a sane thing to do anyway, a minimum size of 1\nMiB is enforced to avoid an explosion of pack files.\n\nSigned-off-by: Junio C Hamano \n---\n Documentation/RelNotes-1.7.0.txt | 8 ++++----\n Documentation/git-fast-import.txt | 4 ++--\n fast-import.c | 14 ++++++++------\n 3 files changed, 14 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt\nindex e66945c..255666f 100644\n--- a/Documentation/RelNotes-1.7.0.txt\n+++ b/Documentation/RelNotes-1.7.0.txt\n@@ -46,10 +46,10 @@ Notes on behaviour change\n environment, and diff.*.command and diff.*.textconv in the config\n file.\n \n- * The --max-pack-size argument to 'git repack' and 'git pack-objects' was\n- assuming the provided size to be expressed in MiB, unlike the\n- corresponding config variable and other similar options accepting a size\n- value. It is now expecting a size expressed in bytes, with a possible\n+ * The --max-pack-size argument to 'git repack', 'git pack-objects', and\n+ 'git fast-import' was assuming the provided size to be expressed in MiB,\n+ unlike the corresponding config variable and other similar options accepting\n+ a size value. It is now expecting a size expressed in bytes, with a possible\n unit suffix of 'k', 'm', or 'g'.\n \n Updates since v1.6.6\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 2691114..6764ff1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -44,8 +44,8 @@ OPTIONS\n \tnot contain the old commit).\n \n --max-pack-size=::\n-\tMaximum size of each output packfile, expressed in MiB.\n-\tThe default is 4096 (4 GiB) as that is the maximum allowed\n+\tMaximum size of each output packfile.\n+\tThe default is 4 GiB as that is the maximum allowed\n \tpackfile size (due to file format limitations). Some\n \timporters may wish to lower this, such as to ensure the\n \tresulting packfiles fit on CDs.\ndiff --git a/fast-import.c b/fast-import.c\nindex a6730d0..09c0817 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2764,11 +2764,6 @@ static void option_date_format(const char *fmt)\n \t\tdie(\"unknown --date-format argument %s\", fmt);\n }\n \n-static void option_max_pack_size(const char *packsize)\n-{\n-\tmax_packsize = strtoumax(packsize, NULL, 0) * 1024 * 1024;\n-}\n-\n static void option_depth(const char *depth)\n {\n \tmax_depth = strtoul(depth, NULL, 0);\n@@ -2798,7 +2793,14 @@ static void option_export_pack_edges(const char *edges)\n static int parse_one_option(const char *option)\n {\n \tif (!prefixcmp(option, \"max-pack-size=\")) {\n-\t\toption_max_pack_size(option + 14);\n+\t\tunsigned long v;\n+\t\tif (!git_parse_ulong(option + 14, &v))\n+\t\t\treturn 0;\n+\t\tif (v < 1024 * 1024) {\n+\t\t\twarning(\"minimum max-pack-size is 1 MiB\");\n+\t\t\tv = 1024 * 1024;\n+\t\t}\n+\t\tmax_packsize = v;\n \t} else if (!prefixcmp(option, \"big-file-threshold=\")) {\n \t\tunsigned long v;\n \t\tif (!git_parse_ulong(option + 19, &v))\n-- \n1.7.0.rc1.199.g9253ab\n"},{"id":"133620","messageId":"20100204172421.GA18548@spearce.org","threadId":"22514","inReplyTo":"7vtytxexjl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-04T17:24:21Z","receivedAt":"2010-02-04T17:24:21Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Subject: [PATCH] fast-import: count --max-pack-size in bytes\n> \tif (!prefixcmp(option, \"max-pack-size=\")) {\n> -\t\toption_max_pack_size(option + 14);\n> +\t\tunsigned long v;\n> +\t\tif (!git_parse_ulong(option + 14, &v))\n> +\t\t\treturn 0;\n> +\t\tif (v < 1024 * 1024) {\n> +\t\t\twarning(\"minimum max-pack-size is 1 MiB\");\n> +\t\t\tv = 1024 * 1024;\n> +\t\t}\n> +\t\tmax_packsize = v;\n\nHow about for a transition period we do:\n\n if (v < 8192) {\n \twarning(\"max-pack-size is now in bytes, assuming %dm\", v);\n\tv *= 1024 * 1024;\n }\n\nSo that existing users won't be completely broken if they are\nrelying on this flag, and have some time to adjust.\n\nGiven the huge magnitude between the old sane value range, and the\nnew sane value range, we can safely assume anything below a small\nnumber like 8192 is an old user, warn them, and assume old behavior.\n\nA local pack smaller than 1 MiB is mostly pointless coming out of\na tool like git repack or git fast-import, unless its a complete\ncopy of the repository. So the old style calling convention of\n4096 for 4 GiB would now imply a pack so small, we probably can't\nget more than 1 object per pack.\n\n-- \nShawn.\n"},{"id":"133624","messageId":"alpine.LFD.2.00.1002041243570.1681@xanadu.home","threadId":"22514","inReplyTo":"7vtytxexjl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T17:49:12Z","receivedAt":"2010-02-04T17:49:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 4 Feb 2010, Junio C Hamano wrote:\n\n> -- >8 --\n> Subject: [PATCH] fast-import: count --max-pack-size in bytes\n> \n> Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git\n> pack-object' count in bytes, 2010-02-03) which made the option by the same\n> name to pack-objects, this counts the pack size limit in bytes.\n> \n> In order not to cause havoc with people used to the previous megabyte\n> scale, and because this is a sane thing to do anyway, a minimum size of 1\n> MiB is enforced to avoid an explosion of pack files.\n> \n> Signed-off-by: Junio C Hamano \n\nACK.\n\n> --max-pack-size=::\n> -\tMaximum size of each output packfile, expressed in MiB.\n> -\tThe default is 4096 (4 GiB) as that is the maximum allowed\n> +\tMaximum size of each output packfile.\n> +\tThe default is 4 GiB as that is the maximum allowed\n> \tpackfile size (due to file format limitations). Some\n> \timporters may wish to lower this, such as to ensure the\n> \tresulting packfiles fit on CDs.\n\nWhat file format limitation is alluded to here? It has been a while \nsince the 4GB limit on pack file format has been removed. If this is a \nlimitation of fast-import only then maybe this should be explained more \nexplicitly.\n\n\nNicolas\n"},{"id":"133626","messageId":"alpine.LFD.2.00.1002041249200.1681@xanadu.home","threadId":"22514","inReplyTo":"20100204172421.GA18548@spearce.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T17:58:11Z","receivedAt":"2010-02-04T17:58:11Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 4 Feb 2010, Shawn O. Pearce wrote:\n\n> How about for a transition period we do:\n> \n> if (v < 8192) {\n> \twarning(\"max-pack-size is now in bytes, assuming %dm\", v);\n> \tv *= 1024 * 1024;\n> }\n> \n> So that existing users won't be completely broken if they are\n> relying on this flag, and have some time to adjust.\n\nFor 'git fast-import' which is not meant to be directly user operated \nthis makes sense. I don't think this is a good idea for 'git repack' \nthough. In the later case it is best if the user simply adjust right \naway.\n\nAlso the warning text above could be less ambigous. Something like:\n\n\t\"max-pack-size is now in bytes, assuming --max-pack-size=%dm\"\n\n\nNicolas\n"},{"id":"133627","messageId":"20100204175918.GB18548@spearce.org","threadId":"22514","inReplyTo":"alpine.LFD.2.00.1002041249200.1681@xanadu.home","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-04T17:59:18Z","receivedAt":"2010-02-04T17:59:18Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Thu, 4 Feb 2010, Shawn O. Pearce wrote:\n> \n> > How about for a transition period we do:\n> > \n> > if (v < 8192) {\n> > \twarning(\"max-pack-size is now in bytes, assuming %dm\", v);\n> > \tv *= 1024 * 1024;\n> > }\n> > \n> > So that existing users won't be completely broken if they are\n> > relying on this flag, and have some time to adjust.\n> \n> For 'git fast-import' which is not meant to be directly user operated \n> this makes sense. I don't think this is a good idea for 'git repack' \n> though. In the later case it is best if the user simply adjust right \n> away.\n> \n> Also the warning text above could be less ambigous. Something like:\n> \n> \t\"max-pack-size is now in bytes, assuming --max-pack-size=%dm\"\n\nACK on the fixed warning text. I lacked enough coffee at the time\nto write intelligent text.\n\n-- \nShawn.\n"},{"id":"133629","messageId":"20100204180015.GC18548@spearce.org","threadId":"22514","inReplyTo":"alpine.LFD.2.00.1002041243570.1681@xanadu.home","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-04T18:00:15Z","receivedAt":"2010-02-04T18:00:15Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> > --max-pack-size=::\n> > -\tMaximum size of each output packfile, expressed in MiB.\n> > -\tThe default is 4096 (4 GiB) as that is the maximum allowed\n> > +\tMaximum size of each output packfile.\n> > +\tThe default is 4 GiB as that is the maximum allowed\n> > \tpackfile size (due to file format limitations). Some\n> > \timporters may wish to lower this, such as to ensure the\n> > \tresulting packfiles fit on CDs.\n> \n> What file format limitation is alluded to here? It has been a while \n> since the 4GB limit on pack file format has been removed.\n\nThe pack index v1 32 bit offset thing. Which you fixed.\n\n> If this is a \n> limitation of fast-import only then maybe this should be explained more \n> explicitly.\n\nDamn. It is. fast-import can't write a v2 index. Ugh.\n\n-- \nShawn.\n"},{"id":"133640","messageId":"alpine.LFD.2.00.1002041309090.1681@xanadu.home","threadId":"22514","inReplyTo":"20100204180015.GC18548@spearce.org","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Nicolas Pitre","fromEmail":"nico@fluxnic.net","sentAt":"2010-02-04T18:11:17Z","receivedAt":"2010-02-04T18:11:17Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"On Thu, 4 Feb 2010, Shawn O. Pearce wrote:\n\n> Nicolas Pitre wrote:\n> > > --max-pack-size=::\n> > > -\tMaximum size of each output packfile, expressed in MiB.\n> > > -\tThe default is 4096 (4 GiB) as that is the maximum allowed\n> > > +\tMaximum size of each output packfile.\n> > > +\tThe default is 4 GiB as that is the maximum allowed\n> > > \tpackfile size (due to file format limitations). Some\n> > > \timporters may wish to lower this, such as to ensure the\n> > > \tresulting packfiles fit on CDs.\n> > \n> > What file format limitation is alluded to here? It has been a while \n> > since the 4GB limit on pack file format has been removed.\n> \n> The pack index v1 32 bit offset thing. Which you fixed.\n> \n> > If this is a \n> > limitation of fast-import only then maybe this should be explained more \n> > explicitly.\n> \n> Damn. It is. fast-import can't write a v2 index. Ugh.\n\nIsn't it using write_idx_file()? That function would do it all for you \nalready.\n\n\nNicolas\n"},{"id":"133632","messageId":"7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org","threadId":"22514","inReplyTo":"20100204175918.GB18548@spearce.org","subject":"[PATCH] fast-import: count --max-pack-size in bytes","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-02-04T19:10:44Z","receivedAt":"2010-02-04T19:10:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git\npack-object' count in bytes, 2010-02-03) which made the option by the same\nname to pack-objects, this counts the pack size limit in bytes.\n\nIn order not to cause havoc with people used to the previous megabyte\nscale an integer smaller than 8092 is interpreted in megabytes but the\nuser gets a warning. Also a minimum size of 1 MiB is enforced to avoid an\nexplosion of pack files.\n\nSigned-off-by: Junio C Hamano \nAcked-by: Shawn O. Pearce \nAcked-by: Nicolas Pitre \n---\n\n Ok, third-time lucky? Knock wood...\n\n Documentation/RelNotes-1.7.0.txt | 8 ++++----\n Documentation/git-fast-import.txt | 4 ++--\n fast-import.c | 17 +++++++++++------\n 3 files changed, 17 insertions(+), 12 deletions(-)\n\ndiff --git a/Documentation/RelNotes-1.7.0.txt b/Documentation/RelNotes-1.7.0.txt\nindex e66945c..255666f 100644\n--- a/Documentation/RelNotes-1.7.0.txt\n+++ b/Documentation/RelNotes-1.7.0.txt\n@@ -46,10 +46,10 @@ Notes on behaviour change\n environment, and diff.*.command and diff.*.textconv in the config\n file.\n \n- * The --max-pack-size argument to 'git repack' and 'git pack-objects' was\n- assuming the provided size to be expressed in MiB, unlike the\n- corresponding config variable and other similar options accepting a size\n- value. It is now expecting a size expressed in bytes, with a possible\n+ * The --max-pack-size argument to 'git repack', 'git pack-objects', and\n+ 'git fast-import' was assuming the provided size to be expressed in MiB,\n+ unlike the corresponding config variable and other similar options accepting\n+ a size value. It is now expecting a size expressed in bytes, with a possible\n unit suffix of 'k', 'm', or 'g'.\n \n Updates since v1.6.6\ndiff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt\nindex 2691114..6764ff1 100644\n--- a/Documentation/git-fast-import.txt\n+++ b/Documentation/git-fast-import.txt\n@@ -44,8 +44,8 @@ OPTIONS\n \tnot contain the old commit).\n \n --max-pack-size=::\n-\tMaximum size of each output packfile, expressed in MiB.\n-\tThe default is 4096 (4 GiB) as that is the maximum allowed\n+\tMaximum size of each output packfile.\n+\tThe default is 4 GiB as that is the maximum allowed\n \tpackfile size (due to file format limitations). Some\n \timporters may wish to lower this, such as to ensure the\n \tresulting packfiles fit on CDs.\ndiff --git a/fast-import.c b/fast-import.c\nindex a6730d0..b477dc6 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -2764,11 +2764,6 @@ static void option_date_format(const char *fmt)\n \t\tdie(\"unknown --date-format argument %s\", fmt);\n }\n \n-static void option_max_pack_size(const char *packsize)\n-{\n-\tmax_packsize = strtoumax(packsize, NULL, 0) * 1024 * 1024;\n-}\n-\n static void option_depth(const char *depth)\n {\n \tmax_depth = strtoul(depth, NULL, 0);\n@@ -2798,7 +2793,17 @@ static void option_export_pack_edges(const char *edges)\n static int parse_one_option(const char *option)\n {\n \tif (!prefixcmp(option, \"max-pack-size=\")) {\n-\t\toption_max_pack_size(option + 14);\n+\t\tunsigned long v;\n+\t\tif (!git_parse_ulong(option + 14, &v))\n+\t\t\treturn 0;\n+\t\tif (v < 8192) {\n+\t\t\twarning(\"max-pack-size is now in bytes, assuming --max-pack-size=%lum\", v);\n+\t\t\tv *= 1024 * 1024;\n+\t\t} else if (v < 1024 * 1024) {\n+\t\t\twarning(\"minimum max-pack-size is 1 MiB\");\n+\t\t\tv = 1024 * 1024;\n+\t\t}\n+\t\tmax_packsize = v;\n \t} else if (!prefixcmp(option, \"big-file-threshold=\")) {\n \t\tunsigned long v;\n \t\tif (!git_parse_ulong(option + 19, &v))\n-- \n1.7.0.rc1.199.g9253ab\n"},{"id":"133633","messageId":"20100204191301.GA18668@spearce.org","threadId":"22514","inReplyTo":"7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: count --max-pack-size in bytes","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-04T19:13:01Z","receivedAt":"2010-02-04T19:13:01Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Junio C Hamano wrote:\n> Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git\n> pack-object' count in bytes, 2010-02-03) which made the option by the same\n> name to pack-objects, this counts the pack size limit in bytes.\n> \n> In order not to cause havoc with people used to the previous megabyte\n> scale an integer smaller than 8092 is interpreted in megabytes but the\n> user gets a warning. Also a minimum size of 1 MiB is enforced to avoid an\n> explosion of pack files.\n> \n> Signed-off-by: Junio C Hamano \n> Acked-by: Shawn O. Pearce \n> Acked-by: Nicolas Pitre \n> ---\n> \n> Ok, third-time lucky? Knock wood...\n\nYea, I like it. Thanks Junio.\n\n-- \nShawn.\n"},{"id":"133636","messageId":"20100204200306.GA11700@Knoppix","threadId":"22514","inReplyTo":"7v4olwbyvf.fsf_-_@alter.siamese.dyndns.org","subject":"Re: [PATCH] fast-import: count --max-pack-size in bytes","fromName":"Ilari Liusvaara","fromEmail":"ilari.liusvaara@elisanet.fi","sentAt":"2010-02-04T20:03:07Z","receivedAt":"2010-02-04T20:03:07Z","isPatch":true,"sender":{"key":"ilari.liusvaara@elisanet.fi","avatar":null},"body":"On Thu, Feb 04, 2010 at 11:10:44AM -0800, Junio C Hamano wrote:\n> Similar in spirit to 07cf0f2 (make --max-pack-size argument to 'git\n> pack-object' count in bytes, 2010-02-03) which made the option by the same\n> name to pack-objects, this counts the pack size limit in bytes.\n> \n> In order not to cause havoc with people used to the previous megabyte\n> scale an integer smaller than 8092 is interpreted in megabytes but the\n\nTypo? Shouldn't it be 8192?\n\n> user gets a warning. Also a minimum size of 1 MiB is enforced to avoid an\n> explosion of pack files.\n\n-Ilari\n"},{"id":"133643","messageId":"20100204212033.GB18668@spearce.org","threadId":"22514","inReplyTo":"alpine.LFD.2.00.1002041309090.1681@xanadu.home","subject":"Re: [PATCH 3/3] make --max-pack-size argument to 'git pack-object' count in bytes","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2010-02-04T21:20:33Z","receivedAt":"2010-02-04T21:20:33Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Nicolas Pitre wrote:\n> On Thu, 4 Feb 2010, Shawn O. Pearce wrote:\n> > Nicolas Pitre wrote:\n> > > > --max-pack-size=::\n> > > > -\tMaximum size of each output packfile, expressed in MiB.\n> > > > -\tThe default is 4096 (4 GiB) as that is the maximum allowed\n> > > > +\tMaximum size of each output packfile.\n> > > > +\tThe default is 4 GiB as that is the maximum allowed\n> > > > \tpackfile size (due to file format limitations). Some\n> > > > \timporters may wish to lower this, such as to ensure the\n> > > > \tresulting packfiles fit on CDs.\n> > > \n> > > What file format limitation is alluded to here? It has been a while \n> > > since the 4GB limit on pack file format has been removed.\n> > \n> > The pack index v1 32 bit offset thing. Which you fixed.\n> > \n> > > If this is a \n> > > limitation of fast-import only then maybe this should be explained more \n> > > explicitly.\n> > \n> > Damn. It is. fast-import can't write a v2 index. Ugh.\n> \n> Isn't it using write_idx_file()? That function would do it all for you \n> already.\n\nNope. Its still using its own code. I never ported it to\nwrite_idx_file(). And I don't have the CRC32 data available for\neach object (it failed to compute/save it as it wrote).\n\nI mean to port to write_idx_file() but didn't... :-(\n\n\n-- \nShawn.\n"}]}