{"thread":{"id":"28421","subject":"[PATCH 0/3] fast-import: fix pack_id corner cases","startedAt":"2011-09-18T19:01:45Z","lastAt":"2011-09-18T21:40:10Z","messageCount":9,"participants":["Dmitry Ivankov","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"175730","messageId":"1316372508-7173-1-git-send-email-divanorama@gmail.com","threadId":"28421","inReplyTo":null,"subject":"[PATCH 0/3] fast-import: fix pack_id corner cases","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T19:01:45Z","receivedAt":"2011-09-18T19:01:45Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"[1/3] is just a precaution unlikely to happen as having 65536+ packs\nproduced in fast-import looks extremely rare.\n[2/2] is more real bug. Shouldn't be too hard to reproduce, but I'm\ncurrently too lazy to as it is quite rare and not likely to get broken\nagain.\n[3/3] is pure cosmetic change while I'm on pack_id topic.\n\nDmitry Ivankov (3):\n  fast-import: die if we produce too many (MAX_PACK_ID) packs\n  fast-import: fix corner case for checkpoint\n  fast-import: rename object_count to pack_object_count\n\n fast-import.c |   29 +++++++++++++++--------------\n 1 files changed, 15 insertions(+), 14 deletions(-)\n\n-- \n1.7.3.4\n"},{"id":"175731","messageId":"1316372508-7173-2-git-send-email-divanorama@gmail.com","threadId":"28421","inReplyTo":"1316372508-7173-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 1/3] fast-import: die if we produce too many (MAX_PACK_ID) packs","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T19:01:46Z","receivedAt":"2011-09-18T19:01:46Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"In fast-import pack_id is 16-bit with MAX_PACK_ID reserved to identify\npre-existing objects. It is unlikely to wrap under reasonable settings\nbut still things in fast-import will break once it happens.\n\nAdd a check and immediate die() as the simplest reaction to being unable\nto continue the import.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n fast-import.c |    2 ++\n 1 files changed, 2 insertions(+), 0 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 742e7da..907cb05 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -1009,6 +1009,8 @@ static void end_packfile(void)\n static void cycle_packfile(void)\n {\n \tend_packfile();\n+\tif (pack_id >= MAX_PACK_ID)\n+\t\tdie(\"too many (%u) packs produced\", pack_id);\n \tstart_packfile();\n }\n \n-- \n1.7.3.4\n"},{"id":"175733","messageId":"1316372508-7173-3-git-send-email-divanorama@gmail.com","threadId":"28421","inReplyTo":"1316372508-7173-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 2/3] fast-import: fix corner case for checkpoint","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T19:01:47Z","receivedAt":"2011-09-18T19:01:47Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"checkpoint command makes fast-import finish current pack and write out\nbranches/tags and marks. In case no new objects are added in current\npack fast-import falls back to no-op. While it is possible that refs\nor marks need to be updated (to point to old objects).\n\nMake fast-import always dump them on checkpoint. But as before do not\ncycle_packfile if there are no objects to write.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n fast-import.c |    9 ++++-----\n 1 files changed, 4 insertions(+), 5 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 907cb05..014a807 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -3025,12 +3025,11 @@ static void parse_ls(struct branch *b)\n static void checkpoint(void)\n {\n \tcheckpoint_requested = 0;\n-\tif (object_count) {\n+\tif (object_count)\n \t\tcycle_packfile();\n-\t\tdump_branches();\n-\t\tdump_tags();\n-\t\tdump_marks();\n-\t}\n+\tdump_branches();\n+\tdump_tags();\n+\tdump_marks();\n }\n \n static void parse_checkpoint(void)\n-- \n1.7.3.4\n"},{"id":"175732","messageId":"1316372508-7173-4-git-send-email-divanorama@gmail.com","threadId":"28421","inReplyTo":"1316372508-7173-1-git-send-email-divanorama@gmail.com","subject":"[PATCH 3/3] fast-import: rename object_count to pack_object_count","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T19:01:48Z","receivedAt":"2011-09-18T19:01:48Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"object_count is used to count objects that'll go to the current pack.\nWhile object_count_by_* are used to count total amount of objects and\nare not used to determine if current packfile is empty.\n\nRename (and move declaration) object_count to pack_object_count to\navoid possible confusion.\n\nSigned-off-by: Dmitry Ivankov <divanorama@gmail.com>\n---\n fast-import.c |   20 ++++++++++----------\n 1 files changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/fast-import.c b/fast-import.c\nindex 014a807..8f2411b 100644\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -290,7 +290,6 @@ static uintmax_t object_count_by_type[1 << TYPE_BITS];\n static uintmax_t duplicate_count_by_type[1 << TYPE_BITS];\n static uintmax_t delta_count_by_type[1 << TYPE_BITS];\n static uintmax_t delta_count_attempts_by_type[1 << TYPE_BITS];\n-static unsigned long object_count;\n static unsigned long branch_count;\n static unsigned long branch_load_count;\n static int failure;\n@@ -316,6 +315,7 @@ static struct sha1file *pack_file;\n static struct packed_git *pack_data;\n static struct packed_git **all_packs;\n static off_t pack_size;\n+static unsigned long pack_object_count;\n \n /* Table of objects we've written. */\n static unsigned int object_entry_alloc = 5000;\n@@ -880,7 +880,7 @@ static void start_packfile(void)\n \n \tpack_data = p;\n \tpack_size = sizeof(hdr);\n-\tobject_count = 0;\n+\tpack_object_count = 0;\n \n \tall_packs = xrealloc(all_packs, sizeof(*all_packs) * (pack_id + 1));\n \tall_packs[pack_id] = p;\n@@ -894,17 +894,17 @@ static const char *create_index(void)\n \tstruct object_entry_pool *o;\n \n \t/* Build the table of object IDs. */\n-\tidx = xmalloc(object_count * sizeof(*idx));\n+\tidx = xmalloc(pack_object_count * sizeof(*idx));\n \tc = idx;\n \tfor (o = blocks; o; o = o->next_pool)\n \t\tfor (e = o->next_free; e-- != o->entries;)\n \t\t\tif (pack_id == e->pack_id)\n \t\t\t\t*c++ = &e->idx;\n-\tlast = idx + object_count;\n+\tlast = idx + pack_object_count;\n \tif (c != last)\n \t\tdie(\"internal consistency error creating the index\");\n \n-\ttmpfile = write_idx_file(NULL, idx, object_count, &pack_idx_opts, pack_data->sha1);\n+\ttmpfile = write_idx_file(NULL, idx, pack_object_count, &pack_idx_opts, pack_data->sha1);\n \tfree(idx);\n \treturn tmpfile;\n }\n@@ -953,7 +953,7 @@ static void end_packfile(void)\n \tstruct packed_git *old_p = pack_data, *new_p;\n \n \tclear_delta_base_cache();\n-\tif (object_count) {\n+\tif (pack_object_count) {\n \t\tunsigned char cur_pack_sha1[20];\n \t\tchar *idx_name;\n \t\tint i;\n@@ -963,7 +963,7 @@ static void end_packfile(void)\n \t\tclose_pack_windows(pack_data);\n \t\tsha1close(pack_file, cur_pack_sha1, 0);\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    pack_data->pack_name, pack_object_count,\n \t\t\t\t    cur_pack_sha1, pack_size);\n \t\tclose(pack_data->pack_fd);\n \t\tidx_name = keep_pack(create_index());\n@@ -1103,7 +1103,7 @@ static int store_object(\n \te->type = type;\n \te->pack_id = pack_id;\n \te->idx.offset = pack_size;\n-\tobject_count++;\n+\tpack_object_count++;\n \tobject_count_by_type[type]++;\n \n \tcrc32_begin(pack_file);\n@@ -1267,7 +1267,7 @@ static void stream_blob(uintmax_t len, unsigned char *sha1out, uintmax_t mark)\n \t\te->pack_id = pack_id;\n \t\te->idx.offset = offset;\n \t\te->idx.crc32 = crc32_end(pack_file);\n-\t\tobject_count++;\n+\t\tpack_object_count++;\n \t\tobject_count_by_type[OBJ_BLOB]++;\n \t}\n \n@@ -3025,7 +3025,7 @@ static void parse_ls(struct branch *b)\n static void checkpoint(void)\n {\n \tcheckpoint_requested = 0;\n-\tif (object_count)\n+\tif (pack_object_count)\n \t\tcycle_packfile();\n \tdump_branches();\n \tdump_tags();\n-- \n1.7.3.4\n"},{"id":"175734","messageId":"20110918191741.GD2308@elie","threadId":"28421","inReplyTo":"1316372508-7173-2-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 1/3] fast-import: die if we produce too many (MAX_PACK_ID) packs","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-18T19:17:41Z","receivedAt":"2011-09-18T19:17:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> In fast-import pack_id is 16-bit with MAX_PACK_ID reserved to identify\n> pre-existing objects. It is unlikely to wrap under reasonable settings\n> but still things in fast-import will break once it happens.\n>\n> Add a check and immediate die() as the simplest reaction to being unable\n> to continue the import.\n\nMakes a lot of sense.  A few possible minor clarity improvements:\n\n - missing commas after \"In fast-import\" and before \"with MAX_PACK_ID\n   reserved\"\n - \"pre-existing objects\": it would be clearer to say something like\n   \"objects this fast-import process instance did not write out to a\n   packfile\", like the comment before gfi_unpack_entry() does\n - I suppose \"under reasonable settings\" means \"with a reasonable\n   max-pack-size setting\"?\n - \"things will break\" is a bit vague.\n - \"immediate\" -> \"immediately\"\n\nMaybe:\n\n\tIn fast-import, pack_id is a 16-bit unsigned integer, with MAX_PACK_ID\n\t(2^16 - 1) reserved for use by objects that are not in a packfile that\n\tthis fast-import process instance wrote.  It is unusual for pack_id to\n\thit MAX_PACK_ID with a reasonable --max-pack-size setting, but when it\n\tdoes, the pack_id stored in each \"struct object_entry\" wraps and\n\tfast-import gets utterly confused.\n\n\tAdd a check and immediately die() so the operator can at least see what\n\twent wrong instead of experiencing an unexplained broken import.\n\nWith or without that clarification,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n\nThanks!  A test would still be nice, if someone has time to write one.\n"},{"id":"175736","messageId":"20110918192851.GE2308@elie","threadId":"28421","inReplyTo":"1316372508-7173-3-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 2/3] fast-import: fix corner case for checkpoint","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-18T19:28:51Z","receivedAt":"2011-09-18T19:28:51Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> checkpoint command makes fast-import finish current pack and write out\n> branches/tags and marks. In case no new objects are added in current\n> pack fast-import falls back to no-op. While it is possible that refs\n> or marks need to be updated (to point to old objects).\n>\n> Make fast-import always dump them on checkpoint. But as before do not\n> cycle_packfile if there are no objects to write.\n\nYeah, that would be annoying to run into.  Rearranging the description\na little for clarity and brevity:\n\n\tfast-import: update refs on checkpoint even if there are no new objects\n\n\tDuring an import using the fast-import command, it is possible for\n\tno new objects to have been added between two checkpoints requested\n\twith the SIGUSR1 signal or the \"checkpoint\" command.  Even in this\n\tcase, fast-import should write out any updated refs and marks to\n\tfulfill the second checkpoint request.\n\n\tAs before, fast-import will not write an empty pack and start a new\n\tone when there are no new objects to write out.\n\nWith that change,\nReviewed-by: Jonathan Nieder <jrnieder@gmail.com>\n"},{"id":"175737","messageId":"20110918193205.GF2308@elie","threadId":"28421","inReplyTo":"1316372508-7173-4-git-send-email-divanorama@gmail.com","subject":"Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-18T19:32:05Z","receivedAt":"2011-09-18T19:32:05Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> object_count is used to count objects that'll go to the current pack.\n> While object_count_by_* are used to count total amount of objects and\n> are not used to determine if current packfile is empty.\n>\n> Rename (and move declaration) object_count to pack_object_count to\n> avoid possible confusion.\n\nNo strong opinion on this one.  I guess the important thing is that\nyou are moving the declaration to the group of declarations labelled as\n\n\t/* The .pack file being generated */\n\n.  Is it important to rename the variable while at it (which will\ndisrupt other patches in flight using that variable if they exist)?\n"},{"id":"175740","messageId":"CA+gfSn8aOWPm=xmTE9WzuXsQY0EfYypFxRAyVb-x3_kmhNUb-Q@mail.gmail.com","threadId":"28421","inReplyTo":"20110918193205.GF2308@elie","subject":"Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count","fromName":"Dmitry Ivankov","fromEmail":"divanorama@gmail.com","sentAt":"2011-09-18T19:51:27Z","receivedAt":"2011-09-18T19:51:27Z","isPatch":true,"sender":{"key":"divanorama@gmail.com","avatar":"https://avatars.githubusercontent.com/u/158999?v=4"},"body":"On Mon, Sep 19, 2011 at 1:32 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:\n> Dmitry Ivankov wrote:\n>\n>> object_count is used to count objects that'll go to the current pack.\n>> While object_count_by_* are used to count total amount of objects and\n>> are not used to determine if current packfile is empty.\n>>\n>> Rename (and move declaration) object_count to pack_object_count to\n>> avoid possible confusion.\n>\n> No strong opinion on this one.  I guess the important thing is that\n> you are moving the declaration to the group of declarations labelled as\n>\n>        /* The .pack file being generated */\n>\n> .  Is it important to rename the variable while at it (which will\n> disrupt other patches in flight using that variable if they exist)?\nNot that important. Maybe a huge comment will do more and better.\nobject_count++ still appears near object_count_by_type[type]++, but\nhopefully one will look for their declarations and thus avoid the confusion.\n\n--- a/fast-import.c\n+++ b/fast-import.c\n@@ -290,7 +290,6 @@ static uintmax_t object_count_by_type[1 << TYPE_BITS];\n static uintmax_t duplicate_count_by_type[1 << TYPE_BITS];\n static uintmax_t delta_count_by_type[1 << TYPE_BITS];\n static uintmax_t delta_count_attempts_by_type[1 << TYPE_BITS];\n-static unsigned long object_count;\n static unsigned long branch_count;\n static unsigned long branch_load_count;\n static int failure;\n@@ -310,8 +309,16 @@ static unsigned int atom_cnt;\n static struct atom_str **atom_table;\n\n /* The .pack file being generated */\n+/*\n+ * objects that are being written to the current pack\n+ * all *must* have current pack_id in struct object_entry.\n+ * And object_count *must* be a count of object_entry's\n+ * having current pack_id. This data is used to create\n+ * index file once current pack_file is finished.\n+ */\n static struct pack_idx_option pack_idx_opts;\n static unsigned int pack_id;\n+static unsigned long object_count;\n static struct sha1file *pack_file;\n static struct packed_git *pack_data;\n static struct packed_git **all_packs;\n"},{"id":"175753","messageId":"20110918214010.GK2308@elie","threadId":"28421","inReplyTo":"CA+gfSn8aOWPm=xmTE9WzuXsQY0EfYypFxRAyVb-x3_kmhNUb-Q@mail.gmail.com","subject":"Re: [PATCH 3/3] fast-import: rename object_count to pack_object_count","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-09-18T21:40:10Z","receivedAt":"2011-09-18T21:40:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Dmitry Ivankov wrote:\n\n> --- a/fast-import.c\n> +++ b/fast-import.c\n[...]\n> @@ -310,8 +309,16 @@ static unsigned int atom_cnt;\n>  static struct atom_str **atom_table;\n> \n>  /* The .pack file being generated */\n> +/*\n> + * objects that are being written to the current pack\n> + * all *must* have current pack_id in struct object_entry.\n> + * And object_count *must* be a count of object_entry's\n> + * having current pack_id. This data is used to create\n> + * index file once current pack_file is finished.\n> + */\n>  static struct pack_idx_option pack_idx_opts;\n>  static unsigned int pack_id;\n> +static unsigned long object_count;\n>  static struct sha1file *pack_file;\n\nCloser.  Now I am tempted to nitpick and say that this should be\na single comment, formatted in complete sentences, and written to\nbe descriptive rather than normative when possible (since norms\nwill inevitably change over time, and future readers should not\nhave an excuse to be afraid to adjust the comment to match code\nchanges).\n\n\t/*\n\t * The .pack file being generated\n\t *\n\t * Objects that are being written to the current pack store the\n\t * current value of \"pack_id\" in struct object_entry.\n\t * \"object_count\" counts the object_entrys with the current\n\t * pack_id.  These values are used to create the pack index\n\t * file when the current pack is finished.\n\t */\n\tstatic struct pack_idx_option pack_idx_opts;\n\tstatic unsigned int pack_id;\n\t...\n"}]}