Re: [PATCH 8/9] clone: die on errors from unpack_trees

2013-03-26 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 When clone is populating the working tree, it ignores the
 return status from unpack_trees; this means we may report a
 successful clone, even when the checkout fails.

 When checkout fails, we may want to leave the $GIT_DIR in
 place, as it might be possible to recover the data through
 further use of git checkout (e.g., if the checkout failed
 due to a transient error, disk full, etc). However, we
 already die on a number of other checkout-related errors, so
 this patch follows that pattern.

 In addition to marking a now-passing test, we need to adjust
 t5710, which blindly assumed it could make bogus clones of
 very deep alternates hierarchies. By using --bare, we can
 avoid it actually touching any objects.

 Signed-off-by: Jeff King p...@peff.net
 ---

Thanks.

 I think the leave the data behind fix may be to just set junk_pid =
 0 a little sooner in cmd_clone (i.e., before checkout()). Then we
 would still die, but at least leave the fetched objects intact.

Yeah, perhaps, but I agree that is a much lower priority change.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[PATCH 8/9] clone: die on errors from unpack_trees

2013-03-25 Thread Jeff King
When clone is populating the working tree, it ignores the
return status from unpack_trees; this means we may report a
successful clone, even when the checkout fails.

When checkout fails, we may want to leave the $GIT_DIR in
place, as it might be possible to recover the data through
further use of git checkout (e.g., if the checkout failed
due to a transient error, disk full, etc). However, we
already die on a number of other checkout-related errors, so
this patch follows that pattern.

In addition to marking a now-passing test, we need to adjust
t5710, which blindly assumed it could make bogus clones of
very deep alternates hierarchies. By using --bare, we can
avoid it actually touching any objects.

Signed-off-by: Jeff King p...@peff.net
---
I think the leave the data behind fix may be to just set junk_pid =
0 a little sooner in cmd_clone (i.e., before checkout()). Then we
would still die, but at least leave the fetched objects intact.

 builtin/clone.c  | 3 ++-
 t/t1060-object-corruption.sh | 2 +-
 t/t5710-info-alternate.sh| 2 +-
 3 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index e0aaf13..7d48ef3 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -579,7 +579,8 @@ static int checkout(void)
tree = parse_tree_indirect(sha1);
parse_tree(tree);
init_tree_desc(t, tree-buffer, tree-size);
-   unpack_trees(1, t, opts);
+   if (unpack_trees(1, t, opts)  0)
+   die(_(unable to checkout working tree));
 
if (write_cache(fd, active_cache, active_nr) ||
commit_locked_index(lock_file))
diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index eb285c0..05ba4e7 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -89,7 +89,7 @@ test_expect_success 'clone --local detects corruption' '
test_must_fail git clone --local bit-error corrupt-checkout
 '
 
-test_expect_failure 'clone --local detects missing objects' '
+test_expect_success 'clone --local detects missing objects' '
test_must_fail git clone --local missing missing-checkout
 '
 
diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh
index aa04529..5a6e49d 100755
--- a/t/t5710-info-alternate.sh
+++ b/t/t5710-info-alternate.sh
@@ -58,7 +58,7 @@ git clone -l -s F G 
 git clone -l -s D E 
 git clone -l -s E F 
 git clone -l -s F G 
-git clone -l -s G H'
+git clone --bare -l -s G H'
 
 test_expect_success 'invalidity of deepest repository' \
 'cd H  {
-- 
1.8.2.13.g0f18d3c

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html