Hi Peff and René

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

Am 07.03.24 um 10:26 schrieb Jeff King:

quoted

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