Re: [PATCH] diff: correct newline in summary for renamed files

From: Stefan Beller <sbeller@google.com>
Date: Sep 27, 2017, 22:34 UTC
Message-ID: <CAGZ79kbqLqPBbzH_c6=YhjSNr+4QAAf=JD7QUNY=aJQ355w8Cw@mail.gmail.com>
In-Reply-To: <20170927220925.u622metv2fcdtcfq@sigill.intra.peff.net>

On Wed, Sep 27, 2017 at 3:09 PM, Jeff King <peff@peff.net> wrote:

Show 31 quoted lines

> On Wed, Sep 27, 2017 at 02:58:52PM -0700, Stefan Beller wrote:
>
>> From: Linus Torvalds <torvalds@linux-foundation.org>
>>
>> In 146fdb0dfe (diff.c: emit_diff_symbol learns about DIFF_SYMBOL_SUMMARY,
>> 2017-06-29), the conversion from direct printing to the symbol emission
>> dropped the new line character for renamed, copied and rewritten files.
>>
>> Add the emission of a newline, add a test for this case.
>>
>> Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>
> The overall substance looks good, but...
>
>> diff --git a/t/t4016-diff-quote.sh b/t/t4016-diff-quote.sh
>> index 9c48e5c2c9..514056dd10 100755
>> --- a/t/t4016-diff-quote.sh
>> +++ b/t/t4016-diff-quote.sh
>> @@ -30,6 +30,7 @@ test_expect_success setup '
>>       git add . &&
>>       git commit -m initial &&
>>       git mv "$P0.0" "R$P0.0" &&
>> +     chmod a+x "R$P0.0" &&
>>       git mv "$P0.1" "R$P1.0" &&
>>       git mv "$P0.2" "R$P2.0" &&
>>       git mv "$P0.3" "R$P3.0" &&
>
> Won't this chmod be a problem for platforms without an executable bit?
> I think you'd need to use "update-index --chmod=+x" here, or require the
> FILEMODE prereq.

I was experimenting with git add --chmod=+x for this patch, but as this test runs "diff --summary -M HEAD", we need it change don the fs.

So let's find another test. Changing the setup of t4013 is a lot of work, but maybe worth it? Looking into t4031, that would also work.

To be fs agnostic, we can only compare commits against each other.

Show 7 quoted lines

> The whole script is marked as !MINGW, so that makes it less of a
> problem, but it's still possible have !FILEMODE on a Linux system, if
> you're on a funny filesystem. That also seems like a good reason to make
> sure this is in a script which is run more widely, since Windows folks
> would want to run this test, too.
>
> -Peff