From: Elijah Newren <newren@gmail.com>
Date: Sun, 03 Oct 2010 02:47:31 GMT
Subject: Re: [PATCHv4 06/15] t3600 (rm): add lots of missing &&
Message-ID: <AANLkTi=oyCFLYuBa6CPCC4PpDt6aPe1wXz98M_F+qary@mail.gmail.com>
In-Reply-To: <20101001104823.GD6816@burratino>

On Fri, Oct 1, 2010 at 4:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Elijah Newren wrote:
>
>> Also delete unnecessary, failing code and add some test_when_finished
>> calls to make sure tests clean up after themselves.
>
> I don't understand this one.
>
> If tests clean up after themselves, each will restore some pristine
> state as though (at least as far as the other tests care) it had never
> run at all, right?
>
>> --- a/t/t3600-rm.sh
>> +++ b/t/t3600-rm.sh
>> @@ -38,37 +38,33 @@ test_expect_success \
>>
>>  test_expect_success \
>>      'Test that git rm --cached foo succeeds if the index matches the file' \
>> -    'echo content > foo
>> -     git add foo
>> +    'echo content > foo &&
>> +     git add foo &&
>>       git rm --cached foo'
>
> Before this runs: foo is empty, absent from index.
> After it runs: foo has content, still absent from index.
> Requires: nothing.
>
>>  test_expect_success \
>>      'Test that git rm --cached foo succeeds if the index matches the file' \
>> -    'echo content > foo
>> -     git add foo
>> -     git commit -m foo
>> -     echo "other content" > foo
>> +    'echo content > foo &&
>> +     git add foo &&
>> +     git commit -m foo &&
>> +     echo "other content" > foo &&
>> +     test_when_finished "git checkout HEAD -- foo" &&
>>       git rm --cached foo'
>
> Before this runs: foo has content, absent from index.
> After this runs (without patch): foo has other content, absent from index.
> After this runs (with patch): foo has content, matching index.
> Requires: nothing.
>
>>  test_expect_success \
>>      'Test that git rm --cached foo fails if the index matches neither the file nor HEAD' '
>> -     echo content > foo
>> -     git add foo
>> -     git commit -m foo
>> -     echo "other content" > foo
>> -     git add foo
>> -     echo "yet another content" > foo
>> +     echo "other content" > foo &&
>> +     git add foo &&
>> +     echo "yet another content" > foo &&
>> +     test_when_finished "git checkout HEAD -- foo" &&
>>       test_must_fail git rm --cached foo
>>  '
>
> Before this runs: see above.
> After this runs (without patch): foo has yet another content, absent from index.
> After this runs (with patch): foo has content, matching index.
> Requires (without patch): nothing.
> Requires (with patch): HEAD:foo is content.
>
> [etc]
>
> So if I understand correctly, this sacrifices the ability to
> skip individual tests.  What does one gain in return?

Good eye.  It does remove failing code and add correct && chaining,
but yes, it will fail if you skip sufficiently many individual tests.
I believe my original patch submission
(http://article.gmane.org/gmane.comp.version-control.git/157080) does
not suffer from this same problem.  Should I revert to it?

Elijah