[PATCH 1/5] t/test-lib-functions.sh: allow to specify the tag name to test_commit

[PATCH 1/5] 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 8889ba5..4d6057e 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>.
+# Both <file> <contents> and <tag> 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.0

[PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s

[Brandon Casey]

The cherry-pick -s functionality is currently broken in two ways.

   1. handling of rfc2822 continuation lines has a bug, and the
      continuation lines are not handled correctly.
   2. the "(cherry picked from ...)" lines are commonly appended to the
      end of the s-o-b footer and should be detected as part of the footer.
      They should not cause a new line to be inserted before the new s-o-b
      is added.

      i.e. we should produce this:

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

      not

         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>
---
 t/t3511-cherry-pick-x.sh | 77 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 77 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..b4e5c65
--- /dev/null
+++ b/t/t3511-cherry-pick-x.sh
@@ -0,0 +1,77 @@
+#!/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
+}
+
+non_rfc2822_mesg='base with footer
+
+Commit message body is here.'
+
+rfc2822_mesg="$non_rfc2822_mesg
+
+Signed-off-by: A.U. Thor
+ <author@example.com>
+Signed-off-by: B.U. Thor <buthor@example.com>"
+
+rfc2822_cherry_mesg="$rfc2822_mesg
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: C.U. Thor <cuthor@example.com>"
+
+
+test_expect_success setup '
+	git config advice.detachedhead false &&
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit "$non_rfc2822_mesg" foo b non-rfc2822-base &&
+	git reset --hard initial &&
+	test_commit "$rfc2822_mesg" foo b rfc2822-base &&
+	git reset --hard initial &&
+	test_commit "$rfc2822_cherry_mesg" foo b rfc2822-cherry-base &&
+	pristine_detach initial &&
+	test_commit conflicting unrelated
+'
+
+test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer' '
+	pristine_detach initial &&
+	git cherry-pick -s non-rfc2822-base &&
+	cat <<-EOF >expect &&
+		$non_rfc2822_mesg
+
+		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 not confused by rfc2822 continuation line' '
+	pristine_detach initial &&
+	git cherry-pick -s rfc2822-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_mesg
+		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 treats -s "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	git cherry-pick -s rfc2822-cherry-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_cherry_mesg
+		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.0

[PATCH 3/5] sequencer.c: handle rfc2822 continuation lines correctly

[Brandon Casey]

ends_rfc2822_footer() was incorrectly checking whether the current line
was a continuation of the previous line.  It was actually checking the
next line instead of the current line.  Let's fix this and mark the test
as expect_success.

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

diff --git a/sequencer.c b/sequencer.c
index be0cb8b..01edec2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1040,7 +1040,7 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if ((buf[k] == ' ' || buf[k] == '\t') && !first)
+		if ((buf[i] == ' ' || buf[i] == '\t') && !first)
 			continue;
 
 		first = 0;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index b4e5c65..b2098e0 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -52,7 +52,7 @@ test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer'
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick -s not confused by rfc2822 continuation line' '
+test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
 	pristine_detach initial &&
 	git cherry-pick -s rfc2822-base &&
 	cat <<-EOF >expect &&
-- 
1.8.0

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

[Brandon Casey]

Currently, if the s-o-b footer of a commit message contains a
"(cherry picked from ..." line that was added by a previous cherry-pick -x,
it is not recognized as a s-o-b footer and will cause a newline to be
inserted before an additional s-o-b is added.

So, rework ends_rfc2822_footer to recognize the "(cherry picked from ..."
string as part of the footer.  Plus mark the test in t3511 as fixed.

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

diff --git a/sequencer.c b/sequencer.c
index 01edec2..27e684c 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: ";
+const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 static void remove_sequencer_state(void)
 {
@@ -492,7 +493,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");
 		}
@@ -1017,13 +1018,34 @@ 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_pick_from_line(const char *buf, int len)
+{
+	return (strlen(cherry_picked_prefix) + 41) <= len &&
+		!prefixcmp(buf, cherry_picked_prefix);
+}
+
 static int ends_rfc2822_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;
-	int first = 1;
+	int last_was_rfc2822 = 0;
 	const char *buf = sb->buf;
 
 	for (i = len - 1; i > 0; i--) {
@@ -1040,20 +1062,12 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if ((buf[i] == ' ' || buf[i] == '\t') && !first)
+		if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
 			continue;
 
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
+		if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
+			is_cherry_pick_from_line(buf+i, k-i)))
 			return 0;
-		}
 	}
 	return 1;
 }
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index b2098e0..785486e 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -63,7 +63,7 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
 	pristine_detach initial &&
 	git cherry-pick -s rfc2822-cherry-base &&
 	cat <<-EOF >expect &&
-- 
1.8.0

[PATCH/RFC 5/5] 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.

Also, introduce tests to test this functionality.

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


This seems like the right thing to do, but it's more of a change in
policy than the others, so I marked it as RFC.  Any disagreement here?

-Brandon


 sequencer.c              | 110 ++++++++++++++++++++++++-----------------------
 t/t3511-cherry-pick-x.sh |  77 +++++++++++++++++++++++++++++++++
 2 files changed, 133 insertions(+), 54 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 27e684c..0da0538 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,60 @@
 const char sign_off_header[] = "Signed-off-by: ";
 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_pick_from_line(const char *buf, int len)
+{
+	return (strlen(cherry_picked_prefix) + 41) <= len &&
+		!prefixcmp(buf, cherry_picked_prefix);
+}
+
+static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+{
+	int hit = 0;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	int last_was_rfc2822 = 0;
+	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 (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
+			continue;
+
+		if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
+			is_cherry_pick_from_line(buf+i, k-i)))
+			return 0;
+	}
+	return 1;
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -493,6 +547,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
+			if (!ends_rfc2822_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");
@@ -1018,60 +1074,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_pick_from_line(const char *buf, int len)
-{
-	return (strlen(cherry_picked_prefix) + 41) <= len &&
-		!prefixcmp(buf, cherry_picked_prefix);
-}
-
-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
-{
-	int hit = 0;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	int last_was_rfc2822 = 0;
-	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 (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
-			continue;
-
-		if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
-			is_cherry_pick_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 785486e..af7a87c 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -40,6 +40,19 @@ test_expect_success setup '
 	test_commit conflicting unrelated
 '
 
+test_expect_success 'cherry-pick -x inserts blank line after non-rfc2822 footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse non-rfc2822-base^0` &&
+	git cherry-pick -x non-rfc2822-base &&
+	cat <<-EOF >expect &&
+		$non_rfc2822_mesg
+
+		(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 non-rfc2822 footer' '
 	pristine_detach initial &&
 	git cherry-pick -s non-rfc2822-base &&
@@ -52,6 +65,32 @@ test_expect_success 'cherry-pick -s inserts blank line after non-rfc2822 footer'
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s inserts blank line after non-rfc2822 footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse non-rfc2822-base^0` &&
+	git cherry-pick -x -s non-rfc2822-base &&
+	cat <<-EOF >expect &&
+		$non_rfc2822_mesg
+
+		(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 not confused by rfc2822 continuation line' '
+	pristine_detach initial &&
+	sha1=`git rev-parse rfc2822-base^0` &&
+	git cherry-pick -x rfc2822-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_mesg
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
 	pristine_detach initial &&
 	git cherry-pick -s rfc2822-base &&
@@ -63,6 +102,31 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s not confused by rfc2822 continuation line' '
+	pristine_detach initial &&
+	sha1=`git rev-parse rfc2822-base^0` &&
+	git cherry-pick -x -s rfc2822-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_mesg
+		(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 treats -x "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse rfc2822-cherry-base^0` &&
+	git cherry-pick -x rfc2822-cherry-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_cherry_mesg
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
 	pristine_detach initial &&
 	git cherry-pick -s rfc2822-cherry-base &&
@@ -74,4 +138,17 @@ test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick treats -x -s "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse rfc2822-cherry-base^0` &&
+	git cherry-pick -x -s rfc2822-cherry-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_cherry_mesg
+		(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.0

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

[Matt Kraai]

On Wed, Nov 14, 2012 at 05:37:50PM -0800, Brandon Casey wrote:
> -# Both <file> and <contents> default to <message>.
> +# Both <file> <contents> and <tag> default to <message>.

I think this line would be better as

 # <file>, <contents>, and <tag> all default to <message>.

since there's now more than two arguments that default to message.

-- 
Matt

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

[Brandon Casey]

Good eye.  Thanks.

On Wed, Nov 14, 2012 at 7:20 PM, Matt Kraai <kraai@ftbfs.org> wrote:
> On Wed, Nov 14, 2012 at 05:37:50PM -0800, Brandon Casey wrote:
>> -# Both <file> and <contents> default to <message>.
>> +# Both <file> <contents> and <tag> default to <message>.
>
> I think this line would be better as
>
>  # <file>, <contents>, and <tag> all default to <message>.
>
> since there's now more than two arguments that default to message.
>
> --
> Matt

[PATCH 1/5 v2] 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 8889ba5..9e2b8b8 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.0

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

[Brandon Casey]

Currently, if the s-o-b footer of a commit message contains a
"(cherry picked from ..." line that was added by a previous cherry-pick -x,
it is not recognized as a s-o-b footer and will cause a newline to be
inserted before an additional s-o-b is added.

So, rework ends_rfc2822_footer to recognize the "(cherry picked from ..."
string as part of the footer.  Plus mark the test in t3511 as fixed.

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

Declare cherry_picked_prefix variable as static.  This is the only change
with respect to v1.

-Brandon

 sequencer.c              | 44 +++++++++++++++++++++++++++++---------------
 t/t3511-cherry-pick-x.sh |  2 +-
 2 files changed, 30 insertions(+), 16 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 01edec2..213fa4f 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)
 {
@@ -492,7 +493,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");
 		}
@@ -1017,13 +1018,34 @@ 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_pick_from_line(const char *buf, int len)
+{
+	return (strlen(cherry_picked_prefix) + 41) <= len &&
+		!prefixcmp(buf, cherry_picked_prefix);
+}
+
 static int ends_rfc2822_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;
-	int first = 1;
+	int last_was_rfc2822 = 0;
 	const char *buf = sb->buf;
 
 	for (i = len - 1; i > 0; i--) {
@@ -1040,20 +1062,12 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if ((buf[i] == ' ' || buf[i] == '\t') && !first)
+		if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
 			continue;
 
-		first = 0;
-
-		for (j = 0; i + j < len; j++) {
-			ch = buf[i + j];
-			if (ch == ':')
-				break;
-			if (isalnum(ch) ||
-			    (ch == '-'))
-				continue;
+		if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
+			is_cherry_pick_from_line(buf+i, k-i)))
 			return 0;
-		}
 	}
 	return 1;
 }
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index b2098e0..785486e 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -63,7 +63,7 @@ test_expect_success 'cherry-pick -s not confused by rfc2822 continuation line' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
+test_expect_success 'cherry-pick treats -s "(cherry picked from..." line as part of footer' '
 	pristine_detach initial &&
 	git cherry-pick -s rfc2822-cherry-base &&
 	cat <<-EOF >expect &&
-- 
1.8.0

[PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines

[Brandon Casey]

Detect whether the s-o-b already exists in the commit footer and refrain
from adding a duplicate.

Update t3511 to test new behavior.

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


Hi Duy,

How about this patch on top of my series as a base for your patch to
unify the code paths that append signoff in format-patch, commit, and
sequencer?

-Brandon


 sequencer.c              | 28 ++++++++++++++++++----------
 t/t3511-cherry-pick-x.sh | 20 ++++++++++++++++++--
 2 files changed, 36 insertions(+), 12 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 7ad1163..546dacb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -42,13 +42,15 @@ static int is_cherry_pick_from_line(const char *buf, int len)
 		!prefixcmp(buf, cherry_picked_prefix);
 }
 
-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
+static int ends_rfc2822_footer(struct strbuf *sb, struct strbuf *sob,
+	int ignore_footer)
 {
 	int hit = 0;
 	int i, k;
 	int len = sb->len - ignore_footer;
 	int last_was_rfc2822 = 0;
 	const char *buf = sb->buf;
+	int found_sob = 0;
 
 	for (i = len - 1; i > 0; i--) {
 		if (hit && buf[i] == '\n')
@@ -66,12 +68,15 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 
 		if (last_was_rfc2822 && (buf[i] == ' ' || buf[i] == '\t'))
 			continue;
+		if ((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) &&
+			sob && !found_sob &&
+			!strncasecmp(buf+i, sob->buf, sob->len))
+			found_sob = 1;
 
-		if (!((last_was_rfc2822 = is_rfc2822_line(buf+i, k-i)) ||
-			is_cherry_pick_from_line(buf+i, k-i)))
+		if (!(last_was_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
 			return 0;
 	}
-	return 1;
+	return 1 + found_sob;
 }
 
 static void remove_sequencer_state(void)
@@ -547,7 +552,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
-			if (!ends_rfc2822_footer(&msgbuf, 0))
+			if (!ends_rfc2822_footer(&msgbuf, NULL, 0))
 				strbuf_addch(&msgbuf, '\n');
 			strbuf_addstr(&msgbuf, cherry_picked_prefix);
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
@@ -1077,6 +1082,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 {
 	struct strbuf sob = STRBUF_INIT;
+	int has_footer = 0;
 	int i;
 
 	strbuf_addstr(&sob, sign_off_header);
@@ -1085,10 +1091,12 @@ 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 || !ends_rfc2822_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 =
+		ends_rfc2822_footer(msgbuf, &sob, ignore_footer)))
+			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				"\n", 1);
+	if (has_footer != 2)
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf,
+			sob.len);
 	strbuf_release(&sob);
 }
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index af7a87c..a15b199 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -11,9 +11,10 @@ pristine_detach () {
 	git clean -d -f -f -q -x
 }
 
-non_rfc2822_mesg='base with footer
+non_rfc2822_mesg="base with footer
 
-Commit message body is here.'
+Commit message body is here.
+Not an s-o-b Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 
 rfc2822_mesg="$non_rfc2822_mesg
 
@@ -25,6 +26,9 @@ rfc2822_cherry_mesg="$rfc2822_mesg
 (cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
 Tested-by: C.U. Thor <cuthor@example.com>"
 
+rfc2822_cherry_sob_mesg="$rfc2822_cherry_mesg
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+Signed-off-by: C.U. Thor <cuthor@example.com>"
 
 test_expect_success setup '
 	git config advice.detachedhead false &&
@@ -36,6 +40,8 @@ test_expect_success setup '
 	test_commit "$rfc2822_mesg" foo b rfc2822-base &&
 	git reset --hard initial &&
 	test_commit "$rfc2822_cherry_mesg" foo b rfc2822-cherry-base &&
+	git reset --hard initial &&
+	test_commit "$rfc2822_cherry_sob_mesg" foo b rfc2822-cherry-sob-base &&
 	pristine_detach initial &&
 	test_commit conflicting unrelated
 '
@@ -151,4 +157,14 @@ test_expect_success 'cherry-pick treats -x -s "(cherry picked from..." line as p
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -s detects committer s-o-b already exists' '
+	pristine_detach initial &&
+	git cherry-pick -s rfc2822-cherry-sob-base &&
+	cat <<-EOF >expect &&
+		$rfc2822_cherry_sob_mesg
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.0

Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> The cherry-pick -s functionality is currently broken in two ways.
>
>    1. handling of rfc2822 continuation lines has a bug, and the
>       continuation lines are not handled correctly.

This is not limited to you, but people should think twice when
writing "has a bug" and "are not handled correctly" in their log
message.  Did you write what the expected and actual behaviours?

> +rfc2822_mesg="$non_rfc2822_mesg
> +
> +Signed-off-by: A.U. Thor
> + <author@example.com>
> +Signed-off-by: B.U. Thor <buthor@example.com>"

The S-o-b: lines are meant to record people's contact info in human
readable forms, and folding the lines like the above makes it a lot
harder to read.  They typically do not have to be folded.

Besides, the footer lines are *not* RFC2822 headers (and are not
used as such when send-email comes up with Cc: list) in the first
place; have we ever said anything about supporting the RFC2822 line
folding in the commit footer?  If not (and I am reasonably sure we
never have), I personally think we should actively *discourage* line
folding there.

>       i.e. we should produce this:
>
>          Signed-off-by: A.U. Thor <author@example.com>
>          (cherry picked from )
>          Signed-off-by: C O Mmitter <committer@example.com>
>
>       not
>
>          Signed-off-by: A.U. Thor <author@example.com>
>          (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>
>          Signed-off-by: C O Mmitter <committer@example.com>

I can buy that, but then this makes it very clear that these footer
lines are not shaped like RFC2822 headers, no?

Thanks.

Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines

[Junio C Hamano]

Brandon Casey <drafnel@gmail.com> writes:

> Detect whether the s-o-b already exists in the commit footer and refrain
> from adding a duplicate.

If you are trying to forbid

	git cherry-pick -s $other

from adding s-o-b: A when $other ends with these two existing s-o-b:

	s-o-b: A
	s-o-b: B

then I think that is a wrong thing to do.  

In such a case, the resulting commit should gain another s-o-b from
A to record the provenance as a chain of events.  A originally wrote
the patch, B forwarded it (possibly with his own tweaks), and then A
picked it up and recorded the result to the history, possibly with a
final tweak or two.

Re: [PATCH 2/5] t/t3511: demonstrate breakage in cherry-pick -s

[Brandon Casey]

On Thu, Nov 15, 2012 at 5:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> The cherry-pick -s functionality is currently broken in two ways.
>>
>>    1. handling of rfc2822 continuation lines has a bug, and the
>>       continuation lines are not handled correctly.
>
> This is not limited to you, but people should think twice when
> writing "has a bug" and "are not handled correctly" in their log
> message.  Did you write what the expected and actual behaviours?

Yeah, I wasn't clear here.  The "bug" was that the incorrect index
variable was being used, which caused the wrong line to be examined to
see if it was an rfc2822 continuation line.  I could have mentioned
that.

>> +rfc2822_mesg="$non_rfc2822_mesg
>> +
>> +Signed-off-by: A.U. Thor
>> + <author@example.com>
>> +Signed-off-by: B.U. Thor <buthor@example.com>"
>
> The S-o-b: lines are meant to record people's contact info in human
> readable forms, and folding the lines like the above makes it a lot
> harder to read.  They typically do not have to be folded.

Well, I wasn't adding functionality here, I was only fixing what I
noticed was broken when I started touching this code.

> Besides, the footer lines are *not* RFC2822 headers (and are not
> used as such when send-email comes up with Cc: list) in the first
> place; have we ever said anything about supporting the RFC2822 line
> folding in the commit footer?  If not (and I am reasonably sure we
> never have), I personally think we should actively *discourage* line
> folding there.

That's fine with me.  I can't think of a reason that would make it
necessary to support line continuation.

>>       i.e. we should produce this:
>>
>>          Signed-off-by: A.U. Thor <author@example.com>
>>          (cherry picked from )
>>          Signed-off-by: C O Mmitter <committer@example.com>
>>
>>       not
>>
>>          Signed-off-by: A.U. Thor <author@example.com>
>>          (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
>>
>>          Signed-off-by: C O Mmitter <committer@example.com>
>
> I can buy that, but then this makes it very clear that these footer
> lines are not shaped like RFC2822 headers, no?

The lines that are _not_ "(cherry picked from ...)" lines do follow
the format defined by rfc2822 for header lines (mostly).  That's
probably why the author of the function in sequencer.c that checks for
a s-o-b footer named it "ends_rfc2822_footer".

I'll remove the *broken* existing code that was intended to support
continuation lines and submit a new patch.

-Brandon

Re: [PATCH 6/5] sequencer.c: refrain from adding duplicate s-o-b lines

[Brandon Casey]

On Thu, Nov 15, 2012 at 6:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Brandon Casey <drafnel@gmail.com> writes:
>
>> Detect whether the s-o-b already exists in the commit footer and refrain
>> from adding a duplicate.
>
> If you are trying to forbid
>
>         git cherry-pick -s $other
>
> from adding s-o-b: A when $other ends with these two existing s-o-b:
>
>         s-o-b: A
>         s-o-b: B
>
> then I think that is a wrong thing to do.
>
> In such a case, the resulting commit should gain another s-o-b from
> A to record the provenance as a chain of events.  A originally wrote
> the patch, B forwarded it (possibly with his own tweaks), and then A
> picked it up and recorded the result to the history, possibly with a
> final tweak or two.

Hmm.  I've never thought that it was necessary to add an additional
sob for the patches that I've cherry picked that I had previously
signed-off-on.  I considered one sign-off to be enough.  In your
example, A is the committer and the patch set already contains A's
sign-off.  For me that indicates that A still considers the commit to
comply with whatever s-o-b implies.

We also seem to have a few tools to help people avoid adding duplicate
sob's, like the current behavior of format-patch and the sample
commit-msg hook.

I did some quick searching through the kernel commits to try to find
some examples that could set a precedence.  I didn't find anything
that supported either argument.  I didn't see any commits that were
cherry-picked _and_ had an existing sob that was not the last sob.  I
didn't see any that had duplicate sob lines either.  I'm not
mentioning this to say that lack of a prior use should mean we should
actively disallow the practice of adding duplicate sob's, I'm just
providing it as a data point.

I've always thought that the reason that 'commit -s' and 'cherry-pick
-s' checked only the last line of the commit message was simply that a
full scan of the footer had not been implemented.

Whichever behavior is determined to be the right way for git to do it,
format-patch should be brought in-line with the others and be built on
top of the code in sequencer.c.  So, if git _should_ create duplicate
sob's then this patch should just be dropped.  Duy's unification patch
can just be built on top of sequencer.c:append_signoff() without
bringing over any of the duplicate sob detection from log-tree.c.

-Brandon