{"thread":{"id":"32749","subject":"[PATCH v3 00/11] unify appending of sob","startedAt":"2013-01-28T01:11:44Z","lastAt":"2013-02-11T18:49:29Z","messageCount":36,"participants":["Brandon Casey","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":3,"patchTotal":11},"messages":[{"id":"208016","messageId":"1359335515-13818-1-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":null,"subject":"[PATCH v3 00/11] unify appending of sob","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:44Z","receivedAt":"2013-01-28T01:11:44Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Round 3.\n\n-Brandon\n\nBrandon Casey (9):\n sequencer.c: rework search for start of footer to improve clarity\n commit, cherry-pick -s: remove broken support for multiline rfc2822\n fields\n t/test-lib-functions.sh: allow to specify the tag name to test_commit\n t/t3511: add some tests of 'cherry-pick -s' functionality\n sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b\n footer\n sequencer.c: always separate \"(cherry picked from\" from commit body\n sequencer.c: teach append_signoff how to detect duplicate s-o-b\n sequencer.c: teach append_signoff to avoid adding a duplicate newline\n Unify appending signoff in format-patch, commit and sequencer\n\nNguyễn Thái Ngọc Duy (2):\n t4014: more tests about appending s-o-b lines\n format-patch: update append_signoff prototype\n\n builtin/commit.c | 2 +-\n builtin/log.c | 13 +--\n log-tree.c | 92 ++---------------\n revision.h | 2 +-\n sequencer.c | 150 ++++++++++++++++++---------\n sequencer.h | 4 +-\n t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 263 +++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 8 +-\n 9 files changed, 600 insertions(+), 153 deletions(-)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208018","messageId":"1359335515-13818-2-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 01/11] sequencer.c: rework search for start of footer to improve clarity","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:45Z","receivedAt":"2013-01-28T01:11:45Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"This code sequence is somewhat difficult to read. Let's rewrite it\nusing more descriptive variable names to try to make it easier to\nunderstand.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 9 ++++++---\n 1 file changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex aef5e8a..cd211b2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1024,16 +1024,19 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n {\n \tint ch;\n-\tint hit = 0;\n+\tint last_char_was_nl, this_char_is_nl;\n \tint i, j, k;\n \tint len = sb->len - ignore_footer;\n \tint first = 1;\n \tconst char *buf = sb->buf;\n \n+\t/* find start of last paragraph */\n+\tlast_char_was_nl = 0;\n \tfor (i = len - 1; i > 0; i--) {\n-\t\tif (hit && buf[i] == '\\n')\n+\t\tthis_char_is_nl = (buf[i] == '\\n');\n+\t\tif (last_char_was_nl && this_char_is_nl)\n \t\t\tbreak;\n-\t\thit = (buf[i] == '\\n');\n+\t\tlast_char_was_nl = this_char_is_nl;\n \t}\n \n \twhile (i < len - 1 && buf[i] == '\\n')\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208017","messageId":"1359335515-13818-3-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 02/11] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:46Z","receivedAt":"2013-01-28T01:11:46Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Starting with c1e01b0c (commit: More generous accepting of RFC-2822 footer\nlines, 2009-10-28), \"git commit -s\" carefully parses the last paragraph of\neach commit message to check if it consists only of RFC2822-style headers,\nin which case the signoff will be added as a new line in the same list:\n\n Reported-by: Reporter \n Signed-off-by: Author \n Acked-by: Lieutenant \n\nIt even included support for accepting indented continuation lines for\nmultiline fields. Unfortunately the multiline field support is broken\nbecause it checks whether buf[k] (the first character of the *next* line)\ninstead of buf[i] is a whitespace character. The result is that any footer\nwith a continuation line is not accepted, since the last continuation line\nneither starts with an RFC2822 field name nor is followed by a continuation\nline.\n\nThat this has remained broken for so long is good evidence that nobody\nactually needed multiline fields. Rip out the broken continuation support.\n\nThere should be no functional change.\n\n[Thanks to Jonathan Nieder for the excellent commit message]\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex cd211b2..39a752b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1027,7 +1027,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \tint last_char_was_nl, this_char_is_nl;\n \tint i, j, k;\n \tint len = sb->len - ignore_footer;\n-\tint first = 1;\n \tconst char *buf = sb->buf;\n \n \t/* find start of last paragraph */\n@@ -1047,11 +1046,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n-\t\t\tcontinue;\n-\n-\t\tfirst = 0;\n-\n \t\tfor (j = 0; i + j < len; j++) {\n \t\t\tch = buf[i + j];\n \t\t\tif (ch == ':')\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208021","messageId":"1359335515-13818-4-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 03/11] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:47Z","receivedAt":"2013-01-28T01:11:47Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"The part of test_commit() may not be appropriate for a tag name.\nSo let's allow test_commit to accept a fourth argument to specify the tag\nname.\n\nSigned-off-by: Brandon Casey \nReviewed-by: Jonathan Nieder \n---\n t/test-lib-functions.sh | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fa62d01..61d0804 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -135,12 +135,12 @@ test_pause () {\n \tfi\n }\n \n-# Call test_commit with the arguments \" [ []]\"\n+# Call test_commit with the arguments \" [ [ []]]\"\n #\n # This will commit a file with the given contents and the given commit\n-# message. It will also add a tag with as name.\n+# message, and tag the resulting commit with the given tag name.\n #\n-# Both and default to .\n+# , , and all default to .\n \n test_commit () {\n \tnotick= &&\n@@ -168,7 +168,7 @@ test_commit () {\n \t\ttest_tick\n \tfi &&\n \tgit commit $signoff -m \"$1\" &&\n-\tgit tag \"$1\"\n+\tgit tag \"${4:-$1}\"\n }\n \n # Call test_merge with the arguments \" \", where \n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208019","messageId":"1359335515-13818-5-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 04/11] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:48Z","receivedAt":"2013-01-28T01:11:48Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Add some tests to ensure that 'cherry-pick -s' operates in the following\nmanner:\n\n * Inserts a blank line before appending a s-o-b to a commit message that\n does not contain a s-o-b footer\n\n * Does not mistake first line \"subject: description\" as a s-o-b footer\n\n * Does not mistake single word message body as conforming to rfc2822\n\n * Appends a s-o-b when last s-o-b in footer does not match committer\n s-o-b, even when committer's s-o-b exists elsewhere in footer.\n\n * Does not append a s-o-b when last s-o-b matches committer s-o-b\n\n * Correctly detects a non-conforming footer containing a mix of s-o-b\n like elements and s-o-b elements. (marked \"expect failure\")\n\nSigned-off-by: Brandon Casey \n---\n t/t3511-cherry-pick-x.sh | 111 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 111 insertions(+)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nnew file mode 100755\nindex 0000000..2a040b7\n--- /dev/null\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -0,0 +1,111 @@\n+#!/bin/sh\n+\n+test_description='Test cherry-pick -x and -s'\n+\n+. ./test-lib.sh\n+\n+pristine_detach () {\n+\tgit cherry-pick --quit &&\n+\tgit checkout -f \"$1^0\" &&\n+\tgit read-tree -u --reset HEAD &&\n+\tgit clean -d -f -f -q -x\n+}\n+\n+mesg_one_line='base: commit message'\n+\n+mesg_no_footer=\"$mesg_one_line\n+\n+OneWordBodyThatsNotA-S-o-B\"\n+\n+mesg_with_footer=\"$mesg_no_footer\n+\n+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+Signed-off-by: A.U. Thor \n+Signed-off-by: B.U. Thor \"\n+\n+mesg_broken_footer=\"$mesg_no_footer\n+\n+The signed-off-by string should begin with the words Signed-off-by followed\n+by a colon and space, and then the signers name and email address. e.g.\n+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\n+mesg_with_footer_sob=\"$mesg_with_footer\n+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\n+\n+test_expect_success setup '\n+\tgit config advice.detachedhead false &&\n+\techo unrelated >unrelated &&\n+\tgit add unrelated &&\n+\ttest_commit initial foo a &&\n+\ttest_commit \"$mesg_one_line\" foo b mesg-one-line &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_no_footer\" foo b mesg-no-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_broken_footer\" foo b mesg-broken-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_footer\" foo b mesg-with-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_footer_sob\" foo b mesg-with-footer-sob &&\n+\tpristine_detach initial &&\n+\ttest_commit conflicting unrelated\n+'\n+\n+test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-one-line &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_line\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-broken-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_broken_footer\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-footer-sob &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer_sob\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208020","messageId":"1359335515-13818-6-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 05/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:49Z","receivedAt":"2013-01-28T01:11:49Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When 'cherry-pick -s' is used to append a signed-off-by line to a cherry\npicked commit, it does not currently detect the \"(cherry picked from...\"\nthat may have been appended by a previous 'cherry-pick -x' as part of the\ns-o-b footer and it will insert a blank line before appending a new s-o-b.\n\nLet's detect \"(cherry picked from...)\" as part of the footer so that we\nwill produce this:\n\n Signed-off-by: A U Thor \n (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n Signed-off-by: C O Mmitter \n\ninstead of this:\n\n Signed-off-by: A U Thor \n (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n\n Signed-off-by: C O Mmitter \n\nSigned-off-by: Brandon Casey \nAcked-by: Jonathan Nieder \n---\n sequencer.c | 46 ++++++++++++++++++++++++++++------------\n t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 13 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 39a752b..0b5cd18 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -18,6 +18,7 @@\n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n const char sign_off_header[] = \"Signed-off-by: \";\n+static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n static void remove_sequencer_state(void)\n {\n@@ -496,7 +497,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \n \t\tif (opts->record_origin) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n+\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n@@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n+static int is_rfc2822_line(const char *buf, int len)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tint ch = buf[i];\n+\t\tif (ch == ':')\n+\t\t\tbreak;\n+\t\tif (isalnum(ch) || (ch == '-'))\n+\t\t\tcontinue;\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_cherry_picked_from_line(const char *buf, int len)\n+{\n+\t/*\n+\t * We only care that it looks roughly like (cherry picked from ...)\n+\t */\n+\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n+\t\t(buf[len - 1] == ')' ||\n+\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n {\n-\tint ch;\n \tint last_char_was_nl, this_char_is_nl;\n-\tint i, j, k;\n+\tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \n@@ -1046,15 +1072,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tfor (j = 0; i + j < len; j++) {\n-\t\t\tch = buf[i + j];\n-\t\t\tif (ch == ':')\n-\t\t\t\tbreak;\n-\t\t\tif (isalnum(ch) ||\n-\t\t\t (ch == '-'))\n-\t\t\t\tcontinue;\n+\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n \t\t\treturn 0;\n-\t\t}\n \t}\n \treturn 1;\n }\n@@ -1071,7 +1091,7 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n \t\t; /* do nothing */\n \tif (prefixcmp(msgbuf->buf + i, sob.buf)) {\n-\t\tif (!i || !ends_rfc2822_footer(msgbuf, ignore_footer))\n+\t\tif (!i || !has_conforming_footer(msgbuf, ignore_footer))\n \t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n \t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);\n \t}\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 2a040b7..73da182 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \n+mesg_with_cherry_footer=\"$mesg_with_footer_sob\n+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Tested-by: C.U. Thor \"\n+\n \n test_expect_success setup '\n \tgit config advice.detachedhead false &&\n@@ -47,6 +51,8 @@ test_expect_success setup '\n \ttest_commit \"$mesg_with_footer\" foo b mesg-with-footer &&\n \tgit reset --hard initial &&\n \ttest_commit \"$mesg_with_footer_sob\" foo b mesg-with-footer-sob &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_cherry_footer\" foo b mesg-with-cherry-footer &&\n \tpristine_detach initial &&\n \ttest_commit conflicting unrelated\n '\n@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-footer^0` &&\n+\tgit cherry-pick -x -s mesg-with-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-footer-sob &&\n@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-footer-sob^0` &&\n+\tgit cherry-pick -x -s mesg-with-footer-sob &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer_sob\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-cherry-footer^0` &&\n+\tgit cherry-pick -x mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208022","messageId":"1359335515-13818-7-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 06/11] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:50Z","receivedAt":"2013-01-28T01:11:50Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Start treating the \"(cherry picked from\" line added by cherry-pick -x\nthe same way that the s-o-b lines are treated. Namely, separate them\nfrom the main commit message body with an empty line.\n\nThis commit is mostly a code movement, but notice that\nhas_conforming_footer() was modified, in addition to being moved.\n\nIntroduce tests to test this functionality.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 120 +++++++++++++++++++++++++----------------------\n t/t3511-cherry-pick-x.sh | 53 +++++++++++++++++++++\n 2 files changed, 116 insertions(+), 57 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 0b5cd18..46d51b2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -20,6 +20,67 @@\n const char sign_off_header[] = \"Signed-off-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n+static int is_rfc2822_line(const char *buf, int len)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tint ch = buf[i];\n+\t\tif (ch == ':')\n+\t\t\tbreak;\n+\t\tif (isalnum(ch) || (ch == '-'))\n+\t\t\tcontinue;\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_cherry_picked_from_line(const char *buf, int len)\n+{\n+\t/*\n+\t * We only care that it looks roughly like (cherry picked from ...)\n+\t */\n+\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n+\t\t(buf[len - 1] == ')' ||\n+\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+{\n+\tint last_char_was_nl, this_char_is_nl;\n+\tint i, k;\n+\tint len = sb->len - ignore_footer;\n+\tconst char *buf = sb->buf;\n+\n+\t/* find start of last paragraph */\n+\tlast_char_was_nl = 0;\n+\tfor (i = len - 1; i > 0; i--) {\n+\t\tthis_char_is_nl = (buf[i] == '\\n');\n+\t\tif (last_char_was_nl && this_char_is_nl)\n+\t\t\tbreak;\n+\t\tlast_char_was_nl = this_char_is_nl;\n+\t}\n+\n+\t/* require at least one blank line */\n+\tif (!last_char_was_nl || buf[i] != '\\n')\n+\t\treturn 0;\n+\n+\twhile (i < len - 1 && buf[i] == '\\n')\n+\t\ti++;\n+\n+\tfor (; i < len; i = k) {\n+\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n+\t\t\t; /* do nothing */\n+\t\tk++;\n+\n+\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static void remove_sequencer_state(void)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n@@ -497,6 +558,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \n \t\tif (opts->record_origin) {\n+\t\t\tif (!has_conforming_footer(&msgbuf, 0))\n+\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n \t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n@@ -1022,63 +1085,6 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n-static int is_rfc2822_line(const char *buf, int len)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tint ch = buf[i];\n-\t\tif (ch == ':')\n-\t\t\tbreak;\n-\t\tif (isalnum(ch) || (ch == '-'))\n-\t\t\tcontinue;\n-\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n-static int is_cherry_picked_from_line(const char *buf, int len)\n-{\n-\t/*\n-\t * We only care that it looks roughly like (cherry picked from ...)\n-\t */\n-\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n-\t\t(buf[len - 1] == ')' ||\n-\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n-}\n-\n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n-{\n-\tint last_char_was_nl, this_char_is_nl;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\n-\t/* find start of last paragraph */\n-\tlast_char_was_nl = 0;\n-\tfor (i = len - 1; i > 0; i--) {\n-\t\tthis_char_is_nl = (buf[i] == '\\n');\n-\t\tif (last_char_was_nl && this_char_is_nl)\n-\t\t\tbreak;\n-\t\tlast_char_was_nl = this_char_is_nl;\n-\t}\n-\n-\twhile (i < len - 1 && buf[i] == '\\n')\n-\t\ti++;\n-\n-\tfor (; i < len; i = k) {\n-\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n-\t\t\t; /* do nothing */\n-\t\tk++;\n-\n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n-\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n-\t\t\treturn 0;\n-\t}\n-\treturn 1;\n-}\n-\n void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n {\n \tstruct strbuf sob = STRBUF_INIT;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 73da182..a845e45 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -57,6 +57,19 @@ test_expect_success setup '\n \ttest_commit conflicting unrelated\n '\n \n+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-one-line^0` &&\n+\tgit cherry-pick -x mesg-one-line &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_line\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-one-line &&\n@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-no-footer^0` &&\n+\tgit cherry-pick -x mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-no-footer &&\n@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-no-footer^0` &&\n+\tgit cherry-pick -x -s mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-footer &&\n@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-cherry-footer^0` &&\n+\tgit cherry-pick -x -s mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208024","messageId":"1359335515-13818-8-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:51Z","receivedAt":"2013-01-28T01:11:51Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\nThis is in preparation to unify the append_signoff implementations in\nlog-tree.c and sequencer.c.\n\nFixes test in t3511.\n\nSigned-off-by: Brandon Casey \n---\n builtin/commit.c | 2 +-\n sequencer.c | 47 +++++++++++++++++++++++++++++++++++++----------\n sequencer.h | 4 +++-\n t/t3511-cherry-pick-x.sh | 2 +-\n 4 files changed, 42 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 38b9a9c..67ea9e7 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -700,7 +700,7 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \t\t\tprevious = eol;\n \t\t}\n \n-\t\tappend_signoff(&sb, ignore_footer);\n+\t\tappend_signoff(&sb, ignore_footer, 0);\n \t}\n \n \tif (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\ndiff --git a/sequencer.c b/sequencer.c\nindex 46d51b2..015cb58 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -46,12 +46,20 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n \t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n }\n \n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+/*\n+ * Returns 0 for non-conforming footer\n+ * Returns 1 for conforming footer\n+ * Returns 2 when sob exists within conforming footer\n+ * Returns 3 when sob exists within conforming footer as last entry\n+ */\n+static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n+\tint ignore_footer)\n {\n \tint last_char_was_nl, this_char_is_nl;\n \tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n+\tint found_sob = 0;\n \n \t/* find start of last paragraph */\n \tlast_char_was_nl = 0;\n@@ -70,14 +78,25 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n \t\ti++;\n \n \tfor (; i < len; i = k) {\n+\t\tint found_rfc2822;\n+\n \t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i);\n+\t\tif (found_rfc2822 && sob &&\n+\t\t\t!strncmp(buf + i, sob->buf, sob->len))\n+\t\t\tfound_sob = k;\n+\n+\t\tif (!(found_rfc2822 ||\n \t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n \t\t\treturn 0;\n \t}\n+\tif (found_sob == i)\n+\t\treturn 3;\n+\tif (found_sob)\n+\t\treturn 2;\n \treturn 1;\n }\n \n@@ -299,7 +318,7 @@ static int do_recursive_merge(struct commit *base, struct commit *next,\n \trollback_lock_file(&index_lock);\n \n \tif (opts->signoff)\n-\t\tappend_signoff(msgbuf, 0);\n+\t\tappend_signoff(msgbuf, 0, 0);\n \n \tif (!clean) {\n \t\tint i;\n@@ -558,7 +577,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \n \t\tif (opts->record_origin) {\n-\t\t\tif (!has_conforming_footer(&msgbuf, 0))\n+\t\t\tif (!has_conforming_footer(&msgbuf, NULL, 0))\n \t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n \t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n@@ -1085,9 +1104,11 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n-void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n {\n+\tunsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\n \tstruct strbuf sob = STRBUF_INIT;\n+\tint has_footer = 0;\n \tint i;\n \n \tstrbuf_addstr(&sob, sign_off_header);\n@@ -1096,10 +1117,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n \tstrbuf_addch(&sob, '\\n');\n \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n \t\t; /* do nothing */\n-\tif (prefixcmp(msgbuf->buf + i, sob.buf)) {\n-\t\tif (!i || !has_conforming_footer(msgbuf, ignore_footer))\n-\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n-\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);\n-\t}\n+\n+\tif (i)\n+\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n+\n+\tif (!has_footer)\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n+\n+\tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\tsob.buf, sob.len);\n+\n \tstrbuf_release(&sob);\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex 9d57d57..1fc22dc 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -6,6 +6,8 @@\n #define SEQ_TODO_FILE\t\"sequencer/todo\"\n #define SEQ_OPTS_FILE\t\"sequencer/opts\"\n \n+#define APPEND_SIGNOFF_DEDUP (1u << 0)\n+\n enum replay_action {\n \tREPLAY_REVERT,\n \tREPLAY_PICK\n@@ -48,6 +50,6 @@ int sequencer_pick_revisions(struct replay_opts *opts);\n \n extern const char sign_off_header[];\n \n-void append_signoff(struct strbuf *msgbuf, int ignore_footer);\n+void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag);\n \n #endif\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex a845e45..f977279 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-broken-footer &&\n \tcat <<-EOF >expect &&\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208023","messageId":"1359335515-13818-9-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 08/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:52Z","receivedAt":"2013-01-28T01:11:52Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Teach append_signoff to detect whether a blank line exists at the position\nthat the signed-off-by line will be added, and avoid adding an additional\none if one already exists. This is necessary to allow format-patch to add a\ns-o-b to a patch with no commit message without adding an extra newline. A\nfollowing patch will make format-patch use this function rather than the\nappend_signoff implementation inside log-tree.c.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 015cb58..404b786 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1118,11 +1118,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n \t\t; /* do nothing */\n \n-\tif (i)\n-\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n-\n-\tif (!has_footer)\n-\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n+\tif (msgbuf->buf[i] != '\\n') {\n+\t\tif (i)\n+\t\t\thas_footer = has_conforming_footer(msgbuf, &sob,\n+\t\t\t\t\tignore_footer);\n+\n+\t\tif (!has_footer)\n+\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\t\t\"\\n\", 1);\n+\t}\n \n \tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n \t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208025","messageId":"1359335515-13818-10-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 09/11] t4014: more tests about appending s-o-b lines","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:53Z","receivedAt":"2013-01-28T01:11:53Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\n[bc: Squash the tests from Duy's original unify-appending-sob series.\n\n Fix test 90 \"signoff: some random signoff-alike\" and mark as failing.\n Correct behavior should insert a blank line after message body and\n signed-off-by.\n\n Add two additional tests:\n\n 1. failure to detect non-conforming elements in the footer when last\n line matches committer's s-o-b.\n 2. ensure various s-o-b -like elements in the footer are handled as\n conforming. e.g. \"Change-id: IXXXX or Bug: 1234\"\n]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n t/t4014-format-patch.sh | 242 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 242 insertions(+)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7fa3647..3868cef 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '\n \tgrep hello actual >/dev/null\n '\n \n+append_signoff()\n+{\n+\tC=`git commit-tree HEAD^^{tree} -p HEAD` &&\n+\tgit format-patch --stdout --signoff ${C}^..${C} |\n+\t\ttee append_signoff.patch |\n+\t\tsed -n \"1,/^---$/p\" |\n+\t\tgrep -n -E \"^Subject|Sign|^$\"\n+}\n+\n+test_expect_success 'signoff: commit with no body' '\n+\tappend_signoff actual &&\n+\tcat <<\\EOF | sed \"s/EOL$//\" >expected &&\n+4:Subject: [PATCH] EOL\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: commit with only subject' '\n+\techo subject | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: commit with only subject that does not end with NL' '\n+\tprintf subject | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: no existing signoffs' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: no existing signoffs and no trailing NL' '\n+\tprintf \"subject\\n\\nbody\" | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: some random signoff' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: my@house\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: some random signoff-alike' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+Fooled-by-me: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: not really a signoff' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+I want to mention about Signed-off-by: here.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:I want to mention about Signed-off-by: here.\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: not really a signoff (2)' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+My unfortunate\n+Signed-off-by: example happens to be wrapped here.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:Signed-off-by: example happens to be wrapped here.\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Signed-off-by: my@house\n+Signed-off-by: your@house\n+\n+A lot of houses.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: my@house\n+10:Signed-off-by: your@house\n+11:\n+13:\n+14:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff at the end' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: C O Mitter \n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff at the end, no trailing NL' '\n+\tprintf \"subject\\n\\nSigned-off-by: C O Mitter \" |\n+\t\tappend_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff NOT at the end' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: C O Mitter \n+Signed-off-by: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+12:Signed-off-by: my@house\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: detect garbage in non-conforming footer' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Tested-by: my@house\n+Some Trash\n+Signed-off-by: C O Mitter \n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+13:Signed-off-by: C O Mitter \n+14:\n+15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: footer begins with non-signoff without @ sign' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Reviewed-id: Noone\n+Tested-by: my@house\n+Change-id: Ideadbeef\n+Signed-off-by: C O Mitter \n+Bug: 1234\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+14:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208026","messageId":"1359335515-13818-11-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 10/11] format-patch: update append_signoff prototype","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:54Z","receivedAt":"2013-01-28T01:11:54Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis is a preparation step for merging with append_signoff from\nsequencer.c\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n builtin/log.c | 13 +------------\n log-tree.c | 17 +++++++++++++----\n revision.h | 2 +-\n 3 files changed, 15 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 8f0b2e8..59de484 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1086,7 +1086,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \tstruct commit *origin = NULL, *head = NULL;\n \tconst char *in_reply_to = NULL;\n \tstruct patch_ids ids;\n-\tchar *add_signoff = NULL;\n \tstruct strbuf buf = STRBUF_INIT;\n \tint use_patch_format = 0;\n \tint quiet = 0;\n@@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\trev.subject_prefix = strbuf_detach(&sprefix, NULL);\n \t}\n \n-\tif (do_signoff) {\n-\t\tconst char *committer;\n-\t\tconst char *endpos;\n-\t\tcommitter = git_committer_info(IDENT_STRICT);\n-\t\tendpos = strchr(committer, '>');\n-\t\tif (!endpos)\n-\t\t\tdie(_(\"bogus committer info %s\"), committer);\n-\t\tadd_signoff = xmemdupz(committer, endpos - committer + 1);\n-\t}\n-\n \tfor (i = 0; i < extra_hdr.nr; i++) {\n \t\tstrbuf_addstr(&buf, extra_hdr.items[i].string);\n \t\tstrbuf_addch(&buf, '\\n');\n@@ -1393,7 +1382,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\ttotal++;\n \t\tstart_number--;\n \t}\n-\trev.add_signoff = add_signoff;\n+\trev.add_signoff = do_signoff;\n \twhile (0 <= --nr) {\n \t\tint shown;\n \t\tcommit = list[nr];\ndiff --git a/log-tree.c b/log-tree.c\nindex 5dc45c4..ac1cd68 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -10,6 +10,8 @@\n #include \"color.h\"\n #include \"gpg-interface.h\"\n \n+#define APPEND_SIGNOFF_DEDUP (1u <<0)\n+\n struct decoration name_decoration = { \"object names\" };\n \n enum decoration_type {\n@@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)\n \treturn seen_head && seen_name;\n }\n \n-static void append_signoff(struct strbuf *sb, const char *signoff)\n+static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)\n {\n+\tunsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\n \tstatic const char signed_off_by[] = \"Signed-off-by: \";\n+\tchar *signoff = xstrdup(fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n \tsize_t signoff_len = strlen(signoff);\n \tint has_signoff = 0;\n \tchar *cp;\n@@ -275,6 +280,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)\n \t\tif (!isspace(cp[signoff_len]))\n \t\t\tcontinue;\n \t\t/* we already have him */\n+\t\tfree(signoff);\n \t\treturn;\n \t}\n \n@@ -287,6 +293,7 @@ static void append_signoff(struct strbuf *sb, const char *signoff)\n \tstrbuf_addstr(sb, signed_off_by);\n \tstrbuf_add(sb, signoff, signoff_len);\n \tstrbuf_addch(sb, '\\n');\n+\tfree(signoff);\n }\n \n static unsigned int digits_in_number(unsigned int number)\n@@ -672,8 +679,10 @@ void show_log(struct rev_info *opt)\n \t/*\n \t * And then the pretty-printed message itself\n \t */\n-\tif (ctx.need_8bit_cte >= 0)\n-\t\tctx.need_8bit_cte = has_non_ascii(opt->add_signoff);\n+\tif (ctx.need_8bit_cte >= 0 && opt->add_signoff)\n+\t\tctx.need_8bit_cte =\n+\t\t\thas_non_ascii(fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n \tctx.date_mode = opt->date_mode;\n \tctx.date_mode_explicit = opt->date_mode_explicit;\n \tctx.abbrev = opt->diffopt.abbrev;\n@@ -686,7 +695,7 @@ void show_log(struct rev_info *opt)\n \tpretty_print_commit(&ctx, commit, &msgbuf);\n \n \tif (opt->add_signoff)\n-\t\tappend_signoff(&msgbuf, opt->add_signoff);\n+\t\tappend_signoff(&msgbuf, 0, APPEND_SIGNOFF_DEDUP);\n \n \tif ((ctx.fmt != CMIT_FMT_USERFORMAT) &&\n \t ctx.notes_message && *ctx.notes_message) {\ndiff --git a/revision.h b/revision.h\nindex 5da09ee..01bd2b7 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -138,7 +138,7 @@ struct rev_info {\n \tint\t\treroll_count;\n \tchar\t\t*message_id;\n \tstruct string_list *ref_message_ids;\n-\tconst char\t*add_signoff;\n+\tint\t\tadd_signoff;\n \tconst char\t*extra_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208027","messageId":"1359335515-13818-12-git-send-email-drafnel@gmail.com","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T01:11:55Z","receivedAt":"2013-01-28T01:11:55Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"There are two implementations of append_signoff in log-tree.c and\nsequencer.c, which do more or less the same thing. Unify on top of the\nsequencer.c implementation.\n\nAdd a test in t4014 to demonstrate support for non-s-o-b elements in the\ncommit footer provided by sequence.c:append_sob. Mark tests fixed as\nappropriate.\n\n[Commit message mostly stolen from Nguyễn Thái Ngọc Duy's original\n unification patch]\n\nSigned-off-by: Brandon Casey \n---\n log-tree.c | 91 +------------------------------------------------\n t/t4014-format-patch.sh | 31 ++++++++++++++---\n 2 files changed, 27 insertions(+), 95 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex ac1cd68..c9d9a37 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -9,8 +9,7 @@\n #include \"string-list.h\"\n #include \"color.h\"\n #include \"gpg-interface.h\"\n-\n-#define APPEND_SIGNOFF_DEDUP (1u <<0)\n+#include \"sequencer.h\"\n \n struct decoration name_decoration = { \"object names\" };\n \n@@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)\n \tputchar(')');\n }\n \n-/*\n- * Search for \"^[-A-Za-z]+: [^@]+@\" pattern. It usually matches\n- * Signed-off-by: and Acked-by: lines.\n- */\n-static int detect_any_signoff(char *letter, int size)\n-{\n-\tchar *cp;\n-\tint seen_colon = 0;\n-\tint seen_at = 0;\n-\tint seen_name = 0;\n-\tint seen_head = 0;\n-\n-\tcp = letter + size;\n-\twhile (letter <= --cp && *cp == '\\n')\n-\t\tcontinue;\n-\n-\twhile (letter <= cp) {\n-\t\tchar ch = *cp--;\n-\t\tif (ch == '\\n')\n-\t\t\tbreak;\n-\n-\t\tif (!seen_at) {\n-\t\t\tif (ch == '@')\n-\t\t\t\tseen_at = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (!seen_colon) {\n-\t\t\tif (ch == '@')\n-\t\t\t\treturn 0;\n-\t\t\telse if (ch == ':')\n-\t\t\t\tseen_colon = 1;\n-\t\t\telse\n-\t\t\t\tseen_name = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\tif (('A' <= ch && ch <= 'Z') ||\n-\t\t ('a' <= ch && ch <= 'z') ||\n-\t\t ch == '-') {\n-\t\t\tseen_head = 1;\n-\t\t\tcontinue;\n-\t\t}\n-\t\t/* no empty last line doesn't match */\n-\t\treturn 0;\n-\t}\n-\treturn seen_head && seen_name;\n-}\n-\n-static void append_signoff(struct strbuf *sb, int ignore_footer, unsigned flag)\n-{\n-\tunsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\n-\tstatic const char signed_off_by[] = \"Signed-off-by: \";\n-\tchar *signoff = xstrdup(fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n-\tsize_t signoff_len = strlen(signoff);\n-\tint has_signoff = 0;\n-\tchar *cp;\n-\n-\tcp = sb->buf;\n-\n-\t/* First see if we already have the sign-off by the signer */\n-\twhile ((cp = strstr(cp, signed_off_by))) {\n-\n-\t\thas_signoff = 1;\n-\n-\t\tcp += strlen(signed_off_by);\n-\t\tif (cp + signoff_len >= sb->buf + sb->len)\n-\t\t\tbreak;\n-\t\tif (strncmp(cp, signoff, signoff_len))\n-\t\t\tcontinue;\n-\t\tif (!isspace(cp[signoff_len]))\n-\t\t\tcontinue;\n-\t\t/* we already have him */\n-\t\tfree(signoff);\n-\t\treturn;\n-\t}\n-\n-\tif (!has_signoff)\n-\t\thas_signoff = detect_any_signoff(sb->buf, sb->len);\n-\n-\tif (!has_signoff)\n-\t\tstrbuf_addch(sb, '\\n');\n-\n-\tstrbuf_addstr(sb, signed_off_by);\n-\tstrbuf_add(sb, signoff, signoff_len);\n-\tstrbuf_addch(sb, '\\n');\n-\tfree(signoff);\n-}\n-\n static unsigned int digits_in_number(unsigned int number)\n {\n \tunsigned int i = 10, result = 1;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 3868cef..d0ec097 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1104,7 +1104,28 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: some random signoff-alike' '\n+test_expect_success 'signoff: misc conforming footer elements' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: my@house\n+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Tested-by: Some One \n+Bug: 1234\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: my@house\n+15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: some random signoff-alike' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1120,7 +1141,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: not really a signoff' '\n+test_expect_success 'signoff: not really a signoff' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1136,7 +1157,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: not really a signoff (2)' '\n+test_expect_success 'signoff: not really a signoff (2)' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1153,7 +1174,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '\n+test_expect_success 'signoff: valid S-o-b paragraph in the middle' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1221,7 +1242,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: detect garbage in non-conforming footer' '\n+test_expect_success 'signoff: detect garbage in non-conforming footer' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n-- \n1.8.1.1.450.g0327af3\n"},{"id":"208033","messageId":"20130128012748.GD8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 01/11] sequencer.c: rework search for start of footer to improve clarity","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T01:27:48Z","receivedAt":"2013-01-28T01:27:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1024,16 +1024,19 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n> static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n> {\n> \tint ch;\n> -\tint hit = 0;\n> +\tint last_char_was_nl, this_char_is_nl;\n> \tint i, j, k;\n> \tint len = sb->len - ignore_footer;\n> \tint first = 1;\n> \tconst char *buf = sb->buf;\n> \n> +\t/* find start of last paragraph */\n> +\tlast_char_was_nl = 0;\n> \tfor (i = len - 1; i > 0; i--) {\n> -\t\tif (hit && buf[i] == '\\n')\n> +\t\tthis_char_is_nl = (buf[i] == '\\n');\n> +\t\tif (last_char_was_nl && this_char_is_nl)\n> \t\t\tbreak;\n> -\t\thit = (buf[i] == '\\n');\n> +\t\tlast_char_was_nl = this_char_is_nl;\n\nI would have been tempted to write\n\n\tchar prev;\n\n\tprev = 0;\n\tfor (i = len - 1; i > 0; i--) {\n\t\tchar ch = buf[i];\n\t\tif (prev == '\\n' && ch == '\\n')\t/* paragraph break */\n\t\t\tbreak;\n\t\tprev = ch;\n\t}\n\nbut your rewrite is just as clear. For what it's worth,\nReviewed-by: Jonathan Nieder \n"},{"id":"208034","messageId":"20130128012954.GE8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-3-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 02/11] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T01:29:54Z","receivedAt":"2013-01-28T01:29:54Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> sequencer.c | 6 ------\n> 1 file changed, 6 deletions(-)\n\nNice simplification.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"208036","messageId":"20130128020810.GF8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-5-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 04/11] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T02:08:11Z","receivedAt":"2013-01-28T02:08:11Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- /dev/null\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -0,0 +1,111 @@\n> +#!/bin/sh\n> +\n> +test_description='Test cherry-pick -x and -s'\n> +\n> +. ./test-lib.sh\n> +\n> +pristine_detach () {\n> +\tgit cherry-pick --quit &&\n> +\tgit checkout -f \"$1^0\" &&\n> +\tgit read-tree -u --reset HEAD &&\n> +\tgit clean -d -f -f -q -x\n> +}\n\nSome day this should move to test-lib-functions.sh. Not relevant\nfor this patch, though.\n\n[...]\n> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n> +\tpristine_detach initial &&\n> +\tgit cherry-pick -s mesg-broken-footer &&\n> +\tcat <<-EOF >expect &&\n> +\t\t$mesg_broken_footer\n> +\n> +\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n\nThis bug is old. When c1e01b0c (commit: More generous accepting of\nRFC-2822 footer lines, 2009-10-28) added more precise parsing of the\nRFC2822 footer when deciding whether to add a newline before a new\nsignoff, it forgot to change the \"sign-off already present\" case to\nmatch.\n\nThanks for the test. The rest of the tests in this file also look\ngood, of course.\n\nReviewed-by: Jonathan Nieder \n"},{"id":"208040","messageId":"20130128022148.GG8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-6-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 05/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T02:21:48Z","receivedAt":"2013-01-28T02:21:48Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Let's detect \"(cherry picked from...)\" as part of the footer so that we\n> will produce this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n> Signed-off-by: C O Mmitter \n>\n> instead of this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n>\n> Signed-off-by: C O Mmitter \n\nHere's the tweak I suggested last time. I think its behavior is\nslightly better in the \"ends with incomplete line\" case because it\nlimits the characters examined by is_rfc2822_line() and\nis_cherry_picked_from_line() not to include buf[len] (which would\npresumably sometimes be '\\0').\n\ndiff --git i/sequencer.c w/sequencer.c\nindex 0b5cd18c..fa29c7c5 100644\n--- i/sequencer.c\n+++ w/sequencer.c\n@@ -1043,9 +1043,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n \t/*\n \t * We only care that it looks roughly like (cherry picked from ...)\n \t */\n-\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n-\t\t(buf[len - 1] == ')' ||\n-\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+\treturn !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n }\n \n static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n@@ -1072,8 +1070,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n-\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n+\t\tif (!is_rfc2822_line(buf + i, k - i - 1) &&\n+\t\t !is_cherry_picked_from_line(buf + i, k - i - 1))\n \t\t\treturn 0;\n \t}\n \treturn 1;\n"},{"id":"208042","messageId":"20130128023409.GH8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-7-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 06/11] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T02:34:10Z","receivedAt":"2013-01-28T02:34:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -20,6 +20,67 @@\n[...]\n> static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n[...]\n> +\t/* require at least one blank line */\n> +\tif (!last_char_was_nl || buf[i] != '\\n')\n> +\t\treturn 0;\n\nMakes sense. append_signoff already added a blank line after a\none-line message\n\n\tfoo: bar\n\nbecause of e5138436 (builtin-commit.c: fix logic to omit empty line\nbefore existing footers, 2009-11-06) but the logic to do so was in the\nwrong place and it didn't handle its ill-formatted cousin\n\n\tfoo: bar\n\tbaz: qux\n\n[...]\n> @@ -497,6 +558,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n> \t\t}\n> \n> \t\tif (opts->record_origin) {\n> +\t\t\tif (!has_conforming_footer(&msgbuf, 0))\n> +\t\t\t\tstrbuf_addch(&msgbuf, '\\n');\n\nWhat should this do in the case of an entirely blank message?\n(Maybe that's too insane a case to worry about.)\n\n[...]\n> --- a/t/t3511-cherry-pick-x.sh\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot\n> \ttest_cmp expect actual\n> '\n> \n> +test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n\nYay. :)\n\nThanks for a clear and well thought-out patch with thorough tests.\nReviewed-by: Jonathan Nieder \n"},{"id":"208044","messageId":"20130128025140.GI8206@elie.Belkin","threadId":"32749","inReplyTo":"20130128022148.GG8206@elie.Belkin","subject":"Re: [PATCH v3 05/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T02:51:41Z","receivedAt":"2013-01-28T02:51:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Here's the tweak I suggested last time. I think its behavior is\n> slightly better in the \"ends with incomplete line\" case because it\n> limits the characters examined by is_rfc2822_line() and\n> is_cherry_picked_from_line() not to include buf[len] (which would\n> presumably sometimes be '\\0').\n\nWhoops, that revealed a subtlety --- the '\\n' or '\\0' is what prevents\nexiting the loop in is_rfc2822_line when the line does not contain a\ncolon. Here's a corrected version of the tweak, that should actually\nwork. :)\n\ndiff --git i/sequencer.c w/sequencer.c\nindex 0b5cd18c..108ea27b 100644\n--- i/sequencer.c\n+++ w/sequencer.c\n@@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)\n \tfor (i = 0; i < len; i++) {\n \t\tint ch = buf[i];\n \t\tif (ch == ':')\n+\t\t\treturn 1;\n+\t\tif (!isalnum(ch) && ch != '-')\n \t\t\tbreak;\n-\t\tif (isalnum(ch) || (ch == '-'))\n-\t\t\tcontinue;\n-\t\treturn 0;\n \t}\n-\n-\treturn 1;\n+\treturn 0;\n }\n \n static int is_cherry_picked_from_line(const char *buf, int len)\n@@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n \t/*\n \t * We only care that it looks roughly like (cherry picked from ...)\n \t */\n-\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n-\t\t(buf[len - 1] == ')' ||\n-\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+\treturn !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n }\n \n static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n@@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n-\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n+\t\tif (!is_rfc2822_line(buf + i, k - i - 1) &&\n+\t\t !is_cherry_picked_from_line(buf + i, k - i - 1))\n \t\t\treturn 0;\n \t}\n \treturn 1;\n"},{"id":"208046","messageId":"20130128030858.GJ8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-8-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:08:58Z","receivedAt":"2013-01-28T03:08:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n\nThis replaces the previous (slightly broken) logic that checked\nwhether the sign-off to be appended would be redundant and puts the\nfixed logic further down the call-chain next to the rest of footer\nparsing.\n\nI am not thrilled with the\n\n\t0 = no rfc-style footer\n\t1 = rfc-style footer, no sign-off found\n\t2 = rfc-style footer, sign-off found but not as last field\n\t3 = rfc-style footer, sign-off found as last field\n\nconvention but since it's local to sequencer.c and this logic to scan\nall lines for the sign-off can probably be ripped out soon I don't\nmind.\n\nThe general direction is good and the execution looks obviously\ncorrect.\nReviewed-by: Jonathan Nieder \n"},{"id":"208047","messageId":"20130128031424.GK8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-8-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:14:24Z","receivedAt":"2013-01-28T03:14:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n[...]\n> @@ -1096,10 +1117,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n> \tstrbuf_addch(&sob, '\\n');\n> \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n> \t\t; /* do nothing */\n> -\tif (prefixcmp(msgbuf->buf + i, sob.buf)) {\n> -\t\tif (!i || !has_conforming_footer(msgbuf, ignore_footer))\n> -\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n> -\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);\n> -\t}\n> +\n> +\tif (i)\n> +\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n\nIs this \"if (i)\" test needed? I'd expect that if the message has no newlines,\nhas_conforming_footer() would notice that and return 0.\n"},{"id":"208048","messageId":"20130128032302.GL8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-9-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 08/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:23:02Z","receivedAt":"2013-01-28T03:23:02Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Teach append_signoff to detect whether a blank line exists at the position\n> that the signed-off-by line will be added, and avoid adding an additional\n> one if one already exists. This is necessary to allow format-patch to add a\n> s-o-b to a patch with no commit message without adding an extra newline.\n\nI assume this means you're preserving historical behavior when adding\na sign-off to a commit with empty description (which is a good thing),\nbut what is that behavior? Is it a deliberate choice or something\nthat developed by default?\n\n[...]\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1118,11 +1118,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n> \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n> \t\t; /* do nothing */\n> \n> -\tif (i)\n> -\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n> -\n> -\tif (!has_footer)\n> -\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n> +\tif (msgbuf->buf[i] != '\\n') {\n> +\t\tif (i)\n> +\t\t\thas_footer = has_conforming_footer(msgbuf, &sob,\n> +\t\t\t\t\tignore_footer);\n> +\n> +\t\tif (!has_footer)\n> +\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n> +\t\t\t\t\t\"\\n\", 1);\n> +\t}\n> \n> \tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n\nThis is too much nesting for my small mind to keep track of. Am I\ncorrect in imagining the effect is the same as the following?\n\n\thas_footer = has_conforming_footer(...)\n\n\tif (!has_footer && msgbuf->buf[i] != '\\n')\n\t\tstrbuf_splice(...);\t/* add blank line */\n\n\tif (has_footer != 3 && ...\n"},{"id":"208049","messageId":"20130128033146.GM8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-10-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:31:46Z","receivedAt":"2013-01-28T03:31:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n[...]\n> --- a/t/t4014-format-patch.sh\n> +++ b/t/t4014-format-patch.sh\n> @@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '\n> \tgrep hello actual >/dev/null\n> '\n> \n> +append_signoff()\n> +{\n> +\tC=`git commit-tree HEAD^^{tree} -p HEAD` &&\n> +\tgit format-patch --stdout --signoff ${C}^..${C} |\n> +\t\ttee append_signoff.patch |\n> +\t\tsed -n \"1,/^---$/p\" |\n> +\t\tgrep -n -E \"^Subject|Sign|^$\"\n> +}\n\nIs \"grep -n\" portable? I didn't find any uses of it elsewhere in the\ntestsuite.\n\nStyle: checking exit status from format-patch, avoiding sed|grep pipeline:\n\n\tC=$(git commit-tree HEAD^ -p HEAD) &&\n\tgit format-patch --stdout --signoff $C^..$C >append_signoff.patch &&\n\tawk '\n\t\t/^---$/ { exit; }\n\t\t/^Subject/ || /^Sign/ || /^$/ { print NR \":\" $0 }\n\t' actual\n"},{"id":"208050","messageId":"20130128033540.GN8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-11-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 10/11] format-patch: update append_signoff prototype","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:35:41Z","receivedAt":"2013-01-28T03:35:41Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> From: Nguyễn Thái Ngọc Duy \n>\n> This is a preparation step for merging with append_signoff from\n> sequencer.c\n\nAvoids a small unfreed allocation, too. Neat.\n\nReviewed-by: Jonathan Nieder \n\n(On the other hand, this means more malloc churn when running\n\"format-patch -s\" on a long series of patches, but I don't think\nanyone will mind.)\n\n[...]\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1193,16 +1192,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> \t\trev.subject_prefix = strbuf_detach(&sprefix, NULL);\n> \t}\n> \n> -\tif (do_signoff) {\n> -\t\tconst char *committer;\n> -\t\tconst char *endpos;\n> -\t\tcommitter = git_committer_info(IDENT_STRICT);\n\nsequencer.c uses fmt_name() which uses IDENT_STRICT, too. Phew.\n"},{"id":"208051","messageId":"20130128033921.GP8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-12-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:39:21Z","receivedAt":"2013-01-28T03:39:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- a/log-tree.c\n> +++ b/log-tree.c\n[...]\n> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)\n> \tputchar(')');\n> }\n> \n> -/*\n> - * Search for \"^[-A-Za-z]+: [^@]+@\" pattern. It usually matches\n> - * Signed-off-by: and Acked-by: lines.\n> - */\n\nThat's stricter than the test from sequencer.c. Maybe it's worth\nstealing to avoid false positives?\n"},{"id":"208052","messageId":"20130128034831.GQ8206@elie.Belkin","threadId":"32749","inReplyTo":"1359335515-13818-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v3 00/11] unify appending of sob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T03:48:31Z","receivedAt":"2013-01-28T03:48:31Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Round 3.\n\nThanks for a pleasant read. My only remaining observations are\ncosmetic, except for a portability question in Duy's test script, a\nsmall behavior change when the commit message ends with an\nRFC2822-style header with no trailing newline and the possibility of\ntightening the pattern in sequencer.c to match the strictness of\nformat-patch (which could easily wait for a later patch).\n\nJonathan\n"},{"id":"208054","messageId":"CA+sFfMfm3k8T-TrXxZPD6pe4hN3LoCH98Txeam4HCmw1scHswA@mail.gmail.com","threadId":"32749","inReplyTo":"20130128025140.GI8206@elie.Belkin","subject":"Re: [PATCH v3 05/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T05:38:51Z","receivedAt":"2013-01-28T05:38:51Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sun, Jan 27, 2013 at 6:51 PM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>\n>> Here's the tweak I suggested last time. I think its behavior is\n>> slightly better in the \"ends with incomplete line\" case because it\n>> limits the characters examined by is_rfc2822_line() and\n>> is_cherry_picked_from_line() not to include buf[len] (which would\n>> presumably sometimes be '\\0').\n>\n> Whoops, that revealed a subtlety --- the '\\n' or '\\0' is what prevents\n> exiting the loop in is_rfc2822_line when the line does not contain a\n> colon. Here's a corrected version of the tweak, that should actually\n> work. :)\n>\n> diff --git i/sequencer.c w/sequencer.c\n> index 0b5cd18c..108ea27b 100644\n> --- i/sequencer.c\n> +++ w/sequencer.c\n> @@ -1029,13 +1029,11 @@ static int is_rfc2822_line(const char *buf, int len)\n> for (i = 0; i < len; i++) {\n> int ch = buf[i];\n> if (ch == ':')\n> + return 1;\n> + if (!isalnum(ch) && ch != '-')\n> break;\n> - if (isalnum(ch) || (ch == '-'))\n> - continue;\n> - return 0;\n> }\n> -\n> - return 1;\n> + return 0;\n> }\n>\n> static int is_cherry_picked_from_line(const char *buf, int len)\n> @@ -1043,9 +1041,7 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n> /*\n> * We only care that it looks roughly like (cherry picked from ...)\n> */\n> - return !prefixcmp(buf, cherry_picked_prefix) &&\n> - (buf[len - 1] == ')' ||\n> - (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n> + return !prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n> }\n>\n> static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n> @@ -1072,8 +1068,8 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n> ; /* do nothing */\n> k++;\n>\n> - if (!(is_rfc2822_line(buf + i, k - i) ||\n> - is_cherry_picked_from_line(buf + i, k - i)))\n> + if (!is_rfc2822_line(buf + i, k - i - 1) &&\n> + !is_cherry_picked_from_line(buf + i, k - i - 1))\n> return 0;\n> }\n> return 1;\n\nLooks good to me.\n\n-Brandon\n"},{"id":"208055","messageId":"7vboc9zwv1.fsf@alter.siamese.dyndns.org","threadId":"32749","inReplyTo":"20130128033146.GM8206@elie.Belkin","subject":"Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T05:42:58Z","receivedAt":"2013-01-28T05:42:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Brandon Casey wrote:\n>\n> [...]\n>> --- a/t/t4014-format-patch.sh\n>> +++ b/t/t4014-format-patch.sh\n>> @@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '\n>> \tgrep hello actual >/dev/null\n>> '\n>> \n>> +append_signoff()\n>> +{\n>> +\tC=`git commit-tree HEAD^^{tree} -p HEAD` &&\n>> +\tgit format-patch --stdout --signoff ${C}^..${C} |\n>> +\t\ttee append_signoff.patch |\n>> +\t\tsed -n \"1,/^---$/p\" |\n>> +\t\tgrep -n -E \"^Subject|Sign|^$\"\n>> +}\n>\n> Is \"grep -n\" portable? I didn't find any uses of it elsewhere in the\n> testsuite.\n\nYes, \"-n\" is in POSIX. Even though we use it ourselves, \"git grep\"\nsupports it, too.\n\nAny Emacs user would scream if their platform \"grep\" does not\nsupport it, as it will make M-x grep (or grep-find) useless.\n\n> Style: checking exit status from format-patch, avoiding sed|grep pipeline:\n>\n> \tC=$(git commit-tree HEAD^ -p HEAD) &&\n> \tgit format-patch --stdout --signoff $C^..$C >append_signoff.patch &&\n> \tawk '\n> \t\t/^---$/ { exit; }\n> \t\t/^Subject/ || /^Sign/ || /^$/ { print NR \":\" $0 }\n> \t' actual\n\nYeah, awk/perl would be fine, too, and it is good that you pointed\nout that the original was losing the exit status from format-patch.\n\nThanks.\n"},{"id":"208057","messageId":"7v7gmxzwkj.fsf@alter.siamese.dyndns.org","threadId":"32749","inReplyTo":"20130128034831.GQ8206@elie.Belkin","subject":"Re: [PATCH v3 00/11] unify appending of sob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T05:49:16Z","receivedAt":"2013-01-28T05:49:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Brandon Casey wrote:\n>\n>> Round 3.\n>\n> Thanks for a pleasant read. My only remaining observations are\n> cosmetic, except for a portability question in Duy's test script, a\n> small behavior change when the commit message ends with an\n> RFC2822-style header with no trailing newline and the possibility of\n> tightening the pattern in sequencer.c to match the strictness of\n> format-patch (which could easily wait for a later patch).\n>\n> Jonathan\n\nThanks for a quick review. I agree that this series is getting very\nclose with your help.\n"},{"id":"208058","messageId":"7v38xlzwh0.fsf@alter.siamese.dyndns.org","threadId":"32749","inReplyTo":"7vboc9zwv1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 09/11] t4014: more tests about appending s-o-b lines","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T05:51:23Z","receivedAt":"2013-01-28T05:51:23Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n>> Is \"grep -n\" portable? I didn't find any uses of it elsewhere in the\n>> testsuite.\n>\n> Yes, \"-n\" is in POSIX. Even though we use it ourselves, \"git grep\"\n> supports it, too.\n\nEhh even though we *DONT* use it ourselves, ... that is.\n\nI do not think we mind, if its use helps our test.\n"},{"id":"208286","messageId":"7vhalylggd.fsf@alter.siamese.dyndns.org","threadId":"32749","inReplyTo":"7v7gmxzwkj.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 00/11] unify appending of sob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-30T17:37:54Z","receivedAt":"2013-01-30T17:37:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jonathan Nieder writes:\n>\n>> Brandon Casey wrote:\n>>\n>>> Round 3.\n>>\n>> Thanks for a pleasant read. My only remaining observations are\n>> cosmetic, except for a portability question in Duy's test script, a\n>> small behavior change when the commit message ends with an\n>> RFC2822-style header with no trailing newline and the possibility of\n>> tightening the pattern in sequencer.c to match the strictness of\n>> format-patch (which could easily wait for a later patch).\n>\n> Thanks for a quick review. I agree that this series is getting very\n> close with your help.\n\nUnless Brandon and/or Jonathan wants to have another chance to\nexcise warts from the recorded history by rerolling the entire\nseries one more time, I think what we have queued is in a good\nenough shape to merge to 'next' and any further improvement and fix\ncan be done incrementally.\n\nOK? Or \"stop, I want to reroll\"?\n\nI'll wait for a day or two.\n"},{"id":"208387","messageId":"CA+sFfMe4NYWKQBM0N0isBfF+RY2AemKugKT+RyA98boO-p1rYQ@mail.gmail.com","threadId":"32749","inReplyTo":"7vhalylggd.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 00/11] unify appending of sob","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-31T18:45:07Z","receivedAt":"2013-01-31T18:45:07Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Wed, Jan 30, 2013 at 9:37 AM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> Jonathan Nieder writes:\n>>\n>>> Brandon Casey wrote:\n>>>\n>>>> Round 3.\n>>>\n>>> Thanks for a pleasant read. My only remaining observations are\n>>> cosmetic, except for a portability question in Duy's test script, a\n>>> small behavior change when the commit message ends with an\n>>> RFC2822-style header with no trailing newline and the possibility of\n>>> tightening the pattern in sequencer.c to match the strictness of\n>>> format-patch (which could easily wait for a later patch).\n>>\n>> Thanks for a quick review. I agree that this series is getting very\n>> close with your help.\n>\n> Unless Brandon and/or Jonathan wants to have another chance to\n> excise warts from the recorded history by rerolling the entire\n> series one more time, I think what we have queued is in a good\n> enough shape to merge to 'next' and any further improvement and fix\n> can be done incrementally.\n>\n> OK? Or \"stop, I want to reroll\"?\n>\n> I'll wait for a day or two.\n\nLet's hold off so I can do another round. I worked on this last night\nand was able to simplify some things nicely. I'll try to finish up\ntonight and resubmit.\n\n-Brandon\n"},{"id":"209188","messageId":"CA+sFfMc8YGZOUqNbTGa=X5ik0K_eBF86Pk4w5gZNppLayiCxVQ@mail.gmail.com","threadId":"32749","inReplyTo":"20130128023409.GH8206@elie.Belkin","subject":"Re: [PATCH v3 06/11] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-10T23:25:42Z","receivedAt":"2013-02-10T23:25:42Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sun, Jan 27, 2013 at 6:34 PM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n>\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n\n>> @@ -497,6 +558,8 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n>> }\n>>\n>> if (opts->record_origin) {\n>> + if (!has_conforming_footer(&msgbuf, 0))\n>> + strbuf_addch(&msgbuf, '\\n');\n>\n> What should this do in the case of an entirely blank message?\n> (Maybe that's too insane a case to worry about.)\n\nYeah, I think there are a number of insane cases that I think we\nshould just say \"behavior is undefined\".\n\n - Entirely blank message\n - Message lacking trailing newline.\n - Oneline message that is actually a signed-off-by (equal or not to\nthe committer's sob).\n - Message body that is entirely 1 or more newlines, or one which has\nmore than one trailing newline.\n\nIt's probably not worth going through contortions to handle these cases.\n\n-Brandon\n"},{"id":"209189","messageId":"CA+sFfMdajb3euvxANyXhbk-XhYASdH+7spW1r8tXz16kU8syEw@mail.gmail.com","threadId":"32749","inReplyTo":"20130128031424.GK8206@elie.Belkin","subject":"Re: [PATCH v3 07/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-10T23:36:31Z","receivedAt":"2013-02-10T23:36:31Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sun, Jan 27, 2013 at 7:14 PM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n>\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n> [...]\n>> @@ -1096,10 +1117,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n>> strbuf_addch(&sob, '\\n');\n>> for (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n>> ; /* do nothing */\n>> - if (prefixcmp(msgbuf->buf + i, sob.buf)) {\n>> - if (!i || !has_conforming_footer(msgbuf, ignore_footer))\n>> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n>> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);\n>> - }\n>> +\n>> + if (i)\n>> + has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n>\n> Is this \"if (i)\" test needed? I'd expect that if the message has no newlines,\n> has_conforming_footer() would notice that and return 0.\n\nThanks for pointing out this awkwardness. It should be pushed into\nhas_conforming_footer().\n\n-Brandon\n"},{"id":"209190","messageId":"CA+sFfMduqEJPtDQyTe2n7QiLmDpTN7MzLGrUNf1hWf-h0rGzvA@mail.gmail.com","threadId":"32749","inReplyTo":"20130128033921.GP8206@elie.Belkin","subject":"Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-10T23:55:47Z","receivedAt":"2013-02-10T23:55:47Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Sun, Jan 27, 2013 at 7:39 PM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n>\n>> --- a/log-tree.c\n>> +++ b/log-tree.c\n> [...]\n>> @@ -208,94 +207,6 @@ void show_decorations(struct rev_info *opt, struct commit *commit)\n>> putchar(')');\n>> }\n>>\n>> -/*\n>> - * Search for \"^[-A-Za-z]+: [^@]+@\" pattern. It usually matches\n>> - * Signed-off-by: and Acked-by: lines.\n>> - */\n>\n> That's stricter than the test from sequencer.c. Maybe it's worth\n> stealing to avoid false positives?\n\nNo, we don't want this stricter test because it assumes that the\nright-hand side of \"[-A-Za-z]+:\" will be an email address, so it\nrequires an '@' to exist. We want to be able to support lines that do\nnot have email addresses on the right-hand side like:\n\n Bug: XXX\n Change-Id: XXX\n\nand perhaps eventually\n\n Cherry-picked-from: XXX\n\nThe current series has retained the same basic test for an\nrfc2822-like line that existed in sequencer.c and would interpret a\nline that contains only a colon as conforming. A follow-on patch\ncould require that at least one character precede the colon, but that\nwould be a change in behavior that is not the goal of this series.\n\n-Brandon\n"},{"id":"209241","messageId":"20130211090009.GR15329@elie.Belkin","threadId":"32749","inReplyTo":"CA+sFfMduqEJPtDQyTe2n7QiLmDpTN7MzLGrUNf1hWf-h0rGzvA@mail.gmail.com","subject":"Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-11T09:00:09Z","receivedAt":"2013-02-11T09:00:09Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> We want to be able to support lines that do\n> not have email addresses on the right-hand side like:\n>\n> Bug: XXX\n> Change-Id: XXX\n\nGood call.\n\nBy the way, regarding what the right \"--signoff\" behavior is for\ncommit, cherry-pick, am, and format-patch:\n\nI think the best behavior would be to check if the last signed-off-by\nline (ignoring acked-by, bug, change-id, and so on lines that follow\nit) matches the one to be added, and if it doesn't, add a new\nsign-off. That way, the sign-off list still would accurately describe\nthe path of the patch, without silliness like\n\n\tSigned-off-by: me\n\tReviewed-by: someone\n\tSigned-off-by: me\n\nthat you mentioned.\n\nI agree that that's orthogonal to this series and just mostly\npreserving behavior (as you already do) is the right thing to do.\n\nThanks for noticing the edge cases.\n\nJonathan\n"},{"id":"209289","messageId":"CA+sFfMdBTmu8JsqZaJTUeA8x8Mb15R90v4w3fMQmUgkCDP5Qjw@mail.gmail.com","threadId":"32749","inReplyTo":"20130211090009.GR15329@elie.Belkin","subject":"Re: [PATCH v3 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-11T18:49:29Z","receivedAt":"2013-02-11T18:49:29Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Feb 11, 2013 at 1:00 AM, Jonathan Nieder wrote:\n\n> By the way, regarding what the right \"--signoff\" behavior is for\n> commit, cherry-pick, am, and format-patch:\n>\n> I think the best behavior would be to check if the last signed-off-by\n> line (ignoring acked-by, bug, change-id, and so on lines that follow\n> it) matches the one to be added, and if it doesn't, add a new\n> sign-off.\n\nThis makes a lot of sense.\n\n-Brandon\n"}]}