# [PATCH] pack-object: tolerate broken packs that have duplicated objects

2 messages from 2011-11-17 to 2011-11-17. Participants: Junio C Hamano, Shawn Pearce.
Thread: https://gitlist.dev/t/28956

## Junio C Hamano, 2011-11-17 06:04

Subject: [PATCH] pack-object: tolerate broken packs that have duplicated objects
Message-ID: <7v8vnfnv3g.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7v8vnfnv3g.fsf%40alter.siamese.dyndns.org

```
When --reuse-delta is in effect (which is the default), and an existing
pack in the repository has the same object registered twice (e.g. one copy
in a non-delta format and the other copy in a delta against some other
object), an attempt to repack the repository can result in a cyclic delta
dependency, causing write_one() function to infinitely recurse into
itself.

Detect such a case and break the loopy dependency by writing out an object
that is involved in such a loop in the non-delta format.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * This may need to be cherry-picked to earlier maintenance series. The
   topic I queued tonight is built on v1.7.6.4.   

 builtin/pack-objects.c |   55 +++++++++++++++++++++++++++++++++++++----------
 1 files changed, 43 insertions(+), 12 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c6e2d87..5ae808b 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -404,25 +404,56 @@ static unsigned long write_object(struct sha1file *f,
 	return hdrlen + datalen;
 }
 
-static int write_one(struct sha1file *f,
-			       struct object_entry *e,
-			       off_t *offset)
+enum write_one_status {
+	WRITE_ONE_SKIP = -1, /* already written */
+	WRITE_ONE_BREAK = 0, /* writing this will bust the limit; not written */
+	WRITE_ONE_WRITTEN = 1, /* normal */
+	WRITE_ONE_RECURSIVE = 2 /* already scheduled to be written */
+};
+
+static enum write_one_status write_one(struct sha1file *f,
+				       struct object_entry *e,
+				       off_t *offset)
 {
 	unsigned long size;
+	int recursing;
 
-	/* offset is non zero if object is written already. */
-	if (e->idx.offset || e->preferred_base)
-		return -1;
+	/*
+	 * we set offset to 1 (which is an impossible value) to mark
+	 * the fact that this object is involved in "write its base
+	 * first before writing a deltified object" recursion.
+	 */
+	recursing = (e->idx.offset == 1);
+	if (recursing) {
+		warning("recursive delta detected for object %s",
+			sha1_to_hex(e->idx.sha1));
+		return WRITE_ONE_RECURSIVE;
+	} else if (e->idx.offset || e->preferred_base) {
+		/* offset is non zero if object is written already. */
+		return WRITE_ONE_SKIP;
+	}
 
 	/* if we are deltified, write out base object first. */
-	if (e->delta && !write_one(f, e->delta, offset))
-		return 0;
+	if (e->delta) {
+		e->idx.offset = 1; /* now recurse */
+		switch (write_one(f, e->delta, offset)) {
+		case WRITE_ONE_RECURSIVE:
+			/* we cannot depend on this one */
+			e->delta = NULL;
+			break;
+		default:
+			break;
+		case WRITE_ONE_BREAK:
+			e->idx.offset = recursing;
+			return WRITE_ONE_BREAK;
+		}
+	}
 
 	e->idx.offset = *offset;
 	size = write_object(f, e, *offset);
 	if (!size) {
-		e->idx.offset = 0;
-		return 0;
+		e->idx.offset = recursing;
+		return WRITE_ONE_BREAK;
 	}
 	written_list[nr_written++] = &e->idx;
 
@@ -430,7 +461,7 @@ static int write_one(struct sha1file *f,
 	if (signed_add_overflows(*offset, size))
 		die("pack too large for current definition of off_t");
 	*offset += size;
-	return 1;
+	return WRITE_ONE_WRITTEN;
 }
 
 static void write_pack_file(void)
@@ -468,7 +499,7 @@ static void write_pack_file(void)
 		offset = sizeof(hdr);
 		nr_written = 0;
 		for (; i < nr_objects; i++) {
-			if (!write_one(f, objects + i, &offset))
+			if (write_one(f, objects + i, &offset) == WRITE_ONE_BREAK)
 				break;
 			display_progress(progress_state, written);
 		}
-- 
1.7.8.rc2.109.g72037

```

## Shawn Pearce, 2011-11-17 20:49

Subject: Re: [PATCH] pack-object: tolerate broken packs that have duplicated objects
Message-ID: <CAJo=hJsB_V0n8af0BPZs+qyQ3pVqBrb1xv8qfpifKgUJO=shTw@mail.gmail.com>
URL: https://gitlist.dev/e/CAJo%3DhJsB_V0n8af0BPZs%2BqyQ3pVqBrb1xv8qfpifKgUJO%3DshTw%40mail.gmail.com
In-Reply-To: <7v8vnfnv3g.fsf@alter.siamese.dyndns.org>

```
On Wed, Nov 16, 2011 at 22:04, Junio C Hamano <gitster@pobox.com> wrote:
> When --reuse-delta is in effect (which is the default), and an existing
> pack in the repository has the same object registered twice (e.g. one copy
> in a non-delta format and the other copy in a delta against some other
> object), an attempt to repack the repository can result in a cyclic delta
> dependency, causing write_one() function to infinitely recurse into
> itself.
>
> Detect such a case and break the loopy dependency by writing out an object
> that is involved in such a loop in the non-delta format.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Looks sane to me, thanks.

```