Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2012, 05:08 UTC
Message-ID: <7vmx8bcv4u.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAK8d-aLXs0yMzYMXm7fKytOGDXesUEx7a8PN_Mg9gw6+Q6OTBA@mail.gmail.com>

Steven Walter <stevenrwalter@gmail.com> writes:

Show 8 quoted lines

>>> +     test -e "$SVN_TREE"/bar/zzz/yyy ' || true
>>
>> Care to explain what this " || true" is doing here, please?
>
> Ahh, good catch.  I think the answer is that it shouldn't be there.
> It was originally there because of the "test_must_fail" line, I think
> (at least the other tests that use test_must_fail also have "||
> true").

Ok, that may explain the copy&paste error.

But I do not think test_must_fail followed by || true makes much sense, either. The purpose of "test_must_fail" is to make sure the tested git command exits with non-zero status in a controlled way (i.e. not crash) so if the tested command that is expected to exit with non-zero status exited with zero status, the test has detected an *error*. E.g. if you know that the index and the working tree are different at one point in the test sequence, you would say:

	... other setup steps ... &&
	test_must_fail git diff --exit-code &&
        ... and other tests ...

so that failure by "git diff --exit-code" to exit with non-zero status (i.e. it did not find any difference when it should have) breaks the && cascade.

I just took a quick look at t9100 but I think all " || true" can be safely removed. None of them is associated with test_must_fail in any way. For whatever reason, these test seem to do

	test_expect_success 'label of the test' '
        	body of the test
	' || true

for no good reason.

> Do you want to just fix that up, or a new version of the original patch,
> or a fix on top of the original patches?

Eric queued the patch and then had me pull it as part of his history already, so it is doubly too late to replace it.

Can you apply this patch and re-test?

 t/t9100-git-svn-basic.sh |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

diff --git a/t/t9100-git-svn-basic.sh b/t/t9100-git-svn-basic.sh
index 4029f84..749b75e 100755
--- a/t/t9100-git-svn-basic.sh
+++ b/t/t9100-git-svn-basic.sh
@@ -65,7 +65,8 @@ test_expect_success "$name" "
 	git update-index --add dir/file/file &&
 	git commit -m '$name' &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch" || true
+		${remotes_git_svn}..mybranch
+"
 
 
 name='detect node change from directory to file #1'
@@ -79,7 +80,8 @@ test_expect_success "$name" '
 	git update-index --add -- bar &&
 	git commit -m "$name" &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch2' || true
+		${remotes_git_svn}..mybranch2
+'
 
 
 name='detect node change from file to directory #2'
@@ -96,7 +98,8 @@ test_expect_success "$name" '
 		${remotes_git_svn}..mybranch3 &&
 	svn_cmd up "$SVN_TREE" &&
 	test -d "$SVN_TREE"/bar/zzz &&
-	test -e "$SVN_TREE"/bar/zzz/yyy ' || true
+	test -e "$SVN_TREE"/bar/zzz/yyy
+'
 
 name='detect node change from directory to file #2'
 test_expect_success "$name" '
@@ -109,7 +112,8 @@ test_expect_success "$name" '
 	git update-index --add -- dir &&
 	git commit -m "$name" &&
 	test_must_fail git svn set-tree --find-copies-harder --rmdir \
-		${remotes_git_svn}..mybranch4' || true
+		${remotes_git_svn}..mybranch4
+'
 
 
 name='remove executable bit from a file'
@@ -162,7 +166,7 @@ test_expect_success "$name" '
 
 name='modify a symlink to become a file'
 test_expect_success "$name" '
-	echo git help > help || true &&
+	echo git help >help &&
 	rm exec-2.sh &&
 	cp help exec-2.sh &&
 	git update-index exec-2.sh &&