[PATCH v2 00/10] unify appending of sob

[PATCH v2 00/10] unify appending of sob

[Brandon Casey]

Here's version 2 of the unify-appending-of-sob series.  Hopefully this
addresses the comments made on the first series:

   http://thread.gmane.org/gmane.comp.version-control.git/210390

The main difference is that the detection of the "(cherry picked from ...)"
line has been relaxed, and the modifications to log-tree.c have been dropped.

Here's the inter-diff of this series against the original series, both built
on top of 2d242fb3fc19fc9ba046accdd9210be8b9913f64 (the actual series in the
following emails is of course built on top of master).

diff --git a/revision.h b/revision.h
index 435a60b..d20defa 100644
--- a/revision.h
+++ b/revision.h
@@ -137,7 +137,7 @@ struct rev_info {
 	int		numbered_files;
 	char		*message_id;
 	struct string_list *ref_message_ids;
-	int              add_signoff;
+	int		add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;
diff --git a/sequencer.c b/sequencer.c
index eb93dd6..54b3cb9 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -36,13 +36,18 @@ static int is_rfc2822_line(const char *buf, int len)
 	return 1;
 }
 
-static int is_cherry_pick_from_line(const char *buf, int len)
+static int is_cherry_picked_from_line(const char *buf, int len)
 {
-	return (strlen(cherry_picked_prefix) + 41) <= len &&
-		!prefixcmp(buf, cherry_picked_prefix);
+	/*
+	 * 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] == ')'));
 }
 
-/* Returns 0 for non-conforming 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
@@ -51,7 +56,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
 	int ignore_footer)
 {
 	int hit = 0;
-	int i, k = 0;
+	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 	int found_sob = 0;
@@ -76,12 +81,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);
 		if (found_rfc2822 && sob &&
-			!strncasecmp(buf+i, sob->buf, sob->len))
+			!strncmp(buf + i, sob->buf, sob->len))
 			found_sob = k;
 
-		if (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
+		if (!(found_rfc2822 ||
+			is_cherry_picked_from_line(buf + i, k - i)))
 			return 0;
 	}
 	if (found_sob == i)
@@ -1103,11 +1109,20 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
 	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' && (!i || !(has_footer =
-		has_conforming_footer(msgbuf, &sob, ignore_footer))))
-		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+
+	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 (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/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c53dc4b..6d00e43 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1211,16 +1211,17 @@ subject
 
 body
 
+Reviewed-id: Noone
 Tested-by: my@house
 Change-id: Ideadbeef
 Signed-off-by: C O Mitter <committer@example.com>
-BUG: 1234
+Bug: 1234
 EOF
 	cat >expected <<\EOF &&
 4:Subject: [PATCH] subject
 8:
 10:
-13:Signed-off-by: C O Mitter <committer@example.com>
+14:Signed-off-by: C O Mitter <committer@example.com>
 EOF
 	test_cmp expected actual
 '

Brandon Casey (8):
  sequencer.c: remove broken support for rfc2822 continuation in footer
  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: 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

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              | 146 +++++++++++++++++---------
 sequencer.h              |   2 +-
 t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++
 t/t4014-format-patch.sh  | 263 +++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh  |   9 +-
 9 files changed, 595 insertions(+), 153 deletions(-)
 create mode 100755 t/t3511-cherry-pick-x.sh

-- 
1.8.1.1.252.gdb33759

[PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Brandon Casey]

Commit c1e01b0c generalized the detection of the last paragraph
signed-off-by footer and used rfc2822 as a guideline.  Support for rfc2822
style continuation lines was also implemented, but not correctly, so it has
never detected a line beginning with space or tab as a continuation of the
previous line.

Since a commit message is not governed by the line length limits imposed
by rfc2822 for email messages, and it does not seem like this functionality
would produce "better" commit messages anyway, let's remove this broken
functionality.

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

diff --git a/sequencer.c b/sequencer.c
index aef5e8a..fe25ef4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1027,7 +1027,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 	int hit = 0;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
-	int first = 1;
 	const char *buf = sb->buf;
 
 	for (i = len - 1; i > 0; i--) {
@@ -1044,11 +1043,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.1.252.gdb33759

[PATCH v2 02/10] 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>
---
 t/test-lib-functions.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index fa62d01..c601918 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -135,12 +135,13 @@ 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.  It will also add a tag with <message> as name unless <tag> is
+# given.
 #
-# Both <file> and <contents> default to <message>.
+# <file>, <contents>, and <tag> all default to <message>.
 
 test_commit () {
 	notick= &&
@@ -168,7 +169,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.1.252.gdb33759

[PATCH v2 03/10] 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>
---
 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..a6c4168
--- /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
+
+Signed-off-by: B.U. Thor <buthor@example.com>
+Not a valid footer element
+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.1.252.gdb33759

[PATCH v2 04/10] 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>

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 sequencer.c              | 46 ++++++++++++++++++++++++++++------------
 t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 88 insertions(+), 13 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fe25ef4..fe76a1d 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,11 +1022,36 @@ 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)
+{
+	int i;
+
+	for (i = 0; i < len; i++) {
+		int ch = buf[i];
+		if (ch == ':')
+			break;
+		if (isalnum(ch) || (ch == '-'))
+			continue;
+		return 0;
+	}
+
+	return 1;
+}
+
+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] == ')'));
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 {
-	int ch;
 	int hit = 0;
-	int i, j, k;
+	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 
@@ -1043,15 +1069,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) ||
+			is_cherry_picked_from_line(buf + i, k - i)))
 			return 0;
-		}
 	}
 	return 1;
 }
@@ -1068,7 +1088,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 a6c4168..32c0bb1 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.1.252.gdb33759

[PATCH v2 05/10] 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>
---
 sequencer.c              | 114 +++++++++++++++++++++++++----------------------
 t/t3511-cherry-pick-x.sh |  53 ++++++++++++++++++++++
 2 files changed, 113 insertions(+), 54 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index fe76a1d..163dc12 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,64 @@
 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 == ':')
+			break;
+		if (isalnum(ch) || (ch == '-'))
+			continue;
+		return 0;
+	}
+
+	return 1;
+}
+
+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] == ')'));
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+	int hit = 0;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	/* require at least one blank line */
+	if (!hit || buf[i] != '\n')
+		return 0;
+
+	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) ||
+			is_cherry_picked_from_line(buf + i, k - i)))
+			return 0;
+	}
+	return 1;
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -497,6 +555,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,60 +1082,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 == ':')
-			break;
-		if (isalnum(ch) || (ch == '-'))
-			continue;
-		return 0;
-	}
-
-	return 1;
-}
-
-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] == ')'));
-}
-
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
-{
-	int hit = 0;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	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) ||
-			is_cherry_picked_from_line(buf + i, k - i)))
-			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 32c0bb1..9dd6d5d 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.1.252.gdb33759

[PATCH v2 06/10] 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              | 46 ++++++++++++++++++++++++++++++++++++----------
 sequencer.h              |  2 +-
 t/t3511-cherry-pick-x.sh |  2 +-
 4 files changed, 39 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 7c2a3d4..081ff66 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 163dc12..d4a2ece 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -46,12 +46,20 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 		 (buf[len - 1] == '\n' && buf[len - 2] == ')'));
 }
 
-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)
 {
 	int hit = 0;
 	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
+	int found_sob = 0;
 
 	for (i = len - 1; i > 0; i--) {
 		if (hit && buf[i] == '\n')
@@ -67,14 +75,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) ||
+		found_rfc2822 = is_rfc2822_line(buf + i, k - i);
+		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)))
 			return 0;
 	}
+	if (found_sob == i)
+		return 3;
+	if (found_sob)
+		return 2;
 	return 1;
 }
 
@@ -296,7 +315,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;
@@ -555,7 +574,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));
@@ -1082,9 +1101,10 @@ 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, int no_dup_sob)
 {
 	struct strbuf sob = STRBUF_INIT;
+	int has_footer = 0;
 	int i;
 
 	strbuf_addstr(&sob, sign_off_header);
@@ -1093,10 +1113,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 	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 (!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);
-	}
+
+	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 (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..c4c7132 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,6 +48,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, int no_dup_sob);
 
 #endif
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9dd6d5d..4b67343 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.1.252.gdb33759

[PATCH v2 07/10] 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 avoid adding an additional
one if one already exists.  This is necessary to allow format-patch to add a
s-o-b to a patch with no commit message without adding an extra newline.  A
following patch will make format-patch use this function rather than the
append_signoff implementation inside log-tree.c.

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

diff --git a/sequencer.c b/sequencer.c
index d4a2ece..d51e6f2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1114,11 +1114,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
 
-	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 (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 (has_footer != 3 && (!no_dup_sob || has_footer != 2))
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
-- 
1.8.1.1.252.gdb33759

[PATCH v2 08/10] 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 | 242 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 242 insertions(+)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 7fa3647..3868cef 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1021,4 +1021,246 @@ 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} |
+		tee append_signoff.patch |
+		sed -n "1,/^---$/p" |
+		grep -n -E "^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.1.252.gdb33759

[PATCH v2 09/10] 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>
---
 builtin/log.c | 13 +------------
 log-tree.c    | 14 ++++++++++----
 revision.h    |  2 +-
 3 files changed, 12 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..83f33f4 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -253,9 +253,11 @@ 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, int no_dup_sob)
 {
 	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 +277,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 +290,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 +676,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 +692,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, 1);
 
 	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.1.252.gdb33759

[PATCH v2 10/10] 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              | 88 +------------------------------------------------
 t/t4014-format-patch.sh | 31 ++++++++++++++---
 2 files changed, 27 insertions(+), 92 deletions(-)

diff --git a/log-tree.c b/log-tree.c
index 83f33f4..299dad3 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
+#include "sequencer.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -206,93 +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, int no_dup_sob)
-{
-	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 3868cef..d0ec097 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1104,7 +1104,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
 
@@ -1120,7 +1141,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
 
@@ -1136,7 +1157,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
 
@@ -1153,7 +1174,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
 
@@ -1221,7 +1242,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.1.252.gdb33759

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Jonathan Nieder]

Hi,

Brandon Casey wrote:

> --- a/sequencer.c
> +++ b/sequencer.c
[...]
> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)

Git is checking if (sb->buf) ends with a "Signed-off-by:" style
line.  If it doesn't, it will need to add an extra blank line
before adding a new sign-off.

First (snipped), it seeks back two newlines from the end and then
forward to the next non-newline character, so (buf + i) is at the
start of the last line of (the interesting part of) sb.  Now:

> 	for (; i < len; i = k) {
> 		for (k = i; k < len && buf[k] != '\n'; k++)
>  			; /* do nothing */
>  		k++;

(buf + k) points to the end of this line.

> -		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
> -			continue;

This is always the first line examined, so this "continue" never
triggers.

> -
> -		first = 0;
> -
>  		for (j = 0; i + j < len; j++) {

If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
the (nonexistent) next line.  Otherwise, it fails.

Do I understand correctly?  If so, this patch should be a no-op, which
is good, I guess.

But in that case, couldn't this function be made much simpler?  As far
as I can tell, all the function needs to do is the following:

	1. Find the last line.
	2. Check if it is blank or matches /^[[:alnum:]-]*:/
	3. There is no step 3.  That's it.

In other words, something like:

	const char *eol, *p;

	/* End of line */
	eol = memrchr(sb->buf, '\n', sb->len - ignore_footer);
	if (!eol)
		eol = sb->buf;

	/* Start of line */
	p = memrchr(sb->buf, '\n', eol - sb->buf);
	if (p)
		p++;
	else
		p = sb->buf;

	if (p == eol)	/* Blank line? */
		return 1;

	/* "Signed-off-by"-style field */
	while ((isalnum(*p) || *p == '-') && p < eol)
		p++;
	return *p == ':';

where memrchr is defined roughly as follows[1]:

	#ifdef __GLIBC_PREREQ
	#if __GLIBC_PREREQ(2, 2)
	#define HAVE_MEMRCHR
	#endif
	#endif

	#ifndef HAVE_MEMRCHR
	#define memrchr gitmemrchr
	static inline void *gitmemrchr(const void *s, int c, size_t n)
	{
		const unsigned char *p = s;
		p += n;
		while (p != s)
			if (*--p == (unsigned char) c)
				return p;
		return NULL;
	}
	#endif

Does that look right?

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/159081/focus=159121

Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit

[Jonathan Nieder]

Brandon Casey 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.

Yes!

[...]
> --- a/t/test-lib-functions.sh
> +++ b/t/test-lib-functions.sh
> @@ -135,12 +135,13 @@ 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.  It will also add a tag with <message> as name unless <tag> is
> +# given.
>  #
> -# Both <file> and <contents> default to <message>.
> +# <file>, <contents>, and <tag> all default to <message>.

Simpler:

 # This will commit a file with the given contents and the given commit
 # message and tag the resulting commit with the given tag name.
 #
 # <file>, <contents>, and <tag> all default to <message>.

With or without that change,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality

[Jonathan Nieder]

Brandon Casey wrote:

> --- /dev/null
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -0,0 +1,111 @@
[...]
> +	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 &&

Neat.

[...]
> +test_expect_success 'cherry-pick -s inserts blank line after one line subject' '

In particular, a blank line after a one-line subject that starts with
the usual "subsystem:" convention is not mistaken for an RFC2822-style
header.  Good.

[...]
> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '

IIUC this is an illustration of false-positives from messages like
this one:

	base: do something great without a sign-off

	If he does that, it will be the best thing in the
	world: or so I think.

A worthy cause.  Could the example broken message be tweaked to
emphasize that use case?  With the current example, I'd consider
either result (blank line or no blank line) to be ok behavior by git.

[...]
> +test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '

This is a simpler version of the previous test, without the distracting
colon.

[...]
> +test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '

Nice test for basic "-s" functionality.

[...]
> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '

And the other side of basic "-s" functionality.

One more test would be interesting: what does "-s" do when asked to
produce a duplicate signoff with an interspersed signoff by someone else?

	test: a patch with a more complicated life

	This patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for
	tweaking, then back to $GIT_COMMITTER_NAME who will be
	recording it in permanent history.

	Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
	Signed-off-by: A U Thor <author@example.com>

With the changes suggested above or similar ones,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Jonathan Nieder]

Brandon Casey wrote:

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

Yes, looks sane.

A downside is that this produces an arguably worse result when using
"-x -s" for a commit that does not already have a sign-off.  Before,
we had:

	test: do something great

	Do something fantastic in a clean and elegant way that
	only takes two lines of explanation.
	(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)

	Signed-off-by: C H Errypicker <cherry-picker@example.com>

Afterwards, we will have:

	test: do something great

	Do something fantastic in a clean and elegant way that
	only takes two lines of explanation.
	(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
	Signed-off-by: C H Errypicker <cherry-picker@example.com>

An ideal result would be completely different:

	test: do something great

	commit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream.

	Do something fantastic in a clean and elegant way that
	only takes two lines of explanation.

	Signed-off-by: C H Errypicker <cherry-picker@example.com>

In other words, the -x output format that puts the commit id at the
end with odd spacing seems to be of questionable taste anyway.  But
given the constraint of leaving that alone, cramming together the
sign-off like this seems like the best we can do, so for what it's
worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v2 05/10] sequencer.c: always separate "(cherry picked from" from commit body

[Jonathan Nieder]

Brandon Casey wrote:

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

Heh, you read my mind.

Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Jonathan Nieder]

Brandon Casey wrote:

> 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.
[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)

Isn't the behavior of passing '1' here just a bug in "format-patch -s"?

Style: callers will be easier to read if the function takes a flag
word with named bits, as in:

	#define APPEND_SIGNOFF_DEDUP (1 << 0)
	void append_signoff(..., int ignore_footer, unsigned flag)

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Brandon Casey]

On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
> [...]
>> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>
> Git is checking if (sb->buf) ends with a "Signed-off-by:" style
> line.  If it doesn't, it will need to add an extra blank line
> before adding a new sign-off.
>
> First (snipped), it seeks back two newlines from the end and then
> forward to the next non-newline character, so (buf + i) is at the
> start of the last line of (the interesting part of) sb.

Did you catch that the two newlines have to be adjacent to each other?
 i.e. it seeks back until it finds a blank line.  Then it seeks
forward so that buf + i points at the first line of the last paragraph
of sb.

> Now:
>
>>       for (; i < len; i = k) {
>>               for (k = i; k < len && buf[k] != '\n'; k++)
>>                       ; /* do nothing */
>>               k++;
>
> (buf + k) points to the end of this line.

buf + k points to the start of the next line.  Not sure if that is the
same thing as what you said.

>> -             if ((buf[k] == ' ' || buf[k] == '\t') && !first)
>> -                     continue;
>
> This is always the first line examined, so this "continue" never
> triggers.

This is just totally broken and always has been.  The index variable
should be 'i' not 'k'.  It is supposed to check whether the current
line is a continuation of the previously inspected line.  An rfc2822
continuation line begins with space or tab.  The first line of the
paragraph obviously can't be a continuation line, so the /first/
variable is used as a guard against matching the first line.

>> -
>> -             first = 0;
>> -
>>               for (j = 0; i + j < len; j++) {
>
> If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to
> the (nonexistent) next line.  Otherwise, it fails.
>
> Do I understand correctly?

No, I think you misread the for loop that you snipped out.  It seeks
back to the beginning of the last paragraph, not the last line.  The
for loop that you included above, inspects each line of that
paragraph.  If any line does not match /^[[:alnum:]-]*:/, then the
function returns false (0).  If every line matches, it returns true
(1).

> If so, this patch should be a no-op, which
> is good, I guess.

You're correct here though.  The patch is a no-op.  This patch removes
the code that was supposed to parse rfc2822 continuation lines, but it
was broken.  Thankfully it was broken utterly and completely so it
never allowed anything through that wasn't /^[[:alnum:]-]*:/.

-Brandon

Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit

[Brandon Casey]

On Tue, Jan 22, 2013 at 12:02 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:

>> --- a/t/test-lib-functions.sh
>> +++ b/t/test-lib-functions.sh
>> @@ -135,12 +135,13 @@ 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.  It will also add a tag with <message> as name unless <tag> is
>> +# given.
>>  #
>> -# Both <file> and <contents> default to <message>.
>> +# <file>, <contents>, and <tag> all default to <message>.
>
> Simpler:
>
>  # This will commit a file with the given contents and the given commit
>  # message and tag the resulting commit with the given tag name.
>  #
>  # <file>, <contents>, and <tag> all default to <message>.

Yes, a nice improvement.

Thanks,
-Brandon

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Jonathan Nieder]

Brandon Casey wrote:
> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> First (snipped), it seeks back two newlines from the end and then
>> forward to the next non-newline character, so (buf + i) is at the
>> start of the last line of (the interesting part of) sb.
>
> Did you catch that the two newlines have to be adjacent to each other?
[...]

Here is the loop in master:

	int hit = 0;
[...]

	for (i = len - 1; i > 0; i--) {
		if (hit && buf[i] == '\n')
			break;
		hit = (buf[i] == '\n');
	}

I don't see any adjacency check.  I agree with you that "two adjacent
newlines" was probably the intent, though.

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Jonathan Nieder]

Jonathan Nieder wrote:

> Here is the loop in master:
>
> 	int hit = 0;
> [...]
> 
> 	for (i = len - 1; i > 0; i--) {
> 		if (hit && buf[i] == '\n')
> 			break;
> 		hit = (buf[i] == '\n');
> 	}
>
> I don't see any adjacency check.

Of course that's because I can't read. :)  Checking again.

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Jonathan Nieder]

Brandon Casey wrote:
> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>> -             if ((buf[k] == ' ' || buf[k] == '\t') && !first)
>>> -                     continue;
>>
>> This is always the first line examined, so this "continue" never
>> triggers.
>
> This is just totally broken and always has been.  The index variable
> should be 'i' not 'k'.

Yes, now I see.

This test trips when the *next* line starts with ' ' or '\t'.

	commit, cherry-pick -s: remove broken support for multiline rfc2822 fields

	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.  So for example, it
	confuses the following for a well-formed RFC2822 footer:

		Reported-by: Reporter <reporter@example.com>
		LINE NOISE
		 continuation
		 continuation

	A typical well-formed footer with multiline fields is not accepted:

		Reported-by: Re Porter
		 <reporter@example.com>
		Signed-off-by: Author <author@example.com>

	That this has remained broken for so long is a good sign that nobody
	actually needed continuation lines.  Rip out the broken continuation
	support.

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Jonathan Nieder]

Jonathan Nieder wrote:

> 	line) instead of buf[i] is a whitespace character.  So for example, it
> 	confuses the following for a well-formed RFC2822 footer:

Luckily it doesn't, since the final continuation line is not followed
by whitespace.  I should have said:

	"... 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.

	No functional change intended."

I have no excuse, since you explained this all out loud to me a couple of
months ago. :)  Sorry for the nonsense.

Thanks,
Jonathan

Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer

[Brandon Casey]

On Tue, Jan 22, 2013 at 1:49 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> Here is the loop in master:
>>
>>       int hit = 0;
>> [...]
>>
>>       for (i = len - 1; i > 0; i--) {
>>               if (hit && buf[i] == '\n')
>>                       break;
>>               hit = (buf[i] == '\n');
>>       }
>>
>> I don't see any adjacency check.
>
> Of course that's because I can't read. :)  Checking again.

Blame the code, not your eyes.  The use of the term 'hit' is what
makes this loop confusing.  It gives the impression that 'hit' gets
set once, after the first newline is /hit/.

It would be much easier to read if it was written like this:

   int last_char_was_nl = 0;
   for (i = len - 1; i > 0; i--) {
           this_char_is_nl = (buf[i] == '\n');
           if (last_char_was_nl && this_char_is_nl)
                   break;
           last_char_was_nl = this_char_is_nl;
   }

I'll slide this in when I resubmit this series with your suggestions.

-Brandon

Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality

[Brandon Casey]

On Tue, Jan 22, 2013 at 12:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- /dev/null
>> +++ b/t/t3511-cherry-pick-x.sh

> [...]
>> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
>
> IIUC this is an illustration of false-positives from messages like
> this one:
>
>         base: do something great without a sign-off
>
>         If he does that, it will be the best thing in the
>         world: or so I think.
>
> A worthy cause.  Could the example broken message be tweaked to
> emphasize that use case?  With the current example, I'd consider
> either result (blank line or no blank line) to be ok behavior by git.

The primary motivation for this test was to exercise an existing
behavior which fails to append a newline and sob if the last line of
the last paragraph matches the sob of the committer regardless of
whether the entire paragraph would be interpreted as a conforming
footer.  Your example is tested as a side-effect of that.  I'll tweak
the string so it looks like this:

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>

> [...]
>> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
>
> And the other side of basic "-s" functionality.
>
> One more test would be interesting: what does "-s" do when asked to
> produce a duplicate signoff with an interspersed signoff by someone else?
>
>         test: a patch with a more complicated life
>
>         This patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for
>         tweaking, then back to $GIT_COMMITTER_NAME who will be
>         recording it in permanent history.
>
>         Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
>         Signed-off-by: A U Thor <author@example.com>

This one exists as "adds sob when last sob doesn't match committer".
In this case an additional sob should be appended to the footer.

-Brandon

Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality

[Jonathan Nieder]

Brandon Casey wrote:

>                                                            I'll tweak
> the string so it looks like this:
>
> 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>

Yep, that looks better than the example I suggested. :)

[...]
> On Tue, Jan 22, 2013 at 12:17 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> One more test would be interesting: what does "-s" do when asked to
>> produce a duplicate signoff with an interspersed signoff by someone else?
[...]
> This one exists as "adds sob when last sob doesn't match committer".

So it does.  Thanks for the sanity check.

Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Brandon Casey]

On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> 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.
> [...]
>> --- a/sequencer.c
>> +++ b/sequencer.c
>> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)
>
> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?

I think that is an open question.

> Style: callers will be easier to read if the function takes a flag
> word with named bits, as in:
>
>         #define APPEND_SIGNOFF_DEDUP (1 << 0)
>         void append_signoff(..., int ignore_footer, unsigned flag)

Can't I just be lazy!!!  Ok, you win. :b

-Brandon

Re: [PATCH v2 04/10] sequencer.c: recognize "(cherry picked from ..." as part of s-o-b footer

[Jonathan Nieder]

Hi,

Brandon Casey wrote:

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

My last review was based on a misunderstanding of
ends_rfc2822_footer().  This time I can unreservedly say I like this.

[...]
> --- 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 ";

The static/nonstatic mismatch looks strange.  Not about this patch,
but probably rest_is_empty() from builtin-commit.c should move here
some day.

[...]
> @@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)
[...]
> +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] == ')'));

These two cases are based on whether the commit message ended with
a '(cherry picked from' with no trailing newline, I guess?

I wonder if switching the convention for 'k' would make this simpler:

	const char *line, *eol;

	line = buf + i;
	eol = memchr(buf, '\n', len - i);
	if (!eol)
		eol = buf + len;

	if (!is_rfc2822_field(line, eol - line) &&
	    !is_cherry_picked_from_line(line, eol - line))
		return 0;

I notice you just sent a new version of the series.  I'll try this out
on top of that.

Thanks,
Jonathan

Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Brandon Casey wrote:
>>
>>> 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.
>> [...]
>>> --- a/sequencer.c
>>> +++ b/sequencer.c
>>> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)
>>
>> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
>
> I think that is an open question.

Yes. as I said in a previous review round, I think it was a mistake
that format-patch chose to pay attention to any S-o-b in the patch
trail, not only the last one, and we may want to correcct it once
this series solidifies as a separate "bugfix" change on top.

Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Junio C Hamano]

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

> Brandon Casey <drafnel@gmail.com> writes:
>
>> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>> Brandon Casey wrote:
>>>
>>>> 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.
>>> [...]
>>>> --- a/sequencer.c
>>>> +++ b/sequencer.c
>>>> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)
>>>
>>> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
>>
>> I think that is an open question.
>
> Yes. as I said in a previous review round, I think it was a mistake
> that format-patch chose to pay attention to any S-o-b in the patch
> trail, not only the last one, and we may want to correcct it once
> this series solidifies as a separate "bugfix" change on top.

This is a tangent, but I _think_ (didn't check, though) "git am -s"
implements this incorrrectly.  Just another LHF somebody may want to
take a look.

Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Brandon Casey]

On 2/9/2013 3:06 PM, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Brandon Casey <drafnel@gmail.com> writes:
>>
>>> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>>>> Brandon Casey wrote:
>>>>
>>>>> 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.
>>>> [...]
>>>>> --- a/sequencer.c
>>>>> +++ b/sequencer.c
>>>>> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)
>>>>
>>>> Isn't the behavior of passing '1' here just a bug in "format-patch -s"?
>>>
>>> I think that is an open question.
>>
>> Yes. as I said in a previous review round, I think it was a mistake
>> that format-patch chose to pay attention to any S-o-b in the patch
>> trail, not only the last one, and we may want to correcct it once
>> this series solidifies as a separate "bugfix" change on top.
> 
> This is a tangent, but I _think_ (didn't check, though) "git am -s"
> implements this incorrrectly.  Just another LHF somebody may want to
> take a look.

(I haven't checked the 'git am -s' behavior, but ...)

One distinction between commit, cherry-pick, am, and format-patch is
that the first three are related to importing commits _into_ your own
repository while format-patch is related to exporting patches _from_
your repository.

Not sure if an "exporting" operation should be treated differently from
an "importing" operation and mean "add my sign-off only if I did not
previously do so when I did commit, cherry-pick, or am" or whether it
should mean the same thing i.e. "append my sign-off, unless it's already
last".

Just pointing something out that may or may not be useful.

Hmm, actually, this series could provide a data point.  When I submitted
the first series, I used 'format-patch -s' to append my sob.  When
Jonathan reviewed the series, he provided Acked-by and Reviewed-by which
I collected and added as I reworked the series.  When I added Jonathan's
Ack's/Rev-by's, I _appended_ them to the commits that I had already
submitted, so they appeared _after_ my own sob.  When I submitted the
later iterations of the series, I probably used 'format-patch -s' again
and I of course did not want an additional sob to be appended after
Jonathan's Acks.

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