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

[PATCH v2 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)

Changes since the previous version are:

* use "%.*s" instead of a strbuf, in patch 2/5

* add new find_trailer_end() function and a call to strbuf_release(),
  in patch 4/5

Thanks to Junio and Peff.

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                     | 46 ++++++++++++++++++++++++++++++++-----------
 5 files changed, 117 insertions(+), 58 deletions(-)

-- 
2.1.2.555.gfbecd99

[PATCH v2 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 v2 2/5] trailer: display a trailer without its trailing newline

[Christian Couder]

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

Helped-by: Junio C Hamano <gitster@pobox.com>
Helped-by: Jeff King <peff@peff.net>
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..219a5a2 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) {
+		int l = strlen(trailer);
+		while (l > 0 && isspace(trailer[l - 1]))
+			l--;
+		return error(_("empty trailer token in trailer '%.*s'"), l, trailer);
+	}
 	if (len < strlen(trailer)) {
 		strbuf_add(tok, trailer, len);
 		strbuf_trim(tok);
-- 
2.1.2.555.gfbecd99

[PATCH v2 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 v2 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.

Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7513-interpret-trailers.sh |  2 ++
 trailer.c                     | 32 +++++++++++++++++++++++++-------
 2 files changed, 27 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 219a5a2..30636d2 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>
@@ -769,6 +770,22 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	return only_spaces ? count : 0;
 }
 
+/* Get the index of the end of the trailers */
+static int find_trailer_end(struct strbuf **lines, int patch_start)
+{
+	struct strbuf sb = STRBUF_INIT;
+	int i, ignore_bytes;
+
+	for (i = 0; i < patch_start; i++)
+		strbuf_addbuf(&sb, lines[i]);
+	ignore_bytes = ignore_non_trailer(&sb);
+	strbuf_release(&sb);
+	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
+		ignore_bytes -= lines[i]->len;
+
+	return i + 1;
+}
+
 static int has_blank_line_before(struct strbuf **lines, int start)
 {
 	for (;start >= 0; start--) {
@@ -791,14 +808,15 @@ static int process_input_file(struct strbuf **lines,
 			      struct trailer_item **in_tok_last)
 {
 	int count = 0;
-	int patch_start, trailer_start, i;
+	int patch_start, trailer_start, trailer_end, i;
 
 	/* Get the line count */
 	while (lines[count])
 		count++;
 
 	patch_start = find_patch_start(lines, count);
-	trailer_start = find_trailer_start(lines, patch_start);
+	trailer_end = find_trailer_end(lines, patch_start);
+	trailer_start = find_trailer_start(lines, trailer_end);
 
 	/* Print lines before the trailers as is */
 	print_lines(lines, 0, trailer_start);
@@ -807,14 +825,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 +849,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 +858,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 +869,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 v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> Make sure we look for trailers before any conflict line
> by reusing the ignore_non_trailer() function.
>
> Helped-by: Junio C Hamano <gitster@pobox.com>
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---

It makes sense to unify the logic to decide where the trailer block
is, I would think.  I however don't think I helped this change in
any way, not more than "maintained the codebase as a solid
foundation to build new features on", but at that point it would
apply to any other change and not worth mentioning ;-).

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

[Christian Couder]

From: Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines
Date: Mon, 10 Nov 2014 09:49:34 -0800

> Christian Couder <chriscool@tuxfamily.org> writes:
> 
>> Make sure we look for trailers before any conflict line
>> by reusing the ignore_non_trailer() function.
>>
>> Helped-by: Junio C Hamano <gitster@pobox.com>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
> 
> It makes sense to unify the logic to decide where the trailer block
> is, I would think.  I however don't think I helped this change in
> any way, not more than "maintained the codebase as a solid
> foundation to build new features on", but at that point it would
> apply to any other change and not worth mentioning ;-).

Do you want me to resend the series with only the Helped-by removed?

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

[Junio C Hamano]

Christian Couder <chriscool@tuxfamily.org> writes:

> From: Junio C Hamano <gitster@pobox.com>
> Subject: Re: [PATCH v2 4/5] trailer: reuse ignore_non_trailer() to ignore conflict lines
> Date: Mon, 10 Nov 2014 09:49:34 -0800
>
>> Christian Couder <chriscool@tuxfamily.org> writes:
>> 
>>> Make sure we look for trailers before any conflict line
>>> by reusing the ignore_non_trailer() function.
>>>
>>> Helped-by: Junio C Hamano <gitster@pobox.com>
>>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>>> ---
>> 
>> It makes sense to unify the logic to decide where the trailer block
>> is, I would think.  I however don't think I helped this change in
>> any way, not more than "maintained the codebase as a solid
>> foundation to build new features on", but at that point it would
>> apply to any other change and not worth mentioning ;-).
>
> Do you want me to resend the series with only the Helped-by removed?

Nah, I've already removed it.  You have better things to spend your
time on ;-)

[PATCH v2 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