[PATCH v2 1/2] trailer: ignore first line of message

[PATCH v2 1/2] trailer: ignore first line of message

[Christian Couder]

When looking for the start of the trailers in the message
we are passed, we should ignore the first line of the message.

The reason is that if we are passed a patch or commit message
then the first line should be the patch title.
If we are passed only trailers we can expect that they start
with an empty line that can be ignored too.

This way we can properly process commit messages that have
only one line with something that looks like a trailer, for
example like "area of code: change we made".
---
 t/t7513-interpret-trailers.sh | 15 ++++++++++++++-
 trailer.c                     |  4 +++-
 2 files changed, 17 insertions(+), 2 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index bd0ab46..9577b4e 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -93,12 +93,25 @@ test_expect_success 'with config option on the command line' '
 		Acked-by: Johan
 		Reviewed-by: Peff
 	EOF
-	echo "Acked-by: Johan" |
+	{ echo; echo "Acked-by: Johan"; } |
 	git -c "trailer.Acked-by.ifexists=addifdifferent" interpret-trailers \
 		--trailer "Reviewed-by: Peff" --trailer "Acked-by: Johan" >actual &&
 	test_cmp expected actual
 '
 
+test_expect_success 'with only a title in the message' '
+	cat >expected <<-\EOF &&
+		area: change
+
+		Reviewed-by: Peff
+		Acked-by: Johan
+	EOF
+	echo "area: 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 4b14a56..b808868 100644
--- a/trailer.c
+++ b/trailer.c
@@ -740,8 +740,10 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	/*
 	 * 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 >= 0; start--) {
+	for (start = count - 1; start >= 1; start--) {
 		if (lines[start]->buf[0] == comment_line_char)
 			continue;
 		if (contains_only_spaces(lines[start]->buf)) {
-- 
2.5.0.401.g009ef9b.dirty

[PATCH v2 2/2] 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'

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.
---
 t/t7513-interpret-trailers.sh | 14 ++++++++++++++
 trailer.c                     |  8 +++++++-
 2 files changed, 21 insertions(+), 1 deletion(-)

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..9a54449 100644
--- a/trailer.c
+++ b/trailer.c
@@ -759,7 +759,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
 		return count;
 	}
 
-	return only_spaces ? count : 0;
+	if (only_spaces)
+		return count;
+
+	if (contains_only_spaces(lines[0]->buf))
+		return 1;
+
+	return count;
 }
 
 /* Get the index of the end of the trailers */
-- 
2.5.0.401.g009ef9b.dirty

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

[Matthieu Moy]

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

> 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'

I confirm that this patch fixes the behavior for me.

Now, I found another issue: I still have this "interpret-trailers" in my
hooks/commit-msg, and it behaves badly when I use "git commit -v". With
-v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to
insert my Sign-off within the diff, like this:

  # Do not touch the line above.
  # Everything below will be removed.
  diff --git a/git-multimail/README b/git-multimail/README
  index f41906b..93d4751 100644
  
  Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
  --- a/git-multimail/README
  +++ b/git-multimail/README

Either commit-msg should be called after stripping the diff from
COMMIT_MSG, or interpret-trailers should learn to stop reading when the
patch starts. I think the first option is better, since it means that
any commit-msg hook does not have to deal with the patch stuff (my guess
is that there are many broken commit-msg hooks out there, but people
didn't notice because they don't use "commit -v").

Thanks,

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

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

[Jacob Keller]

On Tue, Aug 25, 2015 at 11:07 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
> Christian Couder <christian.couder@gmail.com> writes:
>
>> 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'
>
> I confirm that this patch fixes the behavior for me.
>
> Now, I found another issue: I still have this "interpret-trailers" in my
> hooks/commit-msg, and it behaves badly when I use "git commit -v". With
> -v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to
> insert my Sign-off within the diff, like this:
>
>   # Do not touch the line above.
>   # Everything below will be removed.
>   diff --git a/git-multimail/README b/git-multimail/README
>   index f41906b..93d4751 100644
>
>   Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>   --- a/git-multimail/README
>   +++ b/git-multimail/README
>
> Either commit-msg should be called after stripping the diff from
> COMMIT_MSG, or interpret-trailers should learn to stop reading when the
> patch starts. I think the first option is better, since it means that
> any commit-msg hook does not have to deal with the patch stuff (my guess
> is that there are many broken commit-msg hooks out there, but people
> didn't notice because they don't use "commit -v").
>
> Thanks,
>

It's always confused me why commit -v doesn't prepend every inserted
line with "#" to mark it as a comment.

Regards,
Jake

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

[Christian Couder]

Sorry I sent the part below privately by mistake:

On Tue, Aug 25, 2015 at 11:07 PM, Matthieu Moy
<Matthieu.Moy@grenoble-inp.fr> wrote:
>
> Now, I found another issue: I still have this "interpret-trailers" in my
> hooks/commit-msg, and it behaves badly when I use "git commit -v". With
> -v, I get a diff in COMMIT_EDITMSG, and interpret-trailers tries to
> insert my Sign-off within the diff, like this:
>
>   # Do not touch the line above.
>   # Everything below will be removed.
>   diff --git a/git-multimail/README b/git-multimail/README
>   index f41906b..93d4751 100644
>
>   Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
>   --- a/git-multimail/README
>   +++ b/git-multimail/README

This might be a bug. I will have a look.

> Either commit-msg should be called after stripping the diff from
> COMMIT_MSG, or interpret-trailers should learn to stop reading when the
> patch starts.

There is already code to detect a patch in interpret-trailers, but it
relies on the patch starting with a line with only three dashes.

So another option would be to make "commit -v" emit a line with three
dashes just under the "# Everything below will be removed." line.

> I think the first option is better, since it means that
> any commit-msg hook does not have to deal with the patch stuff (my guess
> is that there are many broken commit-msg hooks out there, but people
> didn't notice because they don't use "commit -v").

Maybe. I don't know if there is a reason why the commit-msg is called
before removing the patch.

On Wed, Aug 26, 2015 at 8:28 AM, Jacob Keller <jacob.keller@gmail.com> wrote:
>
> It's always confused me why commit -v doesn't prepend every inserted
> line with "#" to mark it as a comment.

I think that would make interpret-trailers work properly too.

Thanks both,
Christian.

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

[Junio C Hamano]

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

> There is already code to detect a patch in interpret-trailers, but it
> relies on the patch starting with a line with only three dashes.

Hmm, then it can be taught to notice "everything below..." as
another marker, right?

> Maybe. I don't know if there is a reason why the commit-msg is called
> before removing the patch.

Is that "removing", or are you talking about changing the order from

 - prepare log template in-core
 - add comments and patch to that in-core copy
 - write in-core copy out
 - run hook
 - read the hook's result in-core
 - use the message

to

 - prepare log template in-core
 - write in-core copy out
 - run hook
 - read the hook's result in-core
 - add comments and patch to that in-core copy
 - use the message

While the reordering would certainly stop showing the comments and
patch, I am not sure if that is a move in the right direction.  It
will rob from the hooks information that they have traditionally
been given---it will break some hooks.

But if interpret-trailers is almost there to reliably know where the
log message ends, teaching it the one last step would be the right
thing to do anyway.  After all, interpret-trailers was invented
exactly because we did not want individual hooks to roll their own
ways to detect the end of the message proper, so the command should
know where the message ends.

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

[Matthieu Moy]

Junio C Hamano <gitster@pobox.com> writes:

> While the reordering would certainly stop showing the comments and
> patch, I am not sure if that is a move in the right direction.  It
> will rob from the hooks information that they have traditionally
> been given---

The information given in the comments do not have a 100% stable format,
and the hook is executed after letting the user possibly edit or delete
it, so I'm tempted to say that a hook using the commit comment is
broken.

Using the diff in the hook _is_ really broken: it relies on the user
calling "git commit" with -v, and there's nothing to garantee that.

> it will break some hooks.

It will also repair some hooks that were broken, but whose breakage was
never noticed or never explained.

> After all, interpret-trailers was invented exactly because we did not
> want individual hooks to roll their own ways to detect the end of the
> message proper, so the command should know where the message ends.

Right, but you can't prevent people from writting

command-that-shows-stuff >> "$1"

in their commit-msg hook. And these people will keep wondering why their
hook "sometimes doesn't work" (that's how I considered it for a while,
it took me a few commits to notice the correlation between "-v" and lack
of sign-off).

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

Re: [PATCH v2 2/2] 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'
>
> 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)) {

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

[Christian Couder]

On Wed, Aug 26, 2015 at 9:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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,

Ok, will add it.

[...]

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

Yeah, we can do that. It will be clearer.

Thanks,
Christian.