Re: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Fri, Mar 20, 2015 at 1:10 AM, Jeff King p...@peff.net wrote:
On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
Clever (Jonathan's too); much nicer than trying to special case only here-doc.
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the -chaining).
I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.
Thanks for working on this. It looks like this technique should be a
valuable addition to test-lint. (I had intended, but haven't yet found
time to dig into it, so I'm happy to hear of your progress.)
--
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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 9:32 PM, Jeff King p...@peff.net wrote: On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote: --- /dev/null +++ b/t/t5312-prune-corruption.sh @@ -0,0 +1,104 @@ +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref +test_expect_success 'create packed-refs file with broken ref' ' + rm -f .git/refs/heads/master + cat .git/packed-refs -EOF Broken -chain. Thanks. I notice that a large number of broken -chains are on here-docs. I really wish you could put the on the EOF line at the end of the here-doc. I understand _why_ that this not the case, but mentally it is where I want to type it, and I obviously sometimes fail to go back and fix it. I don't think there's a better solution in POSIX sh, though. I wonder if test-lint could be enhanced to detect this sort of problem? -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 09:16:52PM -0400, Eric Sunshine wrote: --- /dev/null +++ b/t/t5312-prune-corruption.sh @@ -0,0 +1,104 @@ +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref +test_expect_success 'create packed-refs file with broken ref' ' + rm -f .git/refs/heads/master + cat .git/packed-refs -EOF Broken -chain. Thanks. I notice that a large number of broken -chains are on here-docs. I really wish you could put the on the EOF line at the end of the here-doc. I understand _why_ that this not the case, but mentally it is where I want to type it, and I obviously sometimes fail to go back and fix it. I don't think there's a better solution in POSIX sh, though. -Peff -- 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
Re: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 10:25:32PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
FWIW, I have spent about a few hours wading through the errors, and am
about 75% done. There are definitely some broken chains that were
causing test results to be ignored (as opposed to just minor setup steps
that we would not expect to fail). In most cases, the tests do passed. I
have a few that I still need to examine more closely, but there may be
some where there are actual test failures (but it's possible that I just
screwed it up while fixing the -chaining).
I hope to post something tonight, but I wanted to drop a note on the off
chance that you were actively looking at it at the same time.
-Peff
--
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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Tue, Mar 17, 2015 at 3:28 AM, Jeff King p...@peff.net wrote: When we are doing a destructive operation like git prune, we want to be extra careful that the set of reachable tips we compute is valid. If there is any corruption or oddity, we are better off aborting the operation and letting the user figure things out rather than plowing ahead and possibly deleting some data that cannot be recovered. Signed-off-by: Jeff King p...@peff.net --- diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh new file mode 100755 index 000..167031e --- /dev/null +++ b/t/t5312-prune-corruption.sh @@ -0,0 +1,104 @@ +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref +test_expect_success 'create packed-refs file with broken ref' ' + rm -f .git/refs/heads/master + cat .git/packed-refs -EOF Broken -chain. + $missing refs/heads/master + $recoverable refs/heads/other + EOF + echo $missing expect + git rev-parse refs/heads/master actual + test_cmp expect actual +' -- 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
test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
On Thu, Mar 19, 2015 at 09:37:12PM -0400, Eric Sunshine wrote:
Thanks. I notice that a large number of broken -chains are on
here-docs. I really wish you could put the on the EOF line at the
end of the here-doc. I understand _why_ that this not the case, but
mentally it is where I want to type it, and I obviously sometimes fail
to go back and fix it. I don't think there's a better solution in POSIX
sh, though.
I wonder if test-lint could be enhanced to detect this sort of problem?
That would be nice, but it's complicated. A naive:
diff --git a/t/check-non-portable-shell.pl b/t/check-non-portable-shell.pl
index b170cbc..3a6d8d8 100755
--- a/t/check-non-portable-shell.pl
+++ b/t/check-non-portable-shell.pl
@@ -22,6 +22,7 @@ while () {
/^\s*[^#]\s*which\s/ and err 'which is not portable (please use type)';
/\btest\s+[^=]*==/ and err 'test a == b is not portable (please use
=)';
/\bexport\s+[A-Za-z0-9_]*=/ and err 'export FOO=bar is not portable
(please use FOO=bar export FOO)';
+ / -?.?EOF(.*)/ $1 !~ // and err 'here-doc with broken -chain';
# this resets our $. for each file
close ARGV if eof;
}
yields quite a few false positives, because of course we don't know
which are meant to be at the end of the chain and which are not. And
finding that out is tough. We'd have to actually parse to the end of
the here-doc ourselves, then see if it was the end of the test_expect
block.
I think it would be simpler to ask the shell to check this for us, like:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
I seem to recall people looked at doing this sort of lint a while ago,
but we never ended up committing anything. I wonder if it was because of
all of these false positives.
diff --git a/t/t3010-ls-files-killed-modified.sh
b/t/t3010-ls-files-killed-modified.sh
index 6d3b828..62fce10 100755
--- a/t/t3010-ls-files-killed-modified.sh
+++ b/t/t3010-ls-files-killed-modified.sh
@@ -62,7 +62,7 @@ test_expect_success 'git update-index --add to add various
paths.' '
cd submod$i git commit --allow-empty -m empty $i
) || break
done
- git update-index --add submod[12]
+ git update-index --add submod[12]
(
cd submod1
git commit --allow-empty -m empty 1 (updated)
@@ -99,12 +99,12 @@ test_expect_success 'git ls-files -k to show killed files.'
'
'
test_expect_success 'git ls-files -k output (w/o icase)' '
- git ls-files -k .output
+ git ls-files -k .output
test_cmp .expected .output
'
test_expect_success 'git ls-files -k output (w/ icase)' '
- git -c core.ignorecase=true ls-files -k .output
+ git -c core.ignorecase=true ls-files -k .output
test_cmp .expected .output
'
-Peff
--
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
Re: test -chain lint (was: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository)
[+cc Jonathan, whose patch I apparently subconsciously copied]
On Thu, Mar 19, 2015 at 10:08:51PM -0400, Jeff King wrote:
diff --git a/t/test-lib.sh b/t/test-lib.sh
index c096778..02a03d5 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -524,6 +524,21 @@ test_eval_ () {
test_run_ () {
test_cleanup=:
expecting_failure=$2
+
+ if test -n $GIT_TEST_CHAIN_LINT; then
+ # 117 is unlikely to match the exit code of
+ # another part of the chain
+ test_eval_ (exit 117) $1
+ if test $? != 117; then
+ # all bets are off for continuing with other tests;
+ # we expected none of the rest of the test commands to
+ # run, but at least some did. Who knows what weird
+ # state we're in? Just bail, and the user can diagnose
+ # by running in --verbose mode
+ error bug in the test script: broken -chain
+ fi
+ fi
+
setup_malloc_check
test_eval_ $1
eval_ret=$?
This turns up an appalling number of failures, but AFAICT they are all
real in the sense that the -chains are broken. In some cases these
are real, but in others the tests are of an older style where they did
not expect some early commands to fail (and we would catch their bogus
output if they did). E.g., in the patch below, I think the first one is
a real potential bug, and the other two are mostly noise. I do not mind
setting a rule and fixing all of them, though.
I seem to recall people looked at doing this sort of lint a while ago,
but we never ended up committing anything. I wonder if it was because of
all of these false positives.
This turns out to be rather annoying to grep for in the list archives,
but I found at least one discussion:
http://article.gmane.org/gmane.comp.version-control.git/235913
I don't know why we didn't follow it up then. Perhaps because the patch
there (which is rather similar to what I have above) was not
conditional, so whole chunks of the test suite needed fixing. There are
enough problems that we would probably want to do this conditionally,
fix them over time, and then finally flip the feature on by default.
-Peff
--
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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
Jeff King p...@peff.net writes: +test_expect_success 'create history with missing tip commit' ' + test_tick git commit --allow-empty -m one + recoverable=$(git rev-parse HEAD) + git cat-file commit $recoverable saved + test_tick git commit --allow-empty -m two + missing=$(git rev-parse HEAD) + # point HEAD elsewhere + git checkout $base Could you spell this as $base^0 (or --detach) to clarify the intention? I have been scraching my head for a few minutes just now, trying to figure out what you are doing here. I _think_ you wanted master to point at the missing two and wanted to make sure all other refs (including HEAD) to point away from it. Mental note: At this point, the history looks like base onetwo o--o--o \ o bogus and because the reference to two is still there but two itself is missing, pruning may well end up losing one, because the reference to it is only through master pointing at two. + rm .git/objects/$(echo $missing | sed s,..,/,) + test_must_fail git cat-file -e $missing +' + +test_expect_failure 'pruning with a corrupted tip does not drop history' ' + test_when_finished git hash-object -w -t commit saved + test_might_fail git prune --expire=now + verbose git cat-file -e $recoverable +' Mental note: OK, this demonstrates that the missing two makes us lose the only reference to one (aka $recoverable in saved). +test_expect_success 'pack-refs does not silently delete broken loose ref' ' + git pack-refs --all --prune + echo $missing expect + git rev-parse refs/heads/master actual + test_cmp expect actual +' + +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref +test_expect_success 'create packed-refs file with broken ref' ' + rm -f .git/refs/heads/master + cat .git/packed-refs -EOF + $missing refs/heads/master + $recoverable refs/heads/other + EOF I do not know offhand if the lack of the pack-refs feature header matters here; I assume it does not? A safer check may be to pack and then make it missing, I guess, but I do not know if the difference matters. -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
Jeff King p...@peff.net writes: A safer check may be to pack and then make it missing, I guess, but I do not know if the difference matters. Yeah, I considered that. The trouble is that we are relying on the earlier setup that made the object go missing. We cannot pack the refs in the setup step, because the earlier tests are checking the loose-ref behavior. So we would have to actually restore the object, pack, and then re-delete it. Yes, restore pack redelete was what I had in mind when I wondered such a sequence of extra steps is worth and the difference between such an approach and an approach to use a hand-crafted packed-refs file matters. -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 01:04:16PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: +test_expect_success 'create history with missing tip commit' ' + test_tick git commit --allow-empty -m one + recoverable=$(git rev-parse HEAD) + git cat-file commit $recoverable saved + test_tick git commit --allow-empty -m two + missing=$(git rev-parse HEAD) + # point HEAD elsewhere + git checkout $base Could you spell this as $base^0 (or --detach) to clarify the intention? I have been scraching my head for a few minutes just now, trying to figure out what you are doing here. I _think_ you wanted master to point at the missing two and wanted to make sure all other refs (including HEAD) to point away from it. Yes, exactly. I've squashed in your suggestion and added a comment explaining it: diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 1001a69..1cdbd9f 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -50,14 +50,24 @@ test_expect_success 'clean up bogus ref' ' rm .git/refs/heads/bogus..name ' +# We create two new objects here, one and two. Our +# master branch points to two, which is deleted, +# corrupting the repository. But we'd like to make sure +# that the otherwise unreachable one is not pruned +# (since it is the user's best bet for recovering +# from the corruption). +# +# Note that we also point HEAD somewhere besides two, +# as we want to make sure we test the case where we +# pick up the reference to two by iterating the refs, +# not by resolving HEAD. test_expect_success 'create history with missing tip commit' ' test_tick git commit --allow-empty -m one recoverable=$(git rev-parse HEAD) git cat-file commit $recoverable saved test_tick git commit --allow-empty -m two missing=$(git rev-parse HEAD) - # point HEAD elsewhere - git checkout $base + git checkout --detach $base rm .git/objects/$(echo $missing | sed s,..,/,) test_must_fail git cat-file -e $missing ' +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref +test_expect_success 'create packed-refs file with broken ref' ' + rm -f .git/refs/heads/master + cat .git/packed-refs -EOF + $missing refs/heads/master + $recoverable refs/heads/other + EOF I do not know offhand if the lack of the pack-refs feature header matters here; I assume it does not? It doesn't matter. We also do similarly gross things in other corruption-related tests, but I suspect if you git-blamed them all you would find that I am responsible. :) A safer check may be to pack and then make it missing, I guess, but I do not know if the difference matters. Yeah, I considered that. The trouble is that we are relying on the earlier setup that made the object go missing. We cannot pack the refs in the setup step, because the earlier tests are checking the loose-ref behavior. So we would have to actually restore the object, pack, and then re-delete it. Another option would be to restructure the whole test script to perform each individual corruption in its own sub-repo. I thought that would end up making things harder to understand due to the extra setup boilerplate, but it would make the tests less fragile with respect to each other (e.g., see the clean up bogus ref step which exists only to clean up our earlier corruption that could influence later tests). -Peff -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 02:23:25PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: A safer check may be to pack and then make it missing, I guess, but I do not know if the difference matters. Yeah, I considered that. The trouble is that we are relying on the earlier setup that made the object go missing. We cannot pack the refs in the setup step, because the earlier tests are checking the loose-ref behavior. So we would have to actually restore the object, pack, and then re-delete it. Yes, restore pack redelete was what I had in mind when I wondered such a sequence of extra steps is worth and the difference between such an approach and an approach to use a hand-crafted packed-refs file matters. I took a look at this. It turns out to be rather annoying, because we can't just restore $missing. The earlier tests may have deleted other random objects (like $recoverable) depending on whether or not they actually failed. So I'm inclined to leave it (we do confirm with the rev-parse call at the end of the setup that our packed-refs file is working) unless you feel strongly. If you do, I'd rather go the route of sticking each corruption in its own separate sub-repo. -Peff -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
Jeff King p...@peff.net writes: So I'm inclined to leave it (we do confirm with the rev-parse call at the end of the setup that our packed-refs file is working) unless you feel strongly. If you do, I'd rather go the route of sticking each corruption in its own separate sub-repo. No, I don't feel strongly either way---otherwise I wouldn't be wondering if it makes a difference, but explaining why hand-crafting is a bad idea (or the other way around). Thanks. -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Thu, Mar 19, 2015 at 02:49:37PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: So I'm inclined to leave it (we do confirm with the rev-parse call at the end of the setup that our packed-refs file is working) unless you feel strongly. If you do, I'd rather go the route of sticking each corruption in its own separate sub-repo. No, I don't feel strongly either way---otherwise I wouldn't be wondering if it makes a difference, but explaining why hand-crafting is a bad idea (or the other way around). And here I thought you were just being polite. ;) -Peff -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
Am 17.03.2015 um 19:55 schrieb Jeff King: + echo $bogus .git/refs/heads/bogus..name ... I assumed the final . in your example wasn't significant (it is not to git), but let me know if I've run afoul of another weird restriction. :) It was actually deliberate (with intents too complicated to explain), but it turns out not to be required. Your updated test case is good. -- Hannes -- 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 1/5] t5312: test object deletion code paths in a corrupted repository
When we are doing a destructive operation like git prune, we want to be extra careful that the set of reachable tips we compute is valid. If there is any corruption or oddity, we are better off aborting the operation and letting the user figure things out rather than plowing ahead and possibly deleting some data that cannot be recovered. The tests here include: 1. Pruning objects mentioned only be refs with invalid names. This used to abort prior to d0f810f (refs.c: allow listing and deleting badly named refs, 2014-09-03), but since then we silently ignore the tip. Likewise, we test repacking that can drop objects (either -ad, which drops anything unreachable, or -Ad --unpack-unreachable=time, which tries to optimize out a loose object write that would be directly pruned). 2. Pruning objects when some refs point to missing objects. We don't know whether any dangling objects would have been reachable from the missing objects. We are better to keep them around, as they are better than nothing for helping the user recover history. 3. Packed refs that point to missing objects can sometimes be dropped. By itself, this is more of an annoyance (you do not have the object anyway; even if you can recover it from elsewhere, all you are losing is a placeholder for your state at the time of corruption). But coupled with (2), if we drop the ref and then go on to prune, we may lose unrecoverable objects. Note that we use test_might_fail for some of the operations. In some cases, it would be appropriate to abort the operation, and in others, it might be acceptable to continue but taking the information into account. The tests don't care either way, and check only for data loss. Signed-off-by: Jeff King p...@peff.net --- t/t5312-prune-corruption.sh | 104 1 file changed, 104 insertions(+) create mode 100755 t/t5312-prune-corruption.sh diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh new file mode 100755 index 000..167031e --- /dev/null +++ b/t/t5312-prune-corruption.sh @@ -0,0 +1,104 @@ +#!/bin/sh + +test_description=' +Test pruning of repositories with minor corruptions. The goal +here is that we should always be erring on the side of safety. So +if we see, for example, a ref with a bogus name, it is OK either to +bail out or to proceed using it as a reachable tip, but it is _not_ +OK to proceed as if it did not exist. Otherwise we might silently +delete objects that cannot be recovered. +' +. ./test-lib.sh + +test_expect_success 'disable reflogs' ' + git config core.logallrefupdates false + rm -rf .git/logs +' + +test_expect_success 'create history reachable only from a bogus-named ref' ' + test_tick git commit --allow-empty -m master + base=$(git rev-parse HEAD) + test_tick git commit --allow-empty -m bogus + bogus=$(git rev-parse HEAD) + git cat-file commit $bogus saved + echo $bogus .git/refs/heads/bogus:name + git reset --hard HEAD^ +' + +test_expect_failure 'pruning does not drop bogus object' ' + test_when_finished git hash-object -w -t commit saved + test_might_fail git prune --expire=now + verbose git cat-file -e $bogus +' + +test_expect_success 'put bogus object into pack' ' + git tag reachable $bogus + git repack -ad + git tag -d reachable + verbose git cat-file -e $bogus +' + +test_expect_failure 'destructive repack keeps packed object' ' + test_might_fail git repack -Ad --unpack-unreachable=now + verbose git cat-file -e $bogus + test_might_fail git repack -ad + verbose git cat-file -e $bogus +' + +# subsequent tests will have different corruptions +test_expect_success 'clean up bogus ref' ' + rm .git/refs/heads/bogus:name +' + +test_expect_success 'create history with missing tip commit' ' + test_tick git commit --allow-empty -m one + recoverable=$(git rev-parse HEAD) + git cat-file commit $recoverable saved + test_tick git commit --allow-empty -m two + missing=$(git rev-parse HEAD) + # point HEAD elsewhere + git checkout $base + rm .git/objects/$(echo $missing | sed s,..,/,) + test_must_fail git cat-file -e $missing +' + +test_expect_failure 'pruning with a corrupted tip does not drop history' ' + test_when_finished git hash-object -w -t commit saved + test_might_fail git prune --expire=now + verbose git cat-file -e $recoverable +' + +test_expect_success 'pack-refs does not silently delete broken loose ref' ' + git pack-refs --all --prune + echo $missing expect + git rev-parse refs/heads/master actual + test_cmp expect actual +' + +# we do not want to count on running pack-refs to +# actually pack it, as it is perfectly reasonable to +# skip processing a broken ref
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
On Tue, Mar 17, 2015 at 07:34:02PM +0100, Johannes Sixt wrote: Am 17.03.2015 um 08:28 schrieb Jeff King: +test_expect_success 'create history reachable only from a bogus-named ref' ' +test_tick git commit --allow-empty -m master +base=$(git rev-parse HEAD) +test_tick git commit --allow-empty -m bogus +bogus=$(git rev-parse HEAD) +git cat-file commit $bogus saved +echo $bogus .git/refs/heads/bogus:name This causes headaches on Windows: It creates an empty file, named bogus, with all the data diverted to the alternate data stream named name. Needless to say that this... Ah, yes. Windows. Our usual workaround would be to put it straight into packed-refs, but in this case, the test really does need the badly named ref in the file system. But... +test_expect_success 'clean up bogus ref' ' +rm .git/refs/heads/bogus:name +' does not remove the file bogus, but only the alternate data stream (if at all---I forgot to check). How about .git/refs/heads/bogus..nam.e? Yes, that works. The colon is what originally brought my attention to this case, but anything that fails git-check-ref-format is fine. I've squashed this in: diff --git a/t/t5312-prune-corruption.sh b/t/t5312-prune-corruption.sh index 167031e..1001a69 100755 --- a/t/t5312-prune-corruption.sh +++ b/t/t5312-prune-corruption.sh @@ -21,7 +21,7 @@ test_expect_success 'create history reachable only from a bogus-named ref' ' test_tick git commit --allow-empty -m bogus bogus=$(git rev-parse HEAD) git cat-file commit $bogus saved - echo $bogus .git/refs/heads/bogus:name + echo $bogus .git/refs/heads/bogus..name git reset --hard HEAD^ ' @@ -47,7 +47,7 @@ test_expect_failure 'destructive repack keeps packed object' ' # subsequent tests will have different corruptions test_expect_success 'clean up bogus ref' ' - rm .git/refs/heads/bogus:name + rm .git/refs/heads/bogus..name ' test_expect_success 'create history with missing tip commit' ' I assumed the final . in your example wasn't significant (it is not to git), but let me know if I've run afoul of another weird restriction. :) Thanks. -Peff -- 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
Re: [PATCH 1/5] t5312: test object deletion code paths in a corrupted repository
Am 17.03.2015 um 08:28 schrieb Jeff King: +test_expect_success 'create history reachable only from a bogus-named ref' ' + test_tick git commit --allow-empty -m master + base=$(git rev-parse HEAD) + test_tick git commit --allow-empty -m bogus + bogus=$(git rev-parse HEAD) + git cat-file commit $bogus saved + echo $bogus .git/refs/heads/bogus:name This causes headaches on Windows: It creates an empty file, named bogus, with all the data diverted to the alternate data stream named name. Needless to say that this... +test_expect_success 'clean up bogus ref' ' + rm .git/refs/heads/bogus:name +' does not remove the file bogus, but only the alternate data stream (if at all---I forgot to check). How about .git/refs/heads/bogus..nam.e? -- Hannes -- 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
