threads / patch / 32196

patch, 11 partsalternative unify appending of sob

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

## tl;dr

18 messages between Nov 26, 2012 and Nov 29, 2012. Diffs are folded; open one to read it.

replies: 17people: 4as markdown or json

Brandon Casey· Nov 26, 2012, 01:45 UTC · lore
From: Brandon Casey <bcasey@nvidia.com>

I hate to have the battle of the patches, but I kinda prefer the append_signoff code in sequencer.c over the code in log-tree.c as a foundation to build on.

So, this series is similar to Duy's "unification" series, but it goes in the opposite direction and builds on top of sequencer.c and additionally adds the elements from my original series to treat the "(cherry picked from..." line added by 'cherry-pick -x' in the same way that other footer elements are treated (after addressing Junio's comments about rfc2822 continuation lines and duplicate s-o-b's).

I've integrated Duy's series with a few minor tweaks. I added a couple of additional tests to t4014 and corrected one of the tests which had incorrect behavior. I think his sign-off's should still be valid, so I kept them in. Sorry that I've been slow, and now the two of us are stepping on each other's toes, but Duy please take a look and let me know if there's anything you dislike.

The main difference between this series and Duy's, aside from the change in behavior with respect to a "(cherry picked from...)" line, is that the detection of a s-o-b footer is done in a somewhat more generic way. The log-tree.c code expects to find something that looks like "[-A-Za-z]+:" on the left-hand side, and something that looks like an email address on the right-hand side. The sequencer.c code relaxes this definition so that an email address is not required on the right hand side. This allows lines like "Change-id: IXXXX" or "Bug: XXX" to be considered valid footer elements.

So, this series should result in s-o-b and "(cherry picked from...)" being appended to commit messages in a more consistent and deterministic way. For example, the decision about whether to insert a blank line before appending a s-o-b. As discussed, cherry-pick and commit will only refrain from appending a s-o-b if the committer's s-o-b appears as the last one in a conforming footer, while format-patch will refrain from appending it if it appears anywhere within the footer.

-Brandon
Brandon Casey (8):
  sequencer.c: remove broken support for rfc2822 continuation in footer
  t/test-lib-functions.sh: allow to specify the tag name to test_commit
  t/t3511: add some tests of 'cherry-pick -s' functionality
  sequencer.c: recognize "(cherry picked from ..." as part of s-o-b
    footer
  sequencer.c: always separate "(cherry picked from" from commit body
  sequencer.c: teach append_signoff how to detect duplicate s-o-b
  sequencer.c: teach append_signoff to avoid adding a duplicate newline
  Unify appending signoff in format-patch, commit and sequencer
Nguyễn Thái Ngọc Duy (3):
  t4014: more tests about appending s-o-b lines
  format-patch: stricter S-o-b detection
  format-patch: update append_signoff prototype
 builtin/commit.c         |   2 +-
 builtin/log.c            |  13 +--
 log-tree.c               |  92 ++---------------
 revision.h               |   2 +-
 sequencer.c              | 133 +++++++++++++++---------
 sequencer.h              |   2 +-
 t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++
 t/t4014-format-patch.sh  | 262 +++++++++++++++++++++++++++++++++++++++++++++++
 t/test-lib-functions.sh  |   9 +-
 9 files changed, 580 insertions(+), 154 deletions(-)
 create mode 100755 t/t3511-cherry-pick-x.sh
-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

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

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

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

Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 sequencer.c | 6 ------
 1 file changed, 6 deletions(-)
Show changes to sequencer.c +0 −6
diff --git a/sequencer.c b/sequencer.c
index 2260490..656df6b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1023,7 +1023,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)
 	int hit = 0;
 	int i, j, k;
 	int len = sb->len - ignore_footer;
-	int first = 1;
 	const char *buf = sb->buf;
 
 	for (i = len - 1; i > 0; i--) {
@@ -1040,11 +1039,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.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 02/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>
---
 t/test-lib-functions.sh | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Show changes to t/test-lib-functions.sh +5 −4
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 8889ba5..9e2b8b8 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -135,12 +135,13 @@ test_pause () {
 	fi
 }
 
-# Call test_commit with the arguments "<message> [<file> [<contents>]]"
+# Call test_commit with the arguments "<message> [<file> [<contents> [<tag>]]]"
 #
 # This will commit a file with the given contents and the given commit
-# message.  It will also add a tag with <message> as name.
+# message.  It will also add a tag with <message> as name unless <tag> is
+# given.
 #
-# Both <file> and <contents> default to <message>.
+# <file>, <contents>, and <tag> all default to <message>.
 
 test_commit () {
 	notick= &&
@@ -168,7 +169,7 @@ test_commit () {
 		test_tick
 	fi &&
 	git commit $signoff -m "$1" &&
-	git tag "$1"
+	git tag "${4:-$1}"
 }
 
 # Call test_merge with the arguments "<message> <commit>", where <commit>

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

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

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 04/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 Mitter <committer@example.com>
instead of this:
   Signed-off-by: A U Thor <author@example.com>
   (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)
   Signed-off-by: C O Mitter <committer@example.com>
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 sequencer.c              | 42 ++++++++++++++++++++++++------------
 t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 84 insertions(+), 13 deletions(-)
Show changes to 2 files +84 −13

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

diff --git a/sequencer.c b/sequencer.c
index 656df6b..19eaf11 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -18,6 +18,7 @@
 #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
 
 const char sign_off_header[] = "Signed-off-by: ";
+static const char cherry_picked_prefix[] = "(cherry picked from commit ";
 
 static void remove_sequencer_state(void)
 {
@@ -492,7 +493,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
-			strbuf_addstr(&msgbuf, "(cherry picked from commit ");
+			strbuf_addstr(&msgbuf, cherry_picked_prefix);
 			strbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));
 			strbuf_addstr(&msgbuf, ")\n");
 		}
@@ -1017,11 +1018,32 @@ 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_pick_from_line(const char *buf, int len)
+{
+	return (strlen(cherry_picked_prefix) + 41) <= len &&
+		!prefixcmp(buf, cherry_picked_prefix);
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
 {
-	int ch;
 	int hit = 0;
-	int i, j, k;
+	int i, k;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
 
@@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))
 			return 0;
-		}
 	}
 	return 1;
 }
@@ -1064,7 +1080,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
 	if (prefixcmp(msgbuf->buf + i, sob.buf)) {
-		if (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))
+		if (!i || !has_conforming_footer(msgbuf, ignore_footer))
 			strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);
 	}
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index a6c4168..32c0bb1 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 mesg_with_footer_sob="$mesg_with_footer
 Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>"
 
+mesg_with_cherry_footer="$mesg_with_footer_sob
+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)
+Tested-by: C.U. Thor <cuthor@example.com>"
+
 
 test_expect_success setup '
 	git config advice.detachedhead false &&
@@ -47,6 +51,8 @@ test_expect_success setup '
 	test_commit "$mesg_with_footer" foo b mesg-with-footer &&
 	git reset --hard initial &&
 	test_commit "$mesg_with_footer_sob" foo b mesg-with-footer-sob &&
+	git reset --hard initial &&
+	test_commit "$mesg_with_cherry_footer" foo b mesg-with-cherry-footer &&
 	pristine_detach initial &&
 	test_commit conflicting unrelated
 '
@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer^0` &&
+	git cherry-pick -x -s mesg-with-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer-sob &&
@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-footer-sob^0` &&
+	git cherry-pick -x -s mesg-with-footer-sob &&
+	cat <<-EOF >expect &&
+		$mesg_with_footer_sob
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -x treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	git cherry-pick -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.8.0.284.g959048a
Junio C Hamano· Nov 29, 2012, 00:02 UTC · re: Brandon Casey · lore

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

Brandon Casey <drafnel@gmail.com> writes:
Show 50 quoted lines
> -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_pick_from_line(const char *buf, int len)
> +{
> +	return (strlen(cherry_picked_prefix) + 41) <= len &&
> +		!prefixcmp(buf, cherry_picked_prefix);
> +}
> +
> +static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>  {
> -	int ch;
>  	int hit = 0;
> -	int i, j, k;
> +	int i, k;
>  	int len = sb->len - ignore_footer;
>  	const char *buf = sb->buf;
>  
> @@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))
>  			return 0;
> -		}
>  	}
>  	return 1;
>  }

Refactored code looks vastly more readable, but I think the is_cherry_pick_from_line() function (by the way, shouldn't it be named is_cherry_picked_from_line()?) shows its ambivalence. It insists that the line has to be long enough to hold 40-hex object name plus a closing parenthesis, but it only makes sure that the prefix matches, without checking if the line has 40-hex object name, or the object name is immediately followed by a closing parenthesis. It also does not care if there are other garbage after it.

If the code is trying to be strict to avoid misidentification, then the check should be tightened (i.e. require the known fixed length, make sure get_sha1_hex() is happy, 41st byte is a close parenthesis that is followed by the end of line). If the code is trying to be more lenient to allow people hand-editing the cherry-picked-from line that was generated, the check could be loosened (people may truncate the 40-hex down to 12-hex or something).

I cannot read from this code which one was intended; the code must make up its mind, whichever way (I do not have a strong preference).

> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '

Is the title of this test appropriate? It looks like it is making sure we do not add an extra blank line after the existing footer to me.

Show 12 quoted lines
> +	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
> +'
> +
Show 12 quoted lines
> +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
> +'

For people on the sideline, $mesg_with_footer_sob ends with s-o-b by the same "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" we are adding here. This is probably a sensible thing to do.

One thing I am not so happy about this whole "(cherry picked from" thing (and I am again agreeing with Linus who made me change the default long time ago not to add this line by default) is this. If you use "cherry-pick -s -x $commit" to transplant a commit from an unrelated part of the history, you will get the object name in the resulting commit. But you can do the same in at least two different ways. The easiest is:

    git format-patch -1 --stdout $commit | git am -s3
and a bit closer to what "cherry-pick" does is:
    git checkout $commit^0 &&
    git rebase --onto @{-1} HEAD^ &&
    git checkout -B @{-1}
i.e. rebase the single commit on top of the branch you were on.

In either way, you don't get "cherry picked from", even though you just did the moral equivalent of it. Also we need to realize that the first one is essentially what happens all the time here; the "|" is implemented by the mailing list. And nobody misses "cherry picked from" to point at the original commit object, which is useless for the recipient of the resulting commit.

But that is an orthogonal issue.
Brandon Casey· Nov 29, 2012, 01:28 UTC · re: Junio C Hamano · lore

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

On 11/28/2012 4:02 PM, Junio C Hamano wrote:
Show 56 quoted lines
> Brandon Casey <drafnel@gmail.com> writes:
>
>> -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_pick_from_line(const char *buf, int len)
>> +{
>> +	return (strlen(cherry_picked_prefix) + 41) <= len &&
>> +		!prefixcmp(buf, cherry_picked_prefix);
>> +}
>> +
>> +static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
>>   {
>> -	int ch;
>>   	int hit = 0;
>> -	int i, j, k;
>> +	int i, k;
>>   	int len = sb->len - ignore_footer;
>>   	const char *buf = sb->buf;
>>
>> @@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))
>>   			return 0;
>> -		}
>>   	}
>>   	return 1;
>>   }
>
> Refactored code looks vastly more readable, but I think the
> is_cherry_pick_from_line() function (by the way, shouldn't it be
> named is_cherry_picked_from_line()?
Yes.
Show 17 quoted lines
> ) shows its ambivalence.  It
> insists that the line has to be long enough to hold 40-hex object
> name plus a closing parenthesis, but it only makes sure that the
> prefix matches, without checking if the line has 40-hex object name,
> or the object name is immediately followed by a closing parenthesis.
> It also does not care if there are other garbage after it.
>
> If the code is trying to be strict to avoid misidentification, then
> the check should be tightened (i.e. require the known fixed length,
> make sure get_sha1_hex() is happy, 41st byte is a close parenthesis
> that is followed by the end of line).  If the code is trying to be
> more lenient to allow people hand-editing the cherry-picked-from
> line that was generated, the check could be loosened (people may
> truncate the 40-hex down to 12-hex or something).
>
> I cannot read from this code which one was intended; the code must
> make up its mind, whichever way (I do not have a strong preference).

The intention was a stricter-type match, but implemented somewhat lazily. Part of me doesn't think that anyone should need to modify the string that 'cherry-pick -x' adds and that a strict match is appropriate. The other part of me doesn't want to reject a line that looks like "(cherry picked from ...)" line that has a trimmed sha1 inside.

I think I'll submit a somewhat loosened check.
Show 5 quoted lines
>> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '
>
> Is the title of this test appropriate?  It looks like it is making
> sure we do not add an extra blank line after the existing footer to
> me.

It's really just part of the series of tests that checks for correct operation when "the last sob doesn't match committer". This test has a few elements in it:

    * existing sob footer contains the committer's sob, but not last.
      ensure that we don't refrain from appending a sob just because
      one already exists in the footer.
    * ensure extra blank isn't inserted after existing footer and
      "(cherry picked from..."
    * ensure a blank isn't inserted between "(cherry picked from..."
      and new sob

The title of the test is me trying to fit "correctly adds \"(cherry picked from...\" and sob when footer contains committer's sob but not last" within a single line.

Show 56 quoted lines
>> +	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 -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
>> +'
>
> For people on the sideline, $mesg_with_footer_sob ends with s-o-b by
> the same "$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>" we are adding
> here.  This is probably a sensible thing to do.
>
> One thing I am not so happy about this whole "(cherry picked from"
> thing (and I am again agreeing with Linus who made me change the
> default long time ago not to add this line by default) is this.  If
> you use "cherry-pick -s -x $commit" to transplant a commit from an
> unrelated part of the history, you will get the object name in the
> resulting commit.  But you can do the same in at least two different
> ways.  The easiest is:
>
>      git format-patch -1 --stdout $commit | git am -s3
>
> and a bit closer to what "cherry-pick" does is:
>
>      git checkout $commit^0 &&
>      git rebase --onto @{-1} HEAD^ &&
>      git checkout -B @{-1}
>
> i.e. rebase the single commit on top of the branch you were on.
>
> In either way, you don't get "cherry picked from", even though you
> just did the moral equivalent of it.  Also we need to realize that
> the first one is essentially what happens all the time here; the "|"
> is implemented by the mailing list.  And nobody misses "cherry
> picked from" to point at the original commit object, which is
> useless for the recipient of the resulting commit.
>
> But that is an orthogonal issue.
Are you suggesting that people shouldn't use 'cherry-pick -x' at all?

I agree that it is useless to use -x when the commit that will be referenced will not be available as part of the permanent history of the project.

I do however think it is useful to add a reference to the original commit when back- or forward- porting a commit. I think it adds a little visibility to the commit to say that it was not originally implemented on top of the base it is built on. I find it helpful to examine the original implementation (the referenced commit) when verifying the correctness or attempting to understand the ported commit.

-Brandon

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

Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 05/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.

Introduce tests to test this functionality.
Signed-off-by: Brandon Casey <bcasey@nvidia.com>
---
 sequencer.c              | 106 +++++++++++++++++++++++++----------------------
 t/t3511-cherry-pick-x.sh |  53 ++++++++++++++++++++++++
 2 files changed, 109 insertions(+), 50 deletions(-)
Show changes to 2 files +109 −50

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

diff --git a/sequencer.c b/sequencer.c
index 19eaf11..7c0852a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -20,6 +20,60 @@
 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_pick_from_line(const char *buf, int len)
+{
+	return (strlen(cherry_picked_prefix) + 41) <= len &&
+		!prefixcmp(buf, cherry_picked_prefix);
+}
+
+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+{
+	int hit = 0;
+	int i, k;
+	int len = sb->len - ignore_footer;
+	const char *buf = sb->buf;
+
+	for (i = len - 1; i > 0; i--) {
+		if (hit && buf[i] == '\n')
+			break;
+		hit = (buf[i] == '\n');
+	}
+
+	/* require at least one blank line */
+	if (!hit || buf[i] != '\n')
+		return 0;
+
+	while (i < len - 1 && buf[i] == '\n')
+		i++;
+
+	for (; i < len; i = k) {
+		for (k = i; k < len && buf[k] != '\n'; k++)
+			; /* do nothing */
+		k++;
+
+		if (!(is_rfc2822_line(buf+i, k-i) ||
+			is_cherry_pick_from_line(buf+i, k-i)))
+			return 0;
+	}
+	return 1;
+}
+
 static void remove_sequencer_state(void)
 {
 	struct strbuf seq_dir = STRBUF_INIT;
@@ -493,6 +547,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
 		}
 
 		if (opts->record_origin) {
+			if (!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");
@@ -1018,56 +1074,6 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-static int is_rfc2822_line(const char *buf, int len)
-{
-	int i;
-
-	for (i = 0; i < len; i++) {
-		int ch = buf[i];
-		if (ch == ':')
-			break;
-		if (isalnum(ch) || (ch == '-'))
-			continue;
-		return 0;
-	}
-
-	return 1;
-}
-
-static int is_cherry_pick_from_line(const char *buf, int len)
-{
-	return (strlen(cherry_picked_prefix) + 41) <= len &&
-		!prefixcmp(buf, cherry_picked_prefix);
-}
-
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
-{
-	int hit = 0;
-	int i, k;
-	int len = sb->len - ignore_footer;
-	const char *buf = sb->buf;
-
-	for (i = len - 1; i > 0; i--) {
-		if (hit && buf[i] == '\n')
-			break;
-		hit = (buf[i] == '\n');
-	}
-
-	while (i < len - 1 && buf[i] == '\n')
-		i++;
-
-	for (; i < len; i = k) {
-		for (k = i; k < len && buf[k] != '\n'; k++)
-			; /* do nothing */
-		k++;
-
-		if (!(is_rfc2822_line(buf+i, k-i) ||
-			is_cherry_pick_from_line(buf+i, k-i)))
-			return 0;
-	}
-	return 1;
-}
-
 void append_signoff(struct strbuf *msgbuf, int ignore_footer)
 {
 	struct strbuf sob = STRBUF_INIT;
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 32c0bb1..9dd6d5d 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -57,6 +57,19 @@ test_expect_success setup '
 	test_commit conflicting unrelated
 '
 
+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-one-line^0` &&
+	git cherry-pick -x mesg-one-line &&
+	cat <<-EOF >expect &&
+		$mesg_one_line
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-one-line &&
@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-no-footer^0` &&
+	git cherry-pick -x mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-no-footer &&
@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-no-footer^0` &&
+	git cherry-pick -x -s mesg-no-footer &&
+	cat <<-EOF >expect &&
+		$mesg_no_footer
+
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-with-footer &&
@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats "(cherry picked from..." line as part
 	test_cmp expect actual
 '
 
+test_expect_success 'cherry-pick -x -s treats "(cherry picked from..." line as part of footer' '
+	pristine_detach initial &&
+	sha1=`git rev-parse mesg-with-cherry-footer^0` &&
+	git cherry-pick -x -s mesg-with-cherry-footer &&
+	cat <<-EOF >expect &&
+		$mesg_with_cherry_footer
+		(cherry picked from commit $sha1)
+		Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>
+	EOF
+	git log -1 --pretty=format:%B >actual &&
+	test_cmp expect actual
+'
+
 test_done

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 06/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              | 43 +++++++++++++++++++++++++++++++------------
 sequencer.h              |  2 +-
 t/t3511-cherry-pick-x.sh |  2 +-
 4 files changed, 34 insertions(+), 15 deletions(-)
Show changes to 4 files +34 −15

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

diff --git a/builtin/commit.c b/builtin/commit.c
index 1dd2ec5..7b9e2ac 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 7c0852a..3062ad4 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -42,12 +42,19 @@ static int is_cherry_pick_from_line(const char *buf, int len)
 		!prefixcmp(buf, cherry_picked_prefix);
 }
 
-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)
+/* Returns 0 for non-conforming footer
+ * Returns 1 for conforming footer
+ * Returns 2 when sob exists within conforming footer
+ * Returns 3 when sob exists within conforming footer as last entry
+ */
+static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
+	int ignore_footer)
 {
 	int hit = 0;
-	int i, k;
+	int i, k = 0;
 	int len = sb->len - ignore_footer;
 	const char *buf = sb->buf;
+	int found_sob = 0;
 
 	for (i = len - 1; i > 0; i--) {
 		if (hit && buf[i] == '\n')
@@ -63,14 +70,24 @@ 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) ||
-			is_cherry_pick_from_line(buf+i, k-i)))
+		found_rfc2822 = is_rfc2822_line(buf+i, k-i);
+		if (found_rfc2822 && sob &&
+			!strncasecmp(buf+i, sob->buf, sob->len))
+			found_sob = k;
+
+		if (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
 			return 0;
 	}
+	if (found_sob == i)
+		return 3;
+	if (found_sob)
+		return 2;
 	return 1;
 }
 
@@ -291,7 +308,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;
@@ -547,7 +564,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));
@@ -1074,9 +1091,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
 	return pick_commits(todo_list, opts);
 }
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer)
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
 {
 	struct strbuf sob = STRBUF_INIT;
+	int has_footer = 0;
 	int i;
 
 	strbuf_addstr(&sob, sign_off_header);
@@ -1085,10 +1103,11 @@ 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)))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
+	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
+		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,
+				sob.buf, sob.len);
 	strbuf_release(&sob);
 }
diff --git a/sequencer.h b/sequencer.h
index 9d57d57..c4c7132 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -48,6 +48,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);
 
 extern const char sign_off_header[];
 
-void append_signoff(struct strbuf *msgbuf, int ignore_footer);
+void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob);
 
 #endif
diff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh
index 9dd6d5d..4b67343 100755
--- a/t/t3511-cherry-pick-x.sh
+++ b/t/t3511-cherry-pick-x.sh
@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '
 	test_cmp expect actual
 '
 
-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '
+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '
 	pristine_detach initial &&
 	git cherry-pick -s mesg-broken-footer &&
 	cat <<-EOF >expect &&

-- 
1.8.0.284.g959048a
Junio C Hamano· Nov 29, 2012, 00:35 UTC · re: Brandon Casey · lore

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

Brandon Casey <drafnel@gmail.com> writes:
> +/* Returns 0 for non-conforming footer
Please format it like this:
/*
 * Returns 0 for ...
Show 10 quoted lines
> + * Returns 1 for conforming footer
> + * Returns 2 when sob exists within conforming footer
> + * Returns 3 when sob exists within conforming footer as last entry
> + */
> +static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,
> +	int ignore_footer)
>  {
>  	int hit = 0;
> -	int i, k;
> +	int i, k = 0;

This is not wrong per-se, but I do not think it is necessary ('k' is always initialized at least to 'i' before it gets used). If we need to do this, I'd prefer to see it done in 1/11 that cleaned up the containing function as a preparation for this series.

Show 19 quoted lines
>  	int len = sb->len - ignore_footer;
>  	const char *buf = sb->buf;
> +	int found_sob = 0;
>  
>  	for (i = len - 1; i > 0; i--) {
>  		if (hit && buf[i] == '\n')
> @@ -63,14 +70,24 @@ 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) ||
> -			is_cherry_pick_from_line(buf+i, k-i)))
> +		found_rfc2822 = is_rfc2822_line(buf+i, k-i);
Not limited to this place but please have SP around binary operators, i.e.
		found_rfc2822 = is_rfc2822_line(buf + i, k - i);
> +		if (found_rfc2822 && sob &&
> +			!strncasecmp(buf+i, sob->buf, sob->len))
> +			found_sob = k;

Are we sure we want strncasecmp() here? I *think* you are trying to catch "signed-off-By:" and other misspellings, and even though I know in practice we know "Brandon Casey" and "brandon casey" are the same person, it still feels somewhat sloppy. Perhaps it is just me.

Show 56 quoted lines
> +
> +		if (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))
>  			return 0;
>  	}
> +	if (found_sob == i)
> +		return 3;
> +	if (found_sob)
> +		return 2;
>  	return 1;
>  }
>  
> @@ -291,7 +308,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;
> @@ -547,7 +564,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));
> @@ -1074,9 +1091,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)
>  	return pick_commits(todo_list, opts);
>  }
>  
> -void append_signoff(struct strbuf *msgbuf, int ignore_footer)
> +void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
>  {
>  	struct strbuf sob = STRBUF_INIT;
> +	int has_footer = 0;
>  	int i;
>  
>  	strbuf_addstr(&sob, sign_off_header);
> @@ -1085,10 +1103,11 @@ 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)))
> +		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);

Avoid assignment inside if () conditional. It is not immediately obvious what value is compared against 3 in the second one, as the above makes it appear as if has_footer is uninitialized when i is zero.

Thanks.
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 07/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 | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to sequencer.c +2 −2
diff --git a/sequencer.c b/sequencer.c
index 3062ad4..eb93dd6 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1103,8 +1103,8 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)
 	strbuf_addch(&sob, '\n');
 	for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\n'; i--)
 		; /* do nothing */
-	if (!i || !(has_footer =
-		has_conforming_footer(msgbuf, &sob, ignore_footer)))
+	if (msgbuf->buf[i] != '\n' && (!i || !(has_footer =
+		has_conforming_footer(msgbuf, &sob, ignore_footer))))
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, "\n", 1);
 	if (has_footer != 3 && (!no_dup_sob || has_footer != 2))
 		strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

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

From: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
[bc: 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 ]
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 | 146 ++++++++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 146 insertions(+)
Show changes to t/t4014-format-patch.sh +146 −0
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 16a4ca1..dfe9209 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -963,4 +963,150 @@ test_expect_success 'format patch ignores color.ui' '
 	test_cmp expect actual
 '
 
+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' '
+	echo -n 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_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_done

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 09/11] format-patch: stricter S-o-b detection

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

S-o-b in the middle of a sentence, at the beginning of the sentence but it is just because of text wrapping, or a full paragraph of valid S-o-b in the middle of the message. All these are not S-o-b, but detected as is. Fix them.

[bc: add two additional tests to demonstrate shortcomings of the current
   code:
   1. failure to detect non-conforming elements in the footer when last
      line matches committer's s-o-b.
   2. failure to handle various s-o-b -like elements in the footer 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>
---
 log-tree.c              | 37 +++++++++++++++++--
 t/t4014-format-patch.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 130 insertions(+), 2 deletions(-)
Show changes to 2 files +130 −2

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

diff --git a/log-tree.c b/log-tree.c
index 4f86def..14ebf69 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -260,14 +260,47 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	int has_signoff = 0;
 	char *cp;
 
-	cp = sb->buf;
+	/*
+	 * Only search in the last paragrah, don't be fooled by a
+	 * paragraph full of valid S-o-b in the middle.
+	 */
+	cp = sb->buf + sb->len - 1;
+	while (cp > sb->buf) {
+		if (cp[0] == '\n' && cp[1] == '\n') {
+			cp += 2;
+			break;
+		}
+		cp--;
+	}
 
 	/* First see if we already have the sign-off by the signer */
 	while ((cp = strstr(cp, signed_off_by))) {
+		const char *s = cp;
+		cp += strlen(signed_off_by);
+
+		if (!has_signoff && s > sb->buf) {
+			/*
+			 * S-o-b in the middle of a sentence is not
+			 * really S-o-b
+			 */
+			if (s[-1] != '\n')
+				continue;
+
+			if (s - 1 > sb->buf && s[-2] == '\n')
+				; /* the first S-o-b */
+			else if (!detect_any_signoff(sb->buf, s - sb->buf))
+				/*
+				 * The previous line looks like an
+				 * S-o-b. There's still no guarantee
+				 * that it's an actual S-o-b. For that
+				 * we need to look back until we find
+				 * a blank line, which is too costly.
+				 */
+				continue;
+		}
 
 		has_signoff = 1;
 
-		cp += strlen(signed_off_by);
 		if (cp + signoff_len >= sb->buf + sb->len)
 			break;
 		if (strncmp(cp, signoff, signoff_len))
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index dfe9209..30e37a7 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1062,6 +1062,60 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success '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_success '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_success '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
@@ -1109,4 +1163,45 @@ 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_failure 'signoff: footer begins with non-signoff without @ sign' '
+	append_signoff <<\EOF >actual &&
+subject
+
+body
+
+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:
+13:Signed-off-by: C O Mitter <committer@example.com>
+EOF
+	test_cmp expected actual
+'
+
 test_done

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 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    | 21 +++++++++++++--------
 revision.h    |  2 +-
 3 files changed, 15 insertions(+), 21 deletions(-)
Show changes to 3 files +15 −21

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

diff --git a/builtin/log.c b/builtin/log.c
index e7b7db1..bb48344 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1058,7 +1058,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;
@@ -1154,16 +1153,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			     PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |
 			     PARSE_OPT_KEEP_DASHDASH);
 
-	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');
@@ -1354,7 +1343,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 14ebf69..be8e7ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -253,9 +253,11 @@ static int detect_any_signoff(char *letter, int size)
 	return seen_head && seen_name;
 }
 
-static void append_signoff(struct strbuf *sb, const char *signoff)
+static void append_signoff(struct strbuf *sb, int flags)
 {
-	static const char signed_off_by[] = "Signed-off-by: ";
+	char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
+					 getenv("GIT_COMMITTER_EMAIL")));
+	static const char sign_off_header[] = "Signed-off-by: ";
 	size_t signoff_len = strlen(signoff);
 	int has_signoff = 0;
 	char *cp;
@@ -274,9 +276,9 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	}
 
 	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, signed_off_by))) {
+	while ((cp = strstr(cp, sign_off_header))) {
 		const char *s = cp;
-		cp += strlen(signed_off_by);
+		cp += strlen(sign_off_header);
 
 		if (!has_signoff && s > sb->buf) {
 			/*
@@ -317,9 +319,10 @@ static void append_signoff(struct strbuf *sb, const char *signoff)
 	if (!has_signoff)
 		strbuf_addch(sb, '\n');
 
-	strbuf_addstr(sb, signed_off_by);
+	strbuf_addstr(sb, sign_off_header);
 	strbuf_add(sb, signoff, signoff_len);
 	strbuf_addch(sb, '\n');
+	free(signoff);
 }
 
 static unsigned int digits_in_number(unsigned int number)
@@ -695,8 +698,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;
@@ -707,7 +712,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);
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message) {
diff --git a/revision.h b/revision.h
index 059bfff..435a60b 100644
--- a/revision.h
+++ b/revision.h
@@ -137,7 +137,7 @@ struct rev_info {
 	int		numbered_files;
 	char		*message_id;
 	struct string_list *ref_message_ids;
-	const char	*add_signoff;
+	int              add_signoff;
 	const char	*extra_headers;
 	const char	*log_reencode;
 	const char	*subject_prefix;

-- 
1.8.0.284.g959048a
Brandon Casey· Nov 26, 2012, 01:45 UTC · re: Brandon Casey · lore

[PATCH 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              | 122 +-----------------------------------------------
 t/t4014-format-patch.sh |  27 +++++++++--
 2 files changed, 26 insertions(+), 123 deletions(-)
Show changes to 2 files +26 −123

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

diff --git a/log-tree.c b/log-tree.c
index be8e7ab..1fb0a16 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -9,6 +9,7 @@
 #include "string-list.h"
 #include "color.h"
 #include "gpg-interface.h"
+#include "sequencer.h"
 
 struct decoration name_decoration = { "object names" };
 
@@ -206,125 +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 flags)
-{
-	char* signoff = xstrdup(fmt_name(getenv("GIT_COMMITTER_NAME"),
-					 getenv("GIT_COMMITTER_EMAIL")));
-	static const char sign_off_header[] = "Signed-off-by: ";
-	size_t signoff_len = strlen(signoff);
-	int has_signoff = 0;
-	char *cp;
-
-	/*
-	 * Only search in the last paragrah, don't be fooled by a
-	 * paragraph full of valid S-o-b in the middle.
-	 */
-	cp = sb->buf + sb->len - 1;
-	while (cp > sb->buf) {
-		if (cp[0] == '\n' && cp[1] == '\n') {
-			cp += 2;
-			break;
-		}
-		cp--;
-	}
-
-	/* First see if we already have the sign-off by the signer */
-	while ((cp = strstr(cp, sign_off_header))) {
-		const char *s = cp;
-		cp += strlen(sign_off_header);
-
-		if (!has_signoff && s > sb->buf) {
-			/*
-			 * S-o-b in the middle of a sentence is not
-			 * really S-o-b
-			 */
-			if (s[-1] != '\n')
-				continue;
-
-			if (s - 1 > sb->buf && s[-2] == '\n')
-				; /* the first S-o-b */
-			else if (!detect_any_signoff(sb->buf, s - sb->buf))
-				/*
-				 * The previous line looks like an
-				 * S-o-b. There's still no guarantee
-				 * that it's an actual S-o-b. For that
-				 * we need to look back until we find
-				 * a blank line, which is too costly.
-				 */
-				continue;
-		}
-
-		has_signoff = 1;
-
-		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 */
-		return;
-	}
-
-	if (!has_signoff)
-		has_signoff = detect_any_signoff(sb->buf, sb->len);
-
-	if (!has_signoff)
-		strbuf_addch(sb, '\n');
-
-	strbuf_addstr(sb, sign_off_header);
-	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;
@@ -712,7 +594,7 @@ void show_log(struct rev_info *opt)
 	pretty_print_commit(&ctx, commit, &msgbuf);
 
 	if (opt->add_signoff)
-		append_signoff(&msgbuf, 0);
+		append_signoff(&msgbuf, 0, 1);
 
 	if ((ctx.fmt != CMIT_FMT_USERFORMAT) &&
 	    ctx.notes_message && *ctx.notes_message) {
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 30e37a7..1dea58b 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1046,7 +1046,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
 
@@ -1163,7 +1184,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
 
@@ -1184,7 +1205,7 @@ EOF
 	test_cmp expected actual
 '
 
-test_expect_failure 'signoff: footer begins with non-signoff without @ sign' '
+test_expect_success 'signoff: footer begins with non-signoff without @ sign' '
 	append_signoff <<\EOF >actual &&
 subject
 

-- 
1.8.0.284.g959048a
Nguyen Thai Ngoc Duy· Nov 26, 2012, 07:56 UTC · re: Brandon Casey · lore

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

On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey <drafnel@gmail.com> wrote:
Show 19 quoted lines
> From: Brandon Casey <bcasey@nvidia.com>
>
> I hate to have the battle of the patches, but I kinda prefer the
> append_signoff code in sequencer.c over the code in log-tree.c as a
> foundation to build on.
>
> So, this series is similar to Duy's "unification" series, but it goes in the
> opposite direction and builds on top of sequencer.c and additionally adds the
> elements from my original series to treat the "(cherry picked from..." line
> added by 'cherry-pick -x' in the same way that other footer elements are
> treated (after addressing Junio's comments about rfc2822 continuation lines
> and duplicate s-o-b's).
>
> I've integrated Duy's series with a few minor tweaks.  I added a couple
> of additional tests to t4014 and corrected one of the tests which had
> incorrect behavior.  I think his sign-off's should still be valid, so I
> kept them in.  Sorry that I've been slow, and now the two of us are stepping
> on each other's toes, but Duy please take a look and let me know if there's
> anything you dislike.

I'm still not sure whether format-patch should follow cherry-pick's rule in appending sob. If it does, it probably makes more sense to fix the sequencer.c code then delete log-tree.c (not fixes on log-tree.c then delete it). I see that your changes pass all the new tests I added in format-patch so sequencer.c is probably good enough, log-tree.c changes are probably not needed. Feel free take over the series :-)

-- 
Duy
Junio C Hamano· Nov 29, 2012, 05:56 UTC · re: Nguyen Thai Ngoc Duy · lore

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

Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
Show 16 quoted lines
> On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey <drafnel@gmail.com> wrote:
>
>> I've integrated Duy's series with a few minor tweaks.  I added a couple
>> of additional tests to t4014 and corrected one of the tests which had
>> incorrect behavior.  I think his sign-off's should still be valid, so I
>> kept them in.  Sorry that I've been slow, and now the two of us are stepping
>> on each other's toes, but Duy please take a look and let me know if there's
>> anything you dislike.
>
> I'm still not sure whether format-patch should follow cherry-pick's
> rule in appending sob. If it does, it probably makes more sense to fix
> the sequencer.c code then delete log-tree.c (not fixes on log-tree.c
> then delete it). I see that your changes pass all the new tests I
> added in format-patch so sequencer.c is probably good enough,
> log-tree.c changes are probably not needed. Feel free take over the
> series :-)
After reading the series over, I agree with the above.

Patch #9 that fixes the copy in log-tree.c only to discard it in patch #11 does not seem to be the best organization of the series. Instead, perhaps we can salvage the tests in patch #9 (but mark failing ones as expecting failure) without updating the one in log-tree.c, adjust prototype in patch #10 (still broken in log-tree.c) to avoid having to make changes to the callers in patch #11, and then conclude the series with #11?

Other than the code in patches #06 and #07 that I already commented on, i.e. assignments in if () condition that make it harder to follow the logic, I did not find anything majorly objectionable in the series.

Thanks for a pleasant read.
Junio C Hamano· Nov 26, 2012, 22:00 UTC · re: Brandon Casey · lore

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

Brandon Casey <drafnel@gmail.com> writes:
Show 7 quoted lines
> So, this series should result in s-o-b and "(cherry picked from...)" being
> appended to commit messages in a more consistent and deterministic way.  For
> example, the decision about whether to insert a blank line before appending
> a s-o-b.  As discussed, cherry-pick and commit will only refrain from
> appending a s-o-b if the committer's s-o-b appears as the last one in a
> conforming footer, while format-patch will refrain from appending it if it
> appears anywhere within the footer.

Sounds sensible; we may want to fix format-patch later, but with something like this series, it is just the matter of flipping one bit.

Will queue. Thanks.

← back to recent threads