Re: [PATCH v2 3/4] apply: do not read from beyond a symbolic link

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> On Tue, Feb 3, 2015 at 11:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> On Mon, Feb 2, 2015 at 3:27 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>>> +       test_must_fail git apply --index patch
>>>> +
>>>> +'
>>>
>>> Is the empty line between the last test_must_fail and the closing `'`
>>> intentional?
>>
>> I think I just mimicked the previous existing one, but I can go with
>> the version without.
>
> It was really a honest question. I've seen and written and sent
> tests without such an ending empty line and it seemed to be ok.

It was a honest answer, too.

I see existing new-style tests [*1*] that have and/or lack the empty
line at the end of the command, and I do not have particular
preference either way.  Having these variations did not bother me
too much, at least until now.  We might want to make them consistent,
and the time to be careful is when we add new tests, so it was very
valid for you to bring it up against this patch.  But I do not know
if we as the Git developer community have preference either way, and
I myself don't either, so...


[Footnote]

*1* The really ancient style we want to clean-up goes like

        cmd for setup > expect
        
        test_expect_success \
                'title of the test' \
                'command && more command &&
                 yet more command > actual &&
                 test_cmp expect actual'

        cmd for cleanup

    and we want them to read more like

        test_expect_success 'title of the test' '
                test_when_finished "cmd for cleanup" &&
                cmd for setup >expect &&
                command &&
                more command &&
                yet more command >actual &&
                test_cmp expect actual
        '