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

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Mar 8, 2024, 10:17 UTC
Message-ID: <d82e11f2-15c1-4ecb-8c7c-19667d2c5fb4@gmail.com>
In-Reply-To: <3f823e48-572c-4e19-ab76-e6d7cab9461f@web.de>

Hi Peff and René

On 07/03/2024 19:42, René Scharfe wrote:

Show 25 quoted lines

> Am 07.03.24 um 10:26 schrieb Jeff King:
>> diff --git a/sequencer.c b/sequencer.c
>> index 991a2dbe96..664986e3b2 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1840,7 +1840,7 @@ static int is_fixup_flag(enum todo_command command, unsigned flag)
>>   static void add_commented_lines(struct strbuf *buf, const void *str, size_t len)
>>   {
>>   	const char *s = str;
>> -	while (len > 0 && s[0] == comment_line_char) {
>> +	while (starts_with_mem(s, len, comment_line_str)) {
>>   		size_t count;
>>   		const char *n = memchr(s, '\n', len);
>>   		if (!n)
>> @@ -2562,7 +2562,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>>   	/* left-trim */
>>   	bol += strspn(bol, " \t");
>>
>> -	if (bol == eol || *bol == '\r' || *bol == comment_line_char) {
>> +	if (bol == eol || *bol == '\r' || starts_with_mem(bol, eol - bol, comment_line_str)) {
> 
> If the strspn() call is safe (which it is, as the caller expects the
> string to be NUL-terminated) then you could use starts_with() here and
> avoid the length calculation.  But that would also match
> comment_line_str values that contain LF, which the _mem version does not > and that's better.

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'.

> 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') ||

Best Wishes

Phillip