[PATCH 0/4] Make other git commands use trailer layout

[PATCH 0/4] Make other git commands use trailer layout

[Jonathan Tan]

This is built off jt/trailer-with-cruft (commit 60ef86a).

This patch set makes "commit -s", "cherry-pick -x", and
"format-patch --signoff" use the new trailer definition implemented in
jt/trailer-with-cruft, with some refactoring along the way. With this
patch set, the aforementioned commands would now handle trailers like
those described in [1].

[1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>

Jonathan Tan (4):
  commit: make ignore_non_trailer take buf/len
  trailer: avoid unnecessary splitting on lines
  trailer: have function to describe trailer layout
  sequencer: use trailer's trailer layout

 builtin/commit.c         |   2 +-
 commit.c                 |  22 ++--
 commit.h                 |   2 +-
 sequencer.c              |  75 +++---------
 t/t3511-cherry-pick-x.sh |  16 ++-
 t/t4014-format-patch.sh  |  40 +++++--
 t/t7501-commit.sh        |  36 ++++++
 trailer.c                | 295 ++++++++++++++++++++++++++++-------------------
 trailer.h                |  25 ++++
 9 files changed, 313 insertions(+), 200 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH 1/4] commit: make ignore_non_trailer take buf/len

[Jonathan Tan]

Make ignore_non_trailer take a buf/len pair instead of struct strbuf.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit.c |  2 +-
 commit.c         | 22 +++++++++++-----------
 commit.h         |  2 +-
 trailer.c        |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1cba3b7..c26b939 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strbuf_stripspace(&sb, 0);
 
 	if (signoff)
-		append_signoff(&sb, ignore_non_trailer(&sb), 0);
+		append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
diff --git a/commit.c b/commit.c
index 856fd4a..f70835a 100644
--- a/commit.c
+++ b/commit.c
@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
 }
 
 /*
- * Inspect sb and determine the true "end" of the log message, in
+ * Inspect the given string 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
@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
  * 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 ignore_non_trailer(const char *buf, size_t len)
 {
 	int boc = 0;
 	int bol = 0;
 	int in_old_conflicts_block = 0;
 
-	while (bol < sb->len) {
-		char *next_line;
+	while (bol < len) {
+		const char *next_line;
 
-		if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
-			next_line = sb->buf + sb->len;
+		if (!(next_line = memchr(buf + bol, '\n', len - bol)))
+			next_line = buf + len;
 		else
 			next_line++;
 
-		if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+		if (buf[bol] == comment_line_char || 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")) {
+		} else if (starts_with(buf + bol, "Conflicts:\n")) {
 			in_old_conflicts_block = 1;
 			if (!boc)
 				boc = bol;
-		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+		} else if (in_old_conflicts_block && 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;
+		bol = next_line - buf;
 	}
-	return boc ? sb->len - boc : 0;
+	return boc ? len - boc : 0;
 }
diff --git a/commit.h b/commit.h
index afd14f3..9c12abb 100644
--- a/commit.h
+++ b/commit.h
@@ -355,7 +355,7 @@ 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);
+extern int ignore_non_trailer(const char *buf, size_t len);
 
 typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
diff --git a/trailer.c b/trailer.c
index d19a92c..6d8375b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -828,7 +828,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
 
 	for (i = 0; i < patch_start; i++)
 		strbuf_addbuf(&sb, lines[i]);
-	ignore_bytes = ignore_non_trailer(&sb);
+	ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
 	strbuf_release(&sb);
 	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
 		ignore_bytes -= lines[i]->len;
-- 
2.8.0.rc3.226.g39d4020

[PATCH 2/4] trailer: avoid unnecessary splitting on lines

[Jonathan Tan]

trailer.c currently splits lines while processing a buffer (and also
rejoins lines when needing to invoke ignore_non_trailer).

Avoid such line splitting, except when generating the strings
corresponding to trailers (for ease of use by clients - a subsequent
patch will allow other components to obtain the layout of a trailer
block in a buffer, including the trailers themselves). The main purpose
of this is to make it easy to return pointers into the original buffer
(for a subsequent patch), but this also significantly reduces the number
of memory allocations required.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 116 insertions(+), 99 deletions(-)

diff --git a/trailer.c b/trailer.c
index 6d8375b..d4d9e10 100644
--- a/trailer.c
+++ b/trailer.c
@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
 	return same_token(a, b) && same_value(a, b);
 }
 
-static inline int contains_only_spaces(const char *str)
+static inline int is_blank_line(const char *str)
 {
 	const char *s = str;
-	while (*s && isspace(*s))
+	while (*s && *s != '\n' && isspace(*s))
 		s++;
-	return !*s;
+	return !*s || *s == '\n';
 }
 
 static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
@@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
 }
 
 /*
- * Return the location of the first separator in line, or -1 if there is no
- * separator.
+ * Return the location of the first separator in the given line, or -1 if there
+ * is no separator.
+ *
+ * The interests parameter must contain the acceptable separators and may
+ * contain '\n'. If interests contains '\n', the input line is considered to
+ * end at the first newline encountered. Otherwise, the input line is
+ * considered to end at its null terminator.
  */
-static int find_separator(const char *line, const char *separators)
+static int find_separator(const char *line, const char *interests)
 {
-	int loc = strcspn(line, separators);
-	if (!line[loc])
+	int loc = strcspn(line, interests);
+	if (!line[loc] || line[loc] == '\n')
 		return -1;
 	return loc;
 }
@@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,
 	free(cl_separators);
 }
 
-static struct strbuf **read_input_file(const char *file)
+static void read_input_file(struct strbuf *sb, const char *file)
 {
-	struct strbuf **lines;
-	struct strbuf sb = STRBUF_INIT;
-
 	if (file) {
-		if (strbuf_read_file(&sb, file, 0) < 0)
+		if (strbuf_read_file(sb, file, 0) < 0)
 			die_errno(_("could not read input file '%s'"), file);
 	} else {
-		if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+		if (strbuf_read(sb, fileno(stdin), 0) < 0)
 			die_errno(_("could not read from stdin"));
 	}
+}
 
-	lines = strbuf_split(&sb, '\n');
+static const char *next_line(const char *str)
+{
+	const char *nl = strchrnul(str, '\n');
+	return nl + !!*nl;
+}
 
-	strbuf_release(&sb);
+/*
+ * Return the position of the start of the last line. If len is 0, return -1.
+ */
+static int last_line(const char *buf, size_t len)
+{
+	int i;
+	if (len == 0)
+		return -1;
+	if (len == 1)
+		return 0;
+	/*
+	 * Skip the last character (in addition to the null terminator),
+	 * because if the last character is a newline, it is considered as part
+	 * of the last line anyway.
+	 */
+	i = len - 2;
 
-	return lines;
+	for (; i >= 0; i--) {
+		if (buf[i] == '\n')
+			return i + 1;
+	}
+	return 0;
 }
 
 /*
- * Return the (0 based) index of the start of the patch or the line
- * count if there is no patch in the message.
+ * Return the position of the start of the patch or the length of str if there
+ * is no patch in the message.
  */
-static int find_patch_start(struct strbuf **lines, int count)
+static int find_patch_start(const char *str)
 {
-	int i;
+	const char *s;
 
-	/* Get the start of the patch part if any */
-	for (i = 0; i < count; i++) {
-		if (starts_with(lines[i]->buf, "---"))
-			return i;
+	for (s = str; *s; s = next_line(s)) {
+		if (starts_with(s, "---"))
+			return s - str;
 	}
 
-	return count;
+	return s - str;
 }
 
 /*
- * Return the (0 based) index of the first trailer line or count if
- * there are no trailers. Trailers are searched only in the lines from
- * index (count - 1) down to index 0.
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
  */
-static int find_trailer_start(struct strbuf **lines, int count)
+static int find_trailer_start(const char *buf, size_t len)
 {
-	int start, end_of_title, only_spaces = 1;
+	const char *s;
+	int title_end, l, only_spaces = 1;
 	int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
 	/*
 	 * Number of possible continuation lines encountered. This will be
@@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	 * are to be considered non-trailers).
 	 */
 	int possible_continuation_lines = 0;
+	int ret;
+
+	char *sep_nl = xstrfmt("%s\n", separators);
 
 	/* The first paragraph is the title and cannot be trailers */
-	for (start = 0; start < count; start++) {
-		if (lines[start]->buf[0] == comment_line_char)
+	for (s = buf; s < buf + len; s = next_line(s)) {
+		if (s[0] == comment_line_char)
 			continue;
-		if (contains_only_spaces(lines[start]->buf))
+		if (is_blank_line(s))
 			break;
 	}
-	end_of_title = start;
+	title_end = s - buf;
 
 	/*
 	 * Get the start of the trailers by looking starting from the end for a
@@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	 * trailers, or (ii) contains at least one Git-generated trailer and
 	 * consists of at least 25% trailers.
 	 */
-	for (start = count - 1; start >= end_of_title; start--) {
+	for (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {
+		const char *bol = buf + l;
 		const char **p;
 		int separator_pos;
 
-		if (lines[start]->buf[0] == comment_line_char) {
+		if (bol[0] == comment_line_char) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;
 		}
-		if (contains_only_spaces(lines[start]->buf)) {
+		if (is_blank_line(bol)) {
 			if (only_spaces)
 				continue;
 			non_trailer_lines += possible_continuation_lines;
 			if (recognized_prefix &&
 			    trailer_lines * 3 >= non_trailer_lines)
-				return start + 1;
-			if (trailer_lines && !non_trailer_lines)
-				return start + 1;
-			return count;
+				ret = next_line(bol) - buf;
+			else if (trailer_lines && !non_trailer_lines)
+				ret = next_line(bol) - buf;
+			else
+				ret = len;
+			goto cleanup;
 		}
 		only_spaces = 0;
 
 		for (p = git_generated_prefixes; *p; p++) {
-			if (starts_with(lines[start]->buf, *p)) {
+			if (starts_with(bol, *p)) {
 				trailer_lines++;
 				possible_continuation_lines = 0;
 				recognized_prefix = 1;
@@ -789,8 +820,8 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			}
 		}
 
-		separator_pos = find_separator(lines[start]->buf, separators);
-		if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
+		separator_pos = find_separator(bol, sep_nl);
+		if (separator_pos >= 1 && !isspace(bol[0])) {
 			struct list_head *pos;
 
 			trailer_lines++;
@@ -800,13 +831,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			list_for_each(pos, &conf_head) {
 				struct arg_item *item;
 				item = list_entry(pos, struct arg_item, list);
-				if (token_matches_item(lines[start]->buf, item,
+				if (token_matches_item(bol, item,
 						       separator_pos)) {
 					recognized_prefix = 1;
 					break;
 				}
 			}
-		} else if (isspace(lines[start]->buf[0]))
+		} else if (isspace(bol[0]))
 			possible_continuation_lines++;
 		else {
 			non_trailer_lines++;
@@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)
 		;
 	}
 
-	return count;
+	ret = len;
+cleanup:
+	free(sep_nl);
+	return ret;
 }
 
-/* Get the index of the end of the trailers */
-static int find_trailer_end(struct strbuf **lines, int patch_start)
+/* Return the position of the end of the trailers. */
+static int find_trailer_end(const char *buf, size_t len)
 {
-	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.buf, sb.len);
-	strbuf_release(&sb);
-	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
-		ignore_bytes -= lines[i]->len;
-
-	return i + 1;
+	return len - ignore_non_trailer(buf, len);
 }
 
-static int has_blank_line_before(struct strbuf **lines, int start)
+static int ends_with_blank_line(const char *buf, size_t len)
 {
-	for (;start >= 0; start--) {
-		if (lines[start]->buf[0] == comment_line_char)
-			continue;
-		return contains_only_spaces(lines[start]->buf);
-	}
-	return 0;
-}
-
-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
-{
-	int i;
-	for (i = start; lines[i] && i < end; i++)
-		fprintf(outfile, "%s", lines[i]->buf);
+	int ll = last_line(buf, len);
+	if (ll < 0)
+		return 0;
+	return is_blank_line(buf + ll);
 }
 
 static int process_input_file(FILE *outfile,
-			      struct strbuf **lines,
+			      const char *str,
 			      struct list_head *head)
 {
-	int count = 0;
-	int patch_start, trailer_start, trailer_end, i;
+	int patch_start, trailer_start, trailer_end;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
 	struct trailer_item *last = NULL;
+	struct strbuf *trailer, **trailer_lines, **ptr;
 
-	/* Get the line count */
-	while (lines[count])
-		count++;
-
-	patch_start = find_patch_start(lines, count);
-	trailer_end = find_trailer_end(lines, patch_start);
-	trailer_start = find_trailer_start(lines, trailer_end);
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
 
 	/* Print lines before the trailers as is */
-	print_lines(outfile, lines, 0, trailer_start);
+	fwrite(str, 1, trailer_start, outfile);
 
-	if (!has_blank_line_before(lines, trailer_start - 1))
+	if (!ends_with_blank_line(str, trailer_start))
 		fprintf(outfile, "\n");
 
 	/* Parse trailer lines */
-	for (i = trailer_start; i < trailer_end; i++) {
+	trailer_lines = strbuf_split_buf(str + trailer_start, 
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
 		int separator_pos;
-		if (lines[i]->buf[0] == comment_line_char)
+		trailer = *ptr;
+		if (trailer->buf[0] == comment_line_char)
 			continue;
-		if (last && isspace(lines[i]->buf[0])) {
+		if (last && isspace(trailer->buf[0])) {
 			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
 			strbuf_strip_suffix(&sb, "\n");
 			free(last->value);
 			last->value = strbuf_detach(&sb, NULL);
 			continue;
 		}
-		separator_pos = find_separator(lines[i]->buf, separators);
+		separator_pos = find_separator(trailer->buf, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, lines[i]->buf,
+			parse_trailer(&tok, &val, NULL, trailer->buf,
 				      separator_pos);
 			last = add_trailer_item(head,
 						strbuf_detach(&tok, NULL),
 						strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, lines[i]);
+			strbuf_addbuf(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
@@ -906,6 +922,7 @@ static int process_input_file(FILE *outfile,
 			last = NULL;
 		}
 	}
+	strbuf_list_free(trailer_lines);
 
 	return trailer_end;
 }
@@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
-	struct strbuf **lines;
+	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
 
@@ -962,13 +979,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	git_config(git_trailer_default_config, NULL);
 	git_config(git_trailer_config, NULL);
 
-	lines = read_input_file(file);
+	read_input_file(&sb, file);
 
 	if (in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, lines, &head);
+	trailer_end = process_input_file(outfile, sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
@@ -979,11 +996,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	print_lines(outfile, lines, trailer_end, INT_MAX);
+	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
-	strbuf_list_free(lines);
+	strbuf_release(&sb);
 }
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines

[Christian Couder]

On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> trailer.c currently splits lines while processing a buffer (and also
> rejoins lines when needing to invoke ignore_non_trailer).
>
> Avoid such line splitting, except when generating the strings
> corresponding to trailers (for ease of use by clients - a subsequent
> patch will allow other components to obtain the layout of a trailer
> block in a buffer, including the trailers themselves). The main purpose
> of this is to make it easy to return pointers into the original buffer
> (for a subsequent patch), but this also significantly reduces the number
> of memory allocations required.
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
>  1 file changed, 116 insertions(+), 99 deletions(-)

IMHO it is telling that this needs 17 more lines.

> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>  {
>         LIST_HEAD(head);
>         LIST_HEAD(arg_head);
> -       struct strbuf **lines;
> +       struct strbuf sb = STRBUF_INIT;

We often use "sb" as the name of strbuf variables, but I think at
least here (and maybe in other places above) we could use something a
bit more telling, like "input_buf" perhaps.

>         int trailer_end;
>         FILE *outfile = stdout;
>

Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines

[Junio C Hamano]

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

> On Sat, Oct 29, 2016 at 2:05 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> trailer.c currently splits lines while processing a buffer (and also
>> rejoins lines when needing to invoke ignore_non_trailer).
>>
>> Avoid such line splitting, except when generating the strings
>> corresponding to trailers (for ease of use by clients - a subsequent
>> patch will allow other components to obtain the layout of a trailer
>> block in a buffer, including the trailers themselves). The main purpose
>> of this is to make it easy to return pointers into the original buffer
>> (for a subsequent patch), but this also significantly reduces the number
>> of memory allocations required.
>>
>> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
>> ---
>>  trailer.c | 215 +++++++++++++++++++++++++++++++++-----------------------------
>>  1 file changed, 116 insertions(+), 99 deletions(-)
>
> IMHO it is telling that this needs 17 more lines.

Given that among which 13 are additional comments to explain what is
going on, I think this is a surprisingly good outcome, relative to
what the patch achieves (reducing a lot of split/rejoin operations
and need for allocation associated with them that are all made
unnecessary).

I agree with you that it is telling to see that this needs only 17
more lines (or 4, if you only count the true code additions) and
clearly shows that strbuf_split() helper function was a not good
match for the codepath touched by this patch.  The helper function
is very tempting in that with a simple single call it will give us
tons of strbufs in an array, each of which are _MUCH_ more flexible
than simple strings if we ever need to manipulate them by trimming,
splicing and inserting etc.  This patch shows us that if we do not
need the flexibility, doing strbuf_split() as the first thing on the
input and having to deal with an array of strbuf is an overall loss,
I would think.

Although I admit that I carefully read only up to [2/4] I am fairly
happy with this series.  It finally fixes the second of the two
issues I pointed out in the review of the original series (the other
one was fixed in the series that this topic depends on), paying down
technical debt.

>> @@ -954,7 +971,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>>  {
>>         LIST_HEAD(head);
>>         LIST_HEAD(arg_head);
>> -       struct strbuf **lines;
>> +       struct strbuf sb = STRBUF_INIT;
>
> We often use "sb" as the name of strbuf variables, but I think at
> least here (and maybe in other places above) we could use something a
> bit more telling, like "input_buf" perhaps.

That sounds sensible.

>
>>         int trailer_end;
>>         FILE *outfile = stdout;
>>

Re: [PATCH 2/4] trailer: avoid unnecessary splitting on lines

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> diff --git a/trailer.c b/trailer.c
> index 6d8375b..d4d9e10 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
>  	return same_token(a, b) && same_value(a, b);
>  }
>  
> -static inline int contains_only_spaces(const char *str)
> +static inline int is_blank_line(const char *str)
>  {
>  	const char *s = str;
> -	while (*s && isspace(*s))
> +	while (*s && *s != '\n' && isspace(*s))
>  		s++;
> -	return !*s;
> +	return !*s || *s == '\n';
>  }

This used to be fed a single line and did not have to worry about
stopping at the end of a line, but now it does have to, and does so
correctly.  OK.

And the updated function name certainly reflects the fact that this
is (now) about a line-oriented processing better, and this renaming
makes sense.

> @@ -566,13 +566,18 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
>  }
>  
>  /*
> - * Return the location of the first separator in line, or -1 if there is no
> - * separator.
> + * Return the location of the first separator in the given line, or -1 if there
> + * is no separator.
> + *
> + * The interests parameter must contain the acceptable separators and may
> + * contain '\n'. If interests contains '\n', the input line is considered to
> + * end at the first newline encountered. Otherwise, the input line is
> + * considered to end at its null terminator.
>   */

The name of that terminating byte is NUL, not NULL.  

> -static int find_separator(const char *line, const char *separators)
> +static int find_separator(const char *line, const char *interests)
>  {
> -	int loc = strcspn(line, separators);
> -	if (!line[loc])
> +	int loc = strcspn(line, interests);
> +	if (!line[loc] || line[loc] == '\n')
>  		return -1;
>  	return loc;
>  }

I am not sure interests is a better name than separators.  If the
original used "interests", I do not think it is worth renaming it to
"separators", and I doubt that the renaming of the parameter in this
patch is an improvement.


> @@ -688,51 +693,71 @@ static void process_command_line_args(struct list_head *arg_head,
> -static struct strbuf **read_input_file(const char *file)
> +static void read_input_file(struct strbuf *sb, const char *file)
>  {
> -	struct strbuf **lines;
> -	struct strbuf sb = STRBUF_INIT;
> -
>  	if (file) {
> -		if (strbuf_read_file(&sb, file, 0) < 0)
> +		if (strbuf_read_file(sb, file, 0) < 0)
>  			die_errno(_("could not read input file '%s'"), file);
>  	} else {
> -		if (strbuf_read(&sb, fileno(stdin), 0) < 0)
> +		if (strbuf_read(sb, fileno(stdin), 0) < 0)
>  			die_errno(_("could not read from stdin"));
>  	}
> +}

The original used to return an array of strbufs but we now just read
into a single strbuf.  The caller may need to do more, or perhaps
the early split into array of strbufs was mostly wasteful use of
more flexible data structure.  We'll find out when we read what
happens to the data read here.

> +static const char *next_line(const char *str)
> +{
> +	const char *nl = strchrnul(str, '\n');
> +	return nl + !!*nl;
> +}

"next_line()" gives a pointer to either the beginning of the next
line, or points at the NUL that terminates the whole buffer.  OK.

> +/*
> + * Return the position of the start of the last line. If len is 0, return -1.
> + */
> +static int last_line(const char *buf, size_t len)
> +{
> +	int i;
> +	if (len == 0)
> +		return -1;
> +	if (len == 1)
> +		return 0;
> +	/*
> +	 * Skip the last character (in addition to the null terminator),
> +	 * because if the last character is a newline, it is considered as part
> +	 * of the last line anyway.
> +	 */
> +	i = len - 2;

... and if the last character is not a newline, the line is an
incomplete last line.  OK.

> -	return lines;
> +	for (; i >= 0; i--) {
> +		if (buf[i] == '\n')
> +			return i + 1;
> +	}
> +	return 0;
>  }

OK.

>  /*
> - * Return the (0 based) index of the start of the patch or the line
> - * count if there is no patch in the message.
> + * Return the position of the start of the patch or the length of str if there
> + * is no patch in the message.
>   */
> -static int find_patch_start(struct strbuf **lines, int count)
> +static int find_patch_start(const char *str)
>  {
> -	int i;
> +	const char *s;
>  
> -	/* Get the start of the patch part if any */
> -	for (i = 0; i < count; i++) {
> -		if (starts_with(lines[i]->buf, "---"))
> -			return i;
> +	for (s = str; *s; s = next_line(s)) {
> +		if (starts_with(s, "---"))
> +			return s - str;
>  	}
>  
> -	return count;
> +	return s - str;
>  }

The original assumed that the input was first split into an array of
strbufs and found the index that begins with a line "---".  We have
a flat buffer of input, and find a line that begins with "---".  The
returned value is different (i.e. used to be line-number, now it is
the byte offset).  OK.

>  /*
> - * Return the (0 based) index of the first trailer line or count if
> - * there are no trailers. Trailers are searched only in the lines from
> - * index (count - 1) down to index 0.
> + * Return the position of the first trailer line or len if there are no
> + * trailers.
>   */

OK, the idea is the same as the above; it used to be line-based, but
now it is counted in byte-offset.

> -static int find_trailer_start(struct strbuf **lines, int count)
> +static int find_trailer_start(const char *buf, size_t len)
>  {
> -	int start, end_of_title, only_spaces = 1;
> +	const char *s;
> +	int title_end, l, only_spaces = 1;

between end-of-title and title-end, I have slight preference for the
former over the latter, but just like separators vs interests, this
rename is probably more distracting than is worth.

>  	int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
>  	/*
>  	 * Number of possible continuation lines encountered. This will be
> @@ -742,15 +767,18 @@ static int find_trailer_start(struct strbuf **lines, int count)
>  	 * are to be considered non-trailers).
>  	 */
>  	int possible_continuation_lines = 0;
> +	int ret;
> +
> +	char *sep_nl = xstrfmt("%s\n", separators);
>  
>  	/* The first paragraph is the title and cannot be trailers */
> -	for (start = 0; start < count; start++) {
> -		if (lines[start]->buf[0] == comment_line_char)
> +	for (s = buf; s < buf + len; s = next_line(s)) {
> +		if (s[0] == comment_line_char)
>  			continue;
> -		if (contains_only_spaces(lines[start]->buf))
> +		if (is_blank_line(s))
>  			break;
>  	}
> -	end_of_title = start;
> +	title_end = s - buf;

OK, we skipped to find the first blank line.  The original had an
array of strbufs to iterate over and didn't have to look for LF but
now we do, but it is just the matter of calling next_line() that is
simple enough.

> @@ -758,30 +786,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
>  	 * trailers, or (ii) contains at least one Git-generated trailer and
>  	 * consists of at least 25% trailers.
>  	 */
> -	for (start = count - 1; start >= end_of_title; start--) {
> +	for (l = last_line(buf, len); l >= title_end; l = last_line(buf, l)) {

The iteration is conceptually the same.  We used to iterate from the
last line in the input down to the first blank line that ends the
title paragraph.  We do the same but now we are byte-offset based,
so we start from last_line() of the entire buffer, and go back
one-line-at-a-time by calling last_line(buf, l).  OK.

> +		const char *bol = buf + l;
>  		const char **p;
>  		int separator_pos;
>  
> -		if (lines[start]->buf[0] == comment_line_char) {
> +		if (bol[0] == comment_line_char) {
>  			non_trailer_lines += possible_continuation_lines;
>  			possible_continuation_lines = 0;
>  			continue;
>  		}
> -		if (contains_only_spaces(lines[start]->buf)) {
> +		if (is_blank_line(bol)) {
>  			if (only_spaces)
>  				continue;
>  			non_trailer_lines += possible_continuation_lines;
>  			if (recognized_prefix &&
>  			    trailer_lines * 3 >= non_trailer_lines)
> -				return start + 1;
> -			if (trailer_lines && !non_trailer_lines)
> -				return start + 1;
> -			return count;
> +				ret = next_line(bol) - buf;

The original called the line we are looking at as "start"; the
corresponding byte-offset is "l", and "bol" is the pointer to the
beginning of the current line.  Returning next_line(bol)-buf is
equivalent to returning "start + 1" in the original.  OK.

> +			else if (trailer_lines && !non_trailer_lines)
> +				ret = next_line(bol) - buf;

Ditto.

> +			else
> +				ret = len;

Ditto.

> +			goto cleanup;

We are no longer "return"ing directly from this part of the code,
and instead saving the return value in "ret" and jumping there where
the real "return" is.  We need cleanup because we have an extra
allocation of separator + LF thing up above.

> ...

The changes to the remainder of this function shows that the
original lines[] were accessed read-only and it only used
lines[]->buf without lines[]->len, which is a strong indication that
an array of strbuf was an overkill.  

The rewrite looks to be a logical equivalent, which is good.

> @@ -817,88 +848,73 @@ static int find_trailer_start(struct strbuf **lines, int count)
> ...
> -/* Get the index of the end of the trailers */
> -static int find_trailer_end(struct strbuf **lines, int patch_start)
> +/* Return the position of the end of the trailers. */
> +static int find_trailer_end(const char *buf, size_t len)
>  {
> -	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.buf, sb.len);
> -	strbuf_release(&sb);
> -	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
> -		ignore_bytes -= lines[i]->len;
> -
> -	return i + 1;

That indeed was grossly wasteful.  Things split into lines[] are
concatenated back only to call a single helper function that reports
a byte offset, and then lines[]->len is counted separately to find
the line that the byte offset falls in.  

> +	return len - ignore_non_trailer(buf, len);

Using the byte-offset based data structure throughout the code makes
it a lot simpler.  Good.

>  }
>  
> -static int has_blank_line_before(struct strbuf **lines, int start)
> +static int ends_with_blank_line(const char *buf, size_t len)
>  {
> -	for (;start >= 0; start--) {
> -		if (lines[start]->buf[0] == comment_line_char)
> -			continue;
> -		return contains_only_spaces(lines[start]->buf);
> -	}
> -	return 0;
> -}
> -
> -static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
> -{
> -	int i;
> -	for (i = start; lines[i] && i < end; i++)
> -		fprintf(outfile, "%s", lines[i]->buf);
> +	int ll = last_line(buf, len);
> +	if (ll < 0)
> +		return 0;
> +	return is_blank_line(buf + ll);
>  }

Two helpers "has-blank-line-before" and "print-lines" are gone.
Let's see how updated code does things without them.

>  static int process_input_file(FILE *outfile,
> -			      struct strbuf **lines,
> +			      const char *str,
>  			      struct list_head *head)
>  {
> -	int count = 0;
> -	int patch_start, trailer_start, trailer_end, i;
> +	int patch_start, trailer_start, trailer_end;
>  	struct strbuf tok = STRBUF_INIT;
>  	struct strbuf val = STRBUF_INIT;
>  	struct trailer_item *last = NULL;
> +	struct strbuf *trailer, **trailer_lines, **ptr;
>  
> -	/* Get the line count */
> -	while (lines[count])
> -		count++;
> -
> -	patch_start = find_patch_start(lines, count);
> -	trailer_end = find_trailer_end(lines, patch_start);
> -	trailer_start = find_trailer_start(lines, trailer_end);
> +	patch_start = find_patch_start(str);
> +	trailer_end = find_trailer_end(str, patch_start);
> +	trailer_start = find_trailer_start(str, trailer_end);
>  
>  	/* Print lines before the trailers as is */
> -	print_lines(outfile, lines, 0, trailer_start);
> +	fwrite(str, 1, trailer_start, outfile);

Ah, of course, if you don't split into an array of strbuf then you
do not need a helper that iterates over the array and prints.

> -	if (!has_blank_line_before(lines, trailer_start - 1))
> +	if (!ends_with_blank_line(str, trailer_start))
>  		fprintf(outfile, "\n");

Then if the part before the trailer doesn't end with a blank, we
force a blank.  

This is not a new issue, but this makes me wonder what happens when
there is no trailer to emit after this.  Do we end up with an extra
blank line?

>  	/* Parse trailer lines */
> -	for (i = trailer_start; i < trailer_end; i++) {
> +	trailer_lines = strbuf_split_buf(str + trailer_start, 
> +					 trailer_end - trailer_start,
> +					 '\n',
> +					 0);

We want each line NUL terminated while going over the trailer part
line-by-line so that it is easy to do _addf(&buf, "%s").  Use of
strbuf_split_buf() to split them into an array of strbufs is close
to the way the original did it, so that is why it is done here.

OK.  If we had a helper function that would split into an array of
"const char *"s, I suspect it would have worked equally well.  In
the loop, we only look at ->buf of the strbuf instances and do not
take advantage of the fact that the length of each line is known.
But that is better left as a further clean-up to the future.

Thanks.  This step looks like a regression-free rewrite.

[PATCH 3/4] trailer: have function to describe trailer layout

[Jonathan Tan]

Create a function that, taking a string, describes the position of its
trailer block (if available) and the contents thereof, and make trailer
use it. This makes it easier for other Git components, in the future, to
interpret trailer blocks in the same way as trailer.

In a subsequent patch, another component will be made to use this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 120 +++++++++++++++++++++++++++++++++++++++++++-------------------
 trailer.h |  25 +++++++++++++
 2 files changed, 108 insertions(+), 37 deletions(-)

diff --git a/trailer.c b/trailer.c
index d4d9e10..2f5c815 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
+static int configured = 0;
+
 #define TRAILER_ARG_STRING "$ARG"
 
 static const char *git_generated_prefixes[] = {
@@ -549,6 +551,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	return 0;
 }
 
+static void ensure_configured(void)
+{
+	if (configured)
+		return;
+
+	/* Default config must be setup first */
+	git_config(git_trailer_default_config, NULL);
+	git_config(git_trailer_config, NULL);
+	configured = 1;
+}
+
 static const char *token_from_item(struct arg_item *item, char *tok)
 {
 	if (item->conf.key)
@@ -872,59 +885,43 @@ static int process_input_file(FILE *outfile,
 			      const char *str,
 			      struct list_head *head)
 {
-	int patch_start, trailer_start, trailer_end;
+	struct trailer_info info;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
-	struct trailer_item *last = NULL;
-	struct strbuf *trailer, **trailer_lines, **ptr;
+	int i;
 
-	patch_start = find_patch_start(str);
-	trailer_end = find_trailer_end(str, patch_start);
-	trailer_start = find_trailer_start(str, trailer_end);
+	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, trailer_start, outfile);
+	fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!ends_with_blank_line(str, trailer_start))
+	if (!info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
-	/* Parse trailer lines */
-	trailer_lines = strbuf_split_buf(str + trailer_start, 
-					 trailer_end - trailer_start,
-					 '\n',
-					 0);
-	for (ptr = trailer_lines; *ptr; ptr++) {
+	for (i = 0; i < info.trailer_nr; i++) {
 		int separator_pos;
-		trailer = *ptr;
-		if (trailer->buf[0] == comment_line_char)
+		char *trailer = info.trailers[i];
+		if (trailer[0] == comment_line_char)
 			continue;
-		if (last && isspace(trailer->buf[0])) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
-			strbuf_strip_suffix(&sb, "\n");
-			free(last->value);
-			last->value = strbuf_detach(&sb, NULL);
-			continue;
-		}
-		separator_pos = find_separator(trailer->buf, separators);
+		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, trailer->buf,
-				      separator_pos);
-			last = add_trailer_item(head,
-						strbuf_detach(&tok, NULL),
-						strbuf_detach(&val, NULL));
+			parse_trailer(&tok, &val, NULL, trailer,
+			              separator_pos);
+			add_trailer_item(head,
+					 strbuf_detach(&tok, NULL),
+					 strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, trailer);
+			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
-			last = NULL;
 		}
 	}
-	strbuf_list_free(trailer_lines);
 
-	return trailer_end;
+	trailer_info_release(&info);
+
+	return info.trailer_end - str;
 }
 
 static void free_all(struct list_head *head)
@@ -975,9 +972,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	int trailer_end;
 	FILE *outfile = stdout;
 
-	/* Default config must be setup first */
-	git_config(git_trailer_default_config, NULL);
-	git_config(git_trailer_config, NULL);
+	ensure_configured();
 
 	read_input_file(&sb, file);
 
@@ -1004,3 +999,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	strbuf_release(&sb);
 }
+
+void trailer_info_get(struct trailer_info *info, const char *str)
+{
+	int patch_start, trailer_end, trailer_start;
+	struct strbuf **trailer_lines, **ptr;
+	char **trailer_strings = NULL;
+	size_t nr = 0, alloc = 0;
+	char **last = NULL;
+
+	ensure_configured();
+
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
+
+	trailer_lines = strbuf_split_buf(str + trailer_start,
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
+		if (last && isspace((*ptr)->buf[0])) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+			strbuf_addbuf(&sb, *ptr);
+			*last = strbuf_detach(&sb, NULL);
+			continue;
+		}
+		ALLOC_GROW(trailer_strings, nr + 1, alloc);
+		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+		last = find_separator(trailer_strings[nr], separators) >= 1
+			? &trailer_strings[nr]
+			: NULL;
+		nr++;
+	}
+	strbuf_list_free(trailer_lines);
+
+	info->blank_line_before_trailer = ends_with_blank_line(str,
+							       trailer_start);
+	info->trailer_start = str + trailer_start;
+	info->trailer_end = str + trailer_end;
+	info->trailers = trailer_strings;
+	info->trailer_nr = nr;
+}
+
+void trailer_info_release(struct trailer_info *info)
+{
+	int i;
+	for (i = 0; i < info->trailer_nr; i++)
+		free(info->trailers[i]);
+	free(info->trailers);
+}
diff --git a/trailer.h b/trailer.h
index 36b40b8..65cc5d7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,7 +1,32 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+struct trailer_info {
+	/*
+	 * True if there is a blank line before the location pointed to by
+	 * trailer_start.
+	 */
+	int blank_line_before_trailer;
+
+	/*
+	 * Pointers to the start and end of the trailer block found. If there
+	 * is no trailer block found, these 2 pointers point to the end of the
+	 * input string.
+	 */
+	const char *trailer_start, *trailer_end;
+
+	/*
+	 * Array of trailers found.
+	 */
+	char **trailers;
+	size_t trailer_nr;
+};
+
 void process_trailers(const char *file, int in_place, int trim_empty,
 		      struct string_list *trailers);
 
+void trailer_info_get(struct trailer_info *info, const char *str);
+
+void trailer_info_release(struct trailer_info *info);
+
 #endif /* TRAILER_H */
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 3/4] trailer: have function to describe trailer layout

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>  static char *separators = ":";
>  
> +static int configured = 0;

Avoid initializing a static to 0 or NULL; instead let .bss take care
of it.

>  static const char *token_from_item(struct arg_item *item, char *tok)
>  {
>  	if (item->conf.key)
> @@ -872,59 +885,43 @@ static int process_input_file(FILE *outfile,
>  			      const char *str,
>  			      struct list_head *head)
>  {
> -	int patch_start, trailer_start, trailer_end;
> +	struct trailer_info info;
>  	struct strbuf tok = STRBUF_INIT;
>  	struct strbuf val = STRBUF_INIT;
> -	struct trailer_item *last = NULL;
> -	struct strbuf *trailer, **trailer_lines, **ptr;
> +	int i;
>  
> -	patch_start = find_patch_start(str);
> -	trailer_end = find_trailer_end(str, patch_start);
> -	trailer_start = find_trailer_start(str, trailer_end);
> +	trailer_info_get(&info, str);

OK, it needs a reading of trailer_info_get() first to understand the
remainder of this function.  The function would grab these fields
into info, among doing other things.

>  	/* Print lines before the trailers as is */
> -	fwrite(str, 1, trailer_start, outfile);
> +	fwrite(str, 1, info.trailer_start - str, outfile);
>  
> -	if (!ends_with_blank_line(str, trailer_start))
> +	if (!info.blank_line_before_trailer)
>  		fprintf(outfile, "\n");

... and one of the "other things" include setting the
->blank_line_before_trailer field.  

> -	/* Parse trailer lines */
> -	trailer_lines = strbuf_split_buf(str + trailer_start, 
> -					 trailer_end - trailer_start,
> -					 '\n',
> -					 0);
> -	for (ptr = trailer_lines; *ptr; ptr++) {
> +	for (i = 0; i < info.trailer_nr; i++) {
>  		int separator_pos;
> -		trailer = *ptr;
> -		if (trailer->buf[0] == comment_line_char)
> +		char *trailer = info.trailers[i];
> +		if (trailer[0] == comment_line_char)
>  			continue;

And info.trailers[] is no longer an array of strbuf; it is an array
of "char *", which I alluded to during my review of [2/4].  Looking
good.

> -		if (last && isspace(trailer->buf[0])) {
> -			struct strbuf sb = STRBUF_INIT;
> -			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
> -			strbuf_strip_suffix(&sb, "\n");
> -			free(last->value);
> -			last->value = strbuf_detach(&sb, NULL);
> -			continue;
> -		}
> -		separator_pos = find_separator(trailer->buf, separators);
> +		separator_pos = find_separator(trailer, separators);

... presumably, the line-folding is already handled in
trailer_info_get(), so we do not need the "last" handling.

>  		if (separator_pos >= 1) {

... and it it is a "mail-header: looking" one, then add it one way.

> -			parse_trailer(&tok, &val, NULL, trailer->buf,
> -				      separator_pos);
> -			last = add_trailer_item(head,
> -						strbuf_detach(&tok, NULL),
> -						strbuf_detach(&val, NULL));
> +			parse_trailer(&tok, &val, NULL, trailer,
> +			              separator_pos);
> +			add_trailer_item(head,
> +					 strbuf_detach(&tok, NULL),
> +					 strbuf_detach(&val, NULL));
>  		} else {
> -			strbuf_addbuf(&val, trailer);
> +			strbuf_addstr(&val, trailer);

... otherwise add it another way.

>  			strbuf_strip_suffix(&val, "\n");
>  			add_trailer_item(head,
>  					 NULL,
>  					 strbuf_detach(&val, NULL));
> -			last = NULL;
>  		}
>  	}
> -	strbuf_list_free(trailer_lines);
>  
> -	return trailer_end;
> +	trailer_info_release(&info);
> +
> +	return info.trailer_end - str;
>  }

Nicely done.

> @@ -1004,3 +999,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
>  
>  	strbuf_release(&sb);
>  }
> +
> +void trailer_info_get(struct trailer_info *info, const char *str)
> +{
> +	int patch_start, trailer_end, trailer_start;
> +	struct strbuf **trailer_lines, **ptr;
> +	char **trailer_strings = NULL;
> +	size_t nr = 0, alloc = 0;
> +	char **last = NULL;
> +
> +	ensure_configured();
> +
> +	patch_start = find_patch_start(str);
> +	trailer_end = find_trailer_end(str, patch_start);
> +	trailer_start = find_trailer_start(str, trailer_end);
> +
> +	trailer_lines = strbuf_split_buf(str + trailer_start,
> +					 trailer_end - trailer_start,
> +					 '\n',
> +					 0);
> +	for (ptr = trailer_lines; *ptr; ptr++) {
> +		if (last && isspace((*ptr)->buf[0])) {
> +			struct strbuf sb = STRBUF_INIT;
> +			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
> +			strbuf_addbuf(&sb, *ptr);
> +			*last = strbuf_detach(&sb, NULL);
> +			continue;
> +		}
> +		ALLOC_GROW(trailer_strings, nr + 1, alloc);
> +		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
> +		last = find_separator(trailer_strings[nr], separators) >= 1
> +			? &trailer_strings[nr]
> +			: NULL;
> +		nr++;
> +	}
> +	strbuf_list_free(trailer_lines);
> +
> +	info->blank_line_before_trailer = ends_with_blank_line(str,
> +							       trailer_start);
> +	info->trailer_start = str + trailer_start;
> +	info->trailer_end = str + trailer_end;
> +	info->trailers = trailer_strings;
> +	info->trailer_nr = nr;
> +}

OK, looking good.

[PATCH 4/4] sequencer: use trailer's trailer layout

[Jonathan Tan]

Make sequencer use trailer.c's trailer layout definition, as opposed to
parsing the footer by itself. This makes "commit -s", "cherry-pick -x",
and "format-patch --signoff" consistent with trailer, allowing
non-trailer lines and multiple-line trailers in trailer blocks under
certain conditions, and therefore suppressing the extra newline in those
cases.

Consistency with trailer extends to respecting trailer configs.  Tests
have been included to show that.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 sequencer.c              | 75 +++++++++---------------------------------------
 t/t3511-cherry-pick-x.sh | 16 +++++++++--
 t/t4014-format-patch.sh  | 40 +++++++++++++++++++++-----
 t/t7501-commit.sh        | 36 +++++++++++++++++++++++
 4 files changed, 96 insertions(+), 71 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index eec8a60..ec0157d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -15,6 +15,7 @@
 #include "merge-recursive.h"
 #include "refs.h"
 #include "argv-array.h"
+#include "trailer.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
 static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
 static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
 
-static int is_rfc2822_line(const char *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		int ch = buf[i];
-		if (ch == ':')
-			return 1;
-		if (!isalnum(ch) && ch != '-')
-			break;
-	}
-
-	return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
-	/*
-	 * We only care that it looks roughly like (cherry picked from ...)
-	 */
-	return len > strlen(cherry_picked_prefix) + 1 &&
-		starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
 /*
  * Returns 0 for non-conforming footer
  * Returns 1 for conforming footer
@@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
 {
-	char prev;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-	int found_sob = 0;
-
-	/* footer must end with newline */
-	if (!len || buf[len - 1] != '\n')
-		return 0;
+	struct trailer_info info;
+	int i;
+	int found_sob = 0, found_sob_last = 0;
 
-	prev = '\0';
-	for (i = len - 1; i > 0; i--) {
-		char ch = buf[i];
-		if (prev == '\n' && ch == '\n') /* paragraph break */
-			break;
-		prev = ch;
-	}
+	trailer_info_get(&info, sb->buf);
 
-	/* require at least one blank line */
-	if (prev != '\n' || buf[i] != '\n')
+	if (info.trailer_start == info.trailer_end)
 		return 0;
 
-	/* advance to start of last paragraph */
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		int found_rfc2822;
-
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
+	for (i = 0; i < info.trailer_nr; i++)
+		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
+			found_sob = 1;
+			if (i == info.trailer_nr - 1)
+				found_sob_last = 1;
+		}
 
-		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
-		if (found_rfc2822 && sob &&
-		    !strncmp(buf + i, sob->buf, sob->len))
-			found_sob = k;
+	trailer_info_release(&info);
 
-		if (!(found_rfc2822 ||
-		      is_cherry_picked_from_line(buf + i, k - i - 1)))
-			return 0;
-	}
-	if (found_sob == i)
+	if (found_sob_last)
 		return 3;
 	if (found_sob)
 		return 2;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..bf0a5c9 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
 
 mesg_broken_footer="$mesg_no_footer
 
-The signed-off-by string should begin with the words Signed-off-by followed
-by a colon and space, and then the signers name and email address. e.g.
-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+This is not recognized as a footer because Myfooter is not a recognized token.
+Myfooter: A.U. Thor <author@example.com>"
 
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -s recognizes trailer config' '
+	pristine_detach initial &&
+	git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
+	cat <<-EOF >expect &&
+		$mesg_broken_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	sha1=$(git rev-parse mesg-no-footer^0) &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..635b394 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1277,8 +1277,7 @@ EOF
 4:Subject: [PATCH] subject
 8:
 9:I want to mention about Signed-off-by: here.
-10:
-11:Signed-off-by: C O Mitter <committer@example.com>
+10:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
@@ -1294,8 +1293,7 @@ EOF
 4:Subject: [PATCH] subject
 8:
 10:Signed-off-by: example happens to be wrapped here.
-11:
-12:Signed-off-by: C O Mitter <committer@example.com>
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
@@ -1368,7 +1366,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_success 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: tolerate garbage in conforming footer' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1383,8 +1381,36 @@ EOF
 8:
 10:
 13:Signed-off-by: C O Mitter <committer@example.com>
-14:
-15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: respect trailer config' '
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual &&
+
+	test_config trailer.Myfooter.ifexists add &&
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..4003a27 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -460,6 +460,42 @@ $alt" &&
 	test_cmp expected actual
 '
 
+test_expect_success 'signoff respects trailer config' '
+
+	echo 5 >positive &&
+	git add positive &&
+	git commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual &&
+
+	echo 6 >positive &&
+	git add positive &&
+	git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> @@ -26,30 +27,6 @@ static GIT_PATH_FUNC(git_path_opts_file, SEQ_OPTS_FILE)
>  static GIT_PATH_FUNC(git_path_seq_dir, SEQ_DIR)
>  static GIT_PATH_FUNC(git_path_head_file, SEQ_HEAD_FILE)
>  
> -static int is_rfc2822_line(const char *buf, int len)
> -{
> -	int i;
> -
> -	for (i = 0; i < len; i++) {
> -		int ch = buf[i];
> -		if (ch == ':')
> -			return 1;
> -		if (!isalnum(ch) && ch != '-')
> -			break;
> -	}
> -
> -	return 0;
> -}
> -
> -static int is_cherry_picked_from_line(const char *buf, int len)
> -{
> -	/*
> -	 * We only care that it looks roughly like (cherry picked from ...)
> -	 */
> -	return len > strlen(cherry_picked_prefix) + 1 &&
> -		starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> -}

We lost two helper functions here, one to detect "Mail-header: like"
line, the other to detect "(cherry picked from") line.  Let's see
how the updated caller can do without these.  We know that both of
these can be caught if we grabbed the block of trailer block.

> @@ -59,49 +36,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
>  static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
>  	int ignore_footer)
>  {
> -	char prev;
> -	int i, k;
> -	int len = sb->len - ignore_footer;
> -	const char *buf = sb->buf;
> -	int found_sob = 0;
> -
> -	/* footer must end with newline */
> -	if (!len || buf[len - 1] != '\n')
> -		return 0;
> +	struct trailer_info info;
> +	int i;
> +	int found_sob = 0, found_sob_last = 0;
>  
> -	prev = '\0';
> -	for (i = len - 1; i > 0; i--) {
> -		char ch = buf[i];
> -		if (prev == '\n' && ch == '\n') /* paragraph break */
> -			break;
> -		prev = ch;
> -	}
> +	trailer_info_get(&info, sb->buf);
>  
> -	/* require at least one blank line */
> -	if (prev != '\n' || buf[i] != '\n')
> +	if (info.trailer_start == info.trailer_end)
>  		return 0;

So we feed the thing to trailer_info_get() which will find the
trailer block.  If there is no trailer block, start and end will
point at the same place, which is trivial.

>  
> -	/* advance to start of last paragraph */
> -	while (i < len - 1 && buf[i] == '\n')
> -		i++;
> -
> -	for (; i < len; i = k) {
> -		int found_rfc2822;
> -
> -		for (k = i; k < len && buf[k] != '\n'; k++)
> -			; /* do nothing */
> -		k++;
> +	for (i = 0; i < info.trailer_nr; i++)
> +		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
> +			found_sob = 1;
> +			if (i == info.trailer_nr - 1)
> +				found_sob_last = 1;
> +		}
>  
> -		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
> -		if (found_rfc2822 && sob &&
> -		    !strncmp(buf + i, sob->buf, sob->len))
> -			found_sob = k;

Then we scan the trailer block and see if we are looking at the same
s-o-b line as we are asked to look for, and if it is at the last
logical line in the trailer block.

> +	trailer_info_release(&info);
>  
> -		if (!(found_rfc2822 ||
> -		      is_cherry_picked_from_line(buf + i, k - i - 1)))
> -			return 0;

We used to reject a "last paragraph" that has "cruft" other than
"Mail-header: like" line or "(cherry-picked from" line.  By reusing
the trailer code, we are getting consistently looser with its logic.

> -	}
> -	if (found_sob == i)
> +	if (found_sob_last)
>  		return 3;
>  	if (found_sob)
>  		return 2;

I found it surprising that you said "commit -s", "cherry-pick -x"
and "format-patch -s" are covered by this patch and saw a change
only to sequencer.c.

It turns out append_signoff() is the central function that is shared
among builtin/commit.c::prepare_to_commit() that does "commit -s",
log-tree.c::show_log() that is called by "format-patch -s", and
sequencer.c::do_recursive_merge() that do_pick_commit() hence "git
cherry-pick -s" uses.  And that function decides where to put a new
S-o-b by calling this has_conforming_footer() function.

In addition, do_pick_commit() also calls has_conforming_footer() to
decide where to add "(cherry picked from".

Whoever did the sequencer.c should be proud to structure the code to
make this change so easy to make (I know it is not me, and you
Jonathan know it is not you).  

Nicely done by both of you.

> diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
> index 9cce5ae..bf0a5c9 100755
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
>  
>  mesg_broken_footer="$mesg_no_footer
>  
> -The signed-off-by string should begin with the words Signed-off-by followed
> -by a colon and space, and then the signers name and email address. e.g.
> -Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> +This is not recognized as a footer because Myfooter is not a recognized token.
> +Myfooter: A.U. Thor <author@example.com>"
>  
>  mesg_with_footer_sob="$mesg_with_footer
>  Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> @@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'cherry-pick -s recognizes trailer config' '
> +	pristine_detach initial &&
> +	git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
> +	cat <<-EOF >expect &&
> +		$mesg_broken_footer
> +		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
> +	EOF
> +	git log -1 --pretty=format:%B >actual &&
> +	test_cmp expect actual
> +'
> +

That "myfooter" one is normally not recognized as a valid trailer,
so the test before this one would add a blank line before adding a
new S-o-b; this one adds "myfooter" thing to the vocabulary and no
longer gets the blank before.  Good.

> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index ba4902d..635b394 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1277,8 +1277,7 @@ EOF
>  4:Subject: [PATCH] subject
>  8:
>  9:I want to mention about Signed-off-by: here.
> -10:
> -11:Signed-off-by: C O Mitter <committer@example.com>
> +10:Signed-off-by: C O Mitter <committer@example.com>
>  EOF
>  	test_cmp expected actual
>  '

The original log message is a single-liner subject line, blank, "I
want to mention..." and when asked to append S-o-b:, we would want
to see a blank before the added S-o-b, no?

This seems a bit weird.

> @@ -1294,8 +1293,7 @@ EOF
>  4:Subject: [PATCH] subject
>  8:
>  10:Signed-off-by: example happens to be wrapped here.
> -11:
> -12:Signed-off-by: C O Mitter <committer@example.com>
> +11:Signed-off-by: C O Mitter <committer@example.com>
>  EOF
>  	test_cmp expected actual
>  '

This one's original is a single-liner "subject", blank, "My
unfortunate", immediately followed by "S-o-b:".  The trailer's
loosened rule (mis)takes the two line body as a trailer block
because one of them is recognised as S-o-b, so it is understandable
that we do not see a blank before the new S-o-b.

This particular example may not look ideal, but it is exactly what
we wanted to happen while we were loosening the rule in the previous
series, so the outcome is understandable.

> @@ -1368,7 +1366,7 @@ EOF
>  	test_cmp expected actual
>  '
>  
> -test_expect_success 'signoff: detect garbage in non-conforming footer' '
> +test_expect_success 'signoff: tolerate garbage in conforming footer' '
>  	append_signoff <<\EOF >actual &&
>  subject
>  
> @@ -1383,8 +1381,36 @@ EOF
>  8:
>  10:
>  13:Signed-off-by: C O Mitter <committer@example.com>
> -14:
> -15:Signed-off-by: C O Mitter <committer@example.com>
> +EOF
> +	test_cmp expected actual
> +'

This is understandable and desirable.  The input has "Tested-by: ",
a cruft that says "Some Trash", and S-o-b and we used to reject that
three line as not-a-trailer.  We now allow that.

> +test_expect_success 'signoff: respect trailer config' '
> +	append_signoff <<\EOF >actual &&
> +subject
> +
> +Myfooter: x
> +Some Trash
> +EOF
> +	cat >expected <<\EOF &&
> +4:Subject: [PATCH] subject
> +8:
> +11:
> +12:Signed-off-by: C O Mitter <committer@example.com>
> +EOF
> +	test_cmp expected actual &&
> +
> +	test_config trailer.Myfooter.ifexists add &&
> +	append_signoff <<\EOF >actual &&
> +subject
> +
> +Myfooter: x
> +Some Trash
> +EOF
> +	cat >expected <<\EOF &&
> +4:Subject: [PATCH] subject
> +8:
> +11:Signed-off-by: C O Mitter <committer@example.com>

OK.  It is not easy to see in the test output, but the shift of the
location from 12 to 11 where a new S-o-b appears is because the last
two lines in the original is taken as a trailer block due to "Myfooter:"
being configured as a valid trailer element.

> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index d84897a..4003a27 100755
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -460,6 +460,42 @@ $alt" &&
>  	test_cmp expected actual
>  '
>  
> +test_expect_success 'signoff respects trailer config' '
> +
> +	echo 5 >positive &&
> +	git add positive &&
> +	git commit -s -m "subject
> +
> +non-trailer line
> +Myfooter: x" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
> +	(
> +		echo subject
> +		echo
> +		echo non-trailer line
> +		echo Myfooter: x
> +		echo
> +		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"

Any reason why this does not use "cat <<-EOF"?  Ahh, this mimicks
the style of existing ones.  OK.

> +	) >expected &&
> +	test_cmp expected actual &&
> +
> +	echo 6 >positive &&
> +	git add positive &&
> +	git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
> +
> +non-trailer line
> +Myfooter: x" &&
> +	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
> +	(
> +		echo subject
> +		echo
> +		echo non-trailer line
> +		echo Myfooter: x
> +		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
> +	) >expected &&
> +	test_cmp expected actual
> +'
> +
>  test_expect_success 'multiple -m' '
>  
>  	>negative &&

Looks good.

Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Jonathan Tan]

On 10/31/2016 06:11 PM, Junio C Hamano wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
>> index ba4902d..635b394 100755
>> --- a/t/t4014-format-patch.sh
>> +++ b/t/t4014-format-patch.sh
>> @@ -1277,8 +1277,7 @@ EOF
>>  4:Subject: [PATCH] subject
>>  8:
>>  9:I want to mention about Signed-off-by: here.
>> -10:
>> -11:Signed-off-by: C O Mitter <committer@example.com>
>> +10:Signed-off-by: C O Mitter <committer@example.com>
>>  EOF
>>  	test_cmp expected actual
>>  '
>
> The original log message is a single-liner subject line, blank, "I
> want to mention..." and when asked to append S-o-b:, we would want
> to see a blank before the added S-o-b, no?
>
> This seems a bit weird.

This is because the "I want to mention" block has 100% trailer lines 
(since its only line contains a colon). We could forbid spaces in 
trailer field names, but as you said [1], it might be better to allow 
them since users might include them.

The original sequencer.c interpreted this block as not a trailer block, 
because it only accepted alphanumeric characters or '-' before the colon 
(and no spaces) - hence the difference in behavior.

[1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>

Re: [PATCH 4/4] sequencer: use trailer's trailer layout

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>>>  9:I want to mention about Signed-off-by: here.
>> ...
>> This seems a bit weird.
>
> This is because the "I want to mention" block has 100% trailer lines
> (since its only line contains a colon). We could forbid spaces in
> trailer field names, but as you said [1], it might be better to allow
> them since users might include them.

That merely means that the implementation of the wish expressed in
[1] was overly loose and needs a bit of tightening, isn't it?

> The original sequencer.c interpreted this block as not a trailer
> block, because it only accepted alphanumeric characters or '-' before
> the colon (and no spaces) - hence the difference in behavior.

That sounds more sensible to me.  Would there be an easy way to
still allow misspelled "Thanks to:" but not be fooled by an obvious
nonsense like this example, without going deep into natural language
processing?  If not, we may want to tighten it back.

>
> [1] <xmqqbmyhr4vt.fsf@gitster.mtv.corp.google.com>

Re: [PATCH 0/4] Make other git commands use trailer layout

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> This is built off jt/trailer-with-cruft (commit 60ef86a).
>
> This patch set makes "commit -s", "cherry-pick -x", and
> "format-patch --signoff" use the new trailer definition implemented in
> jt/trailer-with-cruft, with some refactoring along the way. With this
> patch set, the aforementioned commands would now handle trailers like
> those described in [1].
>
> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>

Ooooh.  Looks delicious ;-)

Re: [PATCH 0/4] Make other git commands use trailer layout

[Christian Couder]

On Sat, Oct 29, 2016 at 3:12 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> This is built off jt/trailer-with-cruft (commit 60ef86a).
>>
>> This patch set makes "commit -s", "cherry-pick -x", and
>> "format-patch --signoff" use the new trailer definition implemented in
>> jt/trailer-with-cruft, with some refactoring along the way. With this
>> patch set, the aforementioned commands would now handle trailers like
>> those described in [1].
>>
>> [1] <84f28caa-2e4b-1231-1a76-3b7e765c0b61@google.com>
>
> Ooooh.  Looks delicious ;-)

Yeah, I am very happy with this goal being reached :-)

Thanks,
Christian.

[PATCH v2 0/5] Make other git commands use trailer layout

[Jonathan Tan]

Thanks for all your comments.

This patch set is now built off master (since jt/trailer-with-cruft is
merged).

I couldn't think of an easy way to clearly decide if a token with spaces
should be considered a token, so I've tightened the restrictions. One
benefit is that we no longer need to create temporary strings that
include '\n' to be passed into the find_separator method.

In 2/4 (now 3/5), I've also changed some variable names as requested
(e.g. sb -> input, and un-did some others).

Jonathan Tan (5):
  trailer: be stricter in parsing separators
  commit: make ignore_non_trailer take buf/len
  trailer: avoid unnecessary splitting on lines
  trailer: have function to describe trailer layout
  sequencer: use trailer's trailer layout

 builtin/commit.c         |   2 +-
 commit.c                 |  22 ++--
 commit.h                 |   2 +-
 sequencer.c              |  75 +++---------
 t/t3511-cherry-pick-x.sh |  16 ++-
 t/t4014-format-patch.sh  |  37 +++++-
 t/t7501-commit.sh        |  36 ++++++
 trailer.c                | 296 ++++++++++++++++++++++++++++-------------------
 trailer.h                |  25 ++++
 9 files changed, 313 insertions(+), 198 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 1/5] trailer: be stricter in parsing separators

[Jonathan Tan]

Currently, a line is interpreted to be a trailer line if it contains a
separator. Make parsing stricter by requiring the text on the left of
the separator, if not the empty string, to be of the "<token><optional
whitespace>" form.

(The find_separator function distinguishes the no-separator case from
the separator-starts-line case because some callers of this function
need such a distinction.)

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 23 +++++++++++++++++------
 1 file changed, 17 insertions(+), 6 deletions(-)

diff --git a/trailer.c b/trailer.c
index f0ecde2..0ee634f 100644
--- a/trailer.c
+++ b/trailer.c
@@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
 }
 
 /*
- * Return the location of the first separator in line, or -1 if there is no
- * separator.
+ * If the given line is of the form 
+ * "<token><optional whitespace><separator>..." or "<separator>...", return the
+ * location of the separator. Otherwise, return -1.
  */
 static int find_separator(const char *line, const char *separators)
 {
-	int loc = strcspn(line, separators);
-	if (!line[loc])
-		return -1;
-	return loc;
+	int whitespace_found = 0;
+	const char *c;
+	for (c = line; *c; c++) {
+		if (strchr(separators, *c))
+			return c - line;
+		if (!whitespace_found && (isalnum(*c) || *c == '-'))
+			continue;
+		if (c != line && (*c == ' ' || *c == '\t')) {
+			whitespace_found = 1;
+			continue;
+		}
+		break;
+	}
+	return -1;
 }
 
 /*
-- 
2.8.0.rc3.226.g39d4020

Re: [PATCH v2 1/5] trailer: be stricter in parsing separators

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

> Currently, a line is interpreted to be a trailer line if it contains a
> separator. Make parsing stricter by requiring the text on the left of
> the separator, if not the empty string, to be of the "<token><optional
> whitespace>" form.

Hmph.  The optional whitespace is to allow for what kind of line?

It is not for "Signed off by:" that is a misspelt "Signed-off-by:";
it may not hurt but I do not think of a case that would be useful
offhand.




> (The find_separator function distinguishes the no-separator case from
> the separator-starts-line case because some callers of this function
> need such a distinction.)
>
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  trailer.c | 23 +++++++++++++++++------
>  1 file changed, 17 insertions(+), 6 deletions(-)
>
> diff --git a/trailer.c b/trailer.c
> index f0ecde2..0ee634f 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -563,15 +563,26 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
>  }
>  
>  /*
> - * Return the location of the first separator in line, or -1 if there is no
> - * separator.
> + * If the given line is of the form 
> + * "<token><optional whitespace><separator>..." or "<separator>...", return the
> + * location of the separator. Otherwise, return -1.
>   */
>  static int find_separator(const char *line, const char *separators)
>  {
> -	int loc = strcspn(line, separators);
> -	if (!line[loc])
> -		return -1;
> -	return loc;
> +	int whitespace_found = 0;
> +	const char *c;
> +	for (c = line; *c; c++) {
> +		if (strchr(separators, *c))
> +			return c - line;
> +		if (!whitespace_found && (isalnum(*c) || *c == '-'))
> +			continue;
> +		if (c != line && (*c == ' ' || *c == '\t')) {
> +			whitespace_found = 1;
> +			continue;
> +		}
> +		break;
> +	}
> +	return -1;
>  }
>  
>  /*

Re: [PATCH v2 1/5] trailer: be stricter in parsing separators

[Junio C Hamano]

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

> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> Currently, a line is interpreted to be a trailer line if it contains a
>> separator. Make parsing stricter by requiring the text on the left of
>> the separator, if not the empty string, to be of the "<token><optional
>> whitespace>" form.
>
> Hmph.  The optional whitespace is to allow for what kind of line?
>
> It is not for "Signed off by:" that is a misspelt "Signed-off-by:";
> it may not hurt but I do not think of a case that would be useful
> offhand.

Other than this "Hmph" (which is not an objection---just something
that the reviewer did not understand), the rest looked good to me.

Will re-queue.

Thanks.

Re: [PATCH v2 1/5] trailer: be stricter in parsing separators

[Jonathan Tan]

On 11/01/2016 01:37 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Tan <jonathantanmy@google.com> writes:
>>
>>> Currently, a line is interpreted to be a trailer line if it contains a
>>> separator. Make parsing stricter by requiring the text on the left of
>>> the separator, if not the empty string, to be of the "<token><optional
>>> whitespace>" form.
>>
>> Hmph.  The optional whitespace is to allow for what kind of line?
>>
>> It is not for "Signed off by:" that is a misspelt "Signed-off-by:";
>> it may not hurt but I do not think of a case that would be useful
>> offhand.

This is to allow trailers of the form "Fix #42" (mentioned in the 
git-interpret-trailers documentation and also in the test).

> Other than this "Hmph" (which is not an objection---just something
> that the reviewer did not understand), the rest looked good to me.
>
> Will re-queue.
>
> Thanks.

Thanks!

Re: [PATCH v2 1/5] trailer: be stricter in parsing separators

[Junio C Hamano]

Jonathan Tan <jonathantanmy@google.com> writes:

>>> Hmph.  The optional whitespace is to allow for what kind of line?
>>>
>>> It is not for "Signed off by:" that is a misspelt "Signed-off-by:";
>>> it may not hurt but I do not think of a case that would be useful
>>> offhand.
>
> This is to allow trailers of the form "Fix #42" (mentioned in the
> git-interpret-trailers documentation and also in the test).

That piece of information belongs to the in-code comment for the
function, I would think.  The comment that describes what it does
(i.e. "token with optional spaces and separator") can be read from
the code easily, but the reason why the code implements that
behaviour cannot.

Thanks.

[PATCH v2 2/5] commit: make ignore_non_trailer take buf/len

[Jonathan Tan]

Make ignore_non_trailer take a buf/len pair instead of struct strbuf.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit.c |  2 +-
 commit.c         | 22 +++++++++++-----------
 commit.h         |  2 +-
 trailer.c        |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d..887ccc7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strbuf_stripspace(&sb, 0);
 
 	if (signoff)
-		append_signoff(&sb, ignore_non_trailer(&sb), 0);
+		append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
diff --git a/commit.c b/commit.c
index 856fd4a..2cf8515 100644
--- a/commit.c
+++ b/commit.c
@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
 }
 
 /*
- * Inspect sb and determine the true "end" of the log message, in
+ * Inspect the given string 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
@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
  * 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 ignore_non_trailer(const char *buf, size_t len)
 {
 	int boc = 0;
 	int bol = 0;
 	int in_old_conflicts_block = 0;
 
-	while (bol < sb->len) {
-		char *next_line;
+	while (bol < len) {
+		const char *next_line = memchr(buf + bol, '\n', len - bol);
 
-		if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
-			next_line = sb->buf + sb->len;
+		if (!next_line)
+			next_line = buf + len;
 		else
 			next_line++;
 
-		if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+		if (buf[bol] == comment_line_char || 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")) {
+		} else if (starts_with(buf + bol, "Conflicts:\n")) {
 			in_old_conflicts_block = 1;
 			if (!boc)
 				boc = bol;
-		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+		} else if (in_old_conflicts_block && 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;
+		bol = next_line - buf;
 	}
-	return boc ? sb->len - boc : 0;
+	return boc ? len - boc : 0;
 }
diff --git a/commit.h b/commit.h
index afd14f3..9c12abb 100644
--- a/commit.h
+++ b/commit.h
@@ -355,7 +355,7 @@ 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);
+extern int ignore_non_trailer(const char *buf, size_t len);
 
 typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
diff --git a/trailer.c b/trailer.c
index 0ee634f..04edab2 100644
--- a/trailer.c
+++ b/trailer.c
@@ -836,7 +836,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
 
 	for (i = 0; i < patch_start; i++)
 		strbuf_addbuf(&sb, lines[i]);
-	ignore_bytes = ignore_non_trailer(&sb);
+	ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
 	strbuf_release(&sb);
 	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
 		ignore_bytes -= lines[i]->len;
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 3/5] trailer: avoid unnecessary splitting on lines

[Jonathan Tan]

trailer.c currently splits lines while processing a buffer (and also
rejoins lines when needing to invoke ignore_non_trailer).

Avoid such line splitting, except when generating the strings
corresponding to trailers (for ease of use by clients - a subsequent
patch will allow other components to obtain the layout of a trailer
block in a buffer, including the trailers themselves). The main purpose
of this is to make it easy to return pointers into the original buffer
(for a subsequent patch), but this also significantly reduces the number
of memory allocations required.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 195 ++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 101 insertions(+), 94 deletions(-)

diff --git a/trailer.c b/trailer.c
index 04edab2..f5427ec 100644
--- a/trailer.c
+++ b/trailer.c
@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
 	return same_token(a, b) && same_value(a, b);
 }
 
-static inline int contains_only_spaces(const char *str)
+static inline int is_blank_line(const char *str)
 {
 	const char *s = str;
-	while (*s && isspace(*s))
+	while (*s && *s != '\n' && isspace(*s))
 		s++;
-	return !*s;
+	return !*s || *s == '\n';
 }
 
 static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
@@ -696,51 +696,71 @@ static void process_command_line_args(struct list_head *arg_head,
 	free(cl_separators);
 }
 
-static struct strbuf **read_input_file(const char *file)
+static void read_input_file(struct strbuf *sb, const char *file)
 {
-	struct strbuf **lines;
-	struct strbuf sb = STRBUF_INIT;
-
 	if (file) {
-		if (strbuf_read_file(&sb, file, 0) < 0)
+		if (strbuf_read_file(sb, file, 0) < 0)
 			die_errno(_("could not read input file '%s'"), file);
 	} else {
-		if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+		if (strbuf_read(sb, fileno(stdin), 0) < 0)
 			die_errno(_("could not read from stdin"));
 	}
+}
 
-	lines = strbuf_split(&sb, '\n');
+static const char *next_line(const char *str)
+{
+	const char *nl = strchrnul(str, '\n');
+	return nl + !!*nl;
+}
 
-	strbuf_release(&sb);
+/*
+ * Return the position of the start of the last line. If len is 0, return -1.
+ */
+static int last_line(const char *buf, size_t len)
+{
+	int i;
+	if (len == 0)
+		return -1;
+	if (len == 1)
+		return 0;
+	/*
+	 * Skip the last character (in addition to the null terminator),
+	 * because if the last character is a newline, it is considered as part
+	 * of the last line anyway.
+	 */
+	i = len - 2;
 
-	return lines;
+	for (; i >= 0; i--) {
+		if (buf[i] == '\n')
+			return i + 1;
+	}
+	return 0;
 }
 
 /*
- * Return the (0 based) index of the start of the patch or the line
- * count if there is no patch in the message.
+ * Return the position of the start of the patch or the length of str if there
+ * is no patch in the message.
  */
-static int find_patch_start(struct strbuf **lines, int count)
+static int find_patch_start(const char *str)
 {
-	int i;
+	const char *s;
 
-	/* Get the start of the patch part if any */
-	for (i = 0; i < count; i++) {
-		if (starts_with(lines[i]->buf, "---"))
-			return i;
+	for (s = str; *s; s = next_line(s)) {
+		if (starts_with(s, "---"))
+			return s - str;
 	}
 
-	return count;
+	return s - str;
 }
 
 /*
- * Return the (0 based) index of the first trailer line or count if
- * there are no trailers. Trailers are searched only in the lines from
- * index (count - 1) down to index 0.
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
  */
-static int find_trailer_start(struct strbuf **lines, int count)
+static int find_trailer_start(const char *buf, size_t len)
 {
-	int start, end_of_title, only_spaces = 1;
+	const char *s;
+	int end_of_title, l, only_spaces = 1;
 	int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
 	/*
 	 * Number of possible continuation lines encountered. This will be
@@ -750,15 +770,16 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	 * are to be considered non-trailers).
 	 */
 	int possible_continuation_lines = 0;
+	int ret;
 
 	/* The first paragraph is the title and cannot be trailers */
-	for (start = 0; start < count; start++) {
-		if (lines[start]->buf[0] == comment_line_char)
+	for (s = buf; s < buf + len; s = next_line(s)) {
+		if (s[0] == comment_line_char)
 			continue;
-		if (contains_only_spaces(lines[start]->buf))
+		if (is_blank_line(s))
 			break;
 	}
-	end_of_title = start;
+	end_of_title = s - buf;
 
 	/*
 	 * Get the start of the trailers by looking starting from the end for a
@@ -766,30 +787,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	 * trailers, or (ii) contains at least one Git-generated trailer and
 	 * consists of at least 25% trailers.
 	 */
-	for (start = count - 1; start >= end_of_title; start--) {
+	for (l = last_line(buf, len);
+	     l >= end_of_title;
+	     l = last_line(buf, l)) {
+		const char *bol = buf + l;
 		const char **p;
 		int separator_pos;
 
-		if (lines[start]->buf[0] == comment_line_char) {
+		if (bol[0] == comment_line_char) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;
 		}
-		if (contains_only_spaces(lines[start]->buf)) {
+		if (is_blank_line(bol)) {
 			if (only_spaces)
 				continue;
 			non_trailer_lines += possible_continuation_lines;
 			if (recognized_prefix &&
 			    trailer_lines * 3 >= non_trailer_lines)
-				return start + 1;
-			if (trailer_lines && !non_trailer_lines)
-				return start + 1;
-			return count;
+				return next_line(bol) - buf;
+			else if (trailer_lines && !non_trailer_lines)
+				return next_line(bol) - buf;
+			return len;
 		}
 		only_spaces = 0;
 
 		for (p = git_generated_prefixes; *p; p++) {
-			if (starts_with(lines[start]->buf, *p)) {
+			if (starts_with(bol, *p)) {
 				trailer_lines++;
 				possible_continuation_lines = 0;
 				recognized_prefix = 1;
@@ -797,8 +821,8 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			}
 		}
 
-		separator_pos = find_separator(lines[start]->buf, separators);
-		if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
+		separator_pos = find_separator(bol, separators);
+		if (separator_pos >= 1 && !isspace(bol[0])) {
 			struct list_head *pos;
 
 			trailer_lines++;
@@ -808,13 +832,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			list_for_each(pos, &conf_head) {
 				struct arg_item *item;
 				item = list_entry(pos, struct arg_item, list);
-				if (token_matches_item(lines[start]->buf, item,
+				if (token_matches_item(bol, item,
 						       separator_pos)) {
 					recognized_prefix = 1;
 					break;
 				}
 			}
-		} else if (isspace(lines[start]->buf[0]))
+		} else if (isspace(bol[0]))
 			possible_continuation_lines++;
 		else {
 			non_trailer_lines++;
@@ -825,88 +849,70 @@ static int find_trailer_start(struct strbuf **lines, int count)
 		;
 	}
 
-	return count;
-}
-
-/* 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.buf, sb.len);
-	strbuf_release(&sb);
-	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
-		ignore_bytes -= lines[i]->len;
-
-	return i + 1;
+	return len;
 }
 
-static int has_blank_line_before(struct strbuf **lines, int start)
+/* Return the position of the end of the trailers. */
+static int find_trailer_end(const char *buf, size_t len)
 {
-	for (;start >= 0; start--) {
-		if (lines[start]->buf[0] == comment_line_char)
-			continue;
-		return contains_only_spaces(lines[start]->buf);
-	}
-	return 0;
+	return len - ignore_non_trailer(buf, len);
 }
 
-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
+static int ends_with_blank_line(const char *buf, size_t len)
 {
-	int i;
-	for (i = start; lines[i] && i < end; i++)
-		fprintf(outfile, "%s", lines[i]->buf);
+	int ll = last_line(buf, len);
+	if (ll < 0)
+		return 0;
+	return is_blank_line(buf + ll);
 }
 
 static int process_input_file(FILE *outfile,
-			      struct strbuf **lines,
+			      const char *str,
 			      struct list_head *head)
 {
-	int count = 0;
-	int patch_start, trailer_start, trailer_end, i;
+	int patch_start, trailer_start, trailer_end;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
 	struct trailer_item *last = NULL;
+	struct strbuf *trailer, **trailer_lines, **ptr;
 
-	/* Get the line count */
-	while (lines[count])
-		count++;
-
-	patch_start = find_patch_start(lines, count);
-	trailer_end = find_trailer_end(lines, patch_start);
-	trailer_start = find_trailer_start(lines, trailer_end);
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
 
 	/* Print lines before the trailers as is */
-	print_lines(outfile, lines, 0, trailer_start);
+	fwrite(str, 1, trailer_start, outfile);
 
-	if (!has_blank_line_before(lines, trailer_start - 1))
+	if (!ends_with_blank_line(str, trailer_start))
 		fprintf(outfile, "\n");
 
 	/* Parse trailer lines */
-	for (i = trailer_start; i < trailer_end; i++) {
+	trailer_lines = strbuf_split_buf(str + trailer_start,
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
 		int separator_pos;
-		if (lines[i]->buf[0] == comment_line_char)
+		trailer = *ptr;
+		if (trailer->buf[0] == comment_line_char)
 			continue;
-		if (last && isspace(lines[i]->buf[0])) {
+		if (last && isspace(trailer->buf[0])) {
 			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
 			strbuf_strip_suffix(&sb, "\n");
 			free(last->value);
 			last->value = strbuf_detach(&sb, NULL);
 			continue;
 		}
-		separator_pos = find_separator(lines[i]->buf, separators);
+		separator_pos = find_separator(trailer->buf, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, lines[i]->buf,
+			parse_trailer(&tok, &val, NULL, trailer->buf,
 				      separator_pos);
 			last = add_trailer_item(head,
 						strbuf_detach(&tok, NULL),
 						strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, lines[i]);
+			strbuf_addbuf(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
@@ -914,6 +920,7 @@ static int process_input_file(FILE *outfile,
 			last = NULL;
 		}
 	}
+	strbuf_list_free(trailer_lines);
 
 	return trailer_end;
 }
@@ -962,7 +969,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
-	struct strbuf **lines;
+	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
 
@@ -970,13 +977,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	git_config(git_trailer_default_config, NULL);
 	git_config(git_trailer_config, NULL);
 
-	lines = read_input_file(file);
+	read_input_file(&sb, file);
 
 	if (in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, lines, &head);
+	trailer_end = process_input_file(outfile, sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
@@ -987,11 +994,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	print_lines(outfile, lines, trailer_end, INT_MAX);
+	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
-	strbuf_list_free(lines);
+	strbuf_release(&sb);
 }
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 4/5] trailer: have function to describe trailer layout

[Jonathan Tan]

Create a function that, taking a string, describes the position of its
trailer block (if available) and the contents thereof, and make trailer
use it. This makes it easier for other Git components, in the future, to
interpret trailer blocks in the same way as trailer.

In a subsequent patch, another component will be made to use this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++-------------------
 trailer.h |  25 +++++++++++++
 2 files changed, 107 insertions(+), 36 deletions(-)

diff --git a/trailer.c b/trailer.c
index f5427ec..7265a50 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
+static int configured;
+
 #define TRAILER_ARG_STRING "$ARG"
 
 static const char *git_generated_prefixes[] = {
@@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	return 0;
 }
 
+static void ensure_configured(void)
+{
+	if (configured)
+		return;
+
+	/* Default config must be setup first */
+	git_config(git_trailer_default_config, NULL);
+	git_config(git_trailer_config, NULL);
+	configured = 1;
+}
+
 static const char *token_from_item(struct arg_item *item, char *tok)
 {
 	if (item->conf.key)
@@ -870,59 +883,43 @@ static int process_input_file(FILE *outfile,
 			      const char *str,
 			      struct list_head *head)
 {
-	int patch_start, trailer_start, trailer_end;
+	struct trailer_info info;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
-	struct trailer_item *last = NULL;
-	struct strbuf *trailer, **trailer_lines, **ptr;
+	int i;
 
-	patch_start = find_patch_start(str);
-	trailer_end = find_trailer_end(str, patch_start);
-	trailer_start = find_trailer_start(str, trailer_end);
+	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, trailer_start, outfile);
+	fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!ends_with_blank_line(str, trailer_start))
+	if (!info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
-	/* Parse trailer lines */
-	trailer_lines = strbuf_split_buf(str + trailer_start,
-					 trailer_end - trailer_start,
-					 '\n',
-					 0);
-	for (ptr = trailer_lines; *ptr; ptr++) {
+	for (i = 0; i < info.trailer_nr; i++) {
 		int separator_pos;
-		trailer = *ptr;
-		if (trailer->buf[0] == comment_line_char)
-			continue;
-		if (last && isspace(trailer->buf[0])) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
-			strbuf_strip_suffix(&sb, "\n");
-			free(last->value);
-			last->value = strbuf_detach(&sb, NULL);
+		char *trailer = info.trailers[i];
+		if (trailer[0] == comment_line_char)
 			continue;
-		}
-		separator_pos = find_separator(trailer->buf, separators);
+		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, trailer->buf,
+			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
-			last = add_trailer_item(head,
-						strbuf_detach(&tok, NULL),
-						strbuf_detach(&val, NULL));
+			add_trailer_item(head,
+					 strbuf_detach(&tok, NULL),
+					 strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, trailer);
+			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
-			last = NULL;
 		}
 	}
-	strbuf_list_free(trailer_lines);
 
-	return trailer_end;
+	trailer_info_release(&info);
+
+	return info.trailer_end - str;
 }
 
 static void free_all(struct list_head *head)
@@ -973,9 +970,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	int trailer_end;
 	FILE *outfile = stdout;
 
-	/* Default config must be setup first */
-	git_config(git_trailer_default_config, NULL);
-	git_config(git_trailer_config, NULL);
+	ensure_configured();
 
 	read_input_file(&sb, file);
 
@@ -1002,3 +997,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	strbuf_release(&sb);
 }
+
+void trailer_info_get(struct trailer_info *info, const char *str)
+{
+	int patch_start, trailer_end, trailer_start;
+	struct strbuf **trailer_lines, **ptr;
+	char **trailer_strings = NULL;
+	size_t nr = 0, alloc = 0;
+	char **last = NULL;
+
+	ensure_configured();
+
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
+
+	trailer_lines = strbuf_split_buf(str + trailer_start,
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
+		if (last && isspace((*ptr)->buf[0])) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+			strbuf_addbuf(&sb, *ptr);
+			*last = strbuf_detach(&sb, NULL);
+			continue;
+		}
+		ALLOC_GROW(trailer_strings, nr + 1, alloc);
+		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+		last = find_separator(trailer_strings[nr], separators) >= 1
+			? &trailer_strings[nr]
+			: NULL;
+		nr++;
+	}
+	strbuf_list_free(trailer_lines);
+
+	info->blank_line_before_trailer = ends_with_blank_line(str,
+							       trailer_start);
+	info->trailer_start = str + trailer_start;
+	info->trailer_end = str + trailer_end;
+	info->trailers = trailer_strings;
+	info->trailer_nr = nr;
+}
+
+void trailer_info_release(struct trailer_info *info)
+{
+	int i;
+	for (i = 0; i < info->trailer_nr; i++)
+		free(info->trailers[i]);
+	free(info->trailers);
+}
diff --git a/trailer.h b/trailer.h
index 36b40b8..65cc5d7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,7 +1,32 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+struct trailer_info {
+	/*
+	 * True if there is a blank line before the location pointed to by
+	 * trailer_start.
+	 */
+	int blank_line_before_trailer;
+
+	/*
+	 * Pointers to the start and end of the trailer block found. If there
+	 * is no trailer block found, these 2 pointers point to the end of the
+	 * input string.
+	 */
+	const char *trailer_start, *trailer_end;
+
+	/*
+	 * Array of trailers found.
+	 */
+	char **trailers;
+	size_t trailer_nr;
+};
+
 void process_trailers(const char *file, int in_place, int trim_empty,
 		      struct string_list *trailers);
 
+void trailer_info_get(struct trailer_info *info, const char *str);
+
+void trailer_info_release(struct trailer_info *info);
+
 #endif /* TRAILER_H */
-- 
2.8.0.rc3.226.g39d4020

[PATCH v2 5/5] sequencer: use trailer's trailer layout

[Jonathan Tan]

Make sequencer use trailer.c's trailer layout definition, as opposed to
parsing the footer by itself. This makes "commit -s", "cherry-pick -x",
and "format-patch --signoff" consistent with trailer, allowing
non-trailer lines and multiple-line trailers in trailer blocks under
certain conditions, and therefore suppressing the extra newline in those
cases.

Consistency with trailer extends to respecting trailer configs.  Tests
have been included to show that.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 sequencer.c              | 75 +++++++++---------------------------------------
 t/t3511-cherry-pick-x.sh | 16 +++++++++--
 t/t4014-format-patch.sh  | 37 ++++++++++++++++++++----
 t/t7501-commit.sh        | 36 +++++++++++++++++++++++
 4 files changed, 95 insertions(+), 69 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5fd75f3..d64c973 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -16,6 +16,7 @@
 #include "refs.h"
 #include "argv-array.h"
 #include "quote.h"
+#include "trailer.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts)
 	return git_path_todo_file();
 }
 
-static int is_rfc2822_line(const char *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		int ch = buf[i];
-		if (ch == ':')
-			return 1;
-		if (!isalnum(ch) && ch != '-')
-			break;
-	}
-
-	return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
-	/*
-	 * We only care that it looks roughly like (cherry picked from ...)
-	 */
-	return len > strlen(cherry_picked_prefix) + 1 &&
-		starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
 /*
  * Returns 0 for non-conforming footer
  * Returns 1 for conforming footer
@@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
 {
-	char prev;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-	int found_sob = 0;
-
-	/* footer must end with newline */
-	if (!len || buf[len - 1] != '\n')
-		return 0;
+	struct trailer_info info;
+	int i;
+	int found_sob = 0, found_sob_last = 0;
 
-	prev = '\0';
-	for (i = len - 1; i > 0; i--) {
-		char ch = buf[i];
-		if (prev == '\n' && ch == '\n') /* paragraph break */
-			break;
-		prev = ch;
-	}
+	trailer_info_get(&info, sb->buf);
 
-	/* require at least one blank line */
-	if (prev != '\n' || buf[i] != '\n')
+	if (info.trailer_start == info.trailer_end)
 		return 0;
 
-	/* advance to start of last paragraph */
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		int found_rfc2822;
-
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
+	for (i = 0; i < info.trailer_nr; i++)
+		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
+			found_sob = 1;
+			if (i == info.trailer_nr - 1)
+				found_sob_last = 1;
+		}
 
-		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
-		if (found_rfc2822 && sob &&
-		    !strncmp(buf + i, sob->buf, sob->len))
-			found_sob = k;
+	trailer_info_release(&info);
 
-		if (!(found_rfc2822 ||
-		      is_cherry_picked_from_line(buf + i, k - i - 1)))
-			return 0;
-	}
-	if (found_sob == i)
+	if (found_sob_last)
 		return 3;
 	if (found_sob)
 		return 2;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..bf0a5c9 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
 
 mesg_broken_footer="$mesg_no_footer
 
-The signed-off-by string should begin with the words Signed-off-by followed
-by a colon and space, and then the signers name and email address. e.g.
-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+This is not recognized as a footer because Myfooter is not a recognized token.
+Myfooter: A.U. Thor <author@example.com>"
 
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -s recognizes trailer config' '
+	pristine_detach initial &&
+	git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
+	cat <<-EOF >expect &&
+		$mesg_broken_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	sha1=$(git rev-parse mesg-no-footer^0) &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..482112c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1294,8 +1294,7 @@ EOF
 4:Subject: [PATCH] subject
 8:
 10:Signed-off-by: example happens to be wrapped here.
-11:
-12:Signed-off-by: C O Mitter <committer@example.com>
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
@@ -1368,7 +1367,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_success 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: tolerate garbage in conforming footer' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1383,8 +1382,36 @@ EOF
 8:
 10:
 13:Signed-off-by: C O Mitter <committer@example.com>
-14:
-15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: respect trailer config' '
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual &&
+
+	test_config trailer.Myfooter.ifexists add &&
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..4003a27 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -460,6 +460,42 @@ $alt" &&
 	test_cmp expected actual
 '
 
+test_expect_success 'signoff respects trailer config' '
+
+	echo 5 >positive &&
+	git add positive &&
+	git commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual &&
+
+	echo 6 >positive &&
+	git add positive &&
+	git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 0/5] Make other git commands use trailer layout

[Jonathan Tan]

This is the same as v2 except that in 1/5, the comment about
find_separators has been moved from the commit message to the code
itself. Also, a trailing whitespace and unused variable fix.

Jonathan Tan (5):
  trailer: be stricter in parsing separators
  commit: make ignore_non_trailer take buf/len
  trailer: avoid unnecessary splitting on lines
  trailer: have function to describe trailer layout
  sequencer: use trailer's trailer layout

 builtin/commit.c         |   2 +-
 commit.c                 |  22 ++--
 commit.h                 |   2 +-
 sequencer.c              |  75 +++---------
 t/t3511-cherry-pick-x.sh |  16 ++-
 t/t4014-format-patch.sh  |  37 +++++-
 t/t7501-commit.sh        |  36 ++++++
 trailer.c                | 299 +++++++++++++++++++++++++++++------------------
 trailer.h                |  25 ++++
 9 files changed, 316 insertions(+), 198 deletions(-)

-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 1/5] trailer: be stricter in parsing separators

[Jonathan Tan]

Currently, a line is interpreted to be a trailer line if it contains a
separator. Make parsing stricter by requiring the text on the left of
the separator, if not the empty string, to be of the "<token><optional
whitespace>" form.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/trailer.c b/trailer.c
index f0ecde2..dc525e3 100644
--- a/trailer.c
+++ b/trailer.c
@@ -563,15 +563,30 @@ static int token_matches_item(const char *tok, struct arg_item *item, int tok_le
 }
 
 /*
- * Return the location of the first separator in line, or -1 if there is no
- * separator.
+ * If the given line is of the form
+ * "<token><optional whitespace><separator>..." or "<separator>...", return the
+ * location of the separator. Otherwise, return -1.
+ *
+ * The separator-starts-line case (in which this function returns 0) is
+ * distinguished from the non-well-formed-line case (in which this function
+ * returns -1) because some callers of this function need such a distinction.
  */
 static int find_separator(const char *line, const char *separators)
 {
-	int loc = strcspn(line, separators);
-	if (!line[loc])
-		return -1;
-	return loc;
+	int whitespace_found = 0;
+	const char *c;
+	for (c = line; *c; c++) {
+		if (strchr(separators, *c))
+			return c - line;
+		if (!whitespace_found && (isalnum(*c) || *c == '-'))
+			continue;
+		if (c != line && (*c == ' ' || *c == '\t')) {
+			whitespace_found = 1;
+			continue;
+		}
+		break;
+	}
+	return -1;
 }
 
 /*
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 2/5] commit: make ignore_non_trailer take buf/len

[Jonathan Tan]

Make ignore_non_trailer take a buf/len pair instead of struct strbuf.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 builtin/commit.c |  2 +-
 commit.c         | 22 +++++++++++-----------
 commit.h         |  2 +-
 trailer.c        |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 8976c3d..887ccc7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -790,7 +790,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 		strbuf_stripspace(&sb, 0);
 
 	if (signoff)
-		append_signoff(&sb, ignore_non_trailer(&sb), 0);
+		append_signoff(&sb, ignore_non_trailer(sb.buf, sb.len), 0);
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
 		die_errno(_("could not write commit template"));
diff --git a/commit.c b/commit.c
index 856fd4a..2cf8515 100644
--- a/commit.c
+++ b/commit.c
@@ -1649,7 +1649,7 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
 }
 
 /*
- * Inspect sb and determine the true "end" of the log message, in
+ * Inspect the given string 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
@@ -1659,37 +1659,37 @@ const char *find_commit_header(const char *msg, const char *key, size_t *out_len
  * 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 ignore_non_trailer(const char *buf, size_t len)
 {
 	int boc = 0;
 	int bol = 0;
 	int in_old_conflicts_block = 0;
 
-	while (bol < sb->len) {
-		char *next_line;
+	while (bol < len) {
+		const char *next_line = memchr(buf + bol, '\n', len - bol);
 
-		if (!(next_line = memchr(sb->buf + bol, '\n', sb->len - bol)))
-			next_line = sb->buf + sb->len;
+		if (!next_line)
+			next_line = buf + len;
 		else
 			next_line++;
 
-		if (sb->buf[bol] == comment_line_char || sb->buf[bol] == '\n') {
+		if (buf[bol] == comment_line_char || 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")) {
+		} else if (starts_with(buf + bol, "Conflicts:\n")) {
 			in_old_conflicts_block = 1;
 			if (!boc)
 				boc = bol;
-		} else if (in_old_conflicts_block && sb->buf[bol] == '\t') {
+		} else if (in_old_conflicts_block && 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;
+		bol = next_line - buf;
 	}
-	return boc ? sb->len - boc : 0;
+	return boc ? len - boc : 0;
 }
diff --git a/commit.h b/commit.h
index afd14f3..9c12abb 100644
--- a/commit.h
+++ b/commit.h
@@ -355,7 +355,7 @@ 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);
+extern int ignore_non_trailer(const char *buf, size_t len);
 
 typedef void (*each_mergetag_fn)(struct commit *commit, struct commit_extra_header *extra,
 				 void *cb_data);
diff --git a/trailer.c b/trailer.c
index dc525e3..9d7765e 100644
--- a/trailer.c
+++ b/trailer.c
@@ -840,7 +840,7 @@ static int find_trailer_end(struct strbuf **lines, int patch_start)
 
 	for (i = 0; i < patch_start; i++)
 		strbuf_addbuf(&sb, lines[i]);
-	ignore_bytes = ignore_non_trailer(&sb);
+	ignore_bytes = ignore_non_trailer(sb.buf, sb.len);
 	strbuf_release(&sb);
 	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
 		ignore_bytes -= lines[i]->len;
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 3/5] trailer: avoid unnecessary splitting on lines

[Jonathan Tan]

trailer.c currently splits lines while processing a buffer (and also
rejoins lines when needing to invoke ignore_non_trailer).

Avoid such line splitting, except when generating the strings
corresponding to trailers (for ease of use by clients - a subsequent
patch will allow other components to obtain the layout of a trailer
block in a buffer, including the trailers themselves). The main purpose
of this is to make it easy to return pointers into the original buffer
(for a subsequent patch), but this also significantly reduces the number
of memory allocations required.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 194 ++++++++++++++++++++++++++++++++------------------------------
 1 file changed, 100 insertions(+), 94 deletions(-)

diff --git a/trailer.c b/trailer.c
index 9d7765e..afbff4b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -102,12 +102,12 @@ static int same_trailer(struct trailer_item *a, struct arg_item *b)
 	return same_token(a, b) && same_value(a, b);
 }
 
-static inline int contains_only_spaces(const char *str)
+static inline int is_blank_line(const char *str)
 {
 	const char *s = str;
-	while (*s && isspace(*s))
+	while (*s && *s != '\n' && isspace(*s))
 		s++;
-	return !*s;
+	return !*s || *s == '\n';
 }
 
 static inline void strbuf_replace(struct strbuf *sb, const char *a, const char *b)
@@ -700,51 +700,71 @@ static void process_command_line_args(struct list_head *arg_head,
 	free(cl_separators);
 }
 
-static struct strbuf **read_input_file(const char *file)
+static void read_input_file(struct strbuf *sb, const char *file)
 {
-	struct strbuf **lines;
-	struct strbuf sb = STRBUF_INIT;
-
 	if (file) {
-		if (strbuf_read_file(&sb, file, 0) < 0)
+		if (strbuf_read_file(sb, file, 0) < 0)
 			die_errno(_("could not read input file '%s'"), file);
 	} else {
-		if (strbuf_read(&sb, fileno(stdin), 0) < 0)
+		if (strbuf_read(sb, fileno(stdin), 0) < 0)
 			die_errno(_("could not read from stdin"));
 	}
+}
 
-	lines = strbuf_split(&sb, '\n');
+static const char *next_line(const char *str)
+{
+	const char *nl = strchrnul(str, '\n');
+	return nl + !!*nl;
+}
 
-	strbuf_release(&sb);
+/*
+ * Return the position of the start of the last line. If len is 0, return -1.
+ */
+static int last_line(const char *buf, size_t len)
+{
+	int i;
+	if (len == 0)
+		return -1;
+	if (len == 1)
+		return 0;
+	/*
+	 * Skip the last character (in addition to the null terminator),
+	 * because if the last character is a newline, it is considered as part
+	 * of the last line anyway.
+	 */
+	i = len - 2;
 
-	return lines;
+	for (; i >= 0; i--) {
+		if (buf[i] == '\n')
+			return i + 1;
+	}
+	return 0;
 }
 
 /*
- * Return the (0 based) index of the start of the patch or the line
- * count if there is no patch in the message.
+ * Return the position of the start of the patch or the length of str if there
+ * is no patch in the message.
  */
-static int find_patch_start(struct strbuf **lines, int count)
+static int find_patch_start(const char *str)
 {
-	int i;
+	const char *s;
 
-	/* Get the start of the patch part if any */
-	for (i = 0; i < count; i++) {
-		if (starts_with(lines[i]->buf, "---"))
-			return i;
+	for (s = str; *s; s = next_line(s)) {
+		if (starts_with(s, "---"))
+			return s - str;
 	}
 
-	return count;
+	return s - str;
 }
 
 /*
- * Return the (0 based) index of the first trailer line or count if
- * there are no trailers. Trailers are searched only in the lines from
- * index (count - 1) down to index 0.
+ * Return the position of the first trailer line or len if there are no
+ * trailers.
  */
-static int find_trailer_start(struct strbuf **lines, int count)
+static int find_trailer_start(const char *buf, size_t len)
 {
-	int start, end_of_title, only_spaces = 1;
+	const char *s;
+	int end_of_title, l, only_spaces = 1;
 	int recognized_prefix = 0, trailer_lines = 0, non_trailer_lines = 0;
 	/*
 	 * Number of possible continuation lines encountered. This will be
@@ -756,13 +776,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	int possible_continuation_lines = 0;
 
 	/* The first paragraph is the title and cannot be trailers */
-	for (start = 0; start < count; start++) {
-		if (lines[start]->buf[0] == comment_line_char)
+	for (s = buf; s < buf + len; s = next_line(s)) {
+		if (s[0] == comment_line_char)
 			continue;
-		if (contains_only_spaces(lines[start]->buf))
+		if (is_blank_line(s))
 			break;
 	}
-	end_of_title = start;
+	end_of_title = s - buf;
 
 	/*
 	 * Get the start of the trailers by looking starting from the end for a
@@ -770,30 +790,33 @@ static int find_trailer_start(struct strbuf **lines, int count)
 	 * trailers, or (ii) contains at least one Git-generated trailer and
 	 * consists of at least 25% trailers.
 	 */
-	for (start = count - 1; start >= end_of_title; start--) {
+	for (l = last_line(buf, len);
+	     l >= end_of_title;
+	     l = last_line(buf, l)) {
+		const char *bol = buf + l;
 		const char **p;
 		int separator_pos;
 
-		if (lines[start]->buf[0] == comment_line_char) {
+		if (bol[0] == comment_line_char) {
 			non_trailer_lines += possible_continuation_lines;
 			possible_continuation_lines = 0;
 			continue;
 		}
-		if (contains_only_spaces(lines[start]->buf)) {
+		if (is_blank_line(bol)) {
 			if (only_spaces)
 				continue;
 			non_trailer_lines += possible_continuation_lines;
 			if (recognized_prefix &&
 			    trailer_lines * 3 >= non_trailer_lines)
-				return start + 1;
-			if (trailer_lines && !non_trailer_lines)
-				return start + 1;
-			return count;
+				return next_line(bol) - buf;
+			else if (trailer_lines && !non_trailer_lines)
+				return next_line(bol) - buf;
+			return len;
 		}
 		only_spaces = 0;
 
 		for (p = git_generated_prefixes; *p; p++) {
-			if (starts_with(lines[start]->buf, *p)) {
+			if (starts_with(bol, *p)) {
 				trailer_lines++;
 				possible_continuation_lines = 0;
 				recognized_prefix = 1;
@@ -801,8 +824,8 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			}
 		}
 
-		separator_pos = find_separator(lines[start]->buf, separators);
-		if (separator_pos >= 1 && !isspace(lines[start]->buf[0])) {
+		separator_pos = find_separator(bol, separators);
+		if (separator_pos >= 1 && !isspace(bol[0])) {
 			struct list_head *pos;
 
 			trailer_lines++;
@@ -812,13 +835,13 @@ static int find_trailer_start(struct strbuf **lines, int count)
 			list_for_each(pos, &conf_head) {
 				struct arg_item *item;
 				item = list_entry(pos, struct arg_item, list);
-				if (token_matches_item(lines[start]->buf, item,
+				if (token_matches_item(bol, item,
 						       separator_pos)) {
 					recognized_prefix = 1;
 					break;
 				}
 			}
-		} else if (isspace(lines[start]->buf[0]))
+		} else if (isspace(bol[0]))
 			possible_continuation_lines++;
 		else {
 			non_trailer_lines++;
@@ -829,88 +852,70 @@ static int find_trailer_start(struct strbuf **lines, int count)
 		;
 	}
 
-	return count;
-}
-
-/* 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.buf, sb.len);
-	strbuf_release(&sb);
-	for (i = patch_start - 1; i >= 0 && ignore_bytes > 0; i--)
-		ignore_bytes -= lines[i]->len;
-
-	return i + 1;
+	return len;
 }
 
-static int has_blank_line_before(struct strbuf **lines, int start)
+/* Return the position of the end of the trailers. */
+static int find_trailer_end(const char *buf, size_t len)
 {
-	for (;start >= 0; start--) {
-		if (lines[start]->buf[0] == comment_line_char)
-			continue;
-		return contains_only_spaces(lines[start]->buf);
-	}
-	return 0;
+	return len - ignore_non_trailer(buf, len);
 }
 
-static void print_lines(FILE *outfile, struct strbuf **lines, int start, int end)
+static int ends_with_blank_line(const char *buf, size_t len)
 {
-	int i;
-	for (i = start; lines[i] && i < end; i++)
-		fprintf(outfile, "%s", lines[i]->buf);
+	int ll = last_line(buf, len);
+	if (ll < 0)
+		return 0;
+	return is_blank_line(buf + ll);
 }
 
 static int process_input_file(FILE *outfile,
-			      struct strbuf **lines,
+			      const char *str,
 			      struct list_head *head)
 {
-	int count = 0;
-	int patch_start, trailer_start, trailer_end, i;
+	int patch_start, trailer_start, trailer_end;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
 	struct trailer_item *last = NULL;
+	struct strbuf *trailer, **trailer_lines, **ptr;
 
-	/* Get the line count */
-	while (lines[count])
-		count++;
-
-	patch_start = find_patch_start(lines, count);
-	trailer_end = find_trailer_end(lines, patch_start);
-	trailer_start = find_trailer_start(lines, trailer_end);
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
 
 	/* Print lines before the trailers as is */
-	print_lines(outfile, lines, 0, trailer_start);
+	fwrite(str, 1, trailer_start, outfile);
 
-	if (!has_blank_line_before(lines, trailer_start - 1))
+	if (!ends_with_blank_line(str, trailer_start))
 		fprintf(outfile, "\n");
 
 	/* Parse trailer lines */
-	for (i = trailer_start; i < trailer_end; i++) {
+	trailer_lines = strbuf_split_buf(str + trailer_start,
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
 		int separator_pos;
-		if (lines[i]->buf[0] == comment_line_char)
+		trailer = *ptr;
+		if (trailer->buf[0] == comment_line_char)
 			continue;
-		if (last && isspace(lines[i]->buf[0])) {
+		if (last && isspace(trailer->buf[0])) {
 			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, lines[i]->buf);
+			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
 			strbuf_strip_suffix(&sb, "\n");
 			free(last->value);
 			last->value = strbuf_detach(&sb, NULL);
 			continue;
 		}
-		separator_pos = find_separator(lines[i]->buf, separators);
+		separator_pos = find_separator(trailer->buf, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, lines[i]->buf,
+			parse_trailer(&tok, &val, NULL, trailer->buf,
 				      separator_pos);
 			last = add_trailer_item(head,
 						strbuf_detach(&tok, NULL),
 						strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, lines[i]);
+			strbuf_addbuf(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
@@ -918,6 +923,7 @@ static int process_input_file(FILE *outfile,
 			last = NULL;
 		}
 	}
+	strbuf_list_free(trailer_lines);
 
 	return trailer_end;
 }
@@ -966,7 +972,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 {
 	LIST_HEAD(head);
 	LIST_HEAD(arg_head);
-	struct strbuf **lines;
+	struct strbuf sb = STRBUF_INIT;
 	int trailer_end;
 	FILE *outfile = stdout;
 
@@ -974,13 +980,13 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	git_config(git_trailer_default_config, NULL);
 	git_config(git_trailer_config, NULL);
 
-	lines = read_input_file(file);
+	read_input_file(&sb, file);
 
 	if (in_place)
 		outfile = create_in_place_tempfile(file);
 
 	/* Print the lines before the trailers */
-	trailer_end = process_input_file(outfile, lines, &head);
+	trailer_end = process_input_file(outfile, sb.buf, &head);
 
 	process_command_line_args(&arg_head, trailers);
 
@@ -991,11 +997,11 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	free_all(&head);
 
 	/* Print the lines after the trailers as is */
-	print_lines(outfile, lines, trailer_end, INT_MAX);
+	fwrite(sb.buf + trailer_end, 1, sb.len - trailer_end, outfile);
 
 	if (in_place)
 		if (rename_tempfile(&trailers_tempfile, file))
 			die_errno(_("could not rename temporary file to %s"), file);
 
-	strbuf_list_free(lines);
+	strbuf_release(&sb);
 }
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 4/5] trailer: have function to describe trailer layout

[Jonathan Tan]

Create a function that, taking a string, describes the position of its
trailer block (if available) and the contents thereof, and make trailer
use it. This makes it easier for other Git components, in the future, to
interpret trailer blocks in the same way as trailer.

In a subsequent patch, another component will be made to use this.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 trailer.c | 118 +++++++++++++++++++++++++++++++++++++++++++-------------------
 trailer.h |  25 +++++++++++++
 2 files changed, 107 insertions(+), 36 deletions(-)

diff --git a/trailer.c b/trailer.c
index afbff4b..bc6893b 100644
--- a/trailer.c
+++ b/trailer.c
@@ -46,6 +46,8 @@ static LIST_HEAD(conf_head);
 
 static char *separators = ":";
 
+static int configured;
+
 #define TRAILER_ARG_STRING "$ARG"
 
 static const char *git_generated_prefixes[] = {
@@ -546,6 +548,17 @@ static int git_trailer_config(const char *conf_key, const char *value, void *cb)
 	return 0;
 }
 
+static void ensure_configured(void)
+{
+	if (configured)
+		return;
+
+	/* Default config must be setup first */
+	git_config(git_trailer_default_config, NULL);
+	git_config(git_trailer_config, NULL);
+	configured = 1;
+}
+
 static const char *token_from_item(struct arg_item *item, char *tok)
 {
 	if (item->conf.key)
@@ -873,59 +886,43 @@ static int process_input_file(FILE *outfile,
 			      const char *str,
 			      struct list_head *head)
 {
-	int patch_start, trailer_start, trailer_end;
+	struct trailer_info info;
 	struct strbuf tok = STRBUF_INIT;
 	struct strbuf val = STRBUF_INIT;
-	struct trailer_item *last = NULL;
-	struct strbuf *trailer, **trailer_lines, **ptr;
+	int i;
 
-	patch_start = find_patch_start(str);
-	trailer_end = find_trailer_end(str, patch_start);
-	trailer_start = find_trailer_start(str, trailer_end);
+	trailer_info_get(&info, str);
 
 	/* Print lines before the trailers as is */
-	fwrite(str, 1, trailer_start, outfile);
+	fwrite(str, 1, info.trailer_start - str, outfile);
 
-	if (!ends_with_blank_line(str, trailer_start))
+	if (!info.blank_line_before_trailer)
 		fprintf(outfile, "\n");
 
-	/* Parse trailer lines */
-	trailer_lines = strbuf_split_buf(str + trailer_start,
-					 trailer_end - trailer_start,
-					 '\n',
-					 0);
-	for (ptr = trailer_lines; *ptr; ptr++) {
+	for (i = 0; i < info.trailer_nr; i++) {
 		int separator_pos;
-		trailer = *ptr;
-		if (trailer->buf[0] == comment_line_char)
-			continue;
-		if (last && isspace(trailer->buf[0])) {
-			struct strbuf sb = STRBUF_INIT;
-			strbuf_addf(&sb, "%s\n%s", last->value, trailer->buf);
-			strbuf_strip_suffix(&sb, "\n");
-			free(last->value);
-			last->value = strbuf_detach(&sb, NULL);
+		char *trailer = info.trailers[i];
+		if (trailer[0] == comment_line_char)
 			continue;
-		}
-		separator_pos = find_separator(trailer->buf, separators);
+		separator_pos = find_separator(trailer, separators);
 		if (separator_pos >= 1) {
-			parse_trailer(&tok, &val, NULL, trailer->buf,
+			parse_trailer(&tok, &val, NULL, trailer,
 				      separator_pos);
-			last = add_trailer_item(head,
-						strbuf_detach(&tok, NULL),
-						strbuf_detach(&val, NULL));
+			add_trailer_item(head,
+					 strbuf_detach(&tok, NULL),
+					 strbuf_detach(&val, NULL));
 		} else {
-			strbuf_addbuf(&val, trailer);
+			strbuf_addstr(&val, trailer);
 			strbuf_strip_suffix(&val, "\n");
 			add_trailer_item(head,
 					 NULL,
 					 strbuf_detach(&val, NULL));
-			last = NULL;
 		}
 	}
-	strbuf_list_free(trailer_lines);
 
-	return trailer_end;
+	trailer_info_release(&info);
+
+	return info.trailer_end - str;
 }
 
 static void free_all(struct list_head *head)
@@ -976,9 +973,7 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 	int trailer_end;
 	FILE *outfile = stdout;
 
-	/* Default config must be setup first */
-	git_config(git_trailer_default_config, NULL);
-	git_config(git_trailer_config, NULL);
+	ensure_configured();
 
 	read_input_file(&sb, file);
 
@@ -1005,3 +1000,54 @@ void process_trailers(const char *file, int in_place, int trim_empty, struct str
 
 	strbuf_release(&sb);
 }
+
+void trailer_info_get(struct trailer_info *info, const char *str)
+{
+	int patch_start, trailer_end, trailer_start;
+	struct strbuf **trailer_lines, **ptr;
+	char **trailer_strings = NULL;
+	size_t nr = 0, alloc = 0;
+	char **last = NULL;
+
+	ensure_configured();
+
+	patch_start = find_patch_start(str);
+	trailer_end = find_trailer_end(str, patch_start);
+	trailer_start = find_trailer_start(str, trailer_end);
+
+	trailer_lines = strbuf_split_buf(str + trailer_start,
+					 trailer_end - trailer_start,
+					 '\n',
+					 0);
+	for (ptr = trailer_lines; *ptr; ptr++) {
+		if (last && isspace((*ptr)->buf[0])) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
+			strbuf_addbuf(&sb, *ptr);
+			*last = strbuf_detach(&sb, NULL);
+			continue;
+		}
+		ALLOC_GROW(trailer_strings, nr + 1, alloc);
+		trailer_strings[nr] = strbuf_detach(*ptr, NULL);
+		last = find_separator(trailer_strings[nr], separators) >= 1
+			? &trailer_strings[nr]
+			: NULL;
+		nr++;
+	}
+	strbuf_list_free(trailer_lines);
+
+	info->blank_line_before_trailer = ends_with_blank_line(str,
+							       trailer_start);
+	info->trailer_start = str + trailer_start;
+	info->trailer_end = str + trailer_end;
+	info->trailers = trailer_strings;
+	info->trailer_nr = nr;
+}
+
+void trailer_info_release(struct trailer_info *info)
+{
+	int i;
+	for (i = 0; i < info->trailer_nr; i++)
+		free(info->trailers[i]);
+	free(info->trailers);
+}
diff --git a/trailer.h b/trailer.h
index 36b40b8..65cc5d7 100644
--- a/trailer.h
+++ b/trailer.h
@@ -1,7 +1,32 @@
 #ifndef TRAILER_H
 #define TRAILER_H
 
+struct trailer_info {
+	/*
+	 * True if there is a blank line before the location pointed to by
+	 * trailer_start.
+	 */
+	int blank_line_before_trailer;
+
+	/*
+	 * Pointers to the start and end of the trailer block found. If there
+	 * is no trailer block found, these 2 pointers point to the end of the
+	 * input string.
+	 */
+	const char *trailer_start, *trailer_end;
+
+	/*
+	 * Array of trailers found.
+	 */
+	char **trailers;
+	size_t trailer_nr;
+};
+
 void process_trailers(const char *file, int in_place, int trim_empty,
 		      struct string_list *trailers);
 
+void trailer_info_get(struct trailer_info *info, const char *str);
+
+void trailer_info_release(struct trailer_info *info);
+
 #endif /* TRAILER_H */
-- 
2.8.0.rc3.226.g39d4020

[PATCH v3 5/5] sequencer: use trailer's trailer layout

[Jonathan Tan]

Make sequencer use trailer.c's trailer layout definition, as opposed to
parsing the footer by itself. This makes "commit -s", "cherry-pick -x",
and "format-patch --signoff" consistent with trailer, allowing
non-trailer lines and multiple-line trailers in trailer blocks under
certain conditions, and therefore suppressing the extra newline in those
cases.

Consistency with trailer extends to respecting trailer configs.  Tests
have been included to show that.

Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
 sequencer.c              | 75 +++++++++---------------------------------------
 t/t3511-cherry-pick-x.sh | 16 +++++++++--
 t/t4014-format-patch.sh  | 37 ++++++++++++++++++++----
 t/t7501-commit.sh        | 36 +++++++++++++++++++++++
 4 files changed, 95 insertions(+), 69 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 5fd75f3..d64c973 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -16,6 +16,7 @@
 #include "refs.h"
 #include "argv-array.h"
 #include "quote.h"
+#include "trailer.h"
 
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
@@ -56,30 +57,6 @@ static const char *get_todo_path(const struct replay_opts *opts)
 	return git_path_todo_file();
 }
 
-static int is_rfc2822_line(const char *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		int ch = buf[i];
-		if (ch == ':')
-			return 1;
-		if (!isalnum(ch) && ch != '-')
-			break;
-	}
-
-	return 0;
-}
-
-static int is_cherry_picked_from_line(const char *buf, int len)
-{
-	/*
-	 * We only care that it looks roughly like (cherry picked from ...)
-	 */
-	return len > strlen(cherry_picked_prefix) + 1 &&
-		starts_with(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
 /*
  * Returns 0 for non-conforming footer
  * Returns 1 for conforming footer
@@ -89,49 +66,25 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
 {
-	char prev;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-	int found_sob = 0;
-
-	/* footer must end with newline */
-	if (!len || buf[len - 1] != '\n')
-		return 0;
+	struct trailer_info info;
+	int i;
+	int found_sob = 0, found_sob_last = 0;
 
-	prev = '\0';
-	for (i = len - 1; i > 0; i--) {
-		char ch = buf[i];
-		if (prev == '\n' && ch == '\n') /* paragraph break */
-			break;
-		prev = ch;
-	}
+	trailer_info_get(&info, sb->buf);
 
-	/* require at least one blank line */
-	if (prev != '\n' || buf[i] != '\n')
+	if (info.trailer_start == info.trailer_end)
 		return 0;
 
-	/* advance to start of last paragraph */
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		int found_rfc2822;
-
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
+	for (i = 0; i < info.trailer_nr; i++)
+		if (sob && !strncmp(info.trailers[i], sob->buf, sob->len)) {
+			found_sob = 1;
+			if (i == info.trailer_nr - 1)
+				found_sob_last = 1;
+		}
 
-		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
-		if (found_rfc2822 && sob &&
-		    !strncmp(buf + i, sob->buf, sob->len))
-			found_sob = k;
+	trailer_info_release(&info);
 
-		if (!(found_rfc2822 ||
-		      is_cherry_picked_from_line(buf + i, k - i - 1)))
-			return 0;
-	}
-	if (found_sob == i)
+	if (found_sob_last)
 		return 3;
 	if (found_sob)
 		return 2;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9cce5ae..bf0a5c9 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -25,9 +25,8 @@ Signed-off-by: B.U. Thor <buthor@example.com>"
 
 mesg_broken_footer="$mesg_no_footer
 
-The signed-off-by string should begin with the words Signed-off-by followed
-by a colon and space, and then the signers name and email address. e.g.
-Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+This is not recognized as a footer because Myfooter is not a recognized token.
+Myfooter: A.U. Thor <author@example.com>"
 
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
@@ -112,6 +111,17 @@ test_expect_success 'cherry-pick -s inserts blank line after non-conforming foot
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -s recognizes trailer config' '
+	pristine_detach initial &&
+	git -c "trailer.Myfooter.ifexists=add" cherry-pick -s mesg-broken-footer &&
+	cat <<-EOF >expect &&
+		$mesg_broken_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	sha1=$(git rev-parse mesg-no-footer^0) &&
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index ba4902d..482112c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1294,8 +1294,7 @@ EOF
 4:Subject: [PATCH] subject
 8:
 10:Signed-off-by: example happens to be wrapped here.
-11:
-12:Signed-off-by: C O Mitter <committer@example.com>
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
@@ -1368,7 +1367,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_success 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: tolerate garbage in conforming footer' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1383,8 +1382,36 @@ EOF
 8:
 10:
 13:Signed-off-by: C O Mitter <committer@example.com>
-14:
-15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: respect trailer config' '
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual &&
+
+	test_config trailer.Myfooter.ifexists add &&
+	append_signoff <<\EOF >actual &&
+subject
+
+Myfooter: x
+Some Trash
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+11:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '
diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index d84897a..4003a27 100755
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -460,6 +460,42 @@ $alt" &&
 	test_cmp expected actual
 '
 
+test_expect_success 'signoff respects trailer config' '
+
+	echo 5 >positive &&
+	git add positive &&
+	git commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual &&
+
+	echo 6 >positive &&
+	git add positive &&
+	git -c "trailer.Myfooter.ifexists=add" commit -s -m "subject
+
+non-trailer line
+Myfooter: x" &&
+	git cat-file commit HEAD | sed -e "1,/^\$/d" > actual &&
+	(
+		echo subject
+		echo
+		echo non-trailer line
+		echo Myfooter: x
+		echo "Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+	) >expected &&
+	test_cmp expected actual
+'
+
 test_expect_success 'multiple -m' '
 
 	>negative &&
-- 
2.8.0.rc3.226.g39d4020