From: Steven Walter <stevenrwalter@gmail.com>
Date: Thu, 23 Feb 2012 23:17:33 GMT
Subject: Re: [PATCH] git-svn.perl: fix a false-positive in the "already exists" test
Message-ID: <CAK8d-aJufwFobREQ6R3Oxr=J7hbVtoZ7wvhurb=LQGUFO9tTsw@mail.gmail.com>
In-Reply-To: <7vmx8bcv4u.fsf@alter.siamese.dyndns.org>

Signed-Off-By: Steven Walter <stevenrwalter@gmail.com>

On Wed, Feb 22, 2012 at 12:08 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Steven Walter <stevenrwalter@gmail.com> writes:
>
>>>> +     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 &&



-- 
-Steven Walter <stevenrwalter@gmail.com>