Subject: Re: [BUG] resolved deltas

From: René Scharfe <[email-masked]>
Date: Sat, 23 Aug 2014 12:12:08 +0200

Am 22.08.2014 um 21:41 schrieb Martin von Gagern:
> On 21.08.2014 13:35, Petr Stodulka wrote:
>> Hi guys,
>> I wanted post you patch here for this bug, but I can't find primary
>> source of this problem [0], because I don't understand some ideas in the
>> code.
>>
>> […]
>>
>> Any next ideas/hints or explanation of these functions? I began study
>> source code and mechanisms of the git this week, so don't beat me yet
>> please :-)
>>
>> Regards,
>> Petr
>>
>> [0] https://bugzilla.redhat.com/show_bug.cgi?id=1099919
> 
> Some pointers to related reports and investigations:
> 
> https://groups.google.com/forum/#!topic/mapsforge-dev/IF6mgmwvZmY
> https://groups.google.com/forum/#!topic/mapsforge-dev/f2KvFALlkvo
> https://code.google.com/p/support/issues/detail?id=31571
> https://groups.google.com/forum/#!topic/mapsforge-dev/nomzr5dkkqc
> http://thread.gmane.org/gmane.comp.version-control.git/254626
> 
> The last is my own bug report to this mailing list here, which
> unfortunately received no reaction yet. In that report, I can confirm
> that the commit introducing the assertion is the same commit which
> causes things to fail:
> https://github.com/git/git/commit/7218a215efc7ae46f7ca8d82442f354e
> 
> In this https://code.google.com/p/mapsforge/ repository, resolve_delta
> gets called twice for some delta. The first time, type and real_type are
> identical, but by the second pass in find_unresolved_deltas the real
> type will have chaned to OBJ_TREE. This caused the old code to simply
> scip the object, but the new code aborts instead.
> 
> So far my understanding. I'm not sure whether this kind of duplicate
> resolution is something normal or indicates some breakage in the
> repository in question. But aborting seems a bad solution in either case.

Git 1.7.6 clones the repo without reporting an error or warning, but a
check shows that a tree object is missing:

  $ git clone https://code.google.com/p/mapsforge/
  Cloning into mapsforge...
  remote: Counting objects: 12879, done.
  Receiving objects: 100% (12879/12879), 10.19 MiB | 2.48 MiB/s, done.
  Resolving deltas: 100% (4349/4349), done.
  $ cd mapsforge/
  $ git fsck
  broken link from    tree eb95fb0268c43f512e2ce512e6625072acafbdb5
                to    tree a0155d8d5bb58afb9a5d20459be023899c9a1cef
  missing tree a0155d8d5bb58afb9a5d20459be023899c9a1cef
  dangling tree b6f32087526f43205bf8b5e6539936da787ecb1a

Git 2.1.0 hits an assertion:

  $ git clone https://code.google.com/p/mapsforge/
  Cloning into 'mapsforge'...
  remote: Counting objects: 12879, done.
  Receiving objects: 100% (12879/12879), 10.19 MiB | 2.53 MiB/s, done.
  git: builtin/index-pack.c:918: find_unresolved_deltas_1: Assertion `child->real_type == OBJ_REF_DELTA' failed.
  error: index-pack died of signal 6
  fatal: index-pack failed

The patch below, which makes index-pack ignore objects with unexpected
real_type as before, changes the shown error message, but clone still
fails:

  $ git clone https://code.google.com/p/mapsforge/
  Cloning into 'mapsforge'...
  remote: Counting objects: 12879, done.
  Receiving objects: 100% (12879/12879), 10.19 MiB | 2.43 MiB/s, done.
  Resolving deltas: 100% (4348/4348), done.
  fatal: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef
  fatal: index-pack failed

Turning that fatal error into a warning get us a bit further:

  $ git clone https://code.google.com/p/mapsforge/
  Cloning into 'mapsforge'...
  remote: Counting objects: 12879, done.
  Receiving objects: 100% (12879/12879), 10.19 MiB | 2.38 MiB/s, done.
  Resolving deltas: 100% (4350/4350), done.
  warning: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef
  fatal: The same object bc386be34bd4781f71bb68d72a6e0aee3124201e appears twice in the pack
  fatal: index-pack failed

Disabling strict checking (WRITE_IDX_STRICT) as well lets clone
succeed, but a check shows that a tree is missing, as wiht git 1.7.6:

  $ git clone https://code.google.com/p/mapsforge/
  Cloning into 'mapsforge'...
  remote: Counting objects: 12879, done.
  Receiving objects: 100% (12879/12879), 10.19 MiB | 2.37 MiB/s, done.
  Resolving deltas: 100% (4349/4349), done.
  warning: did not receive expected object a0155d8d5bb58afb9a5d20459be023899c9a1cef
  Checking connectivity... done.
  $ cd mapsforge/
  $ git fsck
  Checking object directories: 100% (256/256), done.
  Checking objects: 100% (12879/12879), done.
  broken link from    tree eb95fb0268c43f512e2ce512e6625072acafbdb5
                to    tree a0155d8d5bb58afb9a5d20459be023899c9a1cef
  missing tree a0155d8d5bb58afb9a5d20459be023899c9a1cef

Cloning the repo with Egit works fine, but git fsck shows the same
results as above.

https://github.com/applantation/mapsforge has that missing tree
object, by the way.

OK, what now?  It's better to show an error message instead of
failing an assertion if a repo is found to be corrupt because the
issue is caused by external input.  I don't know if the patch
below may have any bad side effects, though, so no sign-off at
this time.

Allowing git to clone a broken repo sounds useful, up to point.
In this particular case older versions had no problem doing that,
so it seems worth supporting.

And how did the tree object went missing in the first place?

René
---
 builtin/index-pack.c | 33 +++++++++++++++++++--------------
 1 file changed, 19 insertions(+), 14 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..f7dc5b0 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -913,28 +913,33 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,
 
 	if (base->ref_first <= base->ref_last) {
 		struct object_entry *child = objects + deltas[base->ref_first].obj_no;
-		struct base_data *result = alloc_base_data();
 
-		assert(child->real_type == OBJ_REF_DELTA);
-		resolve_delta(child, base, result);
-		if (base->ref_first == base->ref_last && base->ofs_last == -1)
-			free_base_data(base);
+		if (child->real_type == OBJ_REF_DELTA) {
+			struct base_data *result = alloc_base_data();
 
-		base->ref_first++;
-		return result;
+			resolve_delta(child, base, result);
+			if (base->ref_first == base->ref_last &&
+			    base->ofs_last == -1)
+				free_base_data(base);
+
+			base->ref_first++;
+			return result;
+		}
 	}
 
 	if (base->ofs_first <= base->ofs_last) {
 		struct object_entry *child = objects + deltas[base->ofs_first].obj_no;
-		struct base_data *result = alloc_base_data();
 
-		assert(child->real_type == OBJ_OFS_DELTA);
-		resolve_delta(child, base, result);
-		if (base->ofs_first == base->ofs_last)
-			free_base_data(base);
+		if (child->real_type == OBJ_OFS_DELTA) {
+			struct base_data *result = alloc_base_data();
 
-		base->ofs_first++;
-		return result;
+			resolve_delta(child, base, result);
+			if (base->ofs_first == base->ofs_last)
+				free_base_data(base);
+
+			base->ofs_first++;
+			return result;
+		}
 	}
 
 	unlink_base_data(base);
-- 
2.1.0