[PATCH 0/5] Small "git interpret-trailers" fixes

[PATCH 0/5] Small "git interpret-trailers" fixes

[Christian Couder]

Here is a series to fix a few small bugs in "git interpret-trailers".

Patch 1/5 and patch 2/5 are independent from the last 3 patches,
and can be applied to master.

Patches 3/5, 4/5 and 5/5 depend on this series by Junio:

	jc/conflict-hint (2014-10-28) 4 commits
	  (merged to 'next' on 2014-10-29 at 693250f)

Christian Couder (5):
  trailer: ignore comment lines inside the trailers
  trailer: display a trailer without its trailing newline
  commit: make ignore_non_trailer() non static
  trailer: reuse ignore_non_trailer() to ignore conflict lines
  trailer: add test with an old style conflict block

 builtin/commit.c              | 46 -------------------------------------------
 commit.c                      | 46 +++++++++++++++++++++++++++++++++++++++++++
 commit.h                      |  3 +++
 t/t7513-interpret-trailers.sh | 34 +++++++++++++++++++++++++++++++-
 trailer.c                     | 39 +++++++++++++++++++++++++-----------
 5 files changed, 110 insertions(+), 58 deletions(-)

-- 
2.1.2.555.gfbecd99

[PATCH 1/5] trailer: ignore comment lines inside the trailers

[Christian Couder]

Otherwise trailers that are commented out might be
processed. We would also error out if the comment line
char is also a separator.

This means that comments inside a trailer block will
disappear, but that was already the case anyway.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/trailer.c b/trailer.c
index 8514566..761b763 100644
--- a/trailer.c
+++ b/trailer.c
@@ -804,8 +804,10 @@ static int process_input_file(struct strbuf **lines,
 
 	/* Parse trailer lines */
 	for (i = trailer_start; i < patch_start; i++) {
-		struct trailer_item *new = create_trailer_item(lines[i]->buf);
-		add_trailer_item(in_tok_first, in_tok_last, new);
+		if (lines[i]->buf[0] != comment_line_char) {
+			struct trailer_item *new = create_trailer_item(lines[i]->buf);
+			add_trailer_item(in_tok_first, in_tok_last, new);
+		}
 	}
 
 	return patch_start;
-- 
2.1.2.555.gfbecd99

[PATCH 2/5] trailer: display a trailer without its trailing newline

[Christian Couder]

Trailers passed to the parse_trailer() function have
a trailing newline. When erroring out, we should display
the invalid trailer properly, that means without any
trailing newline.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 trailer.c | 8 ++++++--
 1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/trailer.c b/trailer.c
index 761b763..f4d51ba 100644
--- a/trailer.c
+++ b/trailer.c
@@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra
 	strbuf_addch(&seps, '=');
 	len = strcspn(trailer, seps.buf);
 	strbuf_release(&seps);
-	if (len == 0)
-		return error(_("empty trailer token in trailer '%s'"), trailer);
+	if (len == 0) {
+		struct strbuf sb = STRBUF_INIT;
+		strbuf_addstr(&sb, trailer);
+		strbuf_rtrim(&sb);
+		return error(_("empty trailer token in trailer '%s'"), sb.buf);
+	}
 	if (len < strlen(trailer)) {
 		strbuf_add(tok, trailer, len);
 		strbuf_trim(tok);
-- 
2.1.2.555.gfbecd99

Re: [PATCH 2/5] trailer: display a trailer without its trailing newline

[Jeff King]

On Fri, Nov 07, 2014 at 07:50:49PM +0100, Christian Couder wrote:

> diff --git a/trailer.c b/trailer.c
> index 761b763..f4d51ba 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok, struct strbuf *val, const char *tra
>  	strbuf_addch(&seps, '=');
>  	len = strcspn(trailer, seps.buf);
>  	strbuf_release(&seps);
> -	if (len == 0)
> -		return error(_("empty trailer token in trailer '%s'"), trailer);
> +	if (len == 0) {
> +		struct strbuf sb = STRBUF_INIT;
> +		strbuf_addstr(&sb, trailer);
> +		strbuf_rtrim(&sb);
> +		return error(_("empty trailer token in trailer '%s'"), sb.buf);
> +	}

Doesn't this leak sb.buf?

-Peff

Re: [PATCH 2/5] trailer: display a trailer without its trailing newline

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Nov 07, 2014 at 07:50:49PM +0100, Christian Couder wrote:
>
>> diff --git a/trailer.c b/trailer.c
>> index 761b763..f4d51ba 100644
>> --- a/trailer.c
>> +++ b/trailer.c
>> @@ -583,8 +583,12 @@ static int parse_trailer(struct strbuf *tok,
>> struct strbuf *val, const char *tra
>>  	strbuf_addch(&seps, '=');
>>  	len = strcspn(trailer, seps.buf);
>>  	strbuf_release(&seps);
>> -	if (len == 0)
>> -		return error(_("empty trailer token in trailer '%s'"), trailer);
>> +	if (len == 0) {
>> +		struct strbuf sb = STRBUF_INIT;
>> +		strbuf_addstr(&sb, trailer);
>> +		strbuf_rtrim(&sb);
>> +		return error(_("empty trailer token in trailer '%s'"), sb.buf);
>> +	}
>
> Doesn't this leak sb.buf?

Yes.  "%.*s" might be your friend.

[PATCH 3/5] commit: make ignore_non_trailer() non static

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/commit.c | 46 ----------------------------------------------
 commit.c         | 46 ++++++++++++++++++++++++++++++++++++++++++++++
 commit.h         |  3 +++
 3 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index e3c60dd..cda74e9 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -677,52 +677,6 @@ static void adjust_comment_line_char(const struct strbuf *sb)
 	comment_line_char = *p;
 }
 
-/*
- * Inspect sb and determine the true "end" of the log message, in
- * order to find where to put a new Signed-off-by: line.  Ignored are
- * trailing comment lines and blank lines, and also the traditional
- * "Conflicts:" block that is not commented out, so that we can use
- * "git commit -s --amend" on an existing commit that forgot to remove
- * it.
- *
- * Returns the number of bytes from the tail to ignore, to be fed as
- * the second parameter to append_signoff().
- */
-static int ignore_non_trailer(struct strbuf *sb)
-{
-	int boc = 0;
-	int bol = 0;
-	int in_old_conflicts_block = 0;
-
-	while (bol < sb->len) {
-		char *next_line;
-
-		if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
-			next_line = sb->buf + sb->len;
-		else
-			next_line++;
-
-		if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
-			/* is this the first of the run of comments? */
-			if (!boc)
-				boc = bol;
-			/* otherwise, it is just continuing */
-		} else if (starts_with(sb->buf + bol, "Conflicts:\n")) {
-			in_old_conflicts_block = 1;
-			if (!boc)
-				boc = bol;
-		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
-			; /* a pathname in the conflicts block */
-		} else if (boc) {
-			/* the previous was not trailing comment */
-			boc = 0;
-			in_old_conflicts_block = 0;
-		}
-		bol = next_line - sb->buf;
-	}
-	return boc ? sb->len - boc : 0;
-}
-
 static int prepare_to_commit(const char *index_file, const char *prefix,
 			     struct commit *current_head,
 			     struct wt_status *s,
diff --git a/commit.c b/commit.c
index 19cf8f9..a54cb9a 100644
--- a/commit.c
+++ b/commit.c
@@ -1640,3 +1640,49 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
 	}
 	return NULL;
 }
+
+/*
+ * Inspect sb and determine the true "end" of the log message, in
+ * order to find where to put a new Signed-off-by: line.  Ignored are
+ * trailing comment lines and blank lines, and also the traditional
+ * "Conflicts:" block that is not commented out, so that we can use
+ * "git commit -s --amend" on an existing commit that forgot to remove
+ * it.
+ *
+ * Returns the number of bytes from the tail to ignore, to be fed as
+ * the second parameter to append_signoff().
+ */
+int ignore_non_trailer(struct strbuf *sb)
+{
+	int boc = 0;
+	int bol = 0;
+	int in_old_conflicts_block = 0;
+
+	while (bol < sb->len) {
+		char *next_line;
+
+		if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
+			next_line = sb->buf + sb->len;
+		else
+			next_line++;
+
+		if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+			/* is this the first of the run of comments? */
+			if (!boc)
+				boc = bol;
+			/* otherwise, it is just continuing */
+		} else if (starts_with(sb->buf + bol, "Conflicts:\n")) {
+			in_old_conflicts_block = 1;
+			if (!boc)
+				boc = bol;
+		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+			; /* a pathname in the conflicts block */
+		} else if (boc) {
+			/* the previous was not trailing comment */
+			boc = 0;
+			in_old_conflicts_block = 0;
+		}
+		bol = next_line - sb->buf;
+	}
+	return boc ? sb->len - boc : 0;
+}
diff --git a/commit.h b/commit.h
index bc68ccb..cd35ac1 100644
--- a/commit.h
+++ b/commit.h
@@ -337,6 +337,9 @@ extern void free_commit_extra_headers(struct commit_extra_header *extra);
 extern const char *find_commit_header(const char *msg, const char *key,
 				      size_t *out_len);
 
+/* Find the end of the log message, the right place for a new trailer. */
+extern int ignore_non_trailer(struct strbuf *sb);
+
 typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
 
-- 
2.1.2.555.gfbecd99

[PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Christian Couder]

Make sure we look for trailers before any conflict line
by reusing the ignore_non_trailer() function.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh |  2 ++
 trailer.c                     | 25 ++++++++++++++++++-------
 2 files changed, 20 insertions(+), 7 deletions(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index 1efb880..fed053a 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -232,6 +232,8 @@ test_expect_success 'with message that has comments' '
 
 		Reviewed-by: Johan
 		Cc: Peff
+		# last comment
+
 	EOF
 	cat basic_patch >>expected &&
 	git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual &&
diff --git a/trailer.c b/trailer.c
index f4d51ba..f6aa181 100644
--- a/trailer.c
+++ b/trailer.c
@@ -2,6 +2,7 @@
 #include "string-list.h"
 #include "run-command.h"
 #include "string-list.h"
+#include "commit.h"
 #include "trailer.h"
 /*
  * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
@@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,
 			      struct trailer_item **in_tok_last)
 {
 	int count = 0;
-	int patch_start, trailer_start, i;
+	int trailer_start, trailer_end, patch_start, ignore_bytes, i;
+	struct strbuf sb;
 
 	/* Get the line count */
 	while (lines[count])
 		count++;
 
 	patch_start = find_patch_start(lines, count);
-	trailer_start = find_trailer_start(lines, patch_start);
+
+	/* Get the index of the end of the trailers */
+	for (i = 0; i < patch_start; i++)
+		strbuf_addbuf(&sb, lines[i]);
+	ignore_bytes = ignore_non_trailer(&sb);
+	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
+		ignore_bytes -= lines[i]->len;
+	trailer_end = i + 1;
+
+	trailer_start = find_trailer_start(lines, trailer_end);
 
 	/* Print lines before the trailers as is */
 	print_lines(lines, 0, trailer_start);
@@ -807,14 +818,14 @@ static int process_input_file(struct strbuf **lines,
 		printf("\n");
 
 	/* Parse trailer lines */
-	for (i = trailer_start; i < patch_start; i++) {
+	for (i = trailer_start; i < trailer_end; i++) {
 		if (lines[i]->buf[0] != comment_line_char) {
 			struct trailer_item *new = create_trailer_item(lines[i]->buf);
 			add_trailer_item(in_tok_first, in_tok_last, new);
 		}
 	}
 
-	return patch_start;
+	return trailer_end;
 }
 
 static void free_all(struct trailer_item **first)
@@ -831,7 +842,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	struct trailer_item *in_tok_last = NULL;
 	struct trailer_item *arg_tok_first;
 	struct strbuf **lines;
-	int patch_start;
+	int trailer_end;
 
 	/* Default config must be setup first */
 	git_config(git_trailer_default_config, NULL);
@@ -840,7 +851,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	lines = read_input_file(file);
 
 	/* Print the lines before the trailers */
-	patch_start = process_input_file(lines, &in_tok_first, &in_tok_last);
+	trailer_end = process_input_file(lines, &in_tok_first, &in_tok_last);
 
 	arg_tok_first = process_command_line_args(trailers);
 
@@ -851,7 +862,7 @@ void process_trailers(const char *file, int trim_empty, struct string_list *trai
 	free_all(&in_tok_first);
 
 	/* Print the lines after the trailers as is */
-	print_lines(lines, patch_start, INT_MAX);
+	print_lines(lines, trailer_end, INT_MAX);
 
 	strbuf_list_free(lines);
 }
-- 
2.1.2.555.gfbecd99

Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
> @@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,
>  			      struct trailer_item **in_tok_last)
>  {
>  	int count = 0;
> -	int patch_start, trailer_start, i;
> +	int trailer_start, trailer_end, patch_start, ignore_bytes, i;
> +	struct strbuf sb;
>  
>  	/* Get the line count */
>  	while (lines[count])
>  		count++;
>  
>  	patch_start = find_patch_start(lines, count);
> -	trailer_start = find_trailer_start(lines, patch_start);
> +
> +	/* Get the index of the end of the trailers */
> +	for (i = 0; i < patch_start; i++)
> +		strbuf_addbuf(&sb, lines[i]);
> +	ignore_bytes = ignore_non_trailer(&sb);
> +	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
> +		ignore_bytes -= lines[i]->len;
> +	trailer_end = i + 1;

Looks like there is an impedance mismatch between the caller and the
callee here.  I can sort of understand why you might want to have an
array of trailer items, one element per line in the trailer block,
as that would make it easier on your brain when you have to reorder
them, insert a new one or a remove an existing one, but you seem to
be keeping _everything_ in that format, an array of strbuf with one
strbuf per line, which sounds really wasteful.  The data structure
might need to be rethoughtbefore this code becomes ready for
production.

I would have expected it to be more like (1) slurp everything in a
single strbuf, (2) find the trailer block inside that single strbuf,
splitting what you read in the previous step into one strbuf for
stuff before the trailer block, another strbuf for stuff after the
trailer block and an array of lines in the tailer block, (3) do
whatever your trailer flipping logic inside that array without
having to worry about stuff before or after the trailer block and
then finally (4) spit the whole thing out by concatenating the stuff
before the trailer block, the stuff in the trailer block and the
stuff after the trailer block.

Oh well...

Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines
Date: Fri, 07 Nov 2014 12:27:03 -0800

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>>   * Copyright (c) 2013, 2014 Christian Couder <chriscool@tuxfamily.org>
>> @@ -791,14 +792,24 @@ static int process_input_file(struct strbuf **lines,
>>  			      struct trailer_item **in_tok_last)
>>  {
>>  	int count = 0;
>> -	int patch_start, trailer_start, i;
>> +	int trailer_start, trailer_end, patch_start, ignore_bytes, i;
>> +	struct strbuf sb;
>>  
>>  	/* Get the line count */
>>  	while (lines[count])
>>  		count++;
>>  
>>  	patch_start = find_patch_start(lines, count);
>> -	trailer_start = find_trailer_start(lines, patch_start);
>> +
>> +	/* Get the index of the end of the trailers */
>> +	for (i = 0; i < patch_start; i++)
>> +		strbuf_addbuf(&sb, lines[i]);
>> +	ignore_bytes = ignore_non_trailer(&sb);
>> +	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
>> +		ignore_bytes -= lines[i]->len;
>> +	trailer_end = i + 1;
> 
> Looks like there is an impedance mismatch between the caller and the
> callee here.  I can sort of understand why you might want to have an
> array of trailer items, one element per line in the trailer block,
> as that would make it easier on your brain when you have to reorder
> them, insert a new one or a remove an existing one, but you seem to
> be keeping _everything_ in that format, an array of strbuf with one
> strbuf per line, which sounds really wasteful.  The data structure
> might need to be rethoughtbefore this code becomes ready for
> production.
> 
> I would have expected it to be more like (1) slurp everything in a
> single strbuf, (2) find the trailer block inside that single strbuf,
> splitting what you read in the previous step into one strbuf for
> stuff before the trailer block, another strbuf for stuff after the
> trailer block and an array of lines in the tailer block, (3) do
> whatever your trailer flipping logic inside that array without
> having to worry about stuff before or after the trailer block and
> then finally (4) spit the whole thing out by concatenating the stuff
> before the trailer block, the stuff in the trailer block and the
> stuff after the trailer block.
> 
> Oh well...

I hope it is better now, as I encapsulated the call to
ignore_non_trailer() into a new find_trailer_end() function. There
were already find_trailer_start() and find_patch_start(), and I think
this way we could have a nice high level line oriented API.

Yeah, it won't be as efficient as using only one strbuf and only byte
oriented functions, and it looks much less manly too :-) But over time in
Git we have developed a number of less efficient but quite clean
abstractions like strbuf, argv_array, sha1_array and so on, that we
are quite happy with.

So yeah, with old age we might have lost some manliness and lament the
time when men were really men, and ... Well, let's wait for the
hopefully big party^W conference next year to do that together :-)

Thanks,
Christian.

Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Yeah, it won't be as efficient as using only one strbuf and only byte
> oriented functions, and it looks much less manly too :-) But over time in
> Git we have developed a number of less efficient but quite clean
> abstractions like strbuf, argv_array, sha1_array and so on, that we
> are quite happy with.

Actually, all these examples you gave are fairly efficient and clean
abstractions.  I find it insulting to pretend that the "one line per
strbuf" is in the same league.  It isn't.

And it is not about manliness.

Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano]

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

> Christian Couder <chriscool@tuxfamily.org> writes:
>
>> Yeah, it won't be as efficient as using only one strbuf and only byte
>> oriented functions, and it looks much less manly too :-) But over time in
>> Git we have developed a number of less efficient but quite clean
>> abstractions like strbuf, argv_array, sha1_array and so on, that we
>> are quite happy with.
>
> Actually, all these examples you gave are fairly efficient and clean
> abstractions.  I find it insulting to pretend that the "one line per
> strbuf" is in the same league.  It isn't.
>
> And it is not about manliness.

By the way, I do not mean to say that all of strbuf (which is a
rather full API) is uniformly efficient and cleanly abstracted.

For example, I see strbuf_split() is a handy but a lousy invitation
to stupid programmers to do stupid things and needs to be used
carefully (rather, you need to carefully consider if a possible
solution that uses it is really a good solution).  If you start with
a single strbuf (e.g. perhaps read from a file or a blob in bulk in
an I/O efficient way), pass it around and then have to manipulate
each and every line in it to massage the information on each line
into other useful form (e.g. perhaps each line is a [+]src:dst
refspec element and you are parsing it into an array of "struct
refspec"), it may be tempting to write a parser from a single strbuf
into a single refspec, use strbuf_split() to break the single strbuf
input into multiple and feed each of them to your parser.

But it does not have to be the only way to create such a caller.
Unless the final "other useful form" is an array of strbuf, and you
have to remember that the whole point of using strbuf is because it
is easy to edit its contents efficiently in place, that needs to
further be modified in various ways, the splitting of N lines into N
strbuf, only to parse each line one by one, is a huge waste.  A
right approach may involve introducing a foreach_strbuf_line
iterator function that takes a callback, or a macro of the same name
that puts a control structure.

Going back to the trailer processing, if you are likely to be
editing each and every line in the input before producing the final
result, an array of strbuf would be a perfectly sensible data
structure to use.  I just didn't get the impression that that is
what is being done.  You would be presented a single text with
multiple lines (whose natural representation would be a <ptr,len>
that fits in a single strbuf), you would be asked to inspect and
manipulate one single section in the middle (namely, the runs of
lines that begin with some "keyword: "), and return the
concatenation of the part before that one section (intact), the
section you have manipulated (i.e. the trailer) and possibly the
remainder (intact).  Outside that single section you will be
touching, I do not see a good reason for each of their lines to be
first stored in a strbuf (while moving bunch of pointers in the
array using ARRAY_GROW() and friends), only to be later concatenated
back into a single string.

Re: [PATCH 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Christian Couder <chriscool@tuxfamily.org> writes:
>>
>>> Yeah, it won't be as efficient as using only one strbuf and only byte
>>> oriented functions, and it looks much less manly too :-) But over time in
>>> Git we have developed a number of less efficient but quite clean
>>> abstractions like strbuf, argv_array, sha1_array and so on, that we
>>> are quite happy with.
>>
>> Actually, all these examples you gave are fairly efficient and clean
>> abstractions.  I find it insulting to pretend that the "one line per
>> strbuf" is in the same league.  It isn't.
>>
>> And it is not about manliness.
>
> By the way, I do not mean to say that all of strbuf (which is a
> rather full API) is uniformly efficient and cleanly abstracted.
> ...

Having said all that, please notice that I said "might need to be
rethought before the code becomes ready for production."

After all, it would have perfectly been OK to experiment with this
new topic in a script; e.g. this topic being text processing, it
might have happened as a Perl script while we get the semantics and
desirable features nailed down before rewriting the real thing in C.

And if that were what happened here, I wouldn't have been talking
about data structures and unnecessary complexity having to have one
strbuf per line only to join them into one string (or printf() out
to a stream one-after-another) later.

In other words, I did not say "this code is too ugly to live and we
should clean it up as soon as possible or we should revert it from
'master'---it was a mistake to merge it".

I consider the "trailer" code in 'master' today as experimental (in
other words, something I might have written in a scripting language
as a mock-up if I were doing it) that needs to be worked on further
to still get the features and semantics right.  And the patch series
in this thread are hopefully taking us in the right direction to get
them right [*1*].

You could have just said "Yeah, I realize that the code is way
suboptimal, but lets get it feature complete first and then clean it
up", or something, instead of getting defensive with unnecessary
excuses.


[Footnote]

*1* ... even though this step exposes why the approach taken,
splitting the input lines first into individual lines without even
looking at them, is a wrong one---if you started from a single
buffer that is not yet split, it would have been much easier to find
where the trailer block is, and that is why you are re-joining the
buffer you have already split into lines (i.e. what I called
"impedance mismatch"), which shows that the initial splitting is
done prematurely.  Hopefully you would realize that by now ;-).

[PATCH 5/5] trailer: add test with an old style conflict block

[Christian Couder]

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/t/t7513-interpret-trailers.sh b/t/t7513-interpret-trailers.sh
index fed053a..bd0ab46 100755
--- a/t/t7513-interpret-trailers.sh
+++ b/t/t7513-interpret-trailers.sh
@@ -213,7 +213,7 @@ test_expect_success 'with 2 files arguments' '
 '
 
 test_expect_success 'with message that has comments' '
-	cat basic_message >>message_with_comments &&
+	cat basic_message >message_with_comments &&
 	sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
 		# comment
 
@@ -240,6 +240,36 @@ test_expect_success 'with message that has comments' '
 	test_cmp expected actual
 '
 
+test_expect_success 'with message that has an old style conflict block' '
+	cat basic_message >message_with_comments &&
+	sed -e "s/ Z\$/ /" >>message_with_comments <<-\EOF &&
+		# comment
+
+		# other comment
+		Cc: Z
+		# yet another comment
+		Reviewed-by: Johan
+		Reviewed-by: Z
+		# last comment
+
+		Conflicts:
+
+	EOF
+	cat basic_message >expected &&
+	cat >>expected <<-\EOF &&
+		# comment
+
+		Reviewed-by: Johan
+		Cc: Peff
+		# last comment
+
+		Conflicts:
+
+	EOF
+	git interpret-trailers --trim-empty --trailer "Cc: Peff" message_with_comments >actual &&
+	test_cmp expected actual
+'
+
 test_expect_success 'with commit complex message and trailer args' '
 	cat complex_message_body >expected &&
 	sed -e "s/ Z\$/ /" >>expected <<-\EOF &&
-- 
2.1.2.555.gfbecd99