[PATCH] Fix "unpack-objects --strict"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 13, 2009, 19:33 UTC
- Message-ID
- <7vd46zbjae.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20090813111933.GZ14475@mail-vs.djpig.de>
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