From 7b6257b0d45f562629ac660cb1845c8a2aac7df1 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 25 Mar 2013 16:21:34 -0400 Subject: add test for streaming corrupt blobs We do not have many tests for handling corrupt objects. This new test at least checks that we detect a byte error in a corrupt blob object while streaming it out with cat-file. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 34 ++++++++++++++++++++++++++++++++++ 1 file changed, 34 insertions(+) create mode 100755 t/t1060-object-corruption.sh (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh new file mode 100755 index 0000000000..c887dd86b0 --- /dev/null +++ b/t/t1060-object-corruption.sh @@ -0,0 +1,34 @@ +#!/bin/sh + +test_description='see how we handle various forms of corruption' +. ./test-lib.sh + +# convert "1234abcd" to ".git/objects/12/34abcd" +obj_to_file() { + echo "$(git rev-parse --git-dir)/objects/$(git rev-parse "$1" | sed 's,..,&/,')" +} + +# Convert byte at offset "$2" of object "$1" into '\0' +corrupt_byte() { + obj_file=$(obj_to_file "$1") && + chmod +w "$obj_file" && + printf '\0' | dd of="$obj_file" bs=1 seek="$2" conv=notrunc +} + +test_expect_success 'setup corrupt repo' ' + git init bit-error && + ( + cd bit-error && + test_commit content && + corrupt_byte HEAD:content.t 10 + ) +' + +test_expect_success 'streaming a corrupt blob fails' ' + ( + cd bit-error && + test_must_fail git cat-file blob HEAD:content.t + ) +' + +test_done -- cgit v1.2.3 From d9c31e14d0aafdd45a382d01fcfd66c65a5f4b95 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 25 Mar 2013 17:49:36 -0400 Subject: streaming_write_entry: propagate streaming errors When we are streaming an index blob to disk, we store the error from stream_blob_to_fd in the "result" variable, and then immediately overwrite that with the return value of "close". That means we catch errors on close (e.g., problems committing the file to disk), but miss anything which happened before then. We can fix this by using bitwise-OR to accumulate errors in our result variable. While we're here, we can also simplify the error handling with an early return, which makes it easier to see under which circumstances we need to clean up. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index c887dd86b0..cfd1f23a11 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -24,6 +24,15 @@ test_expect_success 'setup corrupt repo' ' ) ' +test_expect_success 'setup repo with missing object' ' + git init missing && + ( + cd missing && + test_commit content && + rm -f "$(obj_to_file HEAD:content.t)" + ) +' + test_expect_success 'streaming a corrupt blob fails' ' ( cd bit-error && @@ -31,4 +40,20 @@ test_expect_success 'streaming a corrupt blob fails' ' ) ' +test_expect_success 'read-tree -u detects bit-errors in blobs' ' + ( + cd bit-error && + rm -f content.t && + test_must_fail git read-tree --reset -u HEAD + ) +' + +test_expect_success 'read-tree -u detects missing objects' ' + ( + cd missing && + rm -f content.t && + test_must_fail git read-tree --reset -u HEAD + ) +' + test_done -- cgit v1.2.3 From 0e15ad9b730a1516f8a786523266707c4d26f5ab Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 25 Mar 2013 16:22:29 -0400 Subject: add tests for cloning corrupted repositories We try not to let corruption pass unnoticed over fetches and clones. For the most part, this works, but there are some broken corner cases, including: 1. We do not detect missing objects over git-aware transports. This is a little hard to test, because the sending side will actually complain about the missing object. To fool it, we corrupt a repository such that we have a "misnamed" object: it claims to be sha1 X, but is really Y. This lets the sender blindly transmit it, but it is the receiver's responsibility to verify that what it got is sane (and it does not). 2. We do not detect missing or misnamed blobs during the checkout phase of clone. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 41 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 41 insertions(+) (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index cfd1f23a11..e29406e27a 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -33,6 +33,19 @@ test_expect_success 'setup repo with missing object' ' ) ' +test_expect_success 'setup repo with misnamed object' ' + git init misnamed && + ( + cd misnamed && + test_commit content && + good=$(obj_to_file HEAD:content.t) && + blob=$(echo corrupt | git hash-object -w --stdin) && + bad=$(obj_to_file $blob) && + rm -f "$good" && + mv "$bad" "$good" + ) +' + test_expect_success 'streaming a corrupt blob fails' ' ( cd bit-error && @@ -56,4 +69,32 @@ test_expect_success 'read-tree -u detects missing objects' ' ) ' +# We use --bare to make sure that the transport detects it, not the checkout +# phase. +test_expect_success 'clone --no-local --bare detects corruption' ' + test_must_fail git clone --no-local --bare bit-error corrupt-transport +' + +test_expect_success 'clone --no-local --bare detects missing object' ' + test_must_fail git clone --no-local --bare missing missing-transport +' + +test_expect_failure 'clone --no-local --bare detects misnamed object' ' + test_must_fail git clone --no-local --bare misnamed misnamed-transport +' + +# We do not expect --local to detect corruption at the transport layer, +# so we are really checking the checkout() code path. +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_must_fail git clone --local missing missing-checkout +' + +test_expect_failure 'clone --local detects misnamed objects' ' + test_must_fail git clone --local misnamed misnamed-checkout +' + test_done -- cgit v1.2.3 From 0aac7bb287645dd72ad8ad6b805128b8ff7f111f Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 25 Mar 2013 16:23:59 -0400 Subject: clone: die on errors from unpack_trees 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 Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 2 +- t/t5710-info-alternate.sh | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index e29406e27a..4e7030e613 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 aa045295de..5a6e49d18d 100755 --- a/t/t5710-info-alternate.sh +++ b/t/t5710-info-alternate.sh @@ -58,7 +58,7 @@ test_expect_success 'creating too deep nesting' \ 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 && { -- cgit v1.2.3 From 0433ad128c59f233046b3f8a68246ca3a8a77af8 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Mon, 25 Mar 2013 16:26:27 -0400 Subject: clone: run check_everything_connected When we fetch from a remote, we do a revision walk to make sure that what we received is connected to our existing history. We do not do the same check for clone, which should be able to check that we received an intact history graph. The upside of this patch is that it will make clone more resilient against propagating repository corruption. The downside is that we will now traverse "rev-list --objects --all" down to the roots, which may take some time (it is especially noticeable for a "--local --bare" clone). Note that we need to adjust t5710, which tries to make such a bogus clone. Rather than checking after the fact that our clone is bogus, we can simplify it to just make sure "git clone" reports failure. Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 2 +- t/t5710-info-alternate.sh | 8 +------- 2 files changed, 2 insertions(+), 8 deletions(-) (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index 4e7030e613..c65a57cd22 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -79,7 +79,7 @@ test_expect_success 'clone --no-local --bare detects missing object' ' test_must_fail git clone --no-local --bare missing missing-transport ' -test_expect_failure 'clone --no-local --bare detects misnamed object' ' +test_expect_success 'clone --no-local --bare detects misnamed object' ' test_must_fail git clone --no-local --bare misnamed misnamed-transport ' diff --git a/t/t5710-info-alternate.sh b/t/t5710-info-alternate.sh index 5a6e49d18d..8956c21617 100755 --- a/t/t5710-info-alternate.sh +++ b/t/t5710-info-alternate.sh @@ -58,13 +58,7 @@ test_expect_success 'creating too deep nesting' \ git clone -l -s D E && git clone -l -s E F && git clone -l -s F G && -git clone --bare -l -s G H' - -test_expect_success 'invalidity of deepest repository' \ -'cd H && { - test_valid_repo - test $? -ne 0 -}' +test_must_fail git clone --bare -l -s G H' cd "$base_dir" -- cgit v1.2.3 From d3b34622f699ff14646de4ec1b1ab9afb0bcb056 Mon Sep 17 00:00:00 2001 From: Jeff King Date: Tue, 26 Mar 2013 18:22:09 -0400 Subject: clone: leave repo in place after checkout errors If we manage to clone a remote repository but run into an error in the checkout, it is probably sane to leave the repo directory in place. That lets the user examine the situation without spending time to re-clone from the remote (which may be a lengthy process). Rather than try to convert each die() from the checkout code path into an error(), we simply set a flag that tells the "remove_junk" atexit function to print a helpful message and leave the repo in place. Note that the test added in this patch actually passes without the code change. The reason is that the cleanup code is buggy; we chdir into the working tree for the checkout, but still may use relative paths to remove the directories (which means if you cloned into "foo", we would accidentally remove "foo" from the working tree!). There's no point in fixing it now, since this patch means we will never try to remove anything after the chdir, anyway. [jc: replaced the message with a more succinct version from Jonathan] Signed-off-by: Jeff King Signed-off-by: Junio C Hamano --- t/t1060-object-corruption.sh | 4 ++++ 1 file changed, 4 insertions(+) (limited to 't') diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh index c65a57cd22..3f8705139d 100755 --- a/t/t1060-object-corruption.sh +++ b/t/t1060-object-corruption.sh @@ -89,6 +89,10 @@ test_expect_success 'clone --local detects corruption' ' test_must_fail git clone --local bit-error corrupt-checkout ' +test_expect_success 'error detected during checkout leaves repo intact' ' + test_path_is_dir corrupt-checkout/.git +' + test_expect_success 'clone --local detects missing objects' ' test_must_fail git clone --local missing missing-checkout ' -- cgit v1.2.3