{"thread":{"id":"12490","subject":"[PATCH] fsck.c: fix bogus \"empty tree\" check","startedAt":"2008-03-04T11:21:16Z","lastAt":"2008-03-05T09:17:41Z","messageCount":6,"participants":["Junio C Hamano","Sergey Vlasov","Martin Koegler"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"70921","messageId":"7vbq5u91lf.fsf@gitster.siamese.dyndns.org","threadId":"12490","inReplyTo":null,"subject":"[PATCH] fsck.c: fix bogus \"empty tree\" check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T11:21:16Z","receivedAt":"2008-03-04T11:21:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"ba002f3 (builtin-fsck: move common object checking code to fsck.c) did\nmore than what it claimed to. Most notably, it wrongly made an empty tree\nobject an error by pretending to only move code from fsck_tree() in\nbuiltin-fsck.c to fsck_tree() in fsck.c, but in fact adding a bogus check\nto barf on an empty tree.\n\nAn empty tree object is _unusual_. Recent porcelains try reasonably hard\nnot to let the user create a commit that contains such a tree. Perhaps\nwarning about them in git-fsck may have some merit.\n\nHowever, being unusual and being errorneous are two quite different\nthings. This is especially true now we seem to use the same\nfsck_$object() code in places other than git-fsck itself. For example,\nreceive-pack should not reject unusual objects, even if it would be a good\nidea to tighten it to reject incorrect ones.\n\nSigned-off-by: Junio C Hamano \n---\n\n * I've wasted a few hours tonight hunting for random breakages in \"git\n push\", the symptom of which is \"fatal: unresolved deltas left after\n unpacking.\" I was hoping this patch would fix it, but it seems that\n the problem is elsewhere.\n\n I'll revert the following two commits for now:\n\n d5ef408 (unpack-objects: prevent writing of inconsistent objects)\n 28f72a0 (receive-pack: use strict mode for unpacking objects)\n\n as I have verified that running with receive.fsckobjects set to false\n fixes the issues for me, and the repository at the receiving end (both\n before and after the push) pass git-fsck without problems. Needless to\n say, I am not a happy camper right now.\n\n fsck.c | 2 --\n 1 files changed, 0 insertions(+), 2 deletions(-)\n\ndiff --git a/fsck.c b/fsck.c\nindex 6883d1b..797e317 100644\n--- a/fsck.c\n+++ b/fsck.c\n@@ -155,8 +155,6 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)\n \to_mode = 0;\n \to_name = NULL;\n \to_sha1 = NULL;\n-\tif (!desc.size)\n-\t\treturn error_func(&item->object, FSCK_ERROR, \"empty tree\");\n \n \twhile (desc.size) {\n \t\tunsigned mode;\n-- \n1.5.4.3.529.gb25fb\n\n"},{"id":"70938","messageId":"20080304152635.40451f7c.vsu@altlinux.ru","threadId":"12490","inReplyTo":"7vbq5u91lf.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] fsck.c: fix bogus \"empty tree\" check","fromName":"Sergey Vlasov","fromEmail":"vsu@altlinux.ru","sentAt":"2008-03-04T12:26:35Z","receivedAt":"2008-03-04T12:26:35Z","isPatch":true,"sender":{"key":"vsu@altlinux.ru","avatar":"https://avatars.githubusercontent.com/u/616082?v=4"},"body":"On Tue, 04 Mar 2008 03:21:16 -0800 Junio C Hamano wrote:\n\n> * I've wasted a few hours tonight hunting for random breakages in \"git\n> push\", the symptom of which is \"fatal: unresolved deltas left after\n> unpacking.\" I was hoping this patch would fix it, but it seems that\n> the problem is elsewhere.\n>\n> I'll revert the following two commits for now:\n>\n> d5ef408 (unpack-objects: prevent writing of inconsistent objects)\n> 28f72a0 (receive-pack: use strict mode for unpacking objects)\n>\n> as I have verified that running with receive.fsckobjects set to false\n> fixes the issues for me, and the repository at the receiving end (both\n> before and after the push) pass git-fsck without problems. Needless to\n> say, I am not a happy camper right now.\n\nThis part of commit d5ef408 changes is bogus:\n\n> @@ -144,9 +205,36 @@ static void added_object(unsigned nr, enum object_type type,\n> static void write_object(unsigned nr, enum object_type type,\n> \t\t\t void *buf, unsigned long size)\n> {\n> -\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> -\t\tdie(\"failed to write object\");\n> \tadded_object(nr, type, buf, size);\n\nThe write_sha1_file() call here was calculating obj_list[nr].sha1; now\nit is removed, but added_object() needs this value:\n\n| static void added_object(unsigned nr, enum object_type type,\n| \t\t\t void *data, unsigned long size)\n| {\n| \tstruct delta_info **p = &delta_list;\n| \tstruct delta_info *info;\n|\n| \twhile ((info = *p) != NULL) {\n| \t\tif (!hashcmp(info->base_sha1, obj_list[nr].sha1) ||\n\t\t\t\t\t ^^^^^^^^^^^^^^^^^\n| \t\t info->base_offset == obj_list[nr].offset) {\n| \t\t\t*p = info->next;\n| \t\t\tp = &delta_list;\n| \t\t\tresolve_delta(info->nr, type, data, size,\n| \t\t\t\t info->delta, info->size);\n| \t\t\tfree(info);\n| \t\t\tcontinue;\n| \t\t}\n| \t\tp = &info->next;\n| \t}\n| }\n\nHowever, I do not have time to create a proper test case for this.\n\n> +\tif (!strict) {\n> +\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> +\t\t\tdie(\"failed to write object\");\n> +\t\tfree(buf);\n> +\t\tobj_list[nr].obj = 0;\n> +\t} else if (type == OBJ_BLOB) {\n> +\t\tstruct blob *blob;\n> +\t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n> +\t\t\tdie(\"failed to write object\");\n> +\t\tfree(buf);\n> +\n> +\t\tblob = lookup_blob(obj_list[nr].sha1);\n> +\t\tif (blob)\n> +\t\t\tblob->object.flags |= FLAG_WRITTEN;\n> +\t\telse\n> +\t\t\tdie(\"invalid blob object\");\n> +\t\tobj_list[nr].obj = 0;\n> +\t} else {\n> +\t\tstruct object *obj;\n> +\t\tint eaten;\n> +\t\thash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);\n> +\t\tobj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);\n> +\t\tif (!obj)\n> +\t\t\tdie(\"invalid %s\", typename(type));\n> +\t\t/* buf is stored via add_object_buffer and in obj, if its a tree or commit */\n> +\t\tadd_object_buffer(obj, buf, size);\n> +\t\tobj->flags |= FLAG_OPEN;\n> +\t\tobj_list[nr].obj = obj;\n> +\t}\n> }\n>\n> static void resolve_delta(unsigned nr, enum object_type type,\n\nThe simplest way to fix this would be to duplicate the added_object()\ncall in all branches; invoking hash_sha1_file() unconditionally will\nwork too, but may be wasteful if we need to call write_sha1_file()\nafterwards.\n"},{"id":"70987","messageId":"7vfxv65kkl.fsf@gitster.siamese.dyndns.org","threadId":"12490","inReplyTo":"20080304152635.40451f7c.vsu@altlinux.ru","subject":"Re: [PATCH] fsck.c: fix bogus \"empty tree\" check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-04T19:57:14Z","receivedAt":"2008-03-04T19:57:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Sergey Vlasov writes:\n\n>> I'll revert the following two commits for now:\n>>\n>> d5ef408 (unpack-objects: prevent writing of inconsistent objects)\n>> 28f72a0 (receive-pack: use strict mode for unpacking objects)\n>>\n>> as I have verified that running with receive.fsckobjects set to false\n>> fixes the issues for me, and the repository at the receiving end (both\n>> before and after the push) pass git-fsck without problems. Needless to\n>> say, I am not a happy camper right now.\n>\n> This part of commit d5ef408 changes is bogus:\n>\n>> @@ -144,9 +205,36 @@ static void added_object(unsigned nr, enum object_type type,\n>> static void write_object(unsigned nr, enum object_type type,\n>> \t\t\t void *buf, unsigned long size)\n>> {\n>> -\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n>> -\t\tdie(\"failed to write object\");\n>> \tadded_object(nr, type, buf, size);\n>\n> The write_sha1_file() call here was calculating obj_list[nr].sha1; now\n> it is removed, but added_object() needs this value:\n\nThanks, somehow I missed that when merging it up for 'next'.\n\n> However, I do not have time to create a proper test case for this.\n\nThat's Ok. What we need is a fix but it is not that urgent as the stuff\nis now reverted for now.\n\nSorry for being a sloppy maintainer. I have to admit that I did not read\nevery single line of patches in a few topics merged to 'master' recently,\ndue to workload pressure, and some extra eyeballs after-the-fact are\ngreatly appreciated.\n"},{"id":"70998","messageId":"20080304214801.GA5595@auto.tuwien.ac.at","threadId":"12490","inReplyTo":"20080304152635.40451f7c.vsu@altlinux.ru","subject":"Re: [PATCH] fsck.c: fix bogus \"empty tree\" check","fromName":"Martin Koegler","fromEmail":"mkoegler@auto.tuwien.ac.at","sentAt":"2008-03-04T21:48:01Z","receivedAt":"2008-03-04T21:48:01Z","isPatch":true,"sender":{"key":"mkoegler@auto.tuwien.ac.at","avatar":null},"body":"On Tue, Mar 04, 2008 at 03:26:35PM +0300, Sergey Vlasov wrote:\n> On Tue, 04 Mar 2008 03:21:16 -0800 Junio C Hamano wrote:\n> The simplest way to fix this would be to duplicate the added_object()\n> call in all branches; invoking hash_sha1_file() unconditionally will\n> work too, but may be wasteful if we need to call write_sha1_file()\n> afterwards.\n\nThis is only a part of the problem. Moving added_object only makes\nforward reference for deltas work.\n\nunpack_delta_entry checks for OBJ_REF_DELTA, if there is a sha1 file.\nThis must not be true, if --strict is passed. It needs to check the\ncache too.\n\n>From 843d84fa52ff546bf88f135522e5739070d712aa Mon Sep 17 00:00:00 2001\nFrom: Martin Koegler \nDate: Tue, 4 Mar 2008 22:38:21 +0100\nSubject: [PATCH] unpack-objects: fix delta handling\n\nSigned-off-by: Martin Koegler \n---\n builtin-unpack-objects.c | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c\nindex 1845abc..c0d3c9a 100644\n--- a/builtin-unpack-objects.c\n+++ b/builtin-unpack-objects.c\n@@ -206,16 +206,17 @@ static void added_object(unsigned nr, enum object_type type,\n static void write_object(unsigned nr, enum object_type type,\n \t\t\t void *buf, unsigned long size)\n {\n-\tadded_object(nr, type, buf, size);\n \tif (!strict) {\n \t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n \t\t\tdie(\"failed to write object\");\n+\t\tadded_object(nr, type, buf, size);\n \t\tfree(buf);\n \t\tobj_list[nr].obj = 0;\n \t} else if (type == OBJ_BLOB) {\n \t\tstruct blob *blob;\n \t\tif (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)\n \t\t\tdie(\"failed to write object\");\n+\t\tadded_object(nr, type, buf, size);\n \t\tfree(buf);\n \n \t\tblob = lookup_blob(obj_list[nr].sha1);\n@@ -228,6 +229,7 @@ static void write_object(unsigned nr, enum object_type type,\n \t\tstruct object *obj;\n \t\tint eaten;\n \t\thash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);\n+\t\tadded_object(nr, type, buf, size);\n \t\tobj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);\n \t\tif (!obj)\n \t\t\tdie(\"invalid %s\", typename(type));\n@@ -301,7 +303,10 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,\n \t\t\tfree(delta_data);\n \t\t\treturn;\n \t\t}\n-\t\tif (!has_sha1_file(base_sha1)) {\n+\t\tobj = lookup_object(base_sha1);\n+\t\tif (obj && lookup_object_buffer(obj))\n+\t\t\t;\n+\t\telse if (!has_sha1_file(base_sha1)) {\n \t\t\thashcpy(obj_list[nr].sha1, null_sha1);\n \t\t\tadd_delta_to_list(nr, base_sha1, 0, delta_data, delta_size);\n \t\t\treturn;\n-- \n1.5.4.GIT\n\n\n"},{"id":"71052","messageId":"7v4pblpng7.fsf@gitster.siamese.dyndns.org","threadId":"12490","inReplyTo":"7vfxv65kkl.fsf@gitster.siamese.dyndns.org","subject":"[PATCH] t5300: add test for \"unpack-objects --strict\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-03-05T08:47:04Z","receivedAt":"2008-03-05T08:47:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"This adds a test for unpacking deltified objects with --strict option.\n\nSigned-off-by: Junio C Hamano \n---\n\n Junio C Hamano writes:\n\n > Sergey Vlasov writes:\n > ...\n >> However, I do not have time to create a proper test case for this.\n >\n > That's Ok. What we need is a fix but it is not that urgent as the stuff\n > is now reverted for now.\n\n t/t5300-pack-object.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 files changed, 36 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex cd3c149..0cf0ff7 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -274,4 +274,40 @@ test_expect_success \\\n packname_4=$(git pack-objects test-4 LIST &&\n+\trm -f .git/index &&\n+\tgit update-index --index-info \n---\n * The pack created by the test of the original one contained\n only trees and unpacked into an empty repository, and --strict\n has every right to complain. It was not a good test.\n\n t/t5300-pack-object.sh | 46 ++++++++++++++++++++++++++++++++++++++++++++++\n 1 files changed, 46 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh\nindex cd3c149..2e70e5f 100755\n--- a/t/t5300-pack-object.sh\n+++ b/t/t5300-pack-object.sh\n@@ -274,4 +274,50 @@ test_expect_success \\\n packname_4=$(git pack-objects test-4 LIST &&\n+\trm -f .git/index &&\n+\tgit update-index --index-info