[PATCH] Fix "unpack-objects --strict"

[Junio C Hamano <gitster@pobox.com>]

When unpack-objects is run under the --strict option, objects that have
pointers to other objects are verified for the reachability at the end, by
calling check_object() on each of them, and letting check_object to walk
the reachable objects from them using fsck_walk() recursively.

The function however misunderstands the semantics of fsck_walk() function
when it makes a call to it, setting itself as the callback.  fsck_walk()
expects the callback function to return a non-zero value to signal an
error (negative value causes an immediate abort, positive value is still
an error but allows further checks on sibling objects) and return zero to
signal a success.  The function however returned 1 on some non error
cases, and to cover up this mistake, complained only when fsck_walk() did
not detect any error.

To fix this double-bug, make the function return zero on all success
cases, and also check for non-zero return from fsck_walk() for an error.

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

Caused by b41860b (unpack-objects: prevent writing of inconsistent
objects, 2008-02-25), which introduced these checks and also the code to
keep unverified objects in core until check_objects() verifies their
reachability.  While I think it is a good idea to check for incomplete
pack data, I do not think it is necessary to keep them in core.  We can
simply error out to signal the caller not to update the refs.

We probably should write everything as they become unpackable (i.e. as
their delta bases becomes available) while keeping track of object names
(but not data) of structured objects that we received, and running only
one level of reachability check on them at the end.  That would certainly
reduce the memory consumption and may simplify the complexity of the code
at the same time.

But I'll leave that to other people.  Hint, hint...

 builtin-unpack-objects.c       |    8 ++++----
 t/t5531-deep-submodule-push.sh |   32 ++++++++++++++++++++++++++++++++
 2 files changed, 36 insertions(+), 4 deletions(-)

diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 557148a..109b7c8 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -184,7 +184,7 @@ static int check_object(struct object *obj, int type, void *data)
 		return 0;
 
 	if (obj->flags & FLAG_WRITTEN)
-		return 1;
+		return 0;
 
 	if (type != OBJ_ANY && obj->type != type)
 		die("object type mismatch");
@@ -195,15 +195,15 @@ static int check_object(struct object *obj, int type, void *data)
 		if (type != obj->type || type <= 0)
 			die("object of unexpected type");
 		obj->flags |= FLAG_WRITTEN;
-		return 1;
+		return 0;
 	}
 
 	if (fsck_object(obj, 1, fsck_error_function))
 		die("Error in object");
-	if (!fsck_walk(obj, check_object, NULL))
+	if (fsck_walk(obj, check_object, NULL))
 		die("Error on reachable objects of %s", sha1_to_hex(obj->sha1));
 	write_cached_object(obj);
-	return 1;
+	return 0;
 }
 
 static void write_rest(void)
diff --git a/t/t5531-deep-submodule-push.sh b/t/t5531-deep-submodule-push.sh
new file mode 100755
index 0000000..13b8e40
--- /dev/null
+++ b/t/t5531-deep-submodule-push.sh
@@ -0,0 +1,32 @@
+#!/bin/sh
+
+test_description='unpack-objects'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+	git init --bare pub.git &&
+	GIT_DIR=pub.git git config receive.fsckobjects true &&
+	git init work &&
+	(
+		cd work &&
+		git init gar/bage &&
+		(
+			cd gar/bage &&
+			>junk &&
+			git add junk &&
+			git commit -m "Initial junk"
+		) &&
+		git add gar/bage &&
+		git commit -m "Initial superproject"
+	)
+'
+
+test_expect_failure push '
+	(
+		cd work &&
+		git push ../pub.git master
+	)
+'
+
+test_done