[PATCH] fsck.c: fix bogus "empty tree" check

[PATCH] fsck.c: fix bogus "empty tree" check

[Junio C Hamano]

ba002f3 (builtin-fsck: move common object checking code to fsck.c) did
more than what it claimed to.  Most notably, it wrongly made an empty tree
object an error by pretending to only move code from fsck_tree() in
builtin-fsck.c to fsck_tree() in fsck.c, but in fact adding a bogus check
to barf on an empty tree.

An empty tree object is _unusual_.  Recent porcelains try reasonably hard
not to let the user create a commit that contains such a tree.  Perhaps
warning about them in git-fsck may have some merit.

However, being unusual and being errorneous are two quite different
things.  This is especially true now we seem to use the same
fsck_$object() code in places other than git-fsck itself.  For example,
receive-pack should not reject unusual objects, even if it would be a good
idea to tighten it to reject incorrect ones.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I've wasted a few hours tonight hunting for random breakages in "git
   push", the symptom of which is "fatal: unresolved deltas left after
   unpacking."  I was hoping this patch would fix it, but it seems that
   the problem is elsewhere.

   I'll revert the following two commits for now:

   d5ef408 (unpack-objects: prevent writing of inconsistent objects)
   28f72a0 (receive-pack: use strict mode for unpacking objects)

   as I have verified that running with receive.fsckobjects set to false
   fixes the issues for me, and the repository at the receiving end (both
   before and after the push) pass git-fsck without problems.  Needless to
   say, I am not a happy camper right now.

 fsck.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/fsck.c b/fsck.c
index 6883d1b..797e317 100644
--- a/fsck.c
+++ b/fsck.c
@@ -155,8 +155,6 @@ static int fsck_tree(struct tree *item, int strict, fsck_error error_func)
 	o_mode = 0;
 	o_name = NULL;
 	o_sha1 = NULL;
-	if (!desc.size)
-		return error_func(&item->object, FSCK_ERROR, "empty tree");
 
 	while (desc.size) {
 		unsigned mode;
-- 
1.5.4.3.529.gb25fb

Re: [PATCH] fsck.c: fix bogus "empty tree" check

[Sergey Vlasov]

[-- Attachment #1: Type: text/plain, Size: 3175 bytes --]

On Tue, 04 Mar 2008 03:21:16 -0800 Junio C Hamano wrote:

>  * I've wasted a few hours tonight hunting for random breakages in "git
>    push", the symptom of which is "fatal: unresolved deltas left after
>    unpacking."  I was hoping this patch would fix it, but it seems that
>    the problem is elsewhere.
>
>    I'll revert the following two commits for now:
>
>    d5ef408 (unpack-objects: prevent writing of inconsistent objects)
>    28f72a0 (receive-pack: use strict mode for unpacking objects)
>
>    as I have verified that running with receive.fsckobjects set to false
>    fixes the issues for me, and the repository at the receiving end (both
>    before and after the push) pass git-fsck without problems.  Needless to
>    say, I am not a happy camper right now.

This part of commit d5ef408 changes is bogus:

> @@ -144,9 +205,36 @@ static void added_object(unsigned nr, enum object_type type,
>  static void write_object(unsigned nr, enum object_type type,
>  			 void *buf, unsigned long size)
>  {
> -	if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
> -		die("failed to write object");
>  	added_object(nr, type, buf, size);

The write_sha1_file() call here was calculating obj_list[nr].sha1; now
it is removed, but added_object() needs this value:

| static void added_object(unsigned nr, enum object_type type,
| 			 void *data, unsigned long size)
| {
| 	struct delta_info **p = &delta_list;
| 	struct delta_info *info;
|
| 	while ((info = *p) != NULL) {
| 		if (!hashcmp(info->base_sha1, obj_list[nr].sha1) ||
					      ^^^^^^^^^^^^^^^^^
| 		    info->base_offset == obj_list[nr].offset) {
| 			*p = info->next;
| 			p = &delta_list;
| 			resolve_delta(info->nr, type, data, size,
| 				      info->delta, info->size);
| 			free(info);
| 			continue;
| 		}
| 		p = &info->next;
| 	}
| }

However, I do not have time to create a proper test case for this.

> +	if (!strict) {
> +		if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
> +			die("failed to write object");
> +		free(buf);
> +		obj_list[nr].obj = 0;
> +	} else if (type == OBJ_BLOB) {
> +		struct blob *blob;
> +		if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
> +			die("failed to write object");
> +		free(buf);
> +
> +		blob = lookup_blob(obj_list[nr].sha1);
> +		if (blob)
> +			blob->object.flags |= FLAG_WRITTEN;
> +		else
> +			die("invalid blob object");
> +		obj_list[nr].obj = 0;
> +	} else {
> +		struct object *obj;
> +		int eaten;
> +		hash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);
> +		obj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);
> +		if (!obj)
> +			die("invalid %s", typename(type));
> +		/* buf is stored via add_object_buffer and in obj, if its a tree or commit */
> +		add_object_buffer(obj, buf, size);
> +		obj->flags |= FLAG_OPEN;
> +		obj_list[nr].obj = obj;
> +	}
>  }
>
>  static void resolve_delta(unsigned nr, enum object_type type,

The simplest way to fix this would be to duplicate the added_object()
call in all branches; invoking hash_sha1_file() unconditionally will
work too, but may be wasteful if we need to call write_sha1_file()
afterwards.

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH] fsck.c: fix bogus "empty tree" check

[Junio C Hamano]

Sergey Vlasov <vsu@altlinux.ru> writes:

>>    I'll revert the following two commits for now:
>>
>>    d5ef408 (unpack-objects: prevent writing of inconsistent objects)
>>    28f72a0 (receive-pack: use strict mode for unpacking objects)
>>
>>    as I have verified that running with receive.fsckobjects set to false
>>    fixes the issues for me, and the repository at the receiving end (both
>>    before and after the push) pass git-fsck without problems.  Needless to
>>    say, I am not a happy camper right now.
>
> This part of commit d5ef408 changes is bogus:
>
>> @@ -144,9 +205,36 @@ static void added_object(unsigned nr, enum object_type type,
>>  static void write_object(unsigned nr, enum object_type type,
>>  			 void *buf, unsigned long size)
>>  {
>> -	if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
>> -		die("failed to write object");
>>  	added_object(nr, type, buf, size);
>
> The write_sha1_file() call here was calculating obj_list[nr].sha1; now
> it is removed, but added_object() needs this value:

Thanks, somehow I missed that when merging it up for 'next'.

> However, I do not have time to create a proper test case for this.

That's Ok.  What we need is a fix but it is not that urgent as the stuff
is now reverted for now.

Sorry for being a sloppy maintainer.  I have to admit that I did not read
every single line of patches in a few topics merged to 'master' recently,
due to workload pressure, and some extra eyeballs after-the-fact are
greatly appreciated.

Re: [PATCH] fsck.c: fix bogus "empty tree" check

[Martin Koegler]

On Tue, Mar 04, 2008 at 03:26:35PM +0300, Sergey Vlasov wrote:
> On Tue, 04 Mar 2008 03:21:16 -0800 Junio C Hamano wrote:
> The simplest way to fix this would be to duplicate the added_object()
> call in all branches; invoking hash_sha1_file() unconditionally will
> work too, but may be wasteful if we need to call write_sha1_file()
> afterwards.

This is only a part of the problem. Moving added_object only makes
forward reference for deltas work.

unpack_delta_entry checks for OBJ_REF_DELTA, if there is a sha1 file.
This must not be true, if --strict is passed. It needs to check the
cache too.

>From 843d84fa52ff546bf88f135522e5739070d712aa Mon Sep 17 00:00:00 2001
From: Martin Koegler <mkoegler@auto.tuwien.ac.at>
Date: Tue, 4 Mar 2008 22:38:21 +0100
Subject: [PATCH] unpack-objects: fix delta handling

Signed-off-by: Martin Koegler <mkoegler@auto.tuwien.ac.at>
---
 builtin-unpack-objects.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 1845abc..c0d3c9a 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -206,16 +206,17 @@ static void added_object(unsigned nr, enum object_type type,
 static void write_object(unsigned nr, enum object_type type,
 			 void *buf, unsigned long size)
 {
-	added_object(nr, type, buf, size);
 	if (!strict) {
 		if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
 			die("failed to write object");
+		added_object(nr, type, buf, size);
 		free(buf);
 		obj_list[nr].obj = 0;
 	} else if (type == OBJ_BLOB) {
 		struct blob *blob;
 		if (write_sha1_file(buf, size, typename(type), obj_list[nr].sha1) < 0)
 			die("failed to write object");
+		added_object(nr, type, buf, size);
 		free(buf);
 
 		blob = lookup_blob(obj_list[nr].sha1);
@@ -228,6 +229,7 @@ static void write_object(unsigned nr, enum object_type type,
 		struct object *obj;
 		int eaten;
 		hash_sha1_file(buf, size, typename(type), obj_list[nr].sha1);
+		added_object(nr, type, buf, size);
 		obj = parse_object_buffer(obj_list[nr].sha1, type, size, buf, &eaten);
 		if (!obj)
 			die("invalid %s", typename(type));
@@ -301,7 +303,10 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 			free(delta_data);
 			return;
 		}
-		if (!has_sha1_file(base_sha1)) {
+		obj = lookup_object(base_sha1);
+		if (obj && lookup_object_buffer(obj))
+			;
+		else if (!has_sha1_file(base_sha1)) {
 			hashcpy(obj_list[nr].sha1, null_sha1);
 			add_delta_to_list(nr, base_sha1, 0, delta_data, delta_size);
 			return;
-- 
1.5.4.GIT

[PATCH] t5300: add test for "unpack-objects --strict"

[Junio C Hamano]

This adds a test for unpacking deltified objects with --strict option.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 Junio C Hamano <gitster@pobox.com> writes:

 > Sergey Vlasov <vsu@altlinux.ru> writes:
 > ...
 >> However, I do not have time to create a proper test case for this.
 >
 > That's Ok.  What we need is a fix but it is not that urgent as the stuff
 > is now reverted for now.

 t/t5300-pack-object.sh |   36 ++++++++++++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index cd3c149..0cf0ff7 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -274,4 +274,40 @@ test_expect_success \
      packname_4=$(git pack-objects test-4 <obj-list) &&
      test 3 = $(ls test-4-*.pack | wc -l)'
 
+test_expect_failure 'unpacking with --strict' '
+
+	git config --unset pack.packsizelimit &&
+	COPYING=$(git hash-object -w ../../COPYING) &&
+	for j in a b c d e f g
+	do
+		for i in 0 1 2 3 4 5 6 7 8 9
+		do
+			o=$(echo $j$i | git hash-object -w --stdin) &&
+			echo "100644 $o	0 $j$i"
+		done
+	done >LIST &&
+	rm -f .git/index &&
+	git update-index --index-info <LIST &&
+	LIST=$(git write-tree) &&
+	rm -f .git/index &&
+	head -n 10 LIST | git update-index --index-info &&
+	LI=$(git write-tree) &&
+	rm -f .git/index &&
+	tail -n 10 LIST | git update-index --index-info &&
+	ST=$(git write-tree) &&
+	PACK=$( (
+		echo "$LIST"
+		echo "$LI"
+		echo "$ST"
+	) | git pack-objects test-5 ) &&
+
+	test_create_repo another &&
+
+	(
+		cd another &&
+		git unpack-objects --strict <../test-5-$PACK.pack
+	)
+
+'
+
 test_done
-- 
1.5.4.3.529.gb25fb

[PATCH v2] t5300: add test for "unpack-objects --strict"

[Junio C Hamano]

This adds test for unpacking deltified objects with --strict option.

 - unpacking full trees with --strict should pass;

 - unpacking only trees with --strict should be rejected due to
   missing blobs;

 - unpacking only trees with --strict into an existing
   repository with necessary blobs should succeed.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 * The pack created by the test of the original one contained
   only trees and unpacked into an empty repository, and --strict
   has every right to complain.  It was not a good test.

 t/t5300-pack-object.sh |   46 ++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 46 insertions(+), 0 deletions(-)

diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index cd3c149..2e70e5f 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -274,4 +274,50 @@ test_expect_success \
      packname_4=$(git pack-objects test-4 <obj-list) &&
      test 3 = $(ls test-4-*.pack | wc -l)'
 
+test_expect_failure 'unpacking with --strict' '
+
+	git config --unset pack.packsizelimit &&
+	COPYING=$(git hash-object -w ../../COPYING) &&
+	for j in a b c d e f g
+	do
+		for i in 0 1 2 3 4 5 6 7 8 9
+		do
+			o=$(echo $j$i | git hash-object -w --stdin) &&
+			echo "100644 $o	0 $j$i"
+		done
+	done >LIST &&
+	rm -f .git/index &&
+	git update-index --index-info <LIST &&
+	LIST=$(git write-tree) &&
+	rm -f .git/index &&
+	head -n 10 LIST | git update-index --index-info &&
+	LI=$(git write-tree) &&
+	rm -f .git/index &&
+	tail -n 10 LIST | git update-index --index-info &&
+	ST=$(git write-tree) &&
+	PACK5=$( git rev-list --objects "$LIST" "$LI" "$ST" | \
+		git pack-objects test-5 ) &&
+	PACK6=$( git rev-list "$LIST" "$LI" "$ST" | \
+		git pack-objects test-6 ) &&
+	test_create_repo test-5 &&
+	(
+		cd test-5 &&
+		git unpack-objects --strict <../test-5-$PACK5.pack &&
+		git ls-tree -r $LIST &&
+		git ls-tree -r $LI &&
+		git ls-tree -r $ST
+	) &&
+	test_create_repo test-6 &&
+	(
+		# tree-only into empty repo -- many unreachables
+		cd test-6 &&
+		test_must_fail git unpack-objects --strict <../test-6-$PACK6.pack
+	) &&
+	(
+		# already populated -- no unreachables
+		cd test-5 &&
+		git unpack-objects --strict <../test-6-$PACK6.pack
+	)
+'
+
 test_done
-- 
1.5.4.3.529.gb25fb