Re: [PATCH 11/15] find multi-byte comment chars in unterminated buffers

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 8, 2024, 16:20 UTC
Message-ID: <66a70346-98e8-44dd-a680-36a21df5f32a@gmail.com>
In-Reply-To: <xmqqfrx0hg01.fsf@gitster.g>

On 08/03/2024 15:58, Junio C Hamano wrote:

Show 16 quoted lines

> Phillip Wood <phillip.wood123@gmail.com> writes:
> 
>> I agree with your analysis. I do wonder though if we should reject
>> whitespace and control characters when parsing core.commentChar, it
>> feels like accepting them is a bug waiting to happen. If
>> comment_line_char starts with ' ' or '\t' that part will be eaten by
>> the strspn() above and so starts_with_mem() wont match. Also we will
>> never match a comment if comment_line_str contains '\n'.
> 
> Another thing I was wondering is what we want to do a random
> byte-sequence that may match from the middle of a multi-byte UTF-8
> character.
> 
> The reason I haven't mentioned these "nonsense input" is because
> they will at worst only lead to self-denial-of-service to those who
> are too curious, and will fall into "don't do it then" category.

We could certainly leave it as-is and tell users they are only hurting themselves if they complain when it does not work.

> Also, what exactly is the definition of "nonsense" will become can
> of worms.  I can sympathise if somebody wants to use "#\t" to give
> themselves a bit more room than usual on the left for visibility,
> for example, so there might be a case to want whitespace characters.

That's fair, maybe we could just ban leading whitespace if we do decide to restrict core.commentChar

Best Wishes

Phillip

Show 11 quoted lines

>>> Not sure why lines that start with CR are considered comment lines,
>>> though.
>>
>> I think it is a lazy way of looking for an empty line ending in CR LF,
>> it should really be
>>
>> 	|| (bol[0] == '\r' && bol[1] == '\n') ||
> 
> My recollection matches your speculation.
> 
> IIRC the lazy persono was probably me but I didn't run "git blame".