Re: [PATCH 03/10] diff.c: drop tautologous condition in emit_line_0

From: Stefan Beller <sbeller@google.com>
Date: Sep 16, 2016, 23:04 UTC
Message-ID: <CAGZ79kbRBqEMib=ym9xE2E_Tw4r_nA+wvU9NDfjnJRLKXk8ZzQ@mail.gmail.com>
In-Reply-To: <xmqq1t0oy8t8.fsf@gitster.mtv.corp.google.com>

On Mon, Sep 12, 2016 at 4:53 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 30 quoted lines

> Stefan Beller <stefanbeller@gmail.com> writes:
>
>> diff --git a/diff.c b/diff.c
>> index 156c2aa..9d2e704 100644
>> --- a/diff.c
>> +++ b/diff.c
>> @@ -460,8 +460,7 @@ static void emit_line_0(struct diff_options *o, const char *set, const char *res
>>
>>       if (len == 0) {
>>               has_trailing_newline = (first == '\n');
>> -             has_trailing_carriage_return = (!has_trailing_newline &&
>> -                                             (first == '\r'));
>> +             has_trailing_carriage_return = (first == '\r');
>>               nofirst = has_trailing_newline || has_trailing_carriage_return;
>>       } else {
>>               has_trailing_newline = (len > 0 && line[len-1] == '\n');
>
> Interesting.
>
> This may be a mis-conversion at 250f7993 ("diff.c: split emit_line()
> from the first char and the rest of the line", 2009-09-14), I
> suspect.  The original took line[] with length and peeked for '\n',
> and when it saw one, it decremented length before checking
> line[len-1] for '\r'.
>
> But of course if there is only one byte on the line (i.e. len == 0
> after first is stripped off), it cannot be both '\n' or '\r' at the
> same time.
>
> Thanks for spotting.

Oh, right, it used to be possible to remove \r\n completely and that information was then kept as has_trailing_newline = has_trailing_carriage_return = 1; and the resulting line is kept completely without ending line.

After some thought I don't think I can use this mis-conversion to trigger a bug though, because the len=0 can only ever happen if first is '\n' alone essentially.

Another thing I noticed when playing around with diffs:

    $ printf "\r\n" >crlf
    $ git commit crlf -m "add file crlf, empty line"
    $ printf "non zero length\r\n" >crlf
    $ diff --git a/crlf b/crlf
    $ index d3f5a12..ece7140 100644
    --- a/crlf
    +++ b/crlf
    @@ -1 +1 @@
    -
    +non zero length^M
    $ # The - line is missing a ^M ?