Re: [PATCH 22/25] diff.c: color moved lines differently

From: Stefan Beller <sbeller@google.com>
Date: Jun 30, 2017, 18:13 UTC
Message-ID: <CAGZ79kaK00CpXOtXnx_u7_KHbZq4Mz8vWHKy2a8p1gQ8ogE-OA@mail.gmail.com>
In-Reply-To: <xmqqd19ls6no.fsf@gitster.mtv.corp.google.com>

On Fri, Jun 30, 2017 at 10:54 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> +static int next_byte(const char **cp, const char **endp,
>> +                  const struct diff_options *diffopt)
>> +{
>> +     int retval;
>> +
>> +     if (DIFF_XDL_TST(diffopt, IGNORE_WHITESPACE_AT_EOL)) {
>> +             while (*endp > *cp && isspace(**endp))
>> +                     (*endp)--;
>> +     }
>
> This should be done by the callers (both moved_entry_cmp() and
> get_string_hash()) before starting to iterate over the bytes from
> the beginning, no?

Good point.

Show 5 quoted lines

>> +
>> +     retval = **cp;
>
> The char could be signed, and byte 0xff may become indistinguishable
> from the EOF (i.e. -1) you returned earlier.

Ah, I messed up there. I think EOF is wrong, too. So maybe we'll just return 256 to indicate the end of memory chunk to not have to deal with signedness

Show 6 quoted lines

>> +             if (ca != cb)
>> +                     return 1; /* differs */
>> +             if (!ca)
>
> Shouldn't this check for "ca == -1", as we are not dealing with NUL
> terminated string but a <ptr, len> thing?

Yes, we'd check for the ending symbol instead of 0.