[PATCH v4 00/12] unify appending of sob

[Brandon Casey <drafnel@gmail.com>]

Round 4.

Interdiff against round 3 follows the diff stat.

-Brandon

Brandon Casey (9):
  commit, cherry-pick -s: remove broken support for multiline rfc2822
    fields
  t/test-lib-functions.sh: allow to specify the tag name to test_commit
  t/t3511: add some tests of 'cherry-pick -s' functionality
  sequencer.c: recognize "(cherry picked from ..." as part of s-o-b
    footer
  sequencer.c: require a conforming footer to be preceded by a blank
    line
  sequencer.c: always separate "(cherry picked from" from commit body
  sequencer.c: teach append_signoff how to detect duplicate s-o-b
  sequencer.c: teach append_signoff to avoid adding a duplicate newline
  Unify appending signoff in format-patch, commit and sequencer

Jonathan Nieder (1):
  sequencer.c: rework search for start of footer to improve clarity

Nguyễn Thái Ngọc Duy (2):
  t4014: more tests about appending s-o-b lines
  format-patch: update append_signoff prototype

 builtin/commit.c         |   2 +-
 builtin/log.c            |  13 +--
 log-tree.c               |  92 ++---------------
 revision.h               |   2 +-
 sequencer.c              | 168 +++++++++++++++++++++---------
 sequencer.h              |   4 +-
 t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++
 t/t4014-format-patch.sh  | 262 +++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh  |   8 +-
 9 files changed, 614 insertions(+), 156 deletions(-)
 create mode 100755 t/t3511-cherry-pick-x.sh

-- 
1.8.1.3.579.gd9af3b6

diff --git a/sequencer.c b/sequencer.c
index 404b786..3c63e3a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,13 +27,12 @@ static int is_rfc2822_line(const char *buf, int len)
 	for (i = 0; i < len; i++) {
 		int ch = buf[i];
 		if (ch == ':')
+			return 1;
+		if (!isalnum(ch) && ch != '-')
 			break;
-		if (isalnum(ch) || (ch == '-'))
-			continue;
-		return 0;
 	}
 
-	return 1;
+	return 0;
 }
 
 static int is_cherry_picked_from_line(const char *buf, int len)
@@ -41,9 +40,8 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 	/*
 	 * We only care that it looks roughly like (cherry picked from ...)
 	 */
-	return !prefixcmp(buf, cherry_picked_prefix) &&
-		(buf[len - 1] == ')' ||
-		 (buf[len - 1] == '\n' && buf[len - 2] == ')'));
+	return len > strlen(cherry_picked_prefix) + 1 &&
+		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
 }
 
 /*
@@ -55,25 +53,29 @@ 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)
 {
-	int last_char_was_nl, this_char_is_nl;
+	char prev;
 	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 	int found_sob = 0;
 
-	/* find start of last paragraph */
-	last_char_was_nl = 0;
+	/* footer must end with newline */
+	if (!len || buf[len - 1] != '\n')
+		return 0;
+
+	prev = '\0';
 	for (i = len - 1; i > 0; i--) {
-		this_char_is_nl = (buf[i] == '\n');
-		if (last_char_was_nl && this_char_is_nl)
+		char ch = buf[i];
+		if (prev == '\n' && ch == '\n') /* paragraph break */
 			break;
-		last_char_was_nl = this_char_is_nl;
+		prev = ch;
 	}
 
 	/* require at least one blank line */
-	if (!last_char_was_nl || buf[i] != '\n')
+	if (prev != '\n' || buf[i] != '\n')
 		return 0;
 
+	/* advance to start of last paragraph */
 	while (i < len - 1 && buf[i] == '\n')
 		i++;
 
@@ -84,13 +86,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 			; /* do nothing */
 		k++;
 
-		found_rfc2822 = is_rfc2822_line(buf + i, k - i);
+		found_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);
 		if (found_rfc2822 && sob &&
-			!strncmp(buf + i, sob->buf, sob->len))
+		    !strncmp(buf + i, sob->buf, sob->len))
 			found_sob = k;
 
 		if (!(found_rfc2822 ||
-			is_cherry_picked_from_line(buf + i, k - i)))
+		      is_cherry_picked_from_line(buf + i, k - i - 1)))
 			return 0;
 	}
 	if (found_sob == i)
@@ -1108,26 +1110,36 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
 	struct strbuf sob = STRBUF_INIT;
-	int has_footer = 0;
-	int i;
+	const char *append_newlines = NULL;
+	int has_footer;
 
 	strbuf_addstr(&sob, sign_off_header);
 	strbuf_addstr(&sob, fmt_name(getenv("GIT_COMMITTER_NAME"),
 				getenv("GIT_COMMITTER_EMAIL")));
 	strbuf_addch(&sob, '\n');
-	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
-		; /* do nothing */
 
-	if (msgbuf->buf[i] != '\n') {
-		if (i)
-			has_footer = has_conforming_footer(msgbuf, &sob,
-					ignore_footer);
-
-		if (!has_footer)
-			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-					"\n", 1);
+	/*
+	 * If the whole message buffer is equal to the sob, pretend that we
+	 * found a conforming footer with a matching sob
+	 */
+	if (msgbuf->len - ignore_footer == sob.len &&
+	    !strncmp(msgbuf->buf, sob.buf, sob.len))
+		has_footer = 3;
+	else
+		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
+
+	if (!has_footer) {
+		size_t len = msgbuf->len - ignore_footer;
+		if (len && msgbuf->buf[len - 1] != '\n')
+			append_newlines = "\n\n";
+		else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+			append_newlines = "\n";
 	}
 
+	if (append_newlines)
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+			append_newlines, strlen(append_newlines));
+
 	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
 				sob.buf, sob.len);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index d0ec097..97fde9e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1023,11 +1023,10 @@ test_expect_success 'cover letter using branch description (6)' '
 
 append_signoff()
 {
-	C=`git commit-tree HEAD^^{tree} -p HEAD` &&
-	git format-patch --stdout --signoff ${C}^..${C} |
-		tee append_signoff.patch |
-		sed -n "1,/^---$/p" |
-		grep -n -E "^Subject|Sign|^$"
+	C=$(git commit-tree HEAD^^{tree} -p HEAD) &&
+	git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
+	sed -n -e "1,/^---$/p" append_signoff.patch |
+		egrep -n "^Subject|Sign|^$"
 }
 
 test_expect_success 'signoff: commit with no body' '