Am 07.03.24 um 20:41 schrieb René Scharfe:

Sorry, sent too early.

Am 07.03.24 um 12:08 schrieb Jeff King:

quoted

On Thu, Mar 07, 2024 at 04:26:38AM -0500, Jeff King wrote:

quoted

IMHO this is the trickiest commit of the whole series, as it would be
easy to get the length computations subtly wrong.

And sure enough...

quoted

diff --git a/trailer.c b/trailer.c
index fe18faf6c5..f59c90b4b5 100644
--- a/trailer.c
+++ b/trailer.c

@@ -882,7 +882,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)

 	/* The first paragraph is the title and cannot be trailers */
 	for (s = buf; s < buf + len; s = next_line(s)) {
-		if (s[0] == comment_line_char)
+		if (starts_with_mem(s, buf + len - s, comment_line_str))
 			continue;
 		if (is_blank_line(s))
 			break;

@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
 		const char **p;
 		ssize_t separator_pos;

-		if (bol[0] == comment_line_char) {
+		if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;

This second hunk needs:

diff --git a/trailer.c b/trailer.c
index f59c90b4b5..fdb0b8137e 100644
--- a/trailer.c
+++ b/trailer.c

@@ -902,7 +902,7 @@ static size_t find_trailer_block_start(const char *buf, size_t len)
 		const char **p;
 		ssize_t separator_pos;

-		if (starts_with_mem(bol, buf + end_of_title - bol, comment_line_str)) {
+		if (starts_with_mem(bol, buf + len - bol, comment_line_str)) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;

I was trying to bound the size based on the loop, which is:

          for (l = last_line(buf, len);
               l >= end_of_title;
               l = last_line(buf, l)) {
                  const char *bol = buf + l;

but I misread "end_of_title" as an upper bound, not a lower one. Which
makes sense because we're iterating backwards over the lines. So I
suppose we could bound it by the previous "bol" value. But in practice,
your prefix won't cross such a boundary anyway, as it won't have a
newline in it (maybe that's something we should enforce? I guess you
could set core.commentChar to '\n' even before my series, which would be
slightly insane).

So just bounding ourselves to "buf + len" seems reasonable, as that
makes sure we don't step outside the buffer passed into the function.

If you don't want or expect LF in comment_line_str, better check it.
And if you do that, most callers of starts_with_mem() -- including this
one -- can use starts_with() instead, as mentioned in my reply to your
patch.  Less calculations, less errors..

René