Re: [PATCH v2 2/2] trailer: support multiline title

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> We currently ignore the first line passed to `git interpret-trailers`,
> when looking for the beginning of the trailers.
>
> Unfortunately this does not work well when a commit is created with a
> line break in the title, using for example the following command:
>
> git commit -m 'place of
> code: change we made'
>
> In this special case, it is best to look at the first line and if it
> does not contain only spaces, consider that the second line is not a
> trailer.
> ---

Missing sign-off, but more importantly, "let's special case the
first line" followed by "oh, that is not enough, we need to check
the found one is sensible and tweak it otherwise" smells like
incrementally papering over things.

I think the analysis behind the first patch is correct.  It stops
the backward scan of the main loop to reach there by realizing that
the first line, which must be the first line of the patch title
paragraph, can never be a trailer.

To extend that correct realization to cover the case where the title
paragraph has more than one line, the right thing to do is to scan
forward from the beginning to find the first paragraph break, which
must be the end of the title paragraph, and exclude the whole thing,
wouldn't it?

That is, I am wondering why the patch is not more like this (there
may be off-by-one, but just to illustrate the approach; I didn't
even compile test this one so...)?

Puzzled...

 static int find_trailer_start(struct strbuf **lines, int count)
 {
-	int start, only_spaces = 1;
+	int start, end_of_title, only_spaces = 1;
+
+	/* The first paragraph is the title and cannot be trailer */
+	for (start = 0; start < count; start++)
+		if (!lines[start]->len)
+			break; /* paragraph break */
+	end_of_title = start;
 
 	/*
 	 * Get the start of the trailers by looking starting from the end
 	 * for a line with only spaces before lines with one separator.
-	 * The first line must not be analyzed as the others as it
-	 * should be either the message title or a blank line.
 	 */
-	for (start = count - 1; start >= 1; start--) {
+	for (start = count - 1; start >= end_of_title; start--) {
 		if (lines[start]->buf[0] == comment_line_char)
 			continue;
 		if (contains_only_spaces(lines[start]->buf)) {