{"thread":{"id":"28956","subject":"[PATCH] pack-object: tolerate broken packs that have duplicated objects","startedAt":"2011-11-17T06:04:03Z","lastAt":"2011-11-17T20:49:44Z","messageCount":2,"participants":["Junio C Hamano","Shawn Pearce"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"179614","messageId":"7v8vnfnv3g.fsf@alter.siamese.dyndns.org","threadId":"28956","inReplyTo":null,"subject":"[PATCH] pack-object: tolerate broken packs that have duplicated objects","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-17T06:04:03Z","receivedAt":"2011-11-17T06:04:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When --reuse-delta is in effect (which is the default), and an existing\npack in the repository has the same object registered twice (e.g. one copy\nin a non-delta format and the other copy in a delta against some other\nobject), an attempt to repack the repository can result in a cyclic delta\ndependency, causing write_one() function to infinitely recurse into\nitself.\n\nDetect such a case and break the loopy dependency by writing out an object\nthat is involved in such a loop in the non-delta format.\n\nSigned-off-by: Junio C Hamano \n---\n * This may need to be cherry-picked to earlier maintenance series. The\n topic I queued tonight is built on v1.7.6.4. \n\n builtin/pack-objects.c | 55 +++++++++++++++++++++++++++++++++++++----------\n 1 files changed, 43 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex c6e2d87..5ae808b 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -404,25 +404,56 @@ static unsigned long write_object(struct sha1file *f,\n \treturn hdrlen + datalen;\n }\n \n-static int write_one(struct sha1file *f,\n-\t\t\t struct object_entry *e,\n-\t\t\t off_t *offset)\n+enum write_one_status {\n+\tWRITE_ONE_SKIP = -1, /* already written */\n+\tWRITE_ONE_BREAK = 0, /* writing this will bust the limit; not written */\n+\tWRITE_ONE_WRITTEN = 1, /* normal */\n+\tWRITE_ONE_RECURSIVE = 2 /* already scheduled to be written */\n+};\n+\n+static enum write_one_status write_one(struct sha1file *f,\n+\t\t\t\t struct object_entry *e,\n+\t\t\t\t off_t *offset)\n {\n \tunsigned long size;\n+\tint recursing;\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/*\n+\t * we set offset to 1 (which is an impossible value) to mark\n+\t * the fact that this object is involved in \"write its base\n+\t * first before writing a deltified object\" recursion.\n+\t */\n+\trecursing = (e->idx.offset == 1);\n+\tif (recursing) {\n+\t\twarning(\"recursive delta detected for object %s\",\n+\t\t\tsha1_to_hex(e->idx.sha1));\n+\t\treturn WRITE_ONE_RECURSIVE;\n+\t} else if (e->idx.offset || e->preferred_base) {\n+\t\t/* offset is non zero if object is written already. */\n+\t\treturn WRITE_ONE_SKIP;\n+\t}\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+\tif (e->delta) {\n+\t\te->idx.offset = 1; /* now recurse */\n+\t\tswitch (write_one(f, e->delta, offset)) {\n+\t\tcase WRITE_ONE_RECURSIVE:\n+\t\t\t/* we cannot depend on this one */\n+\t\t\te->delta = NULL;\n+\t\t\tbreak;\n+\t\tdefault:\n+\t\t\tbreak;\n+\t\tcase WRITE_ONE_BREAK:\n+\t\t\te->idx.offset = recursing;\n+\t\t\treturn WRITE_ONE_BREAK;\n+\t\t}\n+\t}\n \n \te->idx.offset = *offset;\n \tsize = write_object(f, e, *offset);\n \tif (!size) {\n-\t\te->idx.offset = 0;\n-\t\treturn 0;\n+\t\te->idx.offset = recursing;\n+\t\treturn WRITE_ONE_BREAK;\n \t}\n \twritten_list[nr_written++] = &e->idx;\n \n@@ -430,7 +461,7 @@ static int write_one(struct sha1file *f,\n \tif (signed_add_overflows(*offset, size))\n \t\tdie(\"pack too large for current definition of off_t\");\n \t*offset += size;\n-\treturn 1;\n+\treturn WRITE_ONE_WRITTEN;\n }\n \n static void write_pack_file(void)\n@@ -468,7 +499,7 @@ static void write_pack_file(void)\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\tif (write_one(f, objects + i, &offset) == WRITE_ONE_BREAK)\n \t\t\t\tbreak;\n \t\t\tdisplay_progress(progress_state, written);\n \t\t}\n-- \n1.7.8.rc2.109.g72037\n"},{"id":"179648","messageId":"CAJo=hJsB_V0n8af0BPZs+qyQ3pVqBrb1xv8qfpifKgUJO=shTw@mail.gmail.com","threadId":"28956","inReplyTo":"7v8vnfnv3g.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] pack-object: tolerate broken packs that have duplicated objects","fromName":"Shawn Pearce","fromEmail":"spearce@spearce.org","sentAt":"2011-11-17T20:49:44Z","receivedAt":"2011-11-17T20:49:44Z","isPatch":true,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"On Wed, Nov 16, 2011 at 22:04, Junio C Hamano wrote:\n> When --reuse-delta is in effect (which is the default), and an existing\n> pack in the repository has the same object registered twice (e.g. one copy\n> in a non-delta format and the other copy in a delta against some other\n> object), an attempt to repack the repository can result in a cyclic delta\n> dependency, causing write_one() function to infinitely recurse into\n> itself.\n>\n> Detect such a case and break the loopy dependency by writing out an object\n> that is involved in such a loop in the non-delta format.\n>\n> Signed-off-by: Junio C Hamano \n\nLooks sane to me, thanks.\n"}]}