[PATCH v3 00/11] unify appending of sob

[PATCH v3 00/11] unify appending of sob

[Brandon Casey]

Round 3.

-Brandon

Brandon Casey (9):
  sequencer.c: rework search for start of footer to improve clarity
  commit, cherry-pick -s: remove broken support for multiline rfc2822
    fields
  t/test-lib-functions.sh: allow to specify the tag name to test_commit
  t/t3511: add some tests of 'cherry-pick -s' functionality
  sequencer.c: recognize "(cherry picked from ..." as part of s-o-b
    footer
  sequencer.c: 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              | 150 ++++++++++++++++++---------
 sequencer.h              |   4 +-
 t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++
 t/t4014-format-patch.sh  | 263 +++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh  |   8 +-
 9 files changed, 600 insertions(+), 153 deletions(-)
 create mode 100755 t/t3511-cherry-pick-x.sh

-- 
1.8.1.1.450.g0327af3

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

[Brandon Casey]

This code sequence is somewhat difficult to read.  Let's rewrite it
using more descriptive variable names to try to make it easier to
understand.

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

diff --git a/sequencer.c b/sequencer.c
index aef5e8a..cd211b2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1024,16 +1024,19 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 {
 	int ch;
-	int hit = 0;
+	int last_char_was_nl, this_char_is_nl;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
 	int first = 1;
 	const char *buf = sb->buf;
 
+	/* find start of last paragraph */
+	last_char_was_nl = 0;
 	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
+		this_char_is_nl = (buf[i] == '\n');
+		if (last_char_was_nl && this_char_is_nl)
 			break;
-		hit = (buf[i] == '\n');
+		last_char_was_nl = this_char_is_nl;
 	}
 
 	while (i < len - 1 && buf[i] == '\n')
-- 
1.8.1.1.450.g0327af3

Re: [PATCH v3 01/11] sequencer.c: rework search for start of footer to improve clarity

[Jonathan Nieder]

Brandon Casey wrote:

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1024,16 +1024,19 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
>  {
>  	int ch;
> -	int hit = 0;
> +	int last_char_was_nl, this_char_is_nl;
>  	int i, j, k;
>  	int len = sb->len - ignore_footer;
>  	int first = 1;
>  	const char *buf = sb->buf;
>  
> +	/* find start of last paragraph */
> +	last_char_was_nl = 0;
>  	for (i = len - 1; i > 0; i--) {
> -		if (hit && buf[i] == '\n')
> +		this_char_is_nl = (buf[i] == '\n');
> +		if (last_char_was_nl && this_char_is_nl)
>  			break;
> -		hit = (buf[i] == '\n');
> +		last_char_was_nl = this_char_is_nl;

I would have been tempted to write

	char prev;

	prev = 0;
	for (i = len - 1; i > 0; i--) {
		char ch = buf[i];
		if (prev == '\n' && ch == '\n')	/* paragraph break */
			break;
		prev = ch;
	}

but your rewrite is just as clear.  For what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Brandon Casey]

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

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

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

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

There should be no functional change.

[Thanks to Jonathan Nieder for the excellent commit message]

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

diff --git a/sequencer.c b/sequencer.c
index cd211b2..39a752b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1027,7 +1027,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 	int last_char_was_nl, this_char_is_nl;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
-	int first = 1;
 	const char *buf = sb->buf;
 
 	/* find start of last paragraph */
@@ -1047,11 +1046,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.450.g0327af3

Re: [PATCH v3 02/11] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields

[Jonathan Nieder]

Brandon Casey wrote:

>  sequencer.c | 6 ------
>  1 file changed, 6 deletions(-)

Nice simplification.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

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

[Brandon Casey]

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

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

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

[PATCH v3 04/11] 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..2a040b7
--- /dev/null
+++ b/t/t3511-cherry-pick-x.sh
@@ -0,0 +1,111 @@
+#!/bin/sh
+
+test_description='Test cherry-pick -x and -s'
+
+. ./test-lib.sh
+
+pristine_detach () {
+	git cherry-pick --quit &&
+	git checkout -f "$1^0" &&
+	git read-tree -u --reset HEAD &&
+	git clean -d -f -f -q -x
+}
+
+mesg_one_line='base: commit message'
+
+mesg_no_footer="$mesg_one_line
+
+OneWordBodyThatsNotA-S-o-B"
+
+mesg_with_footer="$mesg_no_footer
+
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+Signed-off-by: A.U. Thor <author@example.com>
+Signed-off-by: B.U. Thor <buthor@example.com>"
+
+mesg_broken_footer="$mesg_no_footer
+
+The signed-off-by string should begin with the words Signed-off-by followed
+by a colon and space, and then the signers name and email address. e.g.
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+
+mesg_with_footer_sob="$mesg_with_footer
+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
+
+
+test_expect_success setup '
+	git config advice.detachedhead false &&
+	echo unrelated >unrelated &&
+	git add unrelated &&
+	test_commit initial foo a &&
+	test_commit "$mesg_one_line" foo b mesg-one-line &&
+	git reset --hard initial &&
+	test_commit "$mesg_no_footer" foo b mesg-no-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_broken_footer" foo b mesg-broken-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_footer" foo b mesg-with-footer &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+	pristine_detach initial &&
+	test_commit conflicting unrelated
+'
+
+test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-one-line &&
+	cat <<-EOF >expect &&
+		$mesg_one_line
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-broken-footer &&
+	cat <<-EOF >expect &&
+		$mesg_broken_footer
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-footer-sob &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer_sob
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_done
-- 
1.8.1.1.450.g0327af3

Re: [PATCH v3 04/11] 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 @@
> +#!/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
> +}

Some day this should move to test-lib-functions.sh.  Not relevant
for this patch, though.

[...]
> +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>

This bug is old.  When c1e01b0c (commit: More generous accepting of
RFC-2822 footer lines, 2009-10-28) added more precise parsing of the
RFC2822 footer when deciding whether to add a newline before a new
signoff, it forgot to change the "sign-off already present" case to
match.

Thanks for the test.  The rest of the tests in this file also look
good, of course.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

[PATCH v3 05/11] 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>
Acked-by: Jonathan Nieder <jrnieder@gmail.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 39a752b..0b5cd18 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 last_char_was_nl, this_char_is_nl;
-	int i, j, k;
+	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 
@@ -1046,15 +1072,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;
 }
@@ -1071,7 +1091,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
 	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
-		if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+		if (!i || !has_conforming_footer(msgbuf, ignore_footer))
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
 	}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 2a040b7..73da182 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 
+mesg_with_cherry_footer="$mesg_with_footer_sob
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: C.U. Thor <cuthor@example.com>"
+
 
 test_expect_success setup '
 	git config advice.detachedhead false &&
@@ -47,6 +51,8 @@ test_expect_success setup '
 	test_commit "$mesg_with_footer" foo b mesg-with-footer &&
 	git reset --hard initial &&
 	test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
 	pristine_detach initial &&
 	test_commit conflicting unrelated
 '
@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer^0` &&
+	git cherry-pick -x -s mesg-with-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer-sob &&
@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer-sob^0` &&
+	git cherry-pick -x -s mesg-with-footer-sob &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer_sob
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.1.450.g0327af3

Re: [PATCH v3 05/11] 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>

Here's the tweak I suggested last time.  I think its behavior is
slightly better in the "ends with incomplete line" case because it
limits the characters examined by is_rfc2822_line() and
is_cherry_picked_from_line() not to include buf[len] (which would
presumably sometimes be '\0').

diff --git i/sequencer.c w/sequencer.c
index 0b5cd18c..fa29c7c5 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -1043,9 +1043,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 	/*
 	 * We only care that it looks roughly like (cherry picked from ...)
 	 */
-	return !prefixcmp(buf, cherry_picked_prefix) &&
-		(buf[len - 1] == ')' ||
-		 (buf[len - 1] == '\n' && buf[len - 2] == ')'));
+	return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
 }
 
 static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
@@ -1072,8 +1070,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if (!(is_rfc2822_line(buf + i, k - i) ||
-			is_cherry_picked_from_line(buf + i, k - i)))
+		if (!is_rfc2822_line(buf + i, k - i - 1) &&
+		    !is_cherry_picked_from_line(buf + i, k - i - 1))
 			return 0;
 	}
 	return 1;

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

[Jonathan Nieder]

Jonathan Nieder wrote:

> Here's the tweak I suggested last time.  I think its behavior is
> slightly better in the "ends with incomplete line" case because it
> limits the characters examined by is_rfc2822_line() and
> is_cherry_picked_from_line() not to include buf[len] (which would
> presumably sometimes be '\0').

Whoops, that revealed a subtlety --- the '\n' or '\0' is what prevents
exiting the loop in is_rfc2822_line when the line does not contain a
colon.  Here's a corrected version of the tweak, that should actually
work. :)

diff --git i/sequencer.c w/sequencer.c
index 0b5cd18c..108ea27b 100644
--- i/sequencer.c
+++ w/sequencer.c
@@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)
 	for (i = 0; i < len; i++) {
 		int ch = buf[i];
 		if (ch == ':')
+			return 1;
+		if (!isalnum(ch) && ch != '-')
 			break;
-		if (isalnum(ch) || (ch == '-'))
-			continue;
-		return 0;
 	}
-
-	return 1;
+	return 0;
 }
 
 static int is_cherry_picked_from_line(const char *buf, int len)
@@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
 	/*
 	 * We only care that it looks roughly like (cherry picked from ...)
 	 */
-	return !prefixcmp(buf, cherry_picked_prefix) &&
-		(buf[len - 1] == ')' ||
-		 (buf[len - 1] == '\n' && buf[len - 2] == ')'));
+	return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
 }
 
 static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
@@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 			; /* do nothing */
 		k++;
 
-		if (!(is_rfc2822_line(buf + i, k - i) ||
-			is_cherry_picked_from_line(buf + i, k - i)))
+		if (!is_rfc2822_line(buf + i, k - i - 1) &&
+		    !is_cherry_picked_from_line(buf + i, k - i - 1))
 			return 0;
 	}
 	return 1;

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

[Brandon Casey]

On Sun, Jan 27, 2013 at 6:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Jonathan Nieder wrote:
>
>> Here's the tweak I suggested last time.  I think its behavior is
>> slightly better in the "ends with incomplete line" case because it
>> limits the characters examined by is_rfc2822_line() and
>> is_cherry_picked_from_line() not to include buf[len] (which would
>> presumably sometimes be '\0').
>
> Whoops, that revealed a subtlety --- the '\n' or '\0' is what prevents
> exiting the loop in is_rfc2822_line when the line does not contain a
> colon.  Here's a corrected version of the tweak, that should actually
> work. :)
>
> diff --git i/sequencer.c w/sequencer.c
> index 0b5cd18c..108ea27b 100644
> --- i/sequencer.c
> +++ w/sequencer.c
> @@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)
>         for (i = 0; i < len; i++) {
>                 int ch = buf[i];
>                 if (ch == ':')
> +                       return 1;
> +               if (!isalnum(ch) && ch != '-')
>                         break;
> -               if (isalnum(ch) || (ch == '-'))
> -                       continue;
> -               return 0;
>         }
> -
> -       return 1;
> +       return 0;
>  }
>
>  static int is_cherry_picked_from_line(const char *buf, int len)
> @@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)
>         /*
>          * We only care that it looks roughly like (cherry picked from ...)
>          */
> -       return !prefixcmp(buf, cherry_picked_prefix) &&
> -               (buf[len - 1] == ')' ||
> -                (buf[len - 1] == '\n' && buf[len - 2] == ')'));
> +       return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';
>  }
>
>  static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
> @@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>                         ; /* do nothing */
>                 k++;
>
> -               if (!(is_rfc2822_line(buf + i, k - i) ||
> -                       is_cherry_picked_from_line(buf + i, k - i)))
> +               if (!is_rfc2822_line(buf + i, k - i - 1) &&
> +                   !is_cherry_picked_from_line(buf + i, k - i - 1))
>                         return 0;
>         }
>         return 1;

Looks good to me.

-Brandon

[PATCH v3 06/11] 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.

This commit is mostly a code movement, but notice that
has_conforming_footer() was modified, in addition to being moved.

Introduce tests to test this functionality.

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

diff --git a/sequencer.c b/sequencer.c
index 0b5cd18..46d51b2 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,67 @@
 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 last_char_was_nl, this_char_is_nl;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	const char *buf = sb->buf;
+
+	/* find start of last paragraph */
+	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;
+	}
+
+	/* require at least one blank line */
+	if (!last_char_was_nl || 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 +558,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,63 +1085,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 last_char_was_nl, this_char_is_nl;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-
-	/* find start of last paragraph */
-	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;
-	}
-
-	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 73da182..a845e45 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -57,6 +57,19 @@ test_expect_success setup '
 	test_commit conflicting unrelated
 '
 
+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-one-line^0` &&
+	git cherry-pick -x mesg-one-line &&
+	cat <<-EOF >expect &&
+		$mesg_one_line
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-one-line &&
@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-no-footer^0` &&
+	git cherry-pick -x mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-no-footer &&
@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-no-footer^0` &&
+	git cherry-pick -x -s mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer &&
@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
1.8.1.1.450.g0327af3

Re: [PATCH v3 06/11] sequencer.c: always separate "(cherry picked from" from commit body

[Jonathan Nieder]

Brandon Casey wrote:

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -20,6 +20,67 @@
[...]
>  static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
[...]
> +	/* require at least one blank line */
> +	if (!last_char_was_nl || buf[i] != '\n')
> +		return 0;

Makes sense.  append_signoff already added a blank line after a
one-line message

	foo: bar

because of e5138436 (builtin-commit.c: fix logic to omit empty line
before existing footers, 2009-11-06) but the logic to do so was in the
wrong place and it didn't handle its ill-formatted cousin

	foo: bar
	baz: qux

[...]
> @@ -497,6 +558,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');

What should this do in the case of an entirely blank message?
(Maybe that's too insane a case to worry about.)

[...]
> --- a/t/t3511-cherry-pick-x.sh
> +++ b/t/t3511-cherry-pick-x.sh
> @@ -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' '

Yay. :)

Thanks for a clear and well thought-out patch with thorough tests.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v3 06/11] sequencer.c: always separate "(cherry picked from" from commit body

[Brandon Casey]

On Sun, Jan 27, 2013 at 6:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c

>> @@ -497,6 +558,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');
>
> What should this do in the case of an entirely blank message?
> (Maybe that's too insane a case to worry about.)

Yeah, I think there are a number of insane cases that I think we
should just say "behavior is undefined".

  - Entirely blank message
  - Message lacking trailing newline.
  - Oneline message that is actually a signed-off-by (equal or not to
the committer's sob).
  - Message body that is entirely 1 or more newlines, or one which has
more than one trailing newline.

It's probably not worth going through contortions to handle these cases.

-Brandon

[PATCH v3 07/11] 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              | 47 +++++++++++++++++++++++++++++++++++++----------
 sequencer.h              |  4 +++-
 t/t3511-cherry-pick-x.sh |  2 +-
 4 files changed, 42 insertions(+), 13 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 38b9a9c..67ea9e7 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 46d51b2..015cb58 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 last_char_was_nl, this_char_is_nl;
 	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
+	int found_sob = 0;
 
 	/* find start of last paragraph */
 	last_char_was_nl = 0;
@@ -70,14 +78,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;
 }
 
@@ -299,7 +318,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;
@@ -558,7 +577,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));
@@ -1085,9 +1104,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 {
+	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
 	struct strbuf sob = STRBUF_INIT;
+	int has_footer = 0;
 	int i;
 
 	strbuf_addstr(&sob, sign_off_header);
@@ -1096,10 +1117,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..1fc22dc 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -6,6 +6,8 @@
 #define SEQ_TODO_FILE	"sequencer/todo"
 #define SEQ_OPTS_FILE	"sequencer/opts"
 
+#define APPEND_SIGNOFF_DEDUP (1u << 0)
+
 enum replay_action {
 	REPLAY_REVERT,
 	REPLAY_PICK
@@ -48,6 +50,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);
 
 extern const char sign_off_header[];
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);
 
 #endif
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index a845e45..f977279 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-broken-footer &&
 	cat <<-EOF >expect &&
-- 
1.8.1.1.450.g0327af3

Re: [PATCH v3 07/11] 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 replaces the previous (slightly broken) logic that checked
whether the sign-off to be appended would be redundant and puts the
fixed logic further down the call-chain next to the rest of footer
parsing.

I am not thrilled with the

	0 = no rfc-style footer
	1 = rfc-style footer, no sign-off found
	2 = rfc-style footer, sign-off found but not as last field
	3 = rfc-style footer, sign-off found as last field

convention but since it's local to sequencer.c and this logic to scan
all lines for the sign-off can probably be ripped out soon I don't
mind.

The general direction is good and the execution looks obviously
correct.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Jonathan Nieder]

Brandon Casey wrote:

> --- a/sequencer.c
> +++ b/sequencer.c
[...]
> @@ -1096,10 +1117,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);

Is this "if (i)" test needed?  I'd expect that if the message has no newlines,
has_conforming_footer() would notice that and return 0.

Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b

[Brandon Casey]

On Sun, Jan 27, 2013 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
> [...]
>> @@ -1096,10 +1117,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);
>
> Is this "if (i)" test needed?  I'd expect that if the message has no newlines,
> has_conforming_footer() would notice that and return 0.

Thanks for pointing out this awkwardness.  It should be pushed into
has_conforming_footer().

-Brandon

[PATCH v3 08/11] 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 015cb58..404b786 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1118,11 +1118,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
 	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.450.g0327af3

Re: [PATCH v3 08/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline

[Jonathan Nieder]

Brandon Casey wrote:

> Teach append_signoff to detect whether a blank line exists at the position
> that the signed-off-by line will be added, and 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.

I assume this means you're preserving historical behavior when adding
a sign-off to a commit with empty description (which is a good thing),
but what is that behavior?  Is it a deliberate choice or something
that developed by default?

[...]
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1118,11 +1118,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)
>  	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))

This is too much nesting for my small mind to keep track of.  Am I
correct in imagining the effect is the same as the following?

	has_footer = has_conforming_footer(...)

	if (!has_footer && msgbuf->buf[i] != '\n')
		strbuf_splice(...);	/* add blank line */

	if (has_footer != 3 && ...

[PATCH v3 09/11] 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.450.g0327af3

Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines

[Jonathan Nieder]

Brandon Casey wrote:

[...]
> --- 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|^$"
> +}

Is "grep -n" portable?  I didn't find any uses of it elsewhere in the
testsuite.

Style: checking exit status from format-patch, avoiding sed|grep pipeline:

	C=$(git commit-tree HEAD^ -p HEAD) &&
	git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
	awk '
		/^---$/ { exit; }
		/^Subject/ || /^Sign/ || /^$/ { print NR ":" $0 }
	' <append_signoff.patch >actual

Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Brandon Casey wrote:
>
> [...]
>> --- 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|^$"
>> +}
>
> Is "grep -n" portable?  I didn't find any uses of it elsewhere in the
> testsuite.

Yes, "-n" is in POSIX.  Even though we use it ourselves, "git grep"
supports it, too.

Any Emacs user would scream if their platform "grep" does not
support it, as it will make M-x grep (or grep-find) useless.

> Style: checking exit status from format-patch, avoiding sed|grep pipeline:
>
> 	C=$(git commit-tree HEAD^ -p HEAD) &&
> 	git format-patch --stdout --signoff $C^..$C >append_signoff.patch &&
> 	awk '
> 		/^---$/ { exit; }
> 		/^Subject/ || /^Sign/ || /^$/ { print NR ":" $0 }
> 	' <append_signoff.patch >actual

Yeah, awk/perl would be fine, too, and it is good that you pointed
out that the original was losing the exit status from format-patch.

Thanks.

Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines

[Junio C Hamano]

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

>> Is "grep -n" portable?  I didn't find any uses of it elsewhere in the
>> testsuite.
>
> Yes, "-n" is in POSIX.  Even though we use it ourselves, "git grep"
> supports it, too.

Ehh even though we *DONT* use it ourselves, ... that is.

I do not think we mind, if its use helps our test.

[PATCH v3 10/11] 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    | 17 +++++++++++++----
 revision.h    |  2 +-
 3 files changed, 15 insertions(+), 17 deletions(-)

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

Re: [PATCH v3 10/11] format-patch: update append_signoff prototype

[Jonathan Nieder]

Brandon Casey wrote:

> From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> This is a preparation step for merging with append_signoff from
> sequencer.c

Avoids a small unfreed allocation, too.  Neat.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

(On the other hand, this means more malloc churn when running
"format-patch -s" on a long series of patches, but I don't think
anyone will mind.)

[...]
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -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);

sequencer.c uses fmt_name() which uses IDENT_STRICT, too.  Phew.

[PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer

[Brandon Casey]

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

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

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

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

diff --git a/log-tree.c b/log-tree.c
index ac1cd68..c9d9a37 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,8 +9,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
-
-#define APPEND_SIGNOFF_DEDUP (1u <<0)
+#include "sequencer.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
 	putchar(')');
 }
 
-/*
- * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
- * Signed-off-by: and Acked-by: lines.
- */
-static int detect_any_signoff(char *letter, int size)
-{
-	char *cp;
-	int seen_colon = 0;
-	int seen_at = 0;
-	int seen_name = 0;
-	int seen_head = 0;
-
-	cp = letter + size;
-	while (letter <= --cp && *cp == '\n')
-		continue;
-
-	while (letter <= cp) {
-		char ch = *cp--;
-		if (ch == '\n')
-			break;
-
-		if (!seen_at) {
-			if (ch == '@')
-				seen_at = 1;
-			continue;
-		}
-		if (!seen_colon) {
-			if (ch == '@')
-				return 0;
-			else if (ch == ':')
-				seen_colon = 1;
-			else
-				seen_name = 1;
-			continue;
-		}
-		if (('A' <= ch && ch <= 'Z') ||
-		    ('a' <= ch && ch <= 'z') ||
-		    ch == '-') {
-			seen_head = 1;
-			continue;
-		}
-		/* no empty last line doesn't match */
-		return 0;
-	}
-	return seen_head && seen_name;
-}
-
-static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)
-{
-	unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;
-	static const char signed_off_by[] = "Signed-off-by: ";
-	char *signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
-					       getenv("GIT_COMMITTER_EMAIL")));
-	size_t signoff_len = strlen(signoff);
-	int has_signoff = 0;
-	char *cp;
-
-	cp = sb->buf;
-
-	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, signed_off_by))) {
-
-		has_signoff = 1;
-
-		cp += strlen(signed_off_by);
-		if (cp + signoff_len >= sb->buf + sb->len)
-			break;
-		if (strncmp(cp, signoff, signoff_len))
-			continue;
-		if (!isspace(cp[signoff_len]))
-			continue;
-		/* we already have him */
-		free(signoff);
-		return;
-	}
-
-	if (!has_signoff)
-		has_signoff = detect_any_signoff(sb->buf, sb->len);
-
-	if (!has_signoff)
-		strbuf_addch(sb, '\n');
-
-	strbuf_addstr(sb, signed_off_by);
-	strbuf_add(sb, signoff, signoff_len);
-	strbuf_addch(sb, '\n');
-	free(signoff);
-}
-
 static unsigned int digits_in_number(unsigned int number)
 {
 	unsigned int i = 10, result = 1;
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 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.450.g0327af3

Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer

[Jonathan Nieder]

Brandon Casey wrote:

> --- a/log-tree.c
> +++ b/log-tree.c
[...]
> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
>  	putchar(')');
>  }
>  
> -/*
> - * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
> - * Signed-off-by: and Acked-by: lines.
> - */

That's stricter than the test from sequencer.c.  Maybe it's worth
stealing to avoid false positives?

Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer

[Brandon Casey]

On Sun, Jan 27, 2013 at 7:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/log-tree.c
>> +++ b/log-tree.c
> [...]
>> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)
>>       putchar(')');
>>  }
>>
>> -/*
>> - * Search for "^[-A-Za-z]+: [^@]+@" pattern. It usually matches
>> - * Signed-off-by: and Acked-by: lines.
>> - */
>
> That's stricter than the test from sequencer.c.  Maybe it's worth
> stealing to avoid false positives?

No, we don't want this stricter test because it assumes that the
right-hand side of "[-A-Za-z]+:" will be an email address, so it
requires an '@' to exist.  We want to be able to support lines that do
not have email addresses on the right-hand side like:

   Bug: XXX
   Change-Id: XXX

and perhaps eventually

   Cherry-picked-from: XXX

The current series has retained the same basic test for an
rfc2822-like line that existed in sequencer.c and would interpret a
line that contains only a colon as conforming.  A follow-on patch
could require that at least one character precede the colon, but that
would be a change in behavior that is not the goal of this series.

-Brandon

Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer

[Jonathan Nieder]

Brandon Casey wrote:

>                            We want to be able to support lines that do
> not have email addresses on the right-hand side like:
>
>    Bug: XXX
>    Change-Id: XXX

Good call.

By the way, regarding what the right "--signoff" behavior is for
commit, cherry-pick, am, and format-patch:

I think the best behavior would be to check if the last signed-off-by
line (ignoring acked-by, bug, change-id, and so on lines that follow
it) matches the one to be added, and if it doesn't, add a new
sign-off.  That way, the sign-off list still would accurately describe
the path of the patch, without silliness like

	Signed-off-by: me
	Reviewed-by: someone
	Signed-off-by: me

that you mentioned.

I agree that that's orthogonal to this series and just mostly
preserving behavior (as you already do) is the right thing to do.

Thanks for noticing the edge cases.

Jonathan

Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer

[Brandon Casey]

On Mon, Feb 11, 2013 at 1:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

> By the way, regarding what the right "--signoff" behavior is for
> commit, cherry-pick, am, and format-patch:
>
> I think the best behavior would be to check if the last signed-off-by
> line (ignoring acked-by, bug, change-id, and so on lines that follow
> it) matches the one to be added, and if it doesn't, add a new
> sign-off.

This makes a lot of sense.

-Brandon

Re: [PATCH v3 00/11] unify appending of sob

[Jonathan Nieder]

Brandon Casey wrote:

> Round 3.

Thanks for a pleasant read.  My only remaining observations are
cosmetic, except for a portability question in Duy's test script, a
small behavior change when the commit message ends with an
RFC2822-style header with no trailing newline and the possibility of
tightening the pattern in sequencer.c to match the strictness of
format-patch (which could easily wait for a later patch).

Jonathan

Re: [PATCH v3 00/11] unify appending of sob

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Brandon Casey wrote:
>
>> Round 3.
>
> Thanks for a pleasant read.  My only remaining observations are
> cosmetic, except for a portability question in Duy's test script, a
> small behavior change when the commit message ends with an
> RFC2822-style header with no trailing newline and the possibility of
> tightening the pattern in sequencer.c to match the strictness of
> format-patch (which could easily wait for a later patch).
>
> Jonathan

Thanks for a quick review.  I agree that this series is getting very
close with your help.

Re: [PATCH v3 00/11] unify appending of sob

[Junio C Hamano]

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

> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> Brandon Casey wrote:
>>
>>> Round 3.
>>
>> Thanks for a pleasant read.  My only remaining observations are
>> cosmetic, except for a portability question in Duy's test script, a
>> small behavior change when the commit message ends with an
>> RFC2822-style header with no trailing newline and the possibility of
>> tightening the pattern in sequencer.c to match the strictness of
>> format-patch (which could easily wait for a later patch).
>
> Thanks for a quick review.  I agree that this series is getting very
> close with your help.

Unless Brandon and/or Jonathan wants to have another chance to
excise warts from the recorded history by rerolling the entire
series one more time, I think what we have queued is in a good
enough shape to merge to 'next' and any further improvement and fix
can be done incrementally.

OK?  Or "stop, I want to reroll"?

I'll wait for a day or two.

Re: [PATCH v3 00/11] unify appending of sob

[Brandon Casey]

On Wed, Jan 30, 2013 at 9:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jonathan Nieder <jrnieder@gmail.com> writes:
>>
>>> Brandon Casey wrote:
>>>
>>>> Round 3.
>>>
>>> Thanks for a pleasant read.  My only remaining observations are
>>> cosmetic, except for a portability question in Duy's test script, a
>>> small behavior change when the commit message ends with an
>>> RFC2822-style header with no trailing newline and the possibility of
>>> tightening the pattern in sequencer.c to match the strictness of
>>> format-patch (which could easily wait for a later patch).
>>
>> Thanks for a quick review.  I agree that this series is getting very
>> close with your help.
>
> Unless Brandon and/or Jonathan wants to have another chance to
> excise warts from the recorded history by rerolling the entire
> series one more time, I think what we have queued is in a good
> enough shape to merge to 'next' and any further improvement and fix
> can be done incrementally.
>
> OK?  Or "stop, I want to reroll"?
>
> I'll wait for a day or two.

Let's hold off so I can do another round.  I worked on this last night
and was able to simplify some things nicely.  I'll try to finish up
tonight and resubmit.

-Brandon