threads / patch / 35146

patcht3600: fix broken "choking git rm" test

Subject: [PATCH] t3600: fix broken "choking git rm" test

## tl;dr

3 messages between Oct 16, 2013 and Oct 16, 2013. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

SZEDER Gábor· Oct 16, 2013, 00:01 UTC · lore

The test 'choking "git rm" should not let it die with cruft' is supposed to check 'git rm's behavior when interrupted by provoking a SIGPIPE while 'git rm' is busily deleting files from a specially crafted index.

This test is silently broken for the following reasons:
- The test crafts a special index by feeding a large number of index
  entries with null shas to 'git update-index --index-info'.  It was
  OK back then when this test was introduced in commit 0693f9ddad
  (Make sure lockfiles are unlocked when dying on SIGPIPE,
  2008-12-18), but since commit 4337b5856f (do not write null sha1s to
  on-disk index, 2012-07-28) null shas are not allowed in the on-disk
  index causing 'git update-index' to error out.
- The barfing 'git update-index --index-info' should fail the test,
  but it remains unnoticed because of the severely broken && chain:
  the test's result depends solely on whether there is a stale lock
  file left behind, but after 'git update-index' errors out 'git rm'
  won't be executed at all.

To fix this test feed only non-null shas to 'git update-index' and restore the && chain (partly by adding a missing && and by using the test_when_finished helper instead of manual cleanup).

Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
---
A particularly funny one from the fallout of gmane/236183
 t/t3600-rm.sh | 11 ++++-------
 1 file changed, 4 insertions(+), 7 deletions(-)
Show changes to t/t3600-rm.sh +4 −7
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index 85854f338f..4dd0130dd9 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
 	git reset -q --hard &&
+	test_when_finished "rm -f .git/index.lock ; git reset -q --hard" &&
 	i=0 &&
 	while test $i -lt 12000
 	do
-	    echo "100644 $_z40 0	some-file-$i"
+	    echo "100644 1234567890123456789012345678901234567890 0	some-file-$i"
 	    i=$(( $i + 1 ))
 	done | git update-index --index-info &&
-	git rm -n "some-file-*" | :;
-	test -f .git/index.lock
-	status=$?
-	rm -f .git/index.lock
-	git reset -q --hard
-	test "$status" != 0
+	git rm -n "some-file-*" | : &&
+	test ! -f .git/index.lock
 '
 
 test_expect_success 'rm removes subdirectories recursively' '

-- 
1.8.4.1.495.gd8d272e
Jonathan Nieder· Oct 16, 2013, 00:18 UTC · re: SZEDER Gábor · lore

Re: [PATCH] t3600: fix broken "choking git rm" test

SZEDER Gábor wrote:
Show 6 quoted lines
> The test 'choking "git rm" should not let it die with cruft' is
> supposed to check 'git rm's behavior when interrupted by provoking a
> SIGPIPE while 'git rm' is busily deleting files from a specially
> crafted index.
>
> This test is silently broken for the following reasons:
[...]
> Signed-off-by: SZEDER Gábor <szeder@ira.uka.de>
> ---
> A particularly funny one from the fallout of gmane/236183
Fun. :)  Makes sense.
Show 7 quoted lines
> --- a/t/t3600-rm.sh
> +++ b/t/t3600-rm.sh
> @@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
>  
>  test_expect_success 'choking "git rm" should not let it die with cruft' '
>  	git reset -q --hard &&
> +	test_when_finished "rm -f .git/index.lock ; git reset -q --hard" &&

I'd use "&&" here --- the test_cleanup checks the exit status from this scriptlet, so it's a good habit.

[...]
Show 6 quoted lines
> -	test -f .git/index.lock
> -	status=$?
> -	rm -f .git/index.lock
> -	git reset -q --hard
> -	test "$status" != 0
> +	test ! -f .git/index.lock
Gah.  Thanks for cleaning it up.

Maybe test_path_is_missing would make sense here? (It would notice a .git/index.lock directory, which is not very likely :), but more importantly, it says why it is failing the test when it fails.)

With or without the changes mentioned above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks.
Show changes to diff +2 −2
diff --git i/t/t3600-rm.sh w/t/t3600-rm.sh
index 8386b54..540c49b 100755
--- i/t/t3600-rm.sh
+++ w/t/t3600-rm.sh
@@ -240,7 +240,7 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
 
 test_expect_success 'choking "git rm" should not let it die with cruft' '
 	git reset -q --hard &&
-	test_when_finished "rm -f .git/index.lock ; git reset -q --hard" &&
+	test_when_finished "rm -f .git/index.lock && git reset -q --hard" &&
 	i=0 &&
 	while test $i -lt 12000
 	do
@@ -248,7 +248,7 @@ test_expect_success 'choking "git rm" should not let it die with cruft' '
 	    i=$(( $i + 1 ))
 	done | git update-index --index-info &&
 	git rm -n "some-file-*" | : &&
-	test ! -f .git/index.lock
+	test_path_is_missing .git/index.lock
 '
 
 test_expect_success 'rm removes subdirectories recursively' '
SZEDER Gábor· Oct 16, 2013, 00:38 UTC · re: Jonathan Nieder · lore

Re: [PATCH] t3600: fix broken "choking git rm" test

On Tue, Oct 15, 2013 at 05:18:04PM -0700, Jonathan Nieder wrote:
Show 11 quoted lines
> SZEDER Gábor wrote:
> > --- a/t/t3600-rm.sh
> > +++ b/t/t3600-rm.sh
> > @@ -240,18 +240,15 @@ test_expect_success 'refresh index before checking if it is up-to-date' '
> >  
> >  test_expect_success 'choking "git rm" should not let it die with cruft' '
> >  	git reset -q --hard &&
> > +	test_when_finished "rm -f .git/index.lock ; git reset -q --hard" &&
> 
> I'd use "&&" here --- the test_cleanup checks the exit status from
> this scriptlet, so it's a good habit.

OK. My motivation for the ';' was that we should make sure that both steps of the cleanup are executed. However, now thinking about it if regular 'rm -f' can't remove the lock file then all bets are off anyway.

Show 13 quoted lines
> [...]
> > -	test -f .git/index.lock
> > -	status=$?
> > -	rm -f .git/index.lock
> > -	git reset -q --hard
> > -	test "$status" != 0
> > +	test ! -f .git/index.lock
> 
> Gah.  Thanks for cleaning it up.
> 
> Maybe test_path_is_missing would make sense here?  (It would notice a
> .git/index.lock directory, which is not very likely :), but more
> importantly, it says why it is failing the test when it fails.)

I was not aware of the test_path_is_missing helper. I don't think it matters whether it's a file or a directory, because a stale .git/index.lock directory would be just as bad.

← back to recent threads