[PATCH v4 00/12] unify appending of sob

[PATCH v4 00/12] unify appending of sob

[Brandon Casey]

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

[PATCH v4 01/12] sequencer.c: rework search for start of footer to improve clarity

[Brandon Casey]

From: Jonathan Nieder <jrnieder@gmail.com>

This code sequence is somewhat difficult to read.  Let's rewrite it and add
some comments to improve clarity.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 sequencer.c | 10 ++++++----
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index aef5e8a..dbeff01 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1023,19 +1023,21 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 
 static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 {
-	int ch;
-	int hit = 0;
+	char ch, prev;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
 	int first = 1;
 	const char *buf = sb->buf;
 
+	prev = '\0';
 	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
+		ch = buf[i];
+		if (prev == '\n' && ch == '\n') /* paragraph break */
 			break;
-		hit = (buf[i] == '\n');
+		prev = ch;
 	}
 
+	/* advance to start of last paragraph */
 	while (i < len - 1 && buf[i] == '\n')
 		i++;
 
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 02/12] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields

[Brandon Casey]

Starting with c1e01b0c (commit: More generous accepting of RFC-2822 footer
lines, 2009-10-28), "git commit -s" carefully parses the last paragraph of
each commit message to check if it consists only of RFC2822-style headers,
in which case the signoff will be added as a new line in the same list:

   Reported-by: Reporter <reporter@example.com>
   Signed-off-by: Author <author@example.com>
   Acked-by: Lieutenant <lt@example.com>

It even included support for accepting indented continuation lines for
multiline fields.  Unfortunately the multiline field support is broken
because it checks whether buf[k] (the first character of the *next* line)
instead of buf[i] is a whitespace character.  The result is that any footer
with a continuation line is not accepted, since the last continuation line
neither starts with an RFC2822 field name nor is followed by a continuation
line.

That this has remained broken for so long is good evidence that nobody
actually needed multiline fields.  Rip out the broken continuation support.

There should be no functional change.

[Thanks to Jonathan Nieder for the excellent commit message]

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sequencer.c | 6 ------
 1 file changed, 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index dbeff01..aa2cb8e 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1026,7 +1026,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 	char ch, prev;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
-	int first = 1;
 	const char *buf = sb->buf;
 
 	prev = '\0';
@@ -1046,11 +1045,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
-			continue;
-
-		first = 0;
-
 		for (j = 0; i + j < len; j++) {
 			ch = buf[i + j];
 			if (ch == ':')
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 03/12] t/test-lib-functions.sh: allow to specify the tag name to test_commit

[Brandon Casey]

The <message> part of test_commit() may not be appropriate for a tag name.
So let's allow test_commit to accept a fourth argument to specify the tag
name.

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/test-lib-functions.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fa62d01..61d0804 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -135,12 +135,12 @@ test_pause () {
 	fi
 }
 
-# Call test_commit with the arguments "<message> [<file> [<contents>]]"
+# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
-# message.  It will also add a tag with <message> as name.
+# message, and tag the resulting commit with the given tag name.
 #
-# Both <file> and <contents> default to <message>.
+# <file>, <contents>, and <tag> all default to <message>.
 
 test_commit () {
 	notick= &&
@@ -168,7 +168,7 @@ test_commit () {
 		test_tick
 	fi &&
 	git commit $signoff -m "$1" &&
-	git tag "$1"
+	git tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments "<message> <commit>", where <commit>
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 04/12] t/t3511: add some tests of 'cherry-pick -s' functionality

[Brandon Casey]

Add some tests to ensure that 'cherry-pick -s' operates in the following
manner:

   * Inserts a blank line before appending a s-o-b to a commit message that
     does not contain a s-o-b footer

   * Does not mistake first line "subject: description" as a s-o-b footer

   * Does not mistake single word message body as conforming to rfc2822

   * Appends a s-o-b when last s-o-b in footer does not match committer
     s-o-b, even when committer's s-o-b exists elsewhere in footer.

   * Does not append a s-o-b when last s-o-b matches committer s-o-b

   * Correctly detects a non-conforming footer containing a mix of s-o-b
     like elements and s-o-b elements. (marked "expect failure")

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t3511-cherry-pick-x.sh | 111 +++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 111 insertions(+)
 create mode 100755 t/t3511-cherry-pick-x.sh

diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
new file mode 100755
index 0000000..2a040b7
--- /dev/null
+++ b/t/t3511-cherry-pick-x.sh
@@ -0,0 +1,111 @@
+#!/bin/sh
+
+test_description='Test cherry-pick -x and -s'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	git cherry-pick --quit &&
+	git checkout -f "$1^0" &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x
+}
+
+mesg_one_line='base: commit message'
+
+mesg_no_footer="$mesg_one_line
+
+OneWordBodyThatsNotA-S-o-B"
+
+mesg_with_footer="$mesg_no_footer
+
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+Signed-off-by: A.U. Thor <author@example.com>
+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>"
+
+mesg_with_footer_sob="$mesg_with_footer
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+
+
+test_expect_success setup '
+	git config advice.detachedhead false &&
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit "$mesg_one_line" foo b mesg-one-line &&
+	git reset --hard initial &&
+	test_commit "$mesg_no_footer" foo b mesg-no-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_broken_footer" foo b mesg-broken-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_footer" foo b mesg-with-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+	pristine_detach initial &&
+	test_commit conflicting unrelated
+'
+
+test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-one-line &&
+	cat <<-EOF >expect &&
+		$mesg_one_line
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+	pristine_detach initial &&
+	git 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 -s inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_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 -s adds sob when last sob doesnt match committer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_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 -s refrains from adding duplicate trailing sob' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-footer-sob &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer_sob
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Brandon Casey]

When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
picked commit, it does not currently detect the "(cherry picked from..."
that may have been appended by a previous 'cherry-pick -x' as part of the
s-o-b footer and it will insert a blank line before appending a new s-o-b.

Let's detect "(cherry picked from...)" as part of the footer so that we
will produce this:

   Signed-off-by: A U Thor <author@example.com>
   (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
   Signed-off-by: C O Mmitter <committer@example.com>

instead of this:

   Signed-off-by: A U Thor <author@example.com>
   (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)

   Signed-off-by: C O Mmitter <committer@example.com>

[with improvements from Jonathan Nieder]

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sequencer.c              | 51 ++++++++++++++++++++++++++++++++------------
 t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 92 insertions(+), 14 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index aa2cb8e..93495b0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 const char sign_off_header[] = "Signed-off-by: ";
+static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 static void remove_sequencer_state(void)
 {
@@ -496,7 +497,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, cherry_picked_prefix);
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
@@ -1021,16 +1022,44 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+static int is_rfc2822_line(const char *buf, int len)
 {
-	char ch, prev;
-	int i, j, k;
+	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 &&
+		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+	char prev;
+	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 
+	/* footer must end with newline */
+	if (!len || buf[len - 1] != '\n')
+		return 0;
+
 	prev = '\0';
 	for (i = len - 1; i > 0; i--) {
-		ch = buf[i];
+		char ch = buf[i];
 		if (prev == '\n' && ch == '\n') /* paragraph break */
 			break;
 		prev = ch;
@@ -1045,15 +1074,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
+		if (!is_rfc2822_line(buf + i, k - i - 1) &&
+		    !is_cherry_picked_from_line(buf + i, k - i - 1))
 			return 0;
-		}
 	}
 	return 1;
 }
@@ -1070,7 +1093,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
 	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
-		if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+		if (!i || !has_conforming_footer(msgbuf, ignore_footer))
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
 	}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 2a040b7..73da182 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 
+mesg_with_cherry_footer="$mesg_with_footer_sob
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: C.U. Thor <cuthor@example.com>"
+
 
 test_expect_success setup '
 	git config advice.detachedhead false &&
@@ -47,6 +51,8 @@ test_expect_success setup '
 	test_commit "$mesg_with_footer" foo b mesg-with-footer &&
 	git reset --hard initial &&
 	test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
 	pristine_detach initial &&
 	test_commit conflicting unrelated
 '
@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer^0` &&
+	git cherry-pick -x -s mesg-with-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer
+		(cherry picked from commit $sha1)
+		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 -s refrains from adding duplicate trailing sob' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer-sob &&
@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer-sob^0` &&
+	git cherry-pick -x -s mesg-with-footer-sob &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer_sob
+		(cherry picked from commit $sha1)
+		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 treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 06/12] sequencer.c: require a conforming footer to be preceded by a blank line

[Brandon Casey]

Currently, append_signoff() performs a search for the last line of the
commit buffer by searching back from the end until it hits a newline.  If
it reaches the beginning of the buffer without finding a newline, that
means either the commit message was empty, or there was only one line in it.
In this case, append_signoff will skip the call to has_conforming_footer
since it already knows that it is necessary to append a newline before
appending the sob.

Let's perform this function inside of has_conforming_footer where it
appropriately belongs and generalize it so that we require that the
footer paragraph be an actual distinct paragraph separated by a blank
line.

Signed-off-by: Brandon Casey <drafnel@gmail.com>
---
 sequencer.c | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 93495b0..178e84b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1065,6 +1065,10 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 		prev = ch;
 	}
 
+	/* require at least one blank line */
+	if (prev != '\n' || buf[i] != '\n')
+		return 0;
+
 	/* advance to start of last paragraph */
 	while (i < len - 1 && buf[i] == '\n')
 		i++;
@@ -1093,7 +1097,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
 	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
-		if (!i || !has_conforming_footer(msgbuf, ignore_footer))
+		if (!has_conforming_footer(msgbuf, ignore_footer))
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
 	}
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 07/12] sequencer.c: always separate "(cherry picked from" from commit body

[Brandon Casey]

Start treating the "(cherry picked from" line added by cherry-pick -x
the same way that the s-o-b lines are treated.  Namely, separate them
from the main commit message body with an empty line.

Introduce tests to test this functionality.

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 sequencer.c              | 128 ++++++++++++++++++++++++-----------------------
 t/t3511-cherry-pick-x.sh |  53 ++++++++++++++++++++
 2 files changed, 118 insertions(+), 63 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 178e84b..249c4a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,69 @@
 const char sign_off_header[] = "Signed-off-by: ";
 static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
+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 &&
+		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+	char prev;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	const char *buf = sb->buf;
+
+	/* footer must end with newline */
+	if (!len || buf[len - 1] != '\n')
+		return 0;
+
+	prev = '\0';
+	for (i = len - 1; i > 0; i--) {
+		char ch = buf[i];
+		if (prev == '\n' && ch == '\n') /* paragraph break */
+			break;
+		prev = ch;
+	}
+
+	/* require at least one blank line */
+	if (prev != '\n' || buf[i] != '\n')
+		return 0;
+
+	/* advance to start of last paragraph */
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if (!is_rfc2822_line(buf + i, k - i - 1) &&
+		    !is_cherry_picked_from_line(buf + i, k - i - 1))
+			return 0;
+	}
+	return 1;
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -497,6 +560,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
+			if (!has_conforming_footer(&msgbuf, 0))
+				strbuf_addch(&msgbuf, '\n');
 			strbuf_addstr(&msgbuf, cherry_picked_prefix);
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
@@ -1022,69 +1087,6 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-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 &&
-		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
-}
-
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
-{
-	char prev;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-
-	/* footer must end with newline */
-	if (!len || buf[len - 1] != '\n')
-		return 0;
-
-	prev = '\0';
-	for (i = len - 1; i > 0; i--) {
-		char ch = buf[i];
-		if (prev == '\n' && ch == '\n') /* paragraph break */
-			break;
-		prev = ch;
-	}
-
-	/* require at least one blank line */
-	if (prev != '\n' || buf[i] != '\n')
-		return 0;
-
-	/* advance to start of last paragraph */
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if (!is_rfc2822_line(buf + i, k - i - 1) &&
-		    !is_cherry_picked_from_line(buf + i, k - i - 1))
-			return 0;
-	}
-	return 1;
-}
-
 void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 {
 	struct strbuf sob = STRBUF_INIT;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 73da182..a845e45 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -57,6 +57,19 @@ test_expect_success setup '
 	test_commit conflicting unrelated
 '
 
+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-one-line^0` &&
+	git cherry-pick -x mesg-one-line &&
+	cat <<-EOF >expect &&
+		$mesg_one_line
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-one-line &&
@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot
 	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` &&
+	git cherry-pick -x mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-no-footer &&
@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-no-footer^0` &&
+	git cherry-pick -x -s mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+		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 -s adds sob when last sob doesnt match committer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer &&
@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 08/12] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Brandon Casey]

Teach append_signoff how to detect a duplicate s-o-b in the commit footer.
This is in preparation to unify the append_signoff implementations in
log-tree.c and sequencer.c.

Fixes test in t3511.

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 builtin/commit.c         |  2 +-
 sequencer.c              | 59 ++++++++++++++++++++++++++++++++++++------------
 sequencer.h              |  4 +++-
 t/t3511-cherry-pick-x.sh |  2 +-
 4 files changed, 50 insertions(+), 17 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 1a0e5f1..94c96b7 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -700,7 +700,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
 			previous = eol;
 		}
 
-		append_signoff(&sb, ignore_footer);
+		append_signoff(&sb, ignore_footer, 0);
 	}
 
 	if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
diff --git a/sequencer.c b/sequencer.c
index 249c4a0..3364faa 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -44,12 +44,20 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
 }
 
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+/*
+ * Returns 0 for non-conforming footer
+ * Returns 1 for conforming footer
+ * Returns 2 when sob exists within conforming footer
+ * Returns 3 when sob exists within conforming footer as last entry
+ */
+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')
@@ -72,14 +80,25 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 		i++;
 
 	for (; i < len; i = k) {
+		int found_rfc2822;
+
 		for (k = i; k < len && buf[k] != '\n'; k++)
 			; /* do nothing */
 		k++;
 
-		if (!is_rfc2822_line(buf + i, k - i - 1) &&
-		    !is_cherry_picked_from_line(buf + i, k - i - 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;
+
+		if (!(found_rfc2822 ||
+		      is_cherry_picked_from_line(buf + i, k - i - 1)))
 			return 0;
 	}
+	if (found_sob == i)
+		return 3;
+	if (found_sob)
+		return 2;
 	return 1;
 }
 
@@ -301,7 +320,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,
 	rollback_lock_file(&index_lock);
 
 	if (opts->signoff)
-		append_signoff(msgbuf, 0);
+		append_signoff(msgbuf, 0, 0);
 
 	if (!clean) {
 		int i;
@@ -560,7 +579,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
-			if (!has_conforming_footer(&msgbuf, 0))
+			if (!has_conforming_footer(&msgbuf, NULL, 0))
 				strbuf_addch(&msgbuf, '\n');
 			strbuf_addstr(&msgbuf, cherry_picked_prefix);
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
@@ -1087,21 +1106,33 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+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 i;
+	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 (prefixcmp(msgbuf->buf + i, sob.buf)) {
-		if (!has_conforming_footer(msgbuf, ignore_footer))
-			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
-		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
-	}
+
+	/*
+	 * 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)
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+
+	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				sob.buf, sob.len);
+
 	strbuf_release(&sob);
 }
diff --git a/sequencer.h b/sequencer.h
index 9d57d57..1fc22dc 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -6,6 +6,8 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+#define APPEND_SIGNOFF_DEDUP (1u << 0)
+
 enum replay_action {
 	REPLAY_REVERT,
 	REPLAY_PICK
@@ -48,6 +50,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);
 
 extern const char sign_off_header[];
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
 
 #endif
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index a845e45..f977279 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-broken-footer &&
 	cat <<-EOF >expect &&
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Brandon Casey]

Teach append_signoff to detect whether a blank line exists at the position
that the signed-off-by line will be added, and refrain from adding an
additional one if one already exists.  Or, add an additional line if one
is needed to make sure the new footer is separated from the message body
by a blank line.

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 sequencer.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3364faa..3c63e3a 100644


I dropped the mention of format-patch in the commit message.  This
implementation is less about making format-patch work and more about
cleaning up a strangely formatted commit message.

-Brandon

--- a/sequencer.c
+++ b/sequencer.c
@@ -1110,6 +1110,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 {
 	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
 	struct strbuf sob = STRBUF_INIT;
+	const char *append_newlines = NULL;
 	int has_footer;
 
 	strbuf_addstr(&sob, sign_off_header);
@@ -1127,8 +1128,17 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 	else
 		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
 
-	if (!has_footer)
-		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+	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,
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 10/12] t4014: more tests about appending s-o-b lines

[Brandon Casey]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

[bc: Squash the tests from Duy's original unify-appending-sob series.

     Fix test 90 "signoff: some random signoff-alike" and mark as failing.
     Correct behavior should insert a blank line after message body and
     signed-off-by.

     Add two additional tests:

       1. failure to detect non-conforming elements in the footer when last
          line matches committer's s-o-b.
       2. ensure various s-o-b -like elements in the footer are handled as
          conforming. e.g. "Change-id: IXXXX or Bug: 1234"
]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 t/t4014-format-patch.sh | 241 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 241 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7fa3647..a415b89 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1021,4 +1021,245 @@ test_expect_success 'cover letter using branch description (6)' '
 	grep hello actual >/dev/null
 '
 
+append_signoff()
+{
+	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' '
+	append_signoff </dev/null >actual &&
+	cat <<\EOF | sed "s/EOL$//" >expected &&
+4:Subject: [PATCH] EOL
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject' '
+	echo subject | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: commit with only subject that does not end with NL' '
+	printf subject | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: no existing signoffs and no trailing NL' '
+	printf "subject\n\nbody" | append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: my@house
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: my@house
+12:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_failure 'signoff: some random signoff-alike' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+Fooled-by-me: my@house
+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_expect_failure 'signoff: not really a signoff' '
+	append_signoff <<\EOF >actual &&
+subject
+
+I want to mention about Signed-off-by: here.
+EOF
+	cat >expected <<\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>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_failure 'signoff: not really a signoff (2)' '
+	append_signoff <<\EOF >actual &&
+subject
+
+My unfortunate
+Signed-off-by: example happens to be wrapped here.
+EOF
+	cat >expected <<\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>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '
+	append_signoff <<\EOF >actual &&
+subject
+
+Signed-off-by: my@house
+Signed-off-by: your@house
+
+A lot of houses.
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: my@house
+10:Signed-off-by: your@house
+11:
+13:
+14:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff at the end, no trailing NL' '
+	printf "subject\n\nSigned-off-by: C O Mitter <committer@example.com>" |
+		append_signoff >actual &&
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+9:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: the same signoff NOT at the end' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: C O Mitter <committer@example.com>
+Signed-off-by: my@house
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: C O Mitter <committer@example.com>
+12:Signed-off-by: my@house
+EOF
+	test_cmp expected actual
+'
+
+test_expect_failure 'signoff: detect garbage in non-conforming footer' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Tested-by: my@house
+Some Trash
+Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+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: footer begins with non-signoff without @ sign' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Reviewed-id: Noone
+Tested-by: my@house
+Change-id: Ideadbeef
+Signed-off-by: C O Mitter <committer@example.com>
+Bug: 1234
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+14:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 11/12] format-patch: update append_signoff prototype

[Brandon Casey]

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

This is a preparation step for merging with append_signoff from
sequencer.c

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 builtin/log.c | 13 +------------
 log-tree.c    | 17 +++++++++++++----
 revision.h    |  2 +-
 3 files changed, 15 insertions(+), 17 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index 8f0b2e8..59de484 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1086,7 +1086,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	struct commit *origin = NULL, *head = NULL;
 	const char *in_reply_to = NULL;
 	struct patch_ids ids;
-	char *add_signoff = NULL;
 	struct strbuf buf = STRBUF_INIT;
 	int use_patch_format = 0;
 	int quiet = 0;
@@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
 	}
 
-	if (do_signoff) {
-		const char *committer;
-		const char *endpos;
-		committer = git_committer_info(IDENT_STRICT);
-		endpos = strchr(committer, '>');
-		if (!endpos)
-			die(_("bogus committer info %s"), committer);
-		add_signoff = xmemdupz(committer, endpos - committer + 1);
-	}
-
 	for (i = 0; i < extra_hdr.nr; i++) {
 		strbuf_addstr(&buf, extra_hdr.items[i].string);
 		strbuf_addch(&buf, '\n');
@@ -1393,7 +1382,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		total++;
 		start_number--;
 	}
-	rev.add_signoff = add_signoff;
+	rev.add_signoff = do_signoff;
 	while (0 <= --nr) {
 		int shown;
 		commit = list[nr];
diff --git a/log-tree.c b/log-tree.c
index 5dc45c4..ac1cd68 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -10,6 +10,8 @@
 #include "color.h"
 #include "gpg-interface.h"
 
+#define APPEND_SIGNOFF_DEDUP (1u <<0)
+
 struct decoration name_decoration = { "object names" };
 
 enum decoration_type {
@@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static void append_signoff(struct strbuf *sb, const char *signoff)
+static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
 {
+	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
 	static const char signed_off_by[] = "Signed-off-by: ";
+	char *signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					       getenv("GIT_COMMITTER_EMAIL")));
 	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
 	char *cp;
@@ -275,6 +280,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 		if (!isspace(cp[signoff_len]))
 			continue;
 		/* we already have him */
+		free(signoff);
 		return;
 	}
 
@@ -287,6 +293,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	strbuf_addstr(sb, signed_off_by);
 	strbuf_add(sb, signoff, signoff_len);
 	strbuf_addch(sb, '\n');
+	free(signoff);
 }
 
 static unsigned int digits_in_number(unsigned int number)
@@ -672,8 +679,10 @@ void show_log(struct rev_info *opt)
 	/*
 	 * And then the pretty-printed message itself
 	 */
-	if (ctx.need_8bit_cte >= 0)
-		ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
+	if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
+		ctx.need_8bit_cte =
+			has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					       getenv("GIT_COMMITTER_EMAIL")));
 	ctx.date_mode = opt->date_mode;
 	ctx.date_mode_explicit = opt->date_mode_explicit;
 	ctx.abbrev = opt->diffopt.abbrev;
@@ -686,7 +695,7 @@ void show_log(struct rev_info *opt)
 	pretty_print_commit(&ctx, commit, &msgbuf);
 
 	if (opt->add_signoff)
-		append_signoff(&msgbuf, opt->add_signoff);
+		append_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message) {
diff --git a/revision.h b/revision.h
index 5da09ee..01bd2b7 100644
--- a/revision.h
+++ b/revision.h
@@ -138,7 +138,7 @@ struct rev_info {
 	int		reroll_count;
 	char		*message_id;
 	struct string_list *ref_message_ids;
-	const char	*add_signoff;
+	int		add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
-- 
1.8.1.3.579.gd9af3b6

[PATCH v4 12/12] Unify appending signoff in format-patch, commit and sequencer

[Brandon Casey]

There are two implementations of append_signoff in log-tree.c and
sequencer.c, which do more or less the same thing.  Unify on top of the
sequencer.c implementation.

Add a test in t4014 to demonstrate support for non-s-o-b elements in the
commit footer provided by sequence.c:append_sob.  Mark tests fixed as
appropriate.

[Commit message mostly stolen from Nguyễn Thái Ngọc Duy's original
 unification patch]

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 log-tree.c              | 91 +------------------------------------------------
 t/t4014-format-patch.sh | 31 ++++++++++++++---
 2 files changed, 27 insertions(+), 95 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index ac1cd68..c9d9a37 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,8 +9,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
-
-#define APPEND_SIGNOFF_DEDUP (1u <<0)
+#include "sequencer.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	putchar(')');
 }
 
-/*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
- * Signed-off-by: and Acked-by: lines.
- */
-static int detect_any_signoff(char *letter, int size)
-{
-	char *cp;
-	int seen_colon = 0;
-	int seen_at = 0;
-	int seen_name = 0;
-	int seen_head = 0;
-
-	cp = letter + size;
-	while (letter <= --cp && *cp == '\n')
-		continue;
-
-	while (letter <= cp) {
-		char ch = *cp--;
-		if (ch == '\n')
-			break;
-
-		if (!seen_at) {
-			if (ch == '@')
-				seen_at = 1;
-			continue;
-		}
-		if (!seen_colon) {
-			if (ch == '@')
-				return 0;
-			else if (ch == ':')
-				seen_colon = 1;
-			else
-				seen_name = 1;
-			continue;
-		}
-		if (('A' <= ch && ch <= 'Z') ||
-		    ('a' <= ch && ch <= 'z') ||
-		    ch == '-') {
-			seen_head = 1;
-			continue;
-		}
-		/* no empty last line doesn't match */
-		return 0;
-	}
-	return seen_head && seen_name;
-}
-
-static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
-{
-	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
-	static const char signed_off_by[] = "Signed-off-by: ";
-	char *signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
-					       getenv("GIT_COMMITTER_EMAIL")));
-	size_t signoff_len = strlen(signoff);
-	int has_signoff = 0;
-	char *cp;
-
-	cp = sb->buf;
-
-	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, signed_off_by))) {
-
-		has_signoff = 1;
-
-		cp += strlen(signed_off_by);
-		if (cp + signoff_len >= sb->buf + sb->len)
-			break;
-		if (strncmp(cp, signoff, signoff_len))
-			continue;
-		if (!isspace(cp[signoff_len]))
-			continue;
-		/* we already have him */
-		free(signoff);
-		return;
-	}
-
-	if (!has_signoff)
-		has_signoff = detect_any_signoff(sb->buf, sb->len);
-
-	if (!has_signoff)
-		strbuf_addch(sb, '\n');
-
-	strbuf_addstr(sb, signed_off_by);
-	strbuf_add(sb, signoff, signoff_len);
-	strbuf_addch(sb, '\n');
-	free(signoff);
-}
-
 static unsigned int digits_in_number(unsigned int number)
 {
 	unsigned int i = 10, result = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index a415b89..97fde9e 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1103,7 +1103,28 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: some random signoff-alike' '
+test_expect_success 'signoff: misc conforming footer elements' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+Signed-off-by: my@house
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: Some One <someone@example.com>
+Bug: 1234
+EOF
+	cat >expected <<\EOF &&
+4:Subject: [PATCH] subject
+8:
+10:
+11:Signed-off-by: my@house
+15:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
+test_expect_success 'signoff: some random signoff-alike' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1119,7 +1140,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: not really a signoff' '
+test_expect_success 'signoff: not really a signoff' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1135,7 +1156,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: not really a signoff (2)' '
+test_expect_success 'signoff: not really a signoff (2)' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1152,7 +1173,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '
+test_expect_success 'signoff: valid S-o-b paragraph in the middle' '
 	append_signoff <<\EOF >actual &&
 subject
 
@@ -1220,7 +1241,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: detect garbage in non-conforming footer' '
+test_expect_success 'signoff: detect garbage in non-conforming footer' '
 	append_signoff <<\EOF >actual &&
 subject
 
-- 
1.8.1.3.579.gd9af3b6

[PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality

[Brandon Casey]

---

This test tests the behavior of 'cherry-pick -s' of a commit with an empty
commit message.

I created the test when I noticed during my series that cherry-pick was
adding a sob twice when a commit with an empty commit message was
cherry-picked.

I'm not sure we should apply this though.  I'm leaning towards saying that
the 'cherry-pick -s' behavior with respect to a commit with an empty message
body should be undefined.  If we want it to be undefined then we probably
shouldn't introduce a test which would have the effect of defining it.

Junio, if you think we should apply it, it's prepared as a fixup commit and
should autosquash easily.

-Brandon

 t/t3505-cherry-pick-empty.sh | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh
index a0c6e30..da4c60e 100755
--- a/t/t3505-cherry-pick-empty.sh
+++ b/t/t3505-cherry-pick-empty.sh
@@ -58,6 +58,16 @@ test_expect_success 'cherry-pick a commit with an empty message with --allow-emp
 	git cherry-pick --allow-empty-message empty-branch
 '
 
+test_expect_success 'cherry-pick a commit with an empty message with --allow-empty-message and -s' '
+	git reset --hard HEAD^ &&
+	git cherry-pick --allow-empty-message -s empty-branch &&
+	{ git show --pretty=format:%B -s empty-branch &&
+	  printf "Signed-off-by: %s <%s>\n" "$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL"
+	} >expect &&
+	git show --pretty=format:%B -s HEAD >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '
 	git checkout master &&
 	echo fourth >>file2 &&
-- 
1.8.1.1.252.gdb33759

[PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Brandon Casey]

Teach append_signoff to detect whether a blank line exists at the position
that the signed-off-by line will be added, and refrain from adding an
additional one if one already exists.  Or, add an additional line if one
is needed to make sure the new footer is separated from the message body
by a blank line.

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---


A slight tweak.  And I promise, no more are coming.

-Brandon


 sequencer.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 3364faa..084573b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 	else
 		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
 
-	if (!has_footer)
-		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+	if (!has_footer) {
+		const char *append_newlines = NULL;
+		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,
-- 
1.8.1.1.252.gdb33759

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
> picked commit, it does not currently detect the "(cherry picked from..."
> that may have been appended by a previous 'cherry-pick -x' as part of the
> s-o-b footer and it will insert a blank line before appending a new s-o-b.
>
> Let's detect "(cherry picked from...)" as part of the footer so that we
> will produce this:
> ...
> +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 &&
> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
> +}

Does the first "is it longer than the prefix?" check matter?  If it
is not, prefixcmp() would not match anyway, no?

Re: [PATCH v4 11/12] format-patch: update append_signoff prototype

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This is a preparation step for merging with append_signoff from
> sequencer.c
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  builtin/log.c | 13 +------------
>  log-tree.c    | 17 +++++++++++++----
>  revision.h    |  2 +-
>  3 files changed, 15 insertions(+), 17 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 8f0b2e8..59de484 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1086,7 +1086,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	struct commit *origin = NULL, *head = NULL;
>  	const char *in_reply_to = NULL;
>  	struct patch_ids ids;
> -	char *add_signoff = NULL;
>  	struct strbuf buf = STRBUF_INIT;
>  	int use_patch_format = 0;
>  	int quiet = 0;
> @@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.subject_prefix = strbuf_detach(&sprefix, NULL);
>  	}
>  
> -	if (do_signoff) {
> -		const char *committer;
> -		const char *endpos;
> -		committer = git_committer_info(IDENT_STRICT);
> -		endpos = strchr(committer, '>');
> -		if (!endpos)
> -			die(_("bogus committer info %s"), committer);
> -		add_signoff = xmemdupz(committer, endpos - committer + 1);
> -	}
> -
>  	for (i = 0; i < extra_hdr.nr; i++) {
>  		strbuf_addstr(&buf, extra_hdr.items[i].string);
>  		strbuf_addch(&buf, '\n');
> @@ -1393,7 +1382,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		total++;
>  		start_number--;
>  	}
> -	rev.add_signoff = add_signoff;
> +	rev.add_signoff = do_signoff;
>  	while (0 <= --nr) {
>  		int shown;
>  		commit = list[nr];
> diff --git a/log-tree.c b/log-tree.c
> index 5dc45c4..ac1cd68 100644
> --- a/log-tree.c
> +++ b/log-tree.c
> @@ -10,6 +10,8 @@
>  #include "color.h"
>  #include "gpg-interface.h"
>  
> +#define APPEND_SIGNOFF_DEDUP (1u <<0)
> +
>  struct decoration name_decoration = { "object names" };
>  
>  enum decoration_type {
> @@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)
>  	return seen_head && seen_name;
>  }
>  
> -static void append_signoff(struct strbuf *sb, const char *signoff)
> +static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
>  {
> +	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;

Unused variable at this step?

>  	static const char signed_off_by[] = "Signed-off-by: ";
> +	char *signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
> +					       getenv("GIT_COMMITTER_EMAIL")));
>  	size_t signoff_len = strlen(signoff);
>  	int has_signoff = 0;
>  	char *cp;
> @@ -275,6 +280,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
>  		if (!isspace(cp[signoff_len]))
>  			continue;
>  		/* we already have him */
> +		free(signoff);
>  		return;
>  	}
>  
> @@ -287,6 +293,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
>  	strbuf_addstr(sb, signed_off_by);
>  	strbuf_add(sb, signoff, signoff_len);
>  	strbuf_addch(sb, '\n');
> +	free(signoff);
>  }
>  
>  static unsigned int digits_in_number(unsigned int number)
> @@ -672,8 +679,10 @@ void show_log(struct rev_info *opt)
>  	/*
>  	 * And then the pretty-printed message itself
>  	 */
> -	if (ctx.need_8bit_cte >= 0)
> -		ctx.need_8bit_cte = has_non_ascii(opt->add_signoff);
> +	if (ctx.need_8bit_cte >= 0 && opt->add_signoff)
> +		ctx.need_8bit_cte =
> +			has_non_ascii(fmt_name(getenv("GIT_COMMITTER_NAME"),
> +					       getenv("GIT_COMMITTER_EMAIL")));
>  	ctx.date_mode = opt->date_mode;
>  	ctx.date_mode_explicit = opt->date_mode_explicit;
>  	ctx.abbrev = opt->diffopt.abbrev;
> @@ -686,7 +695,7 @@ void show_log(struct rev_info *opt)
>  	pretty_print_commit(&ctx, commit, &msgbuf);
>  
>  	if (opt->add_signoff)
> -		append_signoff(&msgbuf, opt->add_signoff);
> +		append_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);
>  
>  	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
>  	    ctx.notes_message && *ctx.notes_message) {
> diff --git a/revision.h b/revision.h
> index 5da09ee..01bd2b7 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -138,7 +138,7 @@ struct rev_info {
>  	int		reroll_count;
>  	char		*message_id;
>  	struct string_list *ref_message_ids;
> -	const char	*add_signoff;
> +	int		add_signoff;
>  	const char	*extra_headers;
>  	const char	*log_reencode;
>  	const char	*subject_prefix;

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Brandon Casey]

On 2/12/2013 11:13 AM, Junio C Hamano wrote:
> Brandon Casey <drafnel@gmail.com> writes:
> 
>> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry
>> picked commit, it does not currently detect the "(cherry picked from..."
>> that may have been appended by a previous 'cherry-pick -x' as part of the
>> s-o-b footer and it will insert a blank line before appending a new s-o-b.
>>
>> Let's detect "(cherry picked from...)" as part of the footer so that we
>> will produce this:
>> ...
>> +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 &&
>> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>> +}
> 
> Does the first "is it longer than the prefix?" check matter?  If it
> is not, prefixcmp() would not match anyway, no?

Probably not in practice, but technically we should only be accessing
len characters in buf even though buf may be longer than len.  So the
check is just making sure the function doesn't access chars it's not
supposed to.

-Brandon


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Junio C Hamano]

Brandon Casey <bcasey@nvidia.com> writes:

>>> +	return len > strlen(cherry_picked_prefix) + 1 &&
>>> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>> +}
>> 
>> Does the first "is it longer than the prefix?" check matter?  If it
>> is not, prefixcmp() would not match anyway, no?
>
> Probably not in practice, but technically we should only be accessing
> len characters in buf even though buf may be longer than len.  So the
> check is just making sure the function doesn't access chars it's not
> supposed to.

Sorry, I do not follow.  Isn't caller's buf terminated with LF at buf[len],
which would never match cherry_picked_prefix even if len is shorter
than the prefix?

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Jonathan Nieder]

Brandon Casey wrote:
> On 2/12/2013 11:13 AM, Junio C Hamano wrote:
>> Brandon Casey <drafnel@gmail.com> writes:

>>> +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 &&
>>> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>> +}
>>
>> Does the first "is it longer than the prefix?" check matter?  If it
>> is not, prefixcmp() would not match anyway, no?
>
> Probably not in practice, but technically we should only be accessing
> len characters in buf even though buf may be longer than len.

Yep.  Technically the buf[len - 1] == ')' check is enough to avoid
false positives, but if it and the 'len' check were dropped then this
would be checking that buf is a "(cherry-picked from" line instead of
checking that its first 'len' bytes are one.

So it's just paranoid futureproofing.  In the long term, it would be
nice to drop the "number of bytes to ignore at the end" argument to
append_signoff to avoid having to think about this kind of thing.

Jonathan

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Brandon Casey]

On 2/12/2013 11:36 AM, Junio C Hamano wrote:
> Brandon Casey <bcasey@nvidia.com> writes:
> 
>>>> +	return len > strlen(cherry_picked_prefix) + 1 &&
>>>> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>> +}
>>>
>>> Does the first "is it longer than the prefix?" check matter?  If it
>>> is not, prefixcmp() would not match anyway, no?
>>
>> Probably not in practice, but technically we should only be accessing
>> len characters in buf even though buf may be longer than len.  So the
>> check is just making sure the function doesn't access chars it's not
>> supposed to.
> 
> Sorry, I do not follow.  Isn't caller's buf terminated with LF at buf[len],
> which would never match cherry_picked_prefix even if len is shorter
> than the prefix?

Heh, I almost pointed that out in my reply.  Yes, buf will be terminated
with LF at buf[len].  And yes, that means that we will never get a false
positive from prefixcmp even if the comparison overruns buf+len while
doing its comparison.  That's why the check doesn't matter in practice,
i.e. based on the way that is_cherry_picked_from_line is being called
right now and the content of cherry_picked_prefix.

But, hasn't is_cherry_picked_from_line entered into a contract with the
caller and said "I will not access more than len characters"?

It's ok with me if you think it reads better without the check.

-Brandon


-----------------------------------------------------------------------------------
This email message is for the sole use of the intended recipient(s) and may contain
confidential information.  Any unauthorized review, use, disclosure or distribution
is prohibited.  If you are not the intended recipient, please contact the sender by
reply email and destroy all copies of the original message.
-----------------------------------------------------------------------------------

Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality

[Jonathan Nieder]

Brandon Casey wrote:

> I'm not sure we should apply this though.  I'm leaning towards saying that
> the 'cherry-pick -s' behavior with respect to a commit with an empty message
> body should be undefined.  If we want it to be undefined then we probably
> shouldn't introduce a test which would have the effect of defining it.

Maybe it would make sense to just check that cherry-pick doesn't
segfault in this case?

That is, compute the output but don't compare it to expected output, as
in:

	test_expect_success 'adding signoff to empty message does something sane' '
		git reset --hard HEAD^ &&
		git cherry-pick --allow-empty-message -s empty-branch &&
		git show --pretty=format:%B -s empty-branch >actual &&

		# sign-off is included *somewhere*
		grep "^Signed-off-by:.*>\$" actual
	'

Alternatively, if there are only a few sane behaviors, a test can check
for all of them and pass as long as git follows one.  I haven't thought
carefully enough about this example to suggest doing that.

Thanks,
Jonathan

Re: [PATCH v4 05/12] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Junio C Hamano]

Brandon Casey <bcasey@nvidia.com> writes:

> On 2/12/2013 11:36 AM, Junio C Hamano wrote:
>> Brandon Casey <bcasey@nvidia.com> writes:
>> 
>>>>> +	return len > strlen(cherry_picked_prefix) + 1 &&
>>>>> +		!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>>>>> +}
>>>>
>>>> Does the first "is it longer than the prefix?" check matter?  If it
>>>> is not, prefixcmp() would not match anyway, no?
>>>
>>> Probably not in practice, but technically we should only be accessing
>>> len characters in buf even though buf may be longer than len.  So the
>>> check is just making sure the function doesn't access chars it's not
>>> supposed to.
>> 
>> Sorry, I do not follow.  Isn't caller's buf terminated with LF at buf[len],
>> which would never match cherry_picked_prefix even if len is shorter
>> than the prefix?
>
> Heh, I almost pointed that out in my reply.  Yes, buf will be terminated
> with LF at buf[len].  And yes, that means that we will never get a false
> positive from prefixcmp even if the comparison overruns buf+len while
> doing its comparison.  That's why the check doesn't matter in practice,
> i.e. based on the way that is_cherry_picked_from_line is being called
> right now and the content of cherry_picked_prefix.
>
> But, hasn't is_cherry_picked_from_line entered into a contract with the
> caller and said "I will not access more than len characters"?
>
> It's ok with me if you think it reads better without the check.

As Jonathan says, if you rewrite it to

	return buf[len - 1] == ')' && !prefixcmp(buf, cherry_picked_prefix);

then the code can keep its promise without the length check, because
it knows there is no ')' in cherry-picked-prefix, and it also knows
prefixcmp() stops at the first difference.

It is not a huge deal; I was primarily reacting to the ugly multi-line
boolean expresion that is not inside a pair of parentheses (and because
this is a "return" statement, there is no good reason to have parentheses
except that this is a multi-line expression), which looked odd.

Re: [PATCH v4 00/12] unify appending of sob

[Jonathan Nieder]

Brandon Casey wrote:

> Round 4.

Yay.  I think this is cooked now and a good foundation for later
changes on top.

For what it's worth, with or without the two tweaks Junio suggested
(simplifying "(cherry picked from" detection, deferring introduction
of no_dup_sob variable until it is used),
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Brandon Casey wrote:
>
>> I'm not sure we should apply this though.  I'm leaning towards saying that
>> the 'cherry-pick -s' behavior with respect to a commit with an empty message
>> body should be undefined.  If we want it to be undefined then we probably
>> shouldn't introduce a test which would have the effect of defining it.
>
> Maybe it would make sense to just check that cherry-pick doesn't
> segfault in this case?

;-)

>
> That is, compute the output but don't compare it to expected output, as
> in:
>
> 	test_expect_success 'adding signoff to empty message does something sane' '
> 		git reset --hard HEAD^ &&
> 		git cherry-pick --allow-empty-message -s empty-branch &&
> 		git show --pretty=format:%B -s empty-branch >actual &&
>
> 		# sign-off is included *somewhere*
> 		grep "^Signed-off-by:.*>\$" actual
> 	'

Isn't what the current code happens to do is the best we could do?
We would end up showing one entry whose title appears to be
"Signed-off-by: ..." in the shortlog output if we did so.  If we
added an empty line, then the shortlog output will have a single
empty line that is equally unsightly.

We could force a message like this:

	tree d7f87518a26e9f00714675706f165b94f3625177
        parent f459a4b602c0f4d371e1717572de6d0c4d39c6b1
        author Junio C Hamano <gitster@pobox.com> 1360699963 -0800
        committer Junio C Hamano <gitster@pobox.com> 1360699980 -0800

	!!cherry-picked from a commit without any message!!

        Signed-off-by: Junio C Hamano <gitster@pobox.com>

but I do not think that buys us much; it only replaces a totally
uninformative empty line with another totally uninformative junk.

That ugliness is a price the insane person, who is cherry picking a
commit without any justification made by another insane person,
indicates that he is willing to pay by doing so.  At that point I do
not think we should care.

Re: [PATCH v4 00/12] unify appending of sob

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Brandon Casey wrote:
>
>> Round 4.
>
> Yay.  I think this is cooked now and a good foundation for later
> changes on top.
>
> For what it's worth, with or without the two tweaks Junio suggested
> (simplifying "(cherry picked from" detection, deferring introduction
> of no_dup_sob variable until it is used),
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Yeah, I am inclined to merge this to 'next' without any tweak, and
let it cook and get polished incrementally.  I am not sure if we
have enough time to graduate it to 'master' for the upcoming
release, though.

Thanks.

Re: [PATCH v4 11/12] format-patch: update append_signoff prototype

[Brandon Casey]

On Tue, Feb 12, 2013 at 11:29 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:

>> diff --git a/builtin/log.c b/builtin/log.c
>> index 8f0b2e8..59de484 100644
>> --- a/builtin/log.c
>> +++ b/builtin/log.c

>> @@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)
>>       return seen_head && seen_name;
>>  }
>>
>> -static void append_signoff(struct strbuf *sb, const char *signoff)
>> +static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
>>  {
>> +     unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
>
> Unused variable at this step?

Yeah, looks like that line can be dropped.

-Brandon

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[John Keeping]

On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:
> Teach append_signoff to detect whether a blank line exists at the position
> that the signed-off-by line will be added, and refrain from adding an
> additional one if one already exists.  Or, add an additional line if one
> is needed to make sure the new footer is separated from the message body
> by a blank line.
> 
> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
> ---

As Jonathan Nieder wondered before [1], this changes the behaviour when
the commit message is empty.  Before this commit, there is an empty line
followed by the S-O-B line; now the S-O-B is on the first line of the
commit.

The previous behaviour seems better to me since the empty line is
hinting that the user should fill something in.  It looks particularly
strange if your editor has syntax highlighting for commit messages such
that the first line is in a different colour.

[1] http://article.gmane.org/gmane.comp.version-control.git/214796

> diff --git a/sequencer.c b/sequencer.c
> index 3364faa..084573b 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  	else
>  		has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
>  
> -	if (!has_footer)
> -		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
> +	if (!has_footer) {
> +		const char *append_newlines = NULL;
> +		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";

To restore the old behaviour this needs something like this:

		else if (!len)
			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,

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Brandon Casey]

On Thu, Feb 14, 2013 at 9:58 AM, John Keeping <john@keeping.me.uk> wrote:
> On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:
>> Teach append_signoff to detect whether a blank line exists at the position
>> that the signed-off-by line will be added, and refrain from adding an
>> additional one if one already exists.  Or, add an additional line if one
>> is needed to make sure the new footer is separated from the message body
>> by a blank line.
>>
>> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
>> ---
>
> As Jonathan Nieder wondered before [1], this changes the behaviour when
> the commit message is empty.  Before this commit, there is an empty line
> followed by the S-O-B line; now the S-O-B is on the first line of the
> commit.
>
> The previous behaviour seems better to me since the empty line is
> hinting that the user should fill something in.  It looks particularly
> strange if your editor has syntax highlighting for commit messages such
> that the first line is in a different colour.
>
> [1] http://article.gmane.org/gmane.comp.version-control.git/214796
>
>> diff --git a/sequencer.c b/sequencer.c
>> index 3364faa..084573b 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>>       else
>>               has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);
>>
>> -     if (!has_footer)
>> -             strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
>> +     if (!has_footer) {
>> +             const char *append_newlines = NULL;
>> +             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";
>
> To restore the old behaviour this needs something like this:
>
>                 else if (!len)
>                         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,

Are you talking about the output produced by format-patch?  Or are you
talking about what happens when you do 'commit --amend -s' for a
commit with an empty commit message. (The email that you referenced
was about the behavior of format-patch).

I'm thinking you must be talking about the 'commit --amend -s'
behavior since you mentioned your editor.  Is there another case that
is affected by this?  Normally, any extra blank lines that precede or
follow a commit message are removed before the commit object is
created.  So, I guess it wouldn't hurt to insert a newline (or maybe
it should be two?) before the signoff in this case.  Would this
provide an improvement or change for any other commands than 'commit
--amend -s'?

If we want to do this, then I'd probably do it like this:

-               if (len && msgbuf->buf[len - 1] != '\n')
+               if (!len || msgbuf->buf[len - 1] != '\n')
                        append_newlines = "\n\n";
-               else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+               else if (len == 1 || msgbuf->buf[len - 2] != '\n')
                        append_newlines = "\n";

This would ensure there were two newlines preceding the sob.  The
editor would place its cursor on the top line where the user should
begin typing in a commit message.  If an editor was not opened up
(e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then
the normal mechanism that removes extra blank lines would trigger to
remove the extra blank lines.

I think that's reasonable.

It seems 'git cherry-pick -s --edit' follows a different code path,
and the commit message is stripped of newlines by 'git commit' before
it is passed to the editor.  'cherry-pick -s --edit' and 'commit
--amend -s' should probably have the same behavior and present the
same buffer to the user for editing when they encounter a commit with
an empty message.

Maybe something like this is enough(?):

diff --git a/builtin/commit.c b/builtin/commit.c
index 7b9e2ac..0796412 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const char
        if (unset)
                strbuf_setlen(buf, 0);
        else {
+               if (buf->len)
+                       strbuf_addch(buf, '\n');
                strbuf_addstr(buf, arg);
-               strbuf_addstr(buf, "\n\n");
+               strbuf_complete_line(buf);
        }
        return 0;
 }
@@ -673,9 +675,6 @@ static int prepare_to_commit(const char *index_file, const c
        if (s->fp == NULL)
                die_errno(_("could not open '%s'"), git_path(commit_editmsg));

-       if (clean_message_contents)
-               stripspace(&sb, 0);
-
        if (signoff) {
                /*
                 * See if we have a Conflicts: block at the end. If yes, count
@@ -703,6 +702,9 @@ static int prepare_to_commit(const char *index_file, const c
                append_signoff(&sb, ignore_footer, 0);
        }

+       if (clean_message_contents)
+               stripspace(&sb, 0);
+
        if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)
                die_errno(_("could not write commit template"));

I suspect we have a broken test at t7502.15 though that happened to
work because opt_parse_m() was appending two newlines that the test
expected to be there.

-Brandon

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[John Keeping]

On Fri, Feb 15, 2013 at 10:58:38AM -0800, Brandon Casey wrote:
> On Thu, Feb 14, 2013 at 9:58 AM, John Keeping <john@keeping.me.uk> wrote:
> > As Jonathan Nieder wondered before [1], this changes the behaviour when
> > the commit message is empty.  Before this commit, there is an empty line
> > followed by the S-O-B line; now the S-O-B is on the first line of the
> > commit.
> >
> > The previous behaviour seems better to me since the empty line is
> > hinting that the user should fill something in.  It looks particularly
> > strange if your editor has syntax highlighting for commit messages such
> > that the first line is in a different colour.
> 
> Are you talking about the output produced by format-patch?  Or are you
> talking about what happens when you do 'commit --amend -s' for a
> commit with an empty commit message. (The email that you referenced
> was about the behavior of format-patch).

I'm talking about plain 'commit -s' which seems to use the same code
path.

> I'm thinking you must be talking about the 'commit --amend -s'
> behavior since you mentioned your editor.  Is there another case that
> is affected by this?  Normally, any extra blank lines that precede or
> follow a commit message are removed before the commit object is
> created.  So, I guess it wouldn't hurt to insert a newline (or maybe
> it should be two?) before the signoff in this case.  Would this
> provide an improvement or change for any other commands than 'commit
> --amend -s'?
> 
> If we want to do this, then I'd probably do it like this:
> 
> -               if (len && msgbuf->buf[len - 1] != '\n')
> +               if (!len || msgbuf->buf[len - 1] != '\n')
>                         append_newlines = "\n\n";
> -               else if (len > 1 && msgbuf->buf[len - 2] != '\n')
> +               else if (len == 1 || msgbuf->buf[len - 2] != '\n')
>                         append_newlines = "\n";
> 
> This would ensure there were two newlines preceding the sob.  The
> editor would place its cursor on the top line where the user should
> begin typing in a commit message.  If an editor was not opened up
> (e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then
> the normal mechanism that removes extra blank lines would trigger to
> remove the extra blank lines.
> 
> I think that's reasonable.

Two blank lines seems like an improvement to me, FWIW.


John

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> Teach append_signoff to detect whether a blank line exists at the position
> that the signed-off-by line will be added, and refrain from adding an
> additional one if one already exists.  Or, add an additional line if one
> is needed to make sure the new footer is separated from the message body
> by a blank line.
>
> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
> ---
>
>
> A slight tweak.  And I promise, no more are coming.

When I do

	$ git commit -s

it should start my editor with this in the buffer:

	----------------------------------------------------------------

        Signed-off-by: Junio C Hamano <gitster@pobox.com>
	----------------------------------------------------------------

and the cursor blinking at the beginning of the file.  Annoyingly
this step breaks it by removing the leading blank line.

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Brandon Casey]

On Thu, Feb 21, 2013 at 10:51 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> Teach append_signoff to detect whether a blank line exists at the position
>> that the signed-off-by line will be added, and refrain from adding an
>> additional one if one already exists.  Or, add an additional line if one
>> is needed to make sure the new footer is separated from the message body
>> by a blank line.
>>
>> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
>> ---
>>
>>
>> A slight tweak.  And I promise, no more are coming.
>
> When I do
>
>         $ git commit -s
>
> it should start my editor with this in the buffer:
>
>         ----------------------------------------------------------------
>
>         Signed-off-by: Junio C Hamano <gitster@pobox.com>
>         ----------------------------------------------------------------
>
> and the cursor blinking at the beginning of the file.  Annoyingly
> this step breaks it by removing the leading blank line.

Yes.  The fix described by John Keeping restores the above behavior
for 'commit -s'.  Or the fix I described which inserts two preceding
newlines so it looks like this:

   ----------------------------------------------------------------


   Signed-off-by: Junio C Hamano <gitster@pobox.com>
   ----------------------------------------------------------------

So then the cursor would be placed on the first line and a space would
separate it from the sob which is arguably a better indication to the
user that a blank line should separate the commit message body from
the sob.

But, this does not fix the same problem for 'cherry-pick --edit -s'
when used to cherry-pick a commit without a sob.  The cherry-pick part
of it would add the extra preceding newlines, but then cherry-pick
passes the buffer to 'git commit' via .git/MERGE_MSG which then
"cleans" the buffer, removing the empty lines, in prepare_to_commit()
before allowing the editor to operate on it.

Using 'cherry-pick --edit -s' to cherry-pick a commit with an empty
commit message is going to be a pretty rare corner case.  It would be
nice to have the same behavior for it that we decide to have for
'commit -s', but it's probably not worth going through contortions to
make it happen.

-Brandon

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Brandon Casey]

On Thu, Feb 21, 2013 at 12:26 PM, Brandon Casey <drafnel@gmail.com> wrote:

> But, this does not fix the same problem for 'cherry-pick --edit -s'
> when used to cherry-pick a commit without a sob.

Correction: "when used to cherry-pick a commit with an empty commit message."

Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> Yes.  The fix described by John Keeping restores the above behavior
> for 'commit -s'.  Or the fix I described which inserts two preceding
> newlines so it looks like this:
>
>    ----------------------------------------------------------------
>
>
>    Signed-off-by: Junio C Hamano <gitster@pobox.com>
>    ----------------------------------------------------------------
>
> So then the cursor would be placed on the first line and a space would
> separate it from the sob which is arguably a better indication to the
> user that a blank line should separate the commit message body from
> the sob.

That sounds like an improvement to me.

> But, this does not fix the same problem for 'cherry-pick --edit -s'
> when used to cherry-pick a commit without a sob. ...
> Using 'cherry-pick --edit -s' to cherry-pick a commit with an empty
> commit message is going to be a pretty rare corner case....

We actively discourage an empty commit message by requiring users to
say "commit --allow-empty-message".  I think it is in line with the
philosophy for a Porcelain command "git cherry-pick -s" to punish
users by making them work harder to use a commit with an empty
message ;-).

[PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Brandon Casey]

Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
a blank line before the Signed-off-by line.  This provided a nice
hint to the user that something should be filled in.  Let's restore that
behavior, but now let's ensure that the Signed-off-by line is preceded
by two blank lines to hint that something should be filled in, and that
a blank line should separate it from the Signed-off-by line.

Plus, add a test for this behavior.

Reported-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

Ok.  Here's a patch on top of 959a2623 bc/append-signed-off-by.  It
implements the "2 blank lines preceding sob" behavior.

-Brandon

 sequencer.c       |  5 +++--
 t/t7502-commit.sh | 12 ++++++++++++
 2 files changed, 15 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 53ee49a..2dac106 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 		const char *append_newlines = NULL;
 		size_t len = msgbuf->len - ignore_footer;
 
-		if (len && msgbuf->buf[len - 1] != '\n')
+		/* ensure a blank line precedes our signoff */
+		if (!len || msgbuf->buf[len - 1] != '\n')
 			append_newlines = "\n\n";
-		else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+		else if (len == 1 || msgbuf->buf[len - 2] != '\n')
 			append_newlines = "\n";
 
 		if (append_newlines)
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..a53a1e0 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
 
 '
 
+test_expect_success 'commit -s places sob on third line after two empty lines' '
+	git commit -s --allow-empty --allow-empty-message &&
+	cat <<-EOF >expect &&
+
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+
+	EOF
+	egrep -v '^#' .git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 write_script .git/FAKE_EDITOR <<\EOF
 mv "$1" "$1.orig"
 (
-- 
1.8.0.1.253.gfcb57d5.dirty

Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line.  This provided a nice
> hint to the user that something should be filled in.  Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
>
> Plus, add a test for this behavior.
>
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
>
> Ok.  Here's a patch on top of 959a2623 bc/append-signed-off-by.  It
> implements the "2 blank lines preceding sob" behavior.
>
> -Brandon
>
>  sequencer.c       |  5 +++--
>  t/t7502-commit.sh | 12 ++++++++++++
>  2 files changed, 15 insertions(+), 2 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 53ee49a..2dac106 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  		const char *append_newlines = NULL;
>  		size_t len = msgbuf->len - ignore_footer;
>  
> -		if (len && msgbuf->buf[len - 1] != '\n')
> +		/* ensure a blank line precedes our signoff */
> +		if (!len || msgbuf->buf[len - 1] != '\n')
>  			append_newlines = "\n\n";
> -		else if (len > 1 && msgbuf->buf[len - 2] != '\n')
> +		else if (len == 1 || msgbuf->buf[len - 2] != '\n')
>  			append_newlines = "\n";

Maybe I am getting slower with age, but it took me 5 minutes of
staring the above to convince me that it is doing the right thing.
The if/elseif cascade is dealing with three separate things and the
logic is a bit dense:

 * Is the buffer completely empty?  We need to add two LFs to give a
   room for the title and body;

 * Otherwise:

   - Is the final line incomplete?  We need to add one LF to make it a
     complete line whatever we do.

   - Is the final line an empty line?  We need to add one more LF to
     make sure we have a blank line before we add S-o-b.

I wondered if we can rewrite it to make the logic clearer (that is
where I spent most of the 5 minutes), but I did not think of a
better way; probably the above is the best we could do.

Thanks.

By the way, I think we would want to introduce a symbolic constants
for the possible return values from has_conforming_footer().  The
check that appears after this hunk

	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
				sob.buf, sob.len);

is hard to grok without them.

Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Brandon Casey]

On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:

>> diff --git a/sequencer.c b/sequencer.c
>> index 53ee49a..2dac106 100644
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>>               const char *append_newlines = NULL;
>>               size_t len = msgbuf->len - ignore_footer;
>>
>> -             if (len && msgbuf->buf[len - 1] != '\n')
>> +             /* ensure a blank line precedes our signoff */
>> +             if (!len || msgbuf->buf[len - 1] != '\n')
>>                       append_newlines = "\n\n";
>> -             else if (len > 1 && msgbuf->buf[len - 2] != '\n')
>> +             else if (len == 1 || msgbuf->buf[len - 2] != '\n')
>>                       append_newlines = "\n";
>
> Maybe I am getting slower with age, but it took me 5 minutes of
> staring the above to convince me that it is doing the right thing.
>
> The if/elseif cascade is dealing with three separate things and the
> logic is a bit dense:
>
>  * Is the buffer completely empty?  We need to add two LFs to give a
>    room for the title and body;
>
>  * Otherwise:
>
>    - Is the final line incomplete?  We need to add one LF to make it a
>      complete line whatever we do.
>
>    - Is the final line an empty line?  We need to add one more LF to
>      make sure we have a blank line before we add S-o-b.
>
> I wondered if we can rewrite it to make the logic clearer (that is
> where I spent most of the 5 minutes), but I did not think of a
> better way; probably the above is the best we could do.

We could unroll the conditionals into individual blocks and add your
comments from above like:

   if (!len) {
      /* The buffer is completely empty.  Leave room for the title and body. */
      append_newlines = "\n\n";
   } else if (msgbuf->buf[len - 1] != '\n') {
      /* Incomplete line.  Complete the line and add a blank one */
      append_newlines = "\n\n";
   } else if (len == 1) {
      /*
       * Buffer contains a single newline.  Add another so that we leave
       * room for the title and body.
       */
      append_newlines = "\n";
   } ...

Not sure that it will reduce the amount of time needed to understand
what's going on, but at least it describes the expectations made by
each block.

> Thanks.
>
> By the way, I think we would want to introduce a symbolic constants
> for the possible return values from has_conforming_footer().  The
> check that appears after this hunk
>
>         if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
>                 strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
>                                 sob.buf, sob.len);
>
> is hard to grok without them.

Yeah, Jonathan said the same thing and I agree.  I was hoping someone
else would beat me to it.

-Brandon

[PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Brandon Casey]

From: Brandon Casey <drafnel@gmail.com>

Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
a blank line before the Signed-off-by line.  This provided a nice
hint to the user that something should be filled in.  Let's restore that
behavior, but now let's ensure that the Signed-off-by line is preceded
by two blank lines to hint that something should be filled in, and that
a blank line should separate it from the Signed-off-by line.

Plus, add a test for this behavior.

Reported-by: John Keeping <john@keeping.me.uk>
Signed-off-by: Brandon Casey <drafnel@gmail.com>
---

How about something like this?

-Brandon

 sequencer.c       | 27 +++++++++++++++++++++++++--
 t/t7502-commit.sh | 12 ++++++++++++
 2 files changed, 37 insertions(+), 2 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 53ee49a..a07d2d0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1127,10 +1127,33 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 		const char *append_newlines = NULL;
 		size_t len = msgbuf->len - ignore_footer;
 
-		if (len && msgbuf->buf[len - 1] != '\n')
+		if (!len) {
+			/*
+			 * The buffer is completely empty.  Leave foom for
+			 * the title and body to be filled in by the user.
+			 */
 			append_newlines = "\n\n";
-		else if (len > 1 && msgbuf->buf[len - 2] != '\n')
+		} else if (msgbuf->buf[len - 1] != '\n') {
+			/*
+			 * Incomplete line.  Complete the line and add a
+			 * blank one so that there is an empty line between
+			 * the message body and the sob.
+			 */
+			append_newlines = "\n\n";
+		} else if (len == 1) {
+			/*
+			 * Buffer contains a single newline.  Add another
+			 * so that we leave room for the title and body.
+			 */
+			append_newlines = "\n";
+		} else if (msgbuf->buf[len - 2] != '\n') {
+			/*
+			 * Buffer ends with a single newline.  Add another
+			 * so that there is an empty line between the message
+			 * body and the sob.
+			 */
 			append_newlines = "\n";
+		} /* else, the buffer already ends with two newlines. */
 
 		if (append_newlines)
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
diff --git a/t/t7502-commit.sh b/t/t7502-commit.sh
index deb187e..a53a1e0 100755
--- a/t/t7502-commit.sh
+++ b/t/t7502-commit.sh
@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo
 
 '
 
+test_expect_success 'commit -s places sob on third line after two empty lines' '
+	git commit -s --allow-empty --allow-empty-message &&
+	cat <<-EOF >expect &&
+
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+
+	EOF
+	egrep -v '^#' .git/COMMIT_EDITMSG >actual &&
+	test_cmp expect actual
+'
+
 write_script .git/FAKE_EDITOR <<\EOF
 mv "$1" "$1.orig"
 (
-- 
1.8.1.3.566.gaa39828

Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Jeff King]

On Fri, Feb 22, 2013 at 02:05:27PM -0800, Brandon Casey wrote:

> From: Brandon Casey <drafnel@gmail.com>
> 
> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with
> a blank line before the Signed-off-by line.  This provided a nice
> hint to the user that something should be filled in.  Let's restore that
> behavior, but now let's ensure that the Signed-off-by line is preceded
> by two blank lines to hint that something should be filled in, and that
> a blank line should separate it from the Signed-off-by line.
> 
> Plus, add a test for this behavior.
> 
> Reported-by: John Keeping <john@keeping.me.uk>
> Signed-off-by: Brandon Casey <drafnel@gmail.com>
> ---
> 
> How about something like this?

FWIW, as a casual reader of this series, I find this to be way easier
to follow than the previous round.

-Peff

Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> FWIW, as a casual reader of this series, I find this to be way easier
> to follow than the previous round.

It is assuring to know that I am not the only one getting slow with
age ;-)

Thanks.

Re: [PATCH v4 03/12] t/test-lib-functions.sh: allow to specify the tag name to test_commit

[Ævar Arnfjörð Bjarmason]

On Tue, Feb 12, 2013 at 11:17 AM, Brandon Casey <drafnel@gmail.com> wrote:
> The <message> part of test_commit() may not be appropriate for a tag name.
> So let's allow test_commit to accept a fourth argument to specify the tag
> name.

[Kind of late to notice, I know]

I see nobody spotted in four rounds of reviews that this commit didn't
update the corresponding t/README docs for test_commit.

> Signed-off-by: Brandon Casey <bcasey@nvidia.com>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
>  t/test-lib-functions.sh | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
> index fa62d01..61d0804 100644
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -135,12 +135,12 @@ test_pause () {
>         fi
>  }
>
> -# Call test_commit with the arguments "<message> [<file> [<contents>]]"
> +# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
>  #
>  # This will commit a file with the given contents and the given commit
> -# message.  It will also add a tag with <message> as name.
> +# message, and tag the resulting commit with the given tag name.
>  #
> -# Both <file> and <contents> default to <message>.
> +# <file>, <contents>, and <tag> all default to <message>.
>
>  test_commit () {
>         notick= &&
> @@ -168,7 +168,7 @@ test_commit () {
>                 test_tick
>         fi &&
>         git commit $signoff -m "$1" &&
> -       git tag "$1"
> +       git tag "${4:-$1}"
>  }
>
>  # Call test_merge with the arguments "<message> <commit>", where <commit>
> --
> 1.8.1.3.579.gd9af3b6
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html