threads / patch / 32749

v3, 11 partsunify appending of sob

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

## tl;dr

36 messages between Jan 28, 2013 and Feb 11, 2013. Diffs are folded; open one to read it.

replies: 35people: 3as markdown or json

Brandon Casey· Jan 28, 2013, 01:11 UTC · lore
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
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to sequencer.c +6 −3
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
Jonathan Nieder· Jan 28, 2013, 01:27 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
Show 22 quoted lines
> --- 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>
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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

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

It even included support for accepting indented continuation lines for multiline fields. Unfortunately the multiline field support is broken because it checks whether buf[k] (the first character of the *next* line) instead of buf[i] is a whitespace character. 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(-)
Show changes to sequencer.c +0 −6
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
Jonathan Nieder· Jan 28, 2013, 01:29 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
>  sequencer.c | 6 ------
>  1 file changed, 6 deletions(-)
Nice simplification.
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to t/test-lib-functions.sh +4 −4
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
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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
Show changes to t/t3511-cherry-pick-x.sh +111 −0
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
Jonathan Nieder· Jan 28, 2013, 02:08 UTC · re: Brandon Casey · lore

Re: [PATCH v3 04/11] t/t3511: add some tests of 'cherry-pick -s' functionality

Brandon Casey wrote:
Show 15 quoted lines
> --- /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.

[...]
Show 7 quoted lines
> +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>
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to 2 files +88 −13

sequencer.c, t/t3511-cherry-pick-x.sh

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
Jonathan Nieder· Jan 28, 2013, 02:21 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
Show 13 quoted lines
> 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').

Show changes to diff +3 −5
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;
Jonathan Nieder· Jan 28, 2013, 02:51 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder wrote:
Show 5 quoted lines
> 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. :)

Show changes to diff +6 −10
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;
Brandon Casey· Jan 28, 2013, 05:38 UTC · re: Jonathan Nieder · lore

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

On Sun, Jan 27, 2013 at 6:51 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 56 quoted lines
> 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
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to 2 files +116 −57

sequencer.c, t/t3511-cherry-pick-x.sh

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
Jonathan Nieder· Jan 28, 2013, 02:34 UTC · re: Brandon Casey · lore

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

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
[...]
Show 6 quoted lines
> @@ -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.)

[...]
Show 7 quoted lines
> --- 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>
Brandon Casey· Feb 10, 2013, 23:25 UTC · re: Jonathan Nieder · lore

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

On Sun, Jan 27, 2013 at 6:34 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Brandon Casey wrote:
>
>> --- a/sequencer.c
>> +++ b/sequencer.c
Show 9 quoted lines
>> @@ -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
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to 4 files +42 −13

builtin/commit.c, sequencer.c, sequencer.h, t/t3511-cherry-pick-x.sh

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
Jonathan Nieder· Jan 28, 2013, 03:08 UTC · re: Brandon Casey · lore

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

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>
Jonathan Nieder· Jan 28, 2013, 03:14 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
> --- a/sequencer.c
> +++ b/sequencer.c
[...]
Show 12 quoted lines
> @@ -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.

Brandon Casey· Feb 10, 2013, 23:36 UTC · re: Jonathan Nieder · lore

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

On Sun, Jan 27, 2013 at 7:14 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 20 quoted lines
> 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
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to sequencer.c +9 −5
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
Jonathan Nieder· Jan 28, 2013, 03:23 UTC · re: Brandon Casey · lore

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

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?

[...]
Show 22 quoted lines
> --- 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 && ...
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(+)
Show changes to t/t4014-format-patch.sh +242 −0
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
Jonathan Nieder· Jan 28, 2013, 03:31 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
[...]
Show 14 quoted lines
> --- 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
Junio C Hamano· Jan 28, 2013, 05:42 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 20 quoted lines
> 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.

Show 8 quoted lines
> 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.
Junio C Hamano· Jan 28, 2013, 05:51 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 5 quoted lines
>> 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.
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to 3 files +15 −17

builtin/log.c, log-tree.c, revision.h

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
Jonathan Nieder· Jan 28, 2013, 03:35 UTC · re: Brandon Casey · lore

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

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

[...]
Show 10 quoted lines
> --- 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.
Brandon Casey· Jan 28, 2013, 01:11 UTC · re: Brandon Casey · lore

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

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(-)
Show changes to 2 files +27 −95

log-tree.c, t/t4014-format-patch.sh

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
Jonathan Nieder· Jan 28, 2013, 03:39 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
> --- a/log-tree.c
> +++ b/log-tree.c
[...]
Show 8 quoted lines
> @@ -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?

Brandon Casey· Feb 10, 2013, 23:55 UTC · re: Jonathan Nieder · lore

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

On Sun, Jan 27, 2013 at 7:39 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 16 quoted lines
> 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
Jonathan Nieder· Feb 11, 2013, 09:00 UTC · re: Brandon Casey · lore

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

Brandon Casey wrote:
Show 5 quoted lines
>                            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
Brandon Casey· Feb 11, 2013, 18:49 UTC · re: Jonathan Nieder · lore

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

On Mon, Feb 11, 2013 at 1:00 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 7 quoted lines
> 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
Jonathan Nieder· Jan 28, 2013, 03:48 UTC · re: Brandon Casey · lore

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

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
Junio C Hamano· Jan 28, 2013, 05:49 UTC · re: Jonathan Nieder · lore

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

Jonathan Nieder <jrnieder@gmail.com> writes:
Show 12 quoted lines
> 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.

Junio C Hamano· Jan 30, 2013, 17:37 UTC · re: Junio C Hamano · lore

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

Junio C Hamano <gitster@pobox.com> writes:
Show 15 quoted lines
> 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.
Brandon Casey· Jan 31, 2013, 18:45 UTC · re: Junio C Hamano · lore

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

On Wed, Jan 30, 2013 at 9:37 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 27 quoted lines
> 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

← back to recent threads