[PATCH v3] trailer: support multiline title

[PATCH v3] trailer: support multiline title

[Christian Couder]

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'

That's why instead of ignoring only the first line, it is better to
ignore the first paragraph.
---
 t/t7513-interpret-trailers.sh | 14 ++++++++++++++
 trailer.c                     | 15 +++++++++++----
 2 files changed, 25 insertions(+), 4 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 9577b4e..56efe88 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -112,6 +112,20 @@ test_expect_success 'with only a title in the message' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with multiline title in the message' '
+	cat >expected <<-\EOF &&
+		place of
+		code: change
+
+		Reviewed-by: Peff
+		Acked-by: Johan
+	EOF
+	printf "%s\n%s\n" "place of" "code: change" |
+	git interpret-trailers --trailer "Reviewed-by: Peff" \
+		--trailer "Acked-by: Johan" >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with config setup' '
 	git config trailer.ack.key "Acked-by: " &&
 	cat >expected <<-\EOF &&
diff --git a/trailer.c b/trailer.c
index b808868..6f3416f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -735,15 +735,22 @@ static int find_patch_start(struct strbuf **lines, int count)
  */
 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 trailers */
+	for (start = 0; start < count; start++) {
+		if (lines[start]->buf[0] == comment_line_char)
+			continue;
+		if (contains_only_spaces(lines[start]->buf))
+			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)) {
-- 
2.5.0.402.gcabd5e3.dirty

Re: [PATCH v3] trailer: support multiline title

[Matthieu Moy]

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

> That's why instead of ignoring only the first line, it is better to
> ignore the first paragraph.
> ---

Lacks sign-off again.

This replaces PATCH 2/2 in your previous series, but requires PATCH 1/2,
right? If so that would be simpler to resend both patches IMHO.

The patch works for me, thanks.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] trailer: support multiline title

[Christian Couder]

On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> That's why instead of ignoring only the first line, it is better to
>> ignore the first paragraph.
>> ---
>
> Lacks sign-off again.

Ooops, sorry.

> This replaces PATCH 2/2 in your previous series, but requires PATCH 1/2,
> right? If so that would be simpler to resend both patches IMHO.

This first patch is already in master. That's why I only sent the
second one. But yeah I could have explained that after the three
dashes.

> The patch works for me, thanks.

Thanks for reviewing and testing,
Christian.

Re: [PATCH v3] trailer: support multiline title

[Matthieu Moy]

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

> This first patch is already in master. That's why I only sent the
> second one. But yeah I could have explained that after the three
> dashes.

OK, my bad, I should have "git fetch"ed.

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/

Re: [PATCH v3] trailer: support multiline title

[Junio C Hamano]

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'
>
> That's why instead of ignoring only the first line, it is better to
> ignore the first paragraph.
> ---
>  t/t7513-interpret-trailers.sh | 14 ++++++++++++++
>  trailer.c                     | 15 +++++++++++----
>  2 files changed, 25 insertions(+), 4 deletions(-)
>
> diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
> index 9577b4e..56efe88 100755
> --- a/t/t7513-interpret-trailers.sh
> +++ b/t/t7513-interpret-trailers.sh
> @@ -112,6 +112,20 @@ test_expect_success 'with only a title in the message' '
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'with multiline title in the message' '
> +	cat >expected <<-\EOF &&
> +		place of
> +		code: change
> +
> +		Reviewed-by: Peff
> +		Acked-by: Johan
> +	EOF
> +	printf "%s\n%s\n" "place of" "code: change" |

Just FYI, I think "%s\n" is sufficient to produce multi-line output,
e.g.

	printf "%s xyzzy\n" a b c d: e:

> +	git interpret-trailers --trailer "Reviewed-by: Peff" \
> +		--trailer "Acked-by: Johan" >actual &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'with config setup' '
>  	git config trailer.ack.key "Acked-by: " &&
>  	cat >expected <<-\EOF &&
> diff --git a/trailer.c b/trailer.c
> index b808868..6f3416f 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -735,15 +735,22 @@ static int find_patch_start(struct strbuf **lines, int count)
>   */
>  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 trailers */
> +	for (start = 0; start < count; start++) {
> +		if (lines[start]->buf[0] == comment_line_char)
> +			continue;
> +		if (contains_only_spaces(lines[start]->buf))
> +			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)) {

Re: [PATCH v3] trailer: support multiline title

[Junio C Hamano]

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

> On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy
> <Matthieu.Moy@grenoble-inp.fr> wrote:
>> Christian Couder <christian.couder@gmail.com> writes:
>>
>>> That's why instead of ignoring only the first line, it is better to
>>> ignore the first paragraph.
>>> ---
>>
>> Lacks sign-off again.
>
> Ooops, sorry.

Let me forge one just for this time.

>
>> This replaces PATCH 2/2 in your previous series, but requires PATCH 1/2,
>> right? If so that would be simpler to resend both patches IMHO.
>
> This first patch is already in master. That's why I only sent the
> second one. But yeah I could have explained that after the three
> dashes.
>
>> The patch works for me, thanks.
>
> Thanks for reviewing and testing,
> Christian.

Re: [PATCH v3] trailer: support multiline title

[Christian Couder]

On Mon, Aug 31, 2015 at 8:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy
>> <Matthieu.Moy@grenoble-inp.fr> wrote:
>>> Christian Couder <christian.couder@gmail.com> writes:
>>>
>>>> That's why instead of ignoring only the first line, it is better to
>>>> ignore the first paragraph.
>>>> ---
>>>
>>> Lacks sign-off again.
>>
>> Ooops, sorry.
>
> Let me forge one just for this time.

Great, thanks!