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

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 26, 2015, 19:48 UTC
Message-ID: <xmqqegip3k5y.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1440557461-1078-2-git-send-email-chriscool@tuxfamily.org>

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

Show 13 quoted lines

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