{"thread":{"id":"8370","subject":"[PATCH] fix repack with --max-pack-size","startedAt":"2007-05-31T01:43:12Z","lastAt":"2007-06-01T00:01:38Z","messageCount":2,"participants":["Nicolas Pitre","Dana How"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"43671","messageId":"alpine.LFD.0.99.0705302114430.11491@xanadu.home","threadId":"8370","inReplyTo":null,"subject":"[PATCH] fix repack with --max-pack-size","fromName":"Nicolas Pitre","fromEmail":"nico@cam.org","sentAt":"2007-05-31T01:43:12Z","receivedAt":"2007-05-31T01:43:12Z","isPatch":true,"sender":{"key":"nico@fluxnic.net","avatar":"https://avatars.githubusercontent.com/u/702790?v=4"},"body":"Two issues here:\n\n1) git-repack -a --max-pack-size=10 on the GIT repo dies pretty quick.\n   There is a lot of confusion about deltas that were suposed to be \n   reused from another pack but that get stored undeltified due to pack\n   limit and object size doesn't match entry->size anymore.  This test \n   is not really worth the complexity for determining when it is valid\n   so get rid of it.\n\n2) If pack limit is reached, the object buffer is freed, including when \n   it comes from a cached delta data.  In practice the object will be \n   stored in a subsequent pack undeltified, but let's make sure no \n   pointer to freed data subsists by clearing entry->delta_data.\n\nI also reorganized that code a bit to make it more readable.\n\nSigned-off-by: Nicolas Pitre <nico@cam.org>\n---\n\ndiff --git a/builtin-pack-objects.c b/builtin-pack-objects.c\nindex 41472fc..ccb25f6 100644\n--- a/builtin-pack-objects.c\n+++ b/builtin-pack-objects.c\n@@ -410,31 +410,24 @@ static unsigned long write_object(struct sha1file *f,\n \t\tz_stream stream;\n \t\tunsigned long maxsize;\n \t\tvoid *out;\n-\t\tif (entry->delta_data && usable_delta) {\n-\t\t\tbuf = entry->delta_data;\n+\t\tif (!usable_delta) {\n+\t\t\tbuf = read_sha1_file(entry->sha1, &obj_type, &size);\n+\t\t\tif (!buf)\n+\t\t\t\tdie(\"unable to read %s\", sha1_to_hex(entry->sha1));\n+\t\t} else if (entry->delta_data) {\n \t\t\tsize = entry->delta_size;\n+\t\t\tbuf = entry->delta_data;\n+\t\t\tentry->delta_data = NULL;\n \t\t\tobj_type = (allow_ofs_delta && entry->delta->offset) ?\n \t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n \t\t} else {\n \t\t\tbuf = read_sha1_file(entry->sha1, &type, &size);\n \t\t\tif (!buf)\n \t\t\t\tdie(\"unable to read %s\", sha1_to_hex(entry->sha1));\n-\t\t\tif (size != entry->size)\n-\t\t\t\tdie(\"object %s size inconsistency (%lu vs %lu)\",\n-\t\t\t\t    sha1_to_hex(entry->sha1), size, entry->size);\n-\t\t\tif (usable_delta) {\n-\t\t\t\tbuf = delta_against(buf, size, entry);\n-\t\t\t\tsize = entry->delta_size;\n-\t\t\t\tobj_type = (allow_ofs_delta && entry->delta->offset) ?\n-\t\t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n-\t\t\t} else {\n-\t\t\t\t/*\n-\t\t\t\t * recover real object type in case\n-\t\t\t\t * check_object() wanted to re-use a delta,\n-\t\t\t\t * but we couldn't since base was in previous split pack\n-\t\t\t\t */\n-\t\t\t\tobj_type = type;\n-\t\t\t}\n+\t\t\tbuf = delta_against(buf, size, entry);\n+\t\t\tsize = entry->delta_size;\n+\t\t\tobj_type = (allow_ofs_delta && entry->delta->offset) ?\n+\t\t\t\tOBJ_OFS_DELTA : OBJ_REF_DELTA;\n \t\t}\n \t\t/* compress the data to store and put compressed length in datalen */\n \t\tmemset(&stream, 0, sizeof(stream));\n"},{"id":"43708","messageId":"56b7f5510705311701i7f76f06cq82617f0cd1dfc562@mail.gmail.com","threadId":"8370","inReplyTo":"alpine.LFD.0.99.0705302114430.11491@xanadu.home","subject":"Re: [PATCH] fix repack with --max-pack-size","fromName":"Dana How","fromEmail":"danahow@gmail.com","sentAt":"2007-06-01T00:01:38Z","receivedAt":"2007-06-01T00:01:38Z","isPatch":true,"sender":{"key":"danahow@gmail.com","avatar":null},"body":"On 5/30/07, Nicolas Pitre <nico@cam.org> wrote:\n> Two issues here:\n>\n> 1) git-repack -a --max-pack-size=10 on the GIT repo dies pretty quick.\n>    There is a lot of confusion about deltas that were suposed to be\n>    reused from another pack but that get stored undeltified due to pack\n>    limit and object size doesn't match entry->size anymore.  This test\n>    is not really worth the complexity for determining when it is valid\n>    so get rid of it.\n\nThis is very real.  A smaller fix would have been the hunk\n@@ -408,7 +407,7 @@ static unsigned long write_object(struct sha1file *f,\n>               buf = read_sha1_file(entry->sha1, &type, &size);\n>               if (!buf)\n>                       die(\"unable to read %s\", sha1_to_hex(entry->sha1));\n>-               if (size != entry->size)\n>+               if (size != entry->size && type == obj_type)\n>                       die(\"object %s size inconsistency (%lu vs %lu)\",\n>                           sha1_to_hex(entry->sha1), size, entry->size);\n>               if (usable_delta) {\nfrom the max-blob-size patch that hasn't converged yet.\n\nBoth pack splitting and blob-size limiting could cause a delta base\nnot to appear in the pack containing a delta using the base.\nThen you get the size mismatch Nicolas discussed\nwhen the real object must be used and its size doesn't match\nthe delta sized stored in the size field.\nI first saw this with max-blob-size,  but only realized recently\nthat it also applied to max-pack-size.  Sorry I didn't post a patch;\nwe are swamped at the moment.\n\nACK [but I didn't review (2)].\n\nThanks,\n-- \nDana L. How  danahow@gmail.com  +1 650 804 5991 cell\n"}]}