From: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date: Thu, 29 Jul 2010 01:56:50 GMT
Subject: Re: [PATCH v2 2/5] t/t4004-diff-rename-symlink.sh: use three-arg <prereq>
Message-ID: <AANLkTinFMjH6LfDXjCp9XGVEuQF9ZKrL0i8t-p8hmzJ-@mail.gmail.com>
In-Reply-To: <20100729010903.GF29156@dert.cs.uchicago.edu>

On Thu, Jul 29, 2010 at 01:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Ævar Arnfjörð Bjarmason wrote:
>
>> Change the tests that skipped due to unavailable SYMLINKS support to
>> use the three-arg prereq form of test_expect_success.
>>
>> This is like the "tests: implicitly skip SYMLINKS tests using
>> <prereq>" change, but I needed to create an additional test for some
>> setup code. It's in a separate change as suggested by Jonathan Nieder
>> for ease of reviewing.
>
> Hmm, I still don’t understand this.  Do you mean that there is
> some setup that needs to be run before these commands, and so
> the patch fails if that change is not included?
>
> Or is it a matter of "while at it, fix this other problem
> I noticed" (which would be fine, but it is clearer to
> present it as such if so)?

The setup code needs to be inside a test so that it'll only run if we
have SYMLINKS support.

I could also have done:

    if test_have_prereq PERL
    then
        ..setup code..
    fi

But setup code should be inside tests, so that we'll get failure
reporting.

>> diff --git a/t/t4004-diff-rename-symlink.sh b/t/t4004-diff-rename-symlink.sh
>> index 1a09e8d..92a65f4 100755
>> --- a/t/t4004-diff-rename-symlink.sh
>> +++ b/t/t4004-diff-rename-symlink.sh
>> @@ -40,8 +34,9 @@ test_expect_success \
>>  # rezrov and nitfol are rename/copy of frotz and bozbar should be
>>  # a new creation.
>>
>> -GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current
>> -cat >expected <<\EOF
>> +test_expect_success SYMLINKS 'setup diff output' "
>> +    GIT_DIFF_OPTS=--unified=0 git diff-index -M -p $tree >current &&
>> +    cat >expected <<\EOF
>>  diff --git a/bozbar b/bozbar
>>  new file mode 120000
>>  --- /dev/null
>> @@ -65,8 +60,9 @@ deleted file mode 100644
>>  -xyzzy
>>  \ No newline at end of file
>>  EOF
>> +"
>
> Tip for the future: if you use 'straight quotes', then readers
> can avoid carefully scanning through for $ and similar oddities
> (and the test script presented with the "expecting success:"
> prompt will use the friendlier $tree instead of 76c98ds89).

Ah, thanks.

> The patch looks good; my only remaining concern is the log
> message as mentioned above.

Hopefully that's cleared up now.