From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 29 Apr 2025 22:37:35 GMT
Subject: Re: [PATCH v3] diff: check range before dereferencing an array element
Message-ID: <xmqqldri4ms0.fsf@gitster.g>
In-Reply-To: <20250429215847.GB36727@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

>> @@ -892,7 +892,7 @@ static void fill_es_indent_data(struct emitted_diff_symbol *es)
>>  
>>  	/* skip any \v \f \r at start of indentation */
>>  	while (s[off] == '\f' || s[off] == '\v' ||
>> -	       (s[off] == '\r' && off < len - 1))
>> +	       (off < len - 1 && s[off] == '\r'))
>>  		off++;
>
> ...since the same pattern exists for the other s[off] checks, is it
> worth future-proofing this like:
>
>   while (off < len - 1 &&
>          (s[off] == '\f' || s[off] == '\v' || s[off] == '\r')
>
> ?

But doesn't it change the semantics?

s[off] == '\f', even if off is at the end of the string, i.e. (off
== len - 1), must trigger the off++ increment.

On the other hand, CR that is the part of CRLF at the end of line is
*not* treated like other funny whitespace control characters.  This
"is off not at the end of line, if so check CR" comparison is about
that.