Re: [BUG] resolved deltas
- From
René Scharfe <l.s.r@web.de>
- Date
- Aug 23, 2014, 10:12 UTC
- Message-ID
- <53F868F8.9080000@web.de>
- In-Reply-To
- <53F79CE3.60803@gmx.net>
Am 22.08.2014 um 21:41 schrieb Martin von Gagern:
Show 40 quoted lines
> 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 b6f32087526f43205bf8b5e6539936da787ecb1aGit 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 a0155d8d5bb58afb9a5d20459be023899c9a1cefCloning 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