# [PATCH v3] trailer: support multiline title 7 messages from 2015-08-30 to 2015-08-31. Participants: Christian Couder, Matthieu Moy, Junio C Hamano. Thread: https://gitlist.dev/t/40223 ## Christian Couder, 2015-08-30 19:14 Subject: [PATCH v3] trailer: support multiline title Message-ID: <1440962080-24804-1-git-send-email-chriscool@tuxfamily.org> URL: https://gitlist.dev/e/1440962080-24804-1-git-send-email-chriscool%40tuxfamily.org ``` 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 ``` ## Matthieu Moy, 2015-08-31 08:38 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/vpqtwrfg8dt.fsf%40anie.imag.fr In-Reply-To: <1440962080-24804-1-git-send-email-chriscool@tuxfamily.org> ``` Christian Couder 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/ ``` ## Christian Couder, 2015-08-31 09:14 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/CAP8UFD3KH%3D5Y%2BY3MKu7U-FkSTeu1qFkRYapne8LyNWVT_qF9iQ%40mail.gmail.com In-Reply-To: ``` On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy wrote: > Christian Couder 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. ``` ## Matthieu Moy, 2015-08-31 09:17 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/vpqio7ves19.fsf%40anie.imag.fr In-Reply-To: ``` Christian Couder 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/ ``` ## Junio C Hamano, 2015-08-31 18:13 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/xmqq8u8rwclf.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1440962080-24804-1-git-send-email-chriscool@tuxfamily.org> ``` Christian Couder 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)) { ``` ## Junio C Hamano, 2015-08-31 18:13 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/xmqq4mjfwcl0.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Christian Couder writes: > On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy > wrote: >> Christian Couder 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. ``` ## Christian Couder, 2015-08-31 18:26 Subject: Re: [PATCH v3] trailer: support multiline title Message-ID: URL: https://gitlist.dev/e/CAP8UFD0Y9HTzkGkrKnW3NABdGGVhzfcNPRaL6Ay9oNKMn%2BrLUA%40mail.gmail.com In-Reply-To: ``` On Mon, Aug 31, 2015 at 8:13 PM, Junio C Hamano wrote: > Christian Couder writes: > >> On Mon, Aug 31, 2015 at 10:38 AM, Matthieu Moy >> wrote: >>> Christian Couder 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! ```