{"thread":{"id":"32888","subject":"[PATCH v4 00/12] unify appending of sob","startedAt":"2013-02-12T10:17:27Z","lastAt":"2017-05-13T17:42:13Z","messageCount":41,"participants":["Brandon Casey","Junio C Hamano","Jonathan Nieder","John Keeping","Jeff King","Ævar Arnfjörð Bjarmason"],"isPatch":true,"patchVersion":4,"patchTotal":12},"messages":[{"id":"209344","messageId":"1360664260-11803-1-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":null,"subject":"[PATCH v4 00/12] unify appending of sob","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:27Z","receivedAt":"2013-02-12T10:17:27Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Round 4.\n\nInterdiff against round 3 follows the diff stat.\n\n-Brandon\n\nBrandon Casey (9):\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: require a conforming footer to be preceded by a blank\n line\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\nJonathan Nieder (1):\n sequencer.c: rework search for start of footer to improve clarity\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 | 168 +++++++++++++++++++++---------\n sequencer.h | 4 +-\n t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 262 +++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 8 +-\n 9 files changed, 614 insertions(+), 156 deletions(-)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\n-- \n1.8.1.3.579.gd9af3b6\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 404b786..3c63e3a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -27,13 +27,12 @@ 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@@ -41,9 +40,8 @@ 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 len > strlen(cherry_picked_prefix) + 1 &&\n+\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n }\n \n /*\n@@ -55,25 +53,29 @@ static int is_cherry_picked_from_line(const char *buf, int len)\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+\tchar prev;\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+\t/* footer must end with newline */\n+\tif (!len || buf[len - 1] != '\\n')\n+\t\treturn 0;\n+\n+\tprev = '\\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\tchar ch = buf[i];\n+\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n \t\t\tbreak;\n-\t\tlast_char_was_nl = this_char_is_nl;\n+\t\tprev = ch;\n \t}\n \n \t/* require at least one blank line */\n-\tif (!last_char_was_nl || buf[i] != '\\n')\n+\tif (prev != '\\n' || buf[i] != '\\n')\n \t\treturn 0;\n \n+\t/* advance to start of last paragraph */\n \twhile (i < len - 1 && buf[i] == '\\n')\n \t\ti++;\n \n@@ -84,13 +86,13 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i);\n+\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n \t\tif (found_rfc2822 && sob &&\n-\t\t\t!strncmp(buf + i, sob->buf, sob->len))\n+\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 is_cherry_picked_from_line(buf + i, k - i - 1)))\n \t\t\treturn 0;\n \t}\n \tif (found_sob == i)\n@@ -1108,26 +1110,36 @@ 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+\tconst char *append_newlines = NULL;\n+\tint has_footer;\n \n \tstrbuf_addstr(&sob, sign_off_header);\n \tstrbuf_addstr(&sob, fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t\tgetenv(\"GIT_COMMITTER_EMAIL\")));\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 \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+\t * If the whole message buffer is equal to the sob, pretend that we\n+\t * found a conforming footer with a matching sob\n+\t */\n+\tif (msgbuf->len - ignore_footer == sob.len &&\n+\t !strncmp(msgbuf->buf, sob.buf, sob.len))\n+\t\thas_footer = 3;\n+\telse\n+\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n+\n+\tif (!has_footer) {\n+\t\tsize_t len = msgbuf->len - ignore_footer;\n+\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n+\t\t\tappend_newlines = \"\\n\\n\";\n+\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+\t\t\tappend_newlines = \"\\n\";\n \t}\n \n+\tif (append_newlines)\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\tappend_newlines, strlen(append_newlines));\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);\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex d0ec097..97fde9e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1023,11 +1023,10 @@ test_expect_success 'cover letter using branch description (6)' '\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+\tC=$(git commit-tree HEAD^^{tree} -p HEAD) &&\n+\tgit format-patch --stdout --signoff $C^..$C >append_signoff.patch &&\n+\tsed -n -e \"1,/^---$/p\" append_signoff.patch |\n+\t\tegrep -n \"^Subject|Sign|^$\"\n }\n \n test_expect_success 'signoff: commit with no body' '\n"},{"id":"209345","messageId":"1360664260-11803-2-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 01/12] sequencer.c: rework search for start of footer to improve clarity","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:28Z","receivedAt":"2013-02-12T10:17:28Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Jonathan Nieder \n\nThis code sequence is somewhat difficult to read. Let's rewrite it and add\nsome comments to improve clarity.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 10 ++++++----\n 1 file changed, 6 insertions(+), 4 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex aef5e8a..dbeff01 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1023,19 +1023,21 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \n static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n {\n-\tint ch;\n-\tint hit = 0;\n+\tchar ch, prev;\n \tint i, j, k;\n \tint len = sb->len - ignore_footer;\n \tint first = 1;\n \tconst char *buf = sb->buf;\n \n+\tprev = '\\0';\n \tfor (i = len - 1; i > 0; i--) {\n-\t\tif (hit && buf[i] == '\\n')\n+\t\tch = buf[i];\n+\t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n \t\t\tbreak;\n-\t\thit = (buf[i] == '\\n');\n+\t\tprev = ch;\n \t}\n \n+\t/* advance to start of last paragraph */\n \twhile (i < len - 1 && buf[i] == '\\n')\n \t\ti++;\n \n-- \n1.8.1.3.579.gd9af3b6\n"},{"id":"209346","messageId":"1360664260-11803-3-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 02/12] commit, cherry-pick -s: remove broken support for multiline rfc2822 fields","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:29Z","receivedAt":"2013-02-12T10:17:29Z","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 \nReviewed-by: Jonathan Nieder \n---\n sequencer.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex dbeff01..aa2cb8e 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1026,7 +1026,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \tchar ch, prev;\n \tint i, j, k;\n \tint len = sb->len - ignore_footer;\n-\tint first = 1;\n \tconst char *buf = sb->buf;\n \n \tprev = '\\0';\n@@ -1046,11 +1045,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.3.579.gd9af3b6\n"},{"id":"209347","messageId":"1360664260-11803-4-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 03/12] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:30Z","receivedAt":"2013-02-12T10:17:30Z","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.3.579.gd9af3b6\n"},{"id":"209349","messageId":"1360664260-11803-5-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 04/12] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:31Z","receivedAt":"2013-02-12T10:17:31Z","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 \nReviewed-by: Jonathan Nieder \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.3.579.gd9af3b6\n"},{"id":"209350","messageId":"1360664260-11803-6-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:32Z","receivedAt":"2013-02-12T10:17:32Z","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\n[with improvements from Jonathan Nieder]\n\nSigned-off-by: Brandon Casey \nAcked-by: Jonathan Nieder \n---\n sequencer.c | 51 ++++++++++++++++++++++++++++++++------------\n t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 92 insertions(+), 14 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex aa2cb8e..93495b0 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,16 +1022,44 @@ 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-\tchar ch, prev;\n-\tint i, j, k;\n+\tint i;\n+\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}\n+\n+\treturn 0;\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 len > strlen(cherry_picked_prefix) + 1 &&\n+\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+{\n+\tchar prev;\n+\tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \n+\t/* footer must end with newline */\n+\tif (!len || buf[len - 1] != '\\n')\n+\t\treturn 0;\n+\n \tprev = '\\0';\n \tfor (i = len - 1; i > 0; i--) {\n-\t\tch = buf[i];\n+\t\tchar ch = buf[i];\n \t\tif (prev == '\\n' && ch == '\\n') /* paragraph break */\n \t\t\tbreak;\n \t\tprev = ch;\n@@ -1045,15 +1074,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 - 1) &&\n+\t\t !is_cherry_picked_from_line(buf + i, k - i - 1))\n \t\t\treturn 0;\n-\t\t}\n \t}\n \treturn 1;\n }\n@@ -1070,7 +1093,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.3.579.gd9af3b6\n"},{"id":"209348","messageId":"1360664260-11803-7-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 06/12] sequencer.c: require a conforming footer to be preceded by a blank line","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:33Z","receivedAt":"2013-02-12T10:17:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Currently, append_signoff() performs a search for the last line of the\ncommit buffer by searching back from the end until it hits a newline. If\nit reaches the beginning of the buffer without finding a newline, that\nmeans either the commit message was empty, or there was only one line in it.\nIn this case, append_signoff will skip the call to has_conforming_footer\nsince it already knows that it is necessary to append a newline before\nappending the sob.\n\nLet's perform this function inside of has_conforming_footer where it\nappropriately belongs and generalize it so that we require that the\nfooter paragraph be an actual distinct paragraph separated by a blank\nline.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 6 +++++-\n 1 file changed, 5 insertions(+), 1 deletion(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 93495b0..178e84b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1065,6 +1065,10 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n \t\tprev = ch;\n \t}\n \n+\t/* require at least one blank line */\n+\tif (prev != '\\n' || buf[i] != '\\n')\n+\t\treturn 0;\n+\n \t/* advance to start of last paragraph */\n \twhile (i < len - 1 && buf[i] == '\\n')\n \t\ti++;\n@@ -1093,7 +1097,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 || !has_conforming_footer(msgbuf, ignore_footer))\n+\t\tif (!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-- \n1.8.1.3.579.gd9af3b6\n"},{"id":"209351","messageId":"1360664260-11803-8-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 07/12] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:34Z","receivedAt":"2013-02-12T10:17:34Z","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\nIntroduce tests to test this functionality.\n\nSigned-off-by: Brandon Casey \nReviewed-by: Jonathan Nieder \n---\n sequencer.c | 128 ++++++++++++++++++++++++-----------------------\n t/t3511-cherry-pick-x.sh | 53 ++++++++++++++++++++\n 2 files changed, 118 insertions(+), 63 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 178e84b..249c4a0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -20,6 +20,69 @@\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\treturn 1;\n+\t\tif (!isalnum(ch) && ch != '-')\n+\t\t\tbreak;\n+\t}\n+\n+\treturn 0;\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 len > strlen(cherry_picked_prefix) + 1 &&\n+\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+{\n+\tchar prev;\n+\tint i, k;\n+\tint len = sb->len - ignore_footer;\n+\tconst char *buf = sb->buf;\n+\n+\t/* footer must end with newline */\n+\tif (!len || buf[len - 1] != '\\n')\n+\t\treturn 0;\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') /* paragraph break */\n+\t\t\tbreak;\n+\t\tprev = ch;\n+\t}\n+\n+\t/* require at least one blank line */\n+\tif (prev != '\\n' || buf[i] != '\\n')\n+\t\treturn 0;\n+\n+\t/* advance to start of last paragraph */\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 - 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+}\n+\n static void remove_sequencer_state(void)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n@@ -497,6 +560,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,69 +1087,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\treturn 1;\n-\t\tif (!isalnum(ch) && ch != '-')\n-\t\t\tbreak;\n-\t}\n-\n-\treturn 0;\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 len > strlen(cherry_picked_prefix) + 1 &&\n-\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n-}\n-\n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n-{\n-\tchar prev;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\n-\t/* footer must end with newline */\n-\tif (!len || buf[len - 1] != '\\n')\n-\t\treturn 0;\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') /* paragraph break */\n-\t\t\tbreak;\n-\t\tprev = ch;\n-\t}\n-\n-\t/* require at least one blank line */\n-\tif (prev != '\\n' || buf[i] != '\\n')\n-\t\treturn 0;\n-\n-\t/* advance to start of last paragraph */\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 - 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-}\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.3.579.gd9af3b6\n"},{"id":"209352","messageId":"1360664260-11803-9-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 08/12] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:35Z","receivedAt":"2013-02-12T10:17:35Z","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 | 59 ++++++++++++++++++++++++++++++++++++------------\n sequencer.h | 4 +++-\n t/t3511-cherry-pick-x.sh | 2 +-\n 4 files changed, 50 insertions(+), 17 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1a0e5f1..94c96b7 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 249c4a0..3364faa 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -44,12 +44,20 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n \t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\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 \tchar prev;\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/* footer must end with newline */\n \tif (!len || buf[len - 1] != '\\n')\n@@ -72,14 +80,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 - 1) &&\n-\t\t !is_cherry_picked_from_line(buf + i, k - i - 1))\n+\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i - 1);\n+\t\tif (found_rfc2822 && sob &&\n+\t\t !strncmp(buf + i, sob->buf, sob->len))\n+\t\t\tfound_sob = k;\n+\n+\t\tif (!(found_rfc2822 ||\n+\t\t is_cherry_picked_from_line(buf + i, k - i - 1)))\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@@ -301,7 +320,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@@ -560,7 +579,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@@ -1087,21 +1106,33 @@ 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 i;\n+\tint has_footer;\n \n \tstrbuf_addstr(&sob, sign_off_header);\n \tstrbuf_addstr(&sob, fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t\tgetenv(\"GIT_COMMITTER_EMAIL\")));\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 (!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+\t/*\n+\t * If the whole message buffer is equal to the sob, pretend that we\n+\t * found a conforming footer with a matching sob\n+\t */\n+\tif (msgbuf->len - ignore_footer == sob.len &&\n+\t !strncmp(msgbuf->buf, sob.buf, sob.len))\n+\t\thas_footer = 3;\n+\telse\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.3.579.gd9af3b6\n"},{"id":"209353","messageId":"1360664260-11803-10-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:36Z","receivedAt":"2013-02-12T10:17:36Z","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 refrain from adding an\nadditional one if one already exists. Or, add an additional line if one\nis needed to make sure the new footer is separated from the message body\nby a blank line.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 14 ++++++++++++--\n 1 file changed, 12 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3364faa..3c63e3a 100644\n\n\nI dropped the mention of format-patch in the commit message. This\nimplementation is less about making format-patch work and more about\ncleaning up a strangely formatted commit message.\n\n-Brandon\n\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1110,6 +1110,7 @@ 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+\tconst char *append_newlines = NULL;\n \tint has_footer;\n \n \tstrbuf_addstr(&sob, sign_off_header);\n@@ -1127,8 +1128,17 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \telse\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 (!has_footer) {\n+\t\tsize_t len = msgbuf->len - ignore_footer;\n+\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n+\t\t\tappend_newlines = \"\\n\\n\";\n+\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+\t\t\tappend_newlines = \"\\n\";\n+\t}\n+\n+\tif (append_newlines)\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\tappend_newlines, strlen(append_newlines));\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.3.579.gd9af3b6\n"},{"id":"209355","messageId":"1360664260-11803-11-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 10/12] t4014: more tests about appending s-o-b lines","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:37Z","receivedAt":"2013-02-12T10:17:37Z","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 | 241 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 241 insertions(+)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7fa3647..a415b89 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1021,4 +1021,245 @@ 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 >append_signoff.patch &&\n+\tsed -n -e \"1,/^---$/p\" append_signoff.patch |\n+\t\tegrep -n \"^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.3.579.gd9af3b6\n"},{"id":"209354","messageId":"1360664260-11803-12-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 11/12] format-patch: update append_signoff prototype","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:38Z","receivedAt":"2013-02-12T10:17:38Z","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 \nReviewed-by: Jonathan Nieder \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.3.579.gd9af3b6\n"},{"id":"209357","messageId":"1360664260-11803-13-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v4 12/12] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:39Z","receivedAt":"2013-02-12T10:17:39Z","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 a415b89..97fde9e 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1103,7 +1103,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@@ -1119,7 +1140,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@@ -1135,7 +1156,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@@ -1152,7 +1173,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@@ -1220,7 +1241,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.3.579.gd9af3b6\n"},{"id":"209356","messageId":"1360664260-11803-14-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"[PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:17:40Z","receivedAt":"2013-02-12T10:17:40Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"---\n\nThis test tests the behavior of 'cherry-pick -s' of a commit with an empty\ncommit message.\n\nI created the test when I noticed during my series that cherry-pick was\nadding a sob twice when a commit with an empty commit message was\ncherry-picked.\n\nI'm not sure we should apply this though. I'm leaning towards saying that\nthe 'cherry-pick -s' behavior with respect to a commit with an empty message\nbody should be undefined. If we want it to be undefined then we probably\nshouldn't introduce a test which would have the effect of defining it.\n\nJunio, if you think we should apply it, it's prepared as a fixup commit and\nshould autosquash easily.\n\n-Brandon\n\n t/t3505-cherry-pick-empty.sh | 10 ++++++++++\n 1 file changed, 10 insertions(+)\n\ndiff --git a/t/t3505-cherry-pick-empty.sh b/t/t3505-cherry-pick-empty.sh\nindex a0c6e30..da4c60e 100755\n--- a/t/t3505-cherry-pick-empty.sh\n+++ b/t/t3505-cherry-pick-empty.sh\n@@ -58,6 +58,16 @@ test_expect_success 'cherry-pick a commit with an empty message with --allow-emp\n \tgit cherry-pick --allow-empty-message empty-branch\n '\n \n+test_expect_success 'cherry-pick a commit with an empty message with --allow-empty-message and -s' '\n+\tgit reset --hard HEAD^ &&\n+\tgit cherry-pick --allow-empty-message -s empty-branch &&\n+\t{ git show --pretty=format:%B -s empty-branch &&\n+\t printf \"Signed-off-by: %s <%s>\\n\" \"$GIT_COMMITTER_NAME\" \"$GIT_COMMITTER_EMAIL\"\n+\t} >expect &&\n+\tgit show --pretty=format:%B -s HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry pick an empty non-ff commit without --allow-empty' '\n \tgit checkout master &&\n \techo fourth >>file2 &&\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"209359","messageId":"1360665222-3166-1-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-10-git-send-email-drafnel@gmail.com","subject":"[PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T10:33:42Z","receivedAt":"2013-02-12T10:33:42Z","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 refrain from adding an\nadditional one if one already exists. Or, add an additional line if one\nis needed to make sure the new footer is separated from the message body\nby a blank line.\n\nSigned-off-by: Brandon Casey \n---\n\n\nA slight tweak. And I promise, no more are coming.\n\n-Brandon\n\n\n sequencer.c | 15 +++++++++++++--\n 1 file changed, 13 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3364faa..084573b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \telse\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 (!has_footer) {\n+\t\tconst char *append_newlines = NULL;\n+\t\tsize_t len = msgbuf->len - ignore_footer;\n+\n+\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n+\t\t\tappend_newlines = \"\\n\\n\";\n+\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+\t\t\tappend_newlines = \"\\n\";\n+\n+\t\tif (append_newlines)\n+\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\tappend_newlines, strlen(append_newlines));\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.252.gdb33759\n"},{"id":"209389","messageId":"7v621xgxax.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"1360664260-11803-6-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T19:13:42Z","receivedAt":"2013-02-12T19:13:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry\n> picked commit, it does not currently detect the \"(cherry picked from...\"\n> that may have been appended by a previous 'cherry-pick -x' as part of the\n> s-o-b footer and it will insert a blank line before appending a new s-o-b.\n>\n> Let's detect \"(cherry picked from...)\" as part of the footer so that we\n> will produce this:\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 len > strlen(cherry_picked_prefix) + 1 &&\n> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n> +}\n\nDoes the first \"is it longer than the prefix?\" check matter? If it\nis not, prefixcmp() would not match anyway, no?\n"},{"id":"209390","messageId":"7v1uclgwk7.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"1360664260-11803-12-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4 11/12] format-patch: update append_signoff prototype","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T19:29:44Z","receivedAt":"2013-02-12T19:29:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\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>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Brandon Casey \n> Reviewed-by: Jonathan Nieder \n> ---\n> builtin/log.c | 13 +------------\n> log-tree.c | 17 +++++++++++++----\n> revision.h | 2 +-\n> 3 files changed, 15 insertions(+), 17 deletions(-)\n>\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 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];\n> diff --git a/log-tree.c b/log-tree.c\n> index 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\nUnused variable at this step?\n\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) {\n> diff --git a/revision.h b/revision.h\n> index 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"},{"id":"209391","messageId":"511A98C0.70201@nvidia.com","threadId":"32888","inReplyTo":"7v621xgxax.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-02-12T19:32:16Z","receivedAt":"2013-02-12T19:32:16Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"On 2/12/2013 11:13 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>> When 'cherry-pick -s' is used to append a signed-off-by line to a cherry\n>> picked commit, it does not currently detect the \"(cherry picked from...\"\n>> that may have been appended by a previous 'cherry-pick -x' as part of the\n>> s-o-b footer and it will insert a blank line before appending a new s-o-b.\n>>\n>> Let's detect \"(cherry picked from...)\" as part of the footer so that we\n>> will produce this:\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 len > strlen(cherry_picked_prefix) + 1 &&\n>> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n>> +}\n> \n> Does the first \"is it longer than the prefix?\" check matter? If it\n> is not, prefixcmp() would not match anyway, no?\n\nProbably not in practice, but technically we should only be accessing\nlen characters in buf even though buf may be longer than len. So the\ncheck is just making sure the function doesn't access chars it's not\nsupposed to.\n\n-Brandon\n\n\n-----------------------------------------------------------------------------------\nThis email message is for the sole use of the intended recipient(s) and may contain\nconfidential information. Any unauthorized review, use, disclosure or distribution\nis prohibited. If you are not the intended recipient, please contact the sender by\nreply email and destroy all copies of the original message.\n-----------------------------------------------------------------------------------\n"},{"id":"209392","messageId":"7vtxphfhoq.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"511A98C0.70201@nvidia.com","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T19:36:21Z","receivedAt":"2013-02-12T19:36:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n>>> +\treturn len > strlen(cherry_picked_prefix) + 1 &&\n>>> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n>>> +}\n>> \n>> Does the first \"is it longer than the prefix?\" check matter? If it\n>> is not, prefixcmp() would not match anyway, no?\n>\n> Probably not in practice, but technically we should only be accessing\n> len characters in buf even though buf may be longer than len. So the\n> check is just making sure the function doesn't access chars it's not\n> supposed to.\n\nSorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],\nwhich would never match cherry_picked_prefix even if len is shorter\nthan the prefix?\n"},{"id":"209393","messageId":"20130212194502.GA12240@google.com","threadId":"32888","inReplyTo":"511A98C0.70201@nvidia.com","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-12T19:45:34Z","receivedAt":"2013-02-12T19:45:34Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n> On 2/12/2013 11:13 AM, Junio C Hamano wrote:\n>> Brandon Casey writes:\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 len > strlen(cherry_picked_prefix) + 1 &&\n>>> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n>>> +}\n>>\n>> Does the first \"is it longer than the prefix?\" check matter? If it\n>> is not, prefixcmp() would not match anyway, no?\n>\n> Probably not in practice, but technically we should only be accessing\n> len characters in buf even though buf may be longer than len.\n\nYep. Technically the buf[len - 1] == ')' check is enough to avoid\nfalse positives, but if it and the 'len' check were dropped then this\nwould be checking that buf is a \"(cherry-picked from\" line instead of\nchecking that its first 'len' bytes are one.\n\nSo it's just paranoid futureproofing. In the long term, it would be\nnice to drop the \"number of bytes to ignore at the end\" argument to\nappend_signoff to avoid having to think about this kind of thing.\n\nJonathan\n"},{"id":"209394","messageId":"511A9CDB.9060008@nvidia.com","threadId":"32888","inReplyTo":"7vtxphfhoq.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-02-12T19:49:47Z","receivedAt":"2013-02-12T19:49:47Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"On 2/12/2013 11:36 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n> \n>>>> +\treturn len > strlen(cherry_picked_prefix) + 1 &&\n>>>> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n>>>> +}\n>>>\n>>> Does the first \"is it longer than the prefix?\" check matter? If it\n>>> is not, prefixcmp() would not match anyway, no?\n>>\n>> Probably not in practice, but technically we should only be accessing\n>> len characters in buf even though buf may be longer than len. So the\n>> check is just making sure the function doesn't access chars it's not\n>> supposed to.\n> \n> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],\n> which would never match cherry_picked_prefix even if len is shorter\n> than the prefix?\n\nHeh, I almost pointed that out in my reply. Yes, buf will be terminated\nwith LF at buf[len]. And yes, that means that we will never get a false\npositive from prefixcmp even if the comparison overruns buf+len while\ndoing its comparison. That's why the check doesn't matter in practice,\ni.e. based on the way that is_cherry_picked_from_line is being called\nright now and the content of cherry_picked_prefix.\n\nBut, hasn't is_cherry_picked_from_line entered into a contract with the\ncaller and said \"I will not access more than len characters\"?\n\nIt's ok with me if you think it reads better without the check.\n\n-Brandon\n\n\n-----------------------------------------------------------------------------------\nThis email message is for the sole use of the intended recipient(s) and may contain\nconfidential information. Any unauthorized review, use, disclosure or distribution\nis prohibited. If you are not the intended recipient, please contact the sender by\nreply email and destroy all copies of the original message.\n-----------------------------------------------------------------------------------\n"},{"id":"209395","messageId":"20130212195620.GB12240@google.com","threadId":"32888","inReplyTo":"1360664260-11803-14-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-12T19:56:20Z","receivedAt":"2013-02-12T19:56:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> I'm not sure we should apply this though. I'm leaning towards saying that\n> the 'cherry-pick -s' behavior with respect to a commit with an empty message\n> body should be undefined. If we want it to be undefined then we probably\n> shouldn't introduce a test which would have the effect of defining it.\n\nMaybe it would make sense to just check that cherry-pick doesn't\nsegfault in this case?\n\nThat is, compute the output but don't compare it to expected output, as\nin:\n\n\ttest_expect_success 'adding signoff to empty message does something sane' '\n\t\tgit reset --hard HEAD^ &&\n\t\tgit cherry-pick --allow-empty-message -s empty-branch &&\n\t\tgit show --pretty=format:%B -s empty-branch >actual &&\n\n\t\t# sign-off is included *somewhere*\n\t\tgrep \"^Signed-off-by:.*>\\$\" actual\n\t'\n\nAlternatively, if there are only a few sane behaviors, a test can check\nfor all of them and pass as long as git follows one. I haven't thought\ncarefully enough about this example to suggest doing that.\n\nThanks,\nJonathan\n"},{"id":"209396","messageId":"7vpq05fgn4.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"511A9CDB.9060008@nvidia.com","subject":"Re: [PATCH v4 05/12] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T19:58:55Z","receivedAt":"2013-02-12T19:58:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> On 2/12/2013 11:36 AM, Junio C Hamano wrote:\n>> Brandon Casey writes:\n>> \n>>>>> +\treturn len > strlen(cherry_picked_prefix) + 1 &&\n>>>>> +\t\t!prefixcmp(buf, cherry_picked_prefix) && buf[len - 1] == ')';\n>>>>> +}\n>>>>\n>>>> Does the first \"is it longer than the prefix?\" check matter? If it\n>>>> is not, prefixcmp() would not match anyway, no?\n>>>\n>>> Probably not in practice, but technically we should only be accessing\n>>> len characters in buf even though buf may be longer than len. So the\n>>> check is just making sure the function doesn't access chars it's not\n>>> supposed to.\n>> \n>> Sorry, I do not follow. Isn't caller's buf terminated with LF at buf[len],\n>> which would never match cherry_picked_prefix even if len is shorter\n>> than the prefix?\n>\n> Heh, I almost pointed that out in my reply. Yes, buf will be terminated\n> with LF at buf[len]. And yes, that means that we will never get a false\n> positive from prefixcmp even if the comparison overruns buf+len while\n> doing its comparison. That's why the check doesn't matter in practice,\n> i.e. based on the way that is_cherry_picked_from_line is being called\n> right now and the content of cherry_picked_prefix.\n>\n> But, hasn't is_cherry_picked_from_line entered into a contract with the\n> caller and said \"I will not access more than len characters\"?\n>\n> It's ok with me if you think it reads better without the check.\n\nAs Jonathan says, if you rewrite it to\n\n\treturn buf[len - 1] == ')' && !prefixcmp(buf, cherry_picked_prefix);\n\nthen the code can keep its promise without the length check, because\nit knows there is no ')' in cherry-picked-prefix, and it also knows\nprefixcmp() stops at the first difference.\n\nIt is not a huge deal; I was primarily reacting to the ugly multi-line\nboolean expresion that is not inside a pair of parentheses (and because\nthis is a \"return\" statement, there is no good reason to have parentheses\nexcept that this is a multi-line expression), which looked odd.\n"},{"id":"209400","messageId":"20130212201613.GC12240@google.com","threadId":"32888","inReplyTo":"1360664260-11803-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4 00/12] unify appending of sob","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-02-12T20:16:13Z","receivedAt":"2013-02-12T20:16:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Round 4.\n\nYay. I think this is cooked now and a good foundation for later\nchanges on top.\n\nFor what it's worth, with or without the two tweaks Junio suggested\n(simplifying \"(cherry picked from\" detection, deferring introduction\nof no_dup_sob variable until it is used),\nReviewed-by: Jonathan Nieder \n"},{"id":"209401","messageId":"7vliatffnb.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"20130212195620.GB12240@google.com","subject":"Re: [PATCH/FYI v4 13/12] fixup! t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T20:20:24Z","receivedAt":"2013-02-12T20:20:24Z","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>> I'm not sure we should apply this though. I'm leaning towards saying that\n>> the 'cherry-pick -s' behavior with respect to a commit with an empty message\n>> body should be undefined. If we want it to be undefined then we probably\n>> shouldn't introduce a test which would have the effect of defining it.\n>\n> Maybe it would make sense to just check that cherry-pick doesn't\n> segfault in this case?\n\n;-)\n\n>\n> That is, compute the output but don't compare it to expected output, as\n> in:\n>\n> \ttest_expect_success 'adding signoff to empty message does something sane' '\n> \t\tgit reset --hard HEAD^ &&\n> \t\tgit cherry-pick --allow-empty-message -s empty-branch &&\n> \t\tgit show --pretty=format:%B -s empty-branch >actual &&\n>\n> \t\t# sign-off is included *somewhere*\n> \t\tgrep \"^Signed-off-by:.*>\\$\" actual\n> \t'\n\nIsn't what the current code happens to do is the best we could do?\nWe would end up showing one entry whose title appears to be\n\"Signed-off-by: ...\" in the shortlog output if we did so. If we\nadded an empty line, then the shortlog output will have a single\nempty line that is equally unsightly.\n\nWe could force a message like this:\n\n\ttree d7f87518a26e9f00714675706f165b94f3625177\n parent f459a4b602c0f4d371e1717572de6d0c4d39c6b1\n author Junio C Hamano 1360699963 -0800\n committer Junio C Hamano 1360699980 -0800\n\n\t!!cherry-picked from a commit without any message!!\n\n Signed-off-by: Junio C Hamano \n\nbut I do not think that buys us much; it only replaces a totally\nuninformative empty line with another totally uninformative junk.\n\nThat ugliness is a price the insane person, who is cherry picking a\ncommit without any justification made by another insane person,\nindicates that he is willing to pay by doing so. At that point I do\nnot think we should care.\n"},{"id":"209407","messageId":"7vehglfei3.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"20130212201613.GC12240@google.com","subject":"Re: [PATCH v4 00/12] unify appending of sob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-12T20:45:08Z","receivedAt":"2013-02-12T20:45:08Z","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 4.\n>\n> Yay. I think this is cooked now and a good foundation for later\n> changes on top.\n>\n> For what it's worth, with or without the two tweaks Junio suggested\n> (simplifying \"(cherry picked from\" detection, deferring introduction\n> of no_dup_sob variable until it is used),\n> Reviewed-by: Jonathan Nieder \n\nYeah, I am inclined to merge this to 'next' without any tweak, and\nlet it cook and get polished incrementally. I am not sure if we\nhave enough time to graduate it to 'master' for the upcoming\nrelease, though.\n\nThanks.\n"},{"id":"209428","messageId":"CA+sFfMcJf2Jdjs8T3Sxx6gZGNrrtNYQog5p33NV0QOf4dAP-Ww@mail.gmail.com","threadId":"32888","inReplyTo":"7v1uclgwk7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4 11/12] format-patch: update append_signoff prototype","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-12T22:51:48Z","receivedAt":"2013-02-12T22:51:48Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Feb 12, 2013 at 11:29 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n\n>> diff --git a/builtin/log.c b/builtin/log.c\n>> index 8f0b2e8..59de484 100644\n>> --- a/builtin/log.c\n>> +++ b/builtin/log.c\n\n>> @@ -253,9 +255,12 @@ static int detect_any_signoff(char *letter, int size)\n>> return 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>> + unsigned no_dup_sob = flag & APPEND_SIGNOFF_DEDUP;\n>\n> Unused variable at this step?\n\nYeah, looks like that line can be dropped.\n\n-Brandon\n"},{"id":"209535","messageId":"20130214175849.GA27958@farnsworth.metanate.com","threadId":"32888","inReplyTo":"1360665222-3166-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-14T17:58:49Z","receivedAt":"2013-02-14T17:58:49Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:\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 refrain from adding an\n> additional one if one already exists. Or, add an additional line if one\n> is needed to make sure the new footer is separated from the message body\n> by a blank line.\n> \n> Signed-off-by: Brandon Casey \n> ---\n\nAs Jonathan Nieder wondered before [1], this changes the behaviour when\nthe commit message is empty. Before this commit, there is an empty line\nfollowed by the S-O-B line; now the S-O-B is on the first line of the\ncommit.\n\nThe previous behaviour seems better to me since the empty line is\nhinting that the user should fill something in. It looks particularly\nstrange if your editor has syntax highlighting for commit messages such\nthat the first line is in a different colour.\n\n[1] http://article.gmane.org/gmane.comp.version-control.git/214796\n\n> diff --git a/sequencer.c b/sequencer.c\n> index 3364faa..084573b 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n> \telse\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 (!has_footer) {\n> +\t\tconst char *append_newlines = NULL;\n> +\t\tsize_t len = msgbuf->len - ignore_footer;\n> +\n> +\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n> +\t\t\tappend_newlines = \"\\n\\n\";\n> +\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n> +\t\t\tappend_newlines = \"\\n\";\n\nTo restore the old behaviour this needs something like this:\n\n\t\telse if (!len)\n\t\t\tappend_newlines = \"\\n\";\n\n> +\t\tif (append_newlines)\n> +\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n> +\t\t\t\tappend_newlines, strlen(append_newlines));\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"},{"id":"209557","messageId":"CA+sFfMecyfD7x_8Jk-hUDceL_nS5kuKq5nF0vRBqLROWFgdypA@mail.gmail.com","threadId":"32888","inReplyTo":"20130214175849.GA27958@farnsworth.metanate.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-15T18:58:38Z","receivedAt":"2013-02-15T18:58:38Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Feb 14, 2013 at 9:58 AM, John Keeping wrote:\n> On Tue, Feb 12, 2013 at 02:33:42AM -0800, Brandon Casey wrote:\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 refrain from adding an\n>> additional one if one already exists. Or, add an additional line if one\n>> is needed to make sure the new footer is separated from the message body\n>> by a blank line.\n>>\n>> Signed-off-by: Brandon Casey \n>> ---\n>\n> As Jonathan Nieder wondered before [1], this changes the behaviour when\n> the commit message is empty. Before this commit, there is an empty line\n> followed by the S-O-B line; now the S-O-B is on the first line of the\n> commit.\n>\n> The previous behaviour seems better to me since the empty line is\n> hinting that the user should fill something in. It looks particularly\n> strange if your editor has syntax highlighting for commit messages such\n> that the first line is in a different colour.\n>\n> [1] http://article.gmane.org/gmane.comp.version-control.git/214796\n>\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 3364faa..084573b 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -1127,8 +1127,19 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n>> else\n>> has_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n>>\n>> - if (!has_footer)\n>> - strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n>> + if (!has_footer) {\n>> + const char *append_newlines = NULL;\n>> + size_t len = msgbuf->len - ignore_footer;\n>> +\n>> + if (len && msgbuf->buf[len - 1] != '\\n')\n>> + append_newlines = \"\\n\\n\";\n>> + else if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n>> + append_newlines = \"\\n\";\n>\n> To restore the old behaviour this needs something like this:\n>\n> else if (!len)\n> append_newlines = \"\\n\";\n>\n>> + if (append_newlines)\n>> + strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n>> + append_newlines, strlen(append_newlines));\n>> + }\n>>\n>> if (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n>> strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n\nAre you talking about the output produced by format-patch? Or are you\ntalking about what happens when you do 'commit --amend -s' for a\ncommit with an empty commit message. (The email that you referenced\nwas about the behavior of format-patch).\n\nI'm thinking you must be talking about the 'commit --amend -s'\nbehavior since you mentioned your editor. Is there another case that\nis affected by this? Normally, any extra blank lines that precede or\nfollow a commit message are removed before the commit object is\ncreated. So, I guess it wouldn't hurt to insert a newline (or maybe\nit should be two?) before the signoff in this case. Would this\nprovide an improvement or change for any other commands than 'commit\n--amend -s'?\n\nIf we want to do this, then I'd probably do it like this:\n\n- if (len && msgbuf->buf[len - 1] != '\\n')\n+ if (!len || msgbuf->buf[len - 1] != '\\n')\n append_newlines = \"\\n\\n\";\n- else if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+ else if (len == 1 || msgbuf->buf[len - 2] != '\\n')\n append_newlines = \"\\n\";\n\nThis would ensure there were two newlines preceding the sob. The\neditor would place its cursor on the top line where the user should\nbegin typing in a commit message. If an editor was not opened up\n(e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then\nthe normal mechanism that removes extra blank lines would trigger to\nremove the extra blank lines.\n\nI think that's reasonable.\n\nIt seems 'git cherry-pick -s --edit' follows a different code path,\nand the commit message is stripped of newlines by 'git commit' before\nit is passed to the editor. 'cherry-pick -s --edit' and 'commit\n--amend -s' should probably have the same behavior and present the\nsame buffer to the user for editing when they encounter a commit with\nan empty message.\n\nMaybe something like this is enough(?):\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7b9e2ac..0796412 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -124,8 +124,10 @@ static int opt_parse_m(const struct option *opt, const char\n if (unset)\n strbuf_setlen(buf, 0);\n else {\n+ if (buf->len)\n+ strbuf_addch(buf, '\\n');\n strbuf_addstr(buf, arg);\n- strbuf_addstr(buf, \"\\n\\n\");\n+ strbuf_complete_line(buf);\n }\n return 0;\n }\n@@ -673,9 +675,6 @@ static int prepare_to_commit(const char *index_file, const c\n if (s->fp == NULL)\n die_errno(_(\"could not open '%s'\"), git_path(commit_editmsg));\n\n- if (clean_message_contents)\n- stripspace(&sb, 0);\n-\n if (signoff) {\n /*\n * See if we have a Conflicts: block at the end. If yes, count\n@@ -703,6 +702,9 @@ static int prepare_to_commit(const char *index_file, const c\n append_signoff(&sb, ignore_footer, 0);\n }\n\n+ if (clean_message_contents)\n+ stripspace(&sb, 0);\n+\n if (fwrite(sb.buf, 1, sb.len, s->fp) < sb.len)\n die_errno(_(\"could not write commit template\"));\n\nI suspect we have a broken test at t7502.15 though that happened to\nwork because opt_parse_m() was appending two newlines that the test\nexpected to be there.\n\n-Brandon\n"},{"id":"209614","messageId":"20130217224919.GA5011@serenity.lan","threadId":"32888","inReplyTo":"CA+sFfMecyfD7x_8Jk-hUDceL_nS5kuKq5nF0vRBqLROWFgdypA@mail.gmail.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-02-17T22:49:20Z","receivedAt":"2013-02-17T22:49:20Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Feb 15, 2013 at 10:58:38AM -0800, Brandon Casey wrote:\n> On Thu, Feb 14, 2013 at 9:58 AM, John Keeping wrote:\n> > As Jonathan Nieder wondered before [1], this changes the behaviour when\n> > the commit message is empty. Before this commit, there is an empty line\n> > followed by the S-O-B line; now the S-O-B is on the first line of the\n> > commit.\n> >\n> > The previous behaviour seems better to me since the empty line is\n> > hinting that the user should fill something in. It looks particularly\n> > strange if your editor has syntax highlighting for commit messages such\n> > that the first line is in a different colour.\n> \n> Are you talking about the output produced by format-patch? Or are you\n> talking about what happens when you do 'commit --amend -s' for a\n> commit with an empty commit message. (The email that you referenced\n> was about the behavior of format-patch).\n\nI'm talking about plain 'commit -s' which seems to use the same code\npath.\n\n> I'm thinking you must be talking about the 'commit --amend -s'\n> behavior since you mentioned your editor. Is there another case that\n> is affected by this? Normally, any extra blank lines that precede or\n> follow a commit message are removed before the commit object is\n> created. So, I guess it wouldn't hurt to insert a newline (or maybe\n> it should be two?) before the signoff in this case. Would this\n> provide an improvement or change for any other commands than 'commit\n> --amend -s'?\n> \n> If we want to do this, then I'd probably do it like this:\n> \n> - if (len && msgbuf->buf[len - 1] != '\\n')\n> + if (!len || msgbuf->buf[len - 1] != '\\n')\n> append_newlines = \"\\n\\n\";\n> - else if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n> + else if (len == 1 || msgbuf->buf[len - 2] != '\\n')\n> append_newlines = \"\\n\";\n> \n> This would ensure there were two newlines preceding the sob. The\n> editor would place its cursor on the top line where the user should\n> begin typing in a commit message. If an editor was not opened up\n> (e.g. if 'git cherry-pick -s --allow-empty-message ...' was used) then\n> the normal mechanism that removes extra blank lines would trigger to\n> remove the extra blank lines.\n> \n> I think that's reasonable.\n\nTwo blank lines seems like an improvement to me, FWIW.\n\n\nJohn\n"},{"id":"209985","messageId":"7vip5lv6tv.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"1360665222-3166-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T18:51:24Z","receivedAt":"2013-02-21T18:51:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\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 refrain from adding an\n> additional one if one already exists. Or, add an additional line if one\n> is needed to make sure the new footer is separated from the message body\n> by a blank line.\n>\n> Signed-off-by: Brandon Casey \n> ---\n>\n>\n> A slight tweak. And I promise, no more are coming.\n\nWhen I do\n\n\t$ git commit -s\n\nit should start my editor with this in the buffer:\n\n\t----------------------------------------------------------------\n\n Signed-off-by: Junio C Hamano \n\t----------------------------------------------------------------\n\nand the cursor blinking at the beginning of the file. Annoyingly\nthis step breaks it by removing the leading blank line.\n"},{"id":"209993","messageId":"CA+sFfMcNWvPKuQpNWnacegbfgN0ZP=zfuDPDRkXs1G2FMrb+iA@mail.gmail.com","threadId":"32888","inReplyTo":"7vip5lv6tv.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-21T20:26:52Z","receivedAt":"2013-02-21T20:26:52Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Feb 21, 2013 at 10:51 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\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 refrain from adding an\n>> additional one if one already exists. Or, add an additional line if one\n>> is needed to make sure the new footer is separated from the message body\n>> by a blank line.\n>>\n>> Signed-off-by: Brandon Casey \n>> ---\n>>\n>>\n>> A slight tweak. And I promise, no more are coming.\n>\n> When I do\n>\n> $ git commit -s\n>\n> it should start my editor with this in the buffer:\n>\n> ----------------------------------------------------------------\n>\n> Signed-off-by: Junio C Hamano \n> ----------------------------------------------------------------\n>\n> and the cursor blinking at the beginning of the file. Annoyingly\n> this step breaks it by removing the leading blank line.\n\nYes. The fix described by John Keeping restores the above behavior\nfor 'commit -s'. Or the fix I described which inserts two preceding\nnewlines so it looks like this:\n\n ----------------------------------------------------------------\n\n\n Signed-off-by: Junio C Hamano \n ----------------------------------------------------------------\n\nSo then the cursor would be placed on the first line and a space would\nseparate it from the sob which is arguably a better indication to the\nuser that a blank line should separate the commit message body from\nthe sob.\n\nBut, this does not fix the same problem for 'cherry-pick --edit -s'\nwhen used to cherry-pick a commit without a sob. The cherry-pick part\nof it would add the extra preceding newlines, but then cherry-pick\npasses the buffer to 'git commit' via .git/MERGE_MSG which then\n\"cleans\" the buffer, removing the empty lines, in prepare_to_commit()\nbefore allowing the editor to operate on it.\n\nUsing 'cherry-pick --edit -s' to cherry-pick a commit with an empty\ncommit message is going to be a pretty rare corner case. It would be\nnice to have the same behavior for it that we decide to have for\n'commit -s', but it's probably not worth going through contortions to\nmake it happen.\n\n-Brandon\n"},{"id":"209994","messageId":"CA+sFfMcnqJUmpk3OGgYTQxfgZWpZTfhVUZhyhFaf7eJL0t3SRQ@mail.gmail.com","threadId":"32888","inReplyTo":"CA+sFfMcNWvPKuQpNWnacegbfgN0ZP=zfuDPDRkXs1G2FMrb+iA@mail.gmail.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-21T20:29:03Z","receivedAt":"2013-02-21T20:29:03Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Thu, Feb 21, 2013 at 12:26 PM, Brandon Casey wrote:\n\n> But, this does not fix the same problem for 'cherry-pick --edit -s'\n> when used to cherry-pick a commit without a sob.\n\nCorrection: \"when used to cherry-pick a commit with an empty commit message.\"\n"},{"id":"209997","messageId":"7vobfdtl1n.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"CA+sFfMcNWvPKuQpNWnacegbfgN0ZP=zfuDPDRkXs1G2FMrb+iA@mail.gmail.com","subject":"Re: [PATCH v4.1 09/12] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-21T21:27:16Z","receivedAt":"2013-02-21T21:27:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Yes. The fix described by John Keeping restores the above behavior\n> for 'commit -s'. Or the fix I described which inserts two preceding\n> newlines so it looks like this:\n>\n> ----------------------------------------------------------------\n>\n>\n> Signed-off-by: Junio C Hamano \n> ----------------------------------------------------------------\n>\n> So then the cursor would be placed on the first line and a space would\n> separate it from the sob which is arguably a better indication to the\n> user that a blank line should separate the commit message body from\n> the sob.\n\nThat sounds like an improvement to me.\n\n> But, this does not fix the same problem for 'cherry-pick --edit -s'\n> when used to cherry-pick a commit without a sob. ...\n> Using 'cherry-pick --edit -s' to cherry-pick a commit with an empty\n> commit message is going to be a pretty rare corner case....\n\nWe actively discourage an empty commit message by requiring users to\nsay \"commit --allow-empty-message\". I think it is in line with the\nphilosophy for a Porcelain command \"git cherry-pick -s\" to punish\nusers by making them work harder to use a commit with an empty\nmessage ;-).\n"},{"id":"210020","messageId":"1361525158-3648-1-git-send-email-drafnel@gmail.com","threadId":"32888","inReplyTo":"7vobfdtl1n.fsf@alter.siamese.dyndns.org","subject":"[PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-22T09:25:58Z","receivedAt":"2013-02-22T09:25:58Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with\na blank line before the Signed-off-by line. This provided a nice\nhint to the user that something should be filled in. Let's restore that\nbehavior, but now let's ensure that the Signed-off-by line is preceded\nby two blank lines to hint that something should be filled in, and that\na blank line should separate it from the Signed-off-by line.\n\nPlus, add a test for this behavior.\n\nReported-by: John Keeping \nSigned-off-by: Brandon Casey \n---\n\nOk. Here's a patch on top of 959a2623 bc/append-signed-off-by. It\nimplements the \"2 blank lines preceding sob\" behavior.\n\n-Brandon\n\n sequencer.c | 5 +++--\n t/t7502-commit.sh | 12 ++++++++++++\n 2 files changed, 15 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 53ee49a..2dac106 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \t\tconst char *append_newlines = NULL;\n \t\tsize_t len = msgbuf->len - ignore_footer;\n \n-\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n+\t\t/* ensure a blank line precedes our signoff */\n+\t\tif (!len || msgbuf->buf[len - 1] != '\\n')\n \t\t\tappend_newlines = \"\\n\\n\";\n-\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+\t\telse if (len == 1 || msgbuf->buf[len - 2] != '\\n')\n \t\t\tappend_newlines = \"\\n\";\n \n \t\tif (append_newlines)\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex deb187e..a53a1e0 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo\n \n '\n \n+test_expect_success 'commit -s places sob on third line after two empty lines' '\n+\tgit commit -s --allow-empty --allow-empty-message &&\n+\tcat <<-EOF >expect &&\n+\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\n+\tEOF\n+\tegrep -v '^#' .git/COMMIT_EDITMSG >actual &&\n+\ttest_cmp expect actual\n+'\n+\n write_script .git/FAKE_EDITOR <<\\EOF\n mv \"$1\" \"$1.orig\"\n (\n-- \n1.8.0.1.253.gfcb57d5.dirty\n"},{"id":"210045","messageId":"7vbobcdwo7.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"1361525158-3648-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T18:35:04Z","receivedAt":"2013-02-22T18:35:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with\n> a blank line before the Signed-off-by line. This provided a nice\n> hint to the user that something should be filled in. Let's restore that\n> behavior, but now let's ensure that the Signed-off-by line is preceded\n> by two blank lines to hint that something should be filled in, and that\n> a blank line should separate it from the Signed-off-by line.\n>\n> Plus, add a test for this behavior.\n>\n> Reported-by: John Keeping \n> Signed-off-by: Brandon Casey \n> ---\n>\n> Ok. Here's a patch on top of 959a2623 bc/append-signed-off-by. It\n> implements the \"2 blank lines preceding sob\" behavior.\n>\n> -Brandon\n>\n> sequencer.c | 5 +++--\n> t/t7502-commit.sh | 12 ++++++++++++\n> 2 files changed, 15 insertions(+), 2 deletions(-)\n>\n> diff --git a/sequencer.c b/sequencer.c\n> index 53ee49a..2dac106 100644\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n> \t\tconst char *append_newlines = NULL;\n> \t\tsize_t len = msgbuf->len - ignore_footer;\n> \n> -\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n> +\t\t/* ensure a blank line precedes our signoff */\n> +\t\tif (!len || msgbuf->buf[len - 1] != '\\n')\n> \t\t\tappend_newlines = \"\\n\\n\";\n> -\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n> +\t\telse if (len == 1 || msgbuf->buf[len - 2] != '\\n')\n> \t\t\tappend_newlines = \"\\n\";\n\nMaybe I am getting slower with age, but it took me 5 minutes of\nstaring the above to convince me that it is doing the right thing.\nThe if/elseif cascade is dealing with three separate things and the\nlogic is a bit dense:\n\n * Is the buffer completely empty? We need to add two LFs to give a\n room for the title and body;\n\n * Otherwise:\n\n - Is the final line incomplete? We need to add one LF to make it a\n complete line whatever we do.\n\n - Is the final line an empty line? We need to add one more LF to\n make sure we have a blank line before we add S-o-b.\n\nI wondered if we can rewrite it to make the logic clearer (that is\nwhere I spent most of the 5 minutes), but I did not think of a\nbetter way; probably the above is the best we could do.\n\nThanks.\n\nBy the way, I think we would want to introduce a symbolic constants\nfor the possible return values from has_conforming_footer(). The\ncheck that appears after this hunk\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\nis hard to grok without them.\n"},{"id":"210061","messageId":"CA+sFfMdok7wRDhgq7i=b3cu3LB+poExvxLBxYkg8L3pN92bEYg@mail.gmail.com","threadId":"32888","inReplyTo":"7vbobcdwo7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-02-22T22:03:42Z","receivedAt":"2013-02-22T22:03:42Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Fri, Feb 22, 2013 at 10:35 AM, Junio C Hamano wrote:\n> Brandon Casey writes:\n\n>> diff --git a/sequencer.c b/sequencer.c\n>> index 53ee49a..2dac106 100644\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -1127,9 +1127,10 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n>> const char *append_newlines = NULL;\n>> size_t len = msgbuf->len - ignore_footer;\n>>\n>> - if (len && msgbuf->buf[len - 1] != '\\n')\n>> + /* ensure a blank line precedes our signoff */\n>> + if (!len || msgbuf->buf[len - 1] != '\\n')\n>> append_newlines = \"\\n\\n\";\n>> - else if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n>> + else if (len == 1 || msgbuf->buf[len - 2] != '\\n')\n>> append_newlines = \"\\n\";\n>\n> Maybe I am getting slower with age, but it took me 5 minutes of\n> staring the above to convince me that it is doing the right thing.\n>\n> The if/elseif cascade is dealing with three separate things and the\n> logic is a bit dense:\n>\n> * Is the buffer completely empty? We need to add two LFs to give a\n> room for the title and body;\n>\n> * Otherwise:\n>\n> - Is the final line incomplete? We need to add one LF to make it a\n> complete line whatever we do.\n>\n> - Is the final line an empty line? We need to add one more LF to\n> make sure we have a blank line before we add S-o-b.\n>\n> I wondered if we can rewrite it to make the logic clearer (that is\n> where I spent most of the 5 minutes), but I did not think of a\n> better way; probably the above is the best we could do.\n\nWe could unroll the conditionals into individual blocks and add your\ncomments from above like:\n\n if (!len) {\n /* The buffer is completely empty. Leave room for the title and body. */\n append_newlines = \"\\n\\n\";\n } else if (msgbuf->buf[len - 1] != '\\n') {\n /* Incomplete line. Complete the line and add a blank one */\n append_newlines = \"\\n\\n\";\n } else if (len == 1) {\n /*\n * Buffer contains a single newline. Add another so that we leave\n * room for the title and body.\n */\n append_newlines = \"\\n\";\n } ...\n\nNot sure that it will reduce the amount of time needed to understand\nwhat's going on, but at least it describes the expectations made by\neach block.\n\n> Thanks.\n>\n> By the way, I think we would want to introduce a symbolic constants\n> for the possible return values from has_conforming_footer(). The\n> check that appears after this hunk\n>\n> if (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n> strbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n> sob.buf, sob.len);\n>\n> is hard to grok without them.\n\nYeah, Jonathan said the same thing and I agree. I was hoping someone\nelse would beat me to it.\n\n-Brandon\n"},{"id":"210062","messageId":"1361570727-20255-1-git-send-email-bcasey@nvidia.com","threadId":"32888","inReplyTo":"CA+sFfMdok7wRDhgq7i=b3cu3LB+poExvxLBxYkg8L3pN92bEYg@mail.gmail.com","subject":"[PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-02-22T22:05:27Z","receivedAt":"2013-02-22T22:05:27Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"From: Brandon Casey \n\nBefore commit 33f2f9ab, 'commit -s' would populate the edit buffer with\na blank line before the Signed-off-by line. This provided a nice\nhint to the user that something should be filled in. Let's restore that\nbehavior, but now let's ensure that the Signed-off-by line is preceded\nby two blank lines to hint that something should be filled in, and that\na blank line should separate it from the Signed-off-by line.\n\nPlus, add a test for this behavior.\n\nReported-by: John Keeping \nSigned-off-by: Brandon Casey \n---\n\nHow about something like this?\n\n-Brandon\n\n sequencer.c | 27 +++++++++++++++++++++++++--\n t/t7502-commit.sh | 12 ++++++++++++\n 2 files changed, 37 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 53ee49a..a07d2d0 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1127,10 +1127,33 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, unsigned flag)\n \t\tconst char *append_newlines = NULL;\n \t\tsize_t len = msgbuf->len - ignore_footer;\n \n-\t\tif (len && msgbuf->buf[len - 1] != '\\n')\n+\t\tif (!len) {\n+\t\t\t/*\n+\t\t\t * The buffer is completely empty. Leave foom for\n+\t\t\t * the title and body to be filled in by the user.\n+\t\t\t */\n \t\t\tappend_newlines = \"\\n\\n\";\n-\t\telse if (len > 1 && msgbuf->buf[len - 2] != '\\n')\n+\t\t} else if (msgbuf->buf[len - 1] != '\\n') {\n+\t\t\t/*\n+\t\t\t * Incomplete line. Complete the line and add a\n+\t\t\t * blank one so that there is an empty line between\n+\t\t\t * the message body and the sob.\n+\t\t\t */\n+\t\t\tappend_newlines = \"\\n\\n\";\n+\t\t} else if (len == 1) {\n+\t\t\t/*\n+\t\t\t * Buffer contains a single newline. Add another\n+\t\t\t * so that we leave room for the title and body.\n+\t\t\t */\n+\t\t\tappend_newlines = \"\\n\";\n+\t\t} else if (msgbuf->buf[len - 2] != '\\n') {\n+\t\t\t/*\n+\t\t\t * Buffer ends with a single newline. Add another\n+\t\t\t * so that there is an empty line between the message\n+\t\t\t * body and the sob.\n+\t\t\t */\n \t\t\tappend_newlines = \"\\n\";\n+\t\t} /* else, the buffer already ends with two newlines. */\n \n \t\tif (append_newlines)\n \t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex deb187e..a53a1e0 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -349,6 +349,18 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo\n \n '\n \n+test_expect_success 'commit -s places sob on third line after two empty lines' '\n+\tgit commit -s --allow-empty --allow-empty-message &&\n+\tcat <<-EOF >expect &&\n+\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\n+\tEOF\n+\tegrep -v '^#' .git/COMMIT_EDITMSG >actual &&\n+\ttest_cmp expect actual\n+'\n+\n write_script .git/FAKE_EDITOR <<\\EOF\n mv \"$1\" \"$1.orig\"\n (\n-- \n1.8.1.3.566.gaa39828\n"},{"id":"210065","messageId":"20130222223513.GA21579@sigill.intra.peff.net","threadId":"32888","inReplyTo":"1361570727-20255-1-git-send-email-bcasey@nvidia.com","subject":"Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-02-22T22:35:13Z","receivedAt":"2013-02-22T22:35:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 22, 2013 at 02:05:27PM -0800, Brandon Casey wrote:\n\n> From: Brandon Casey \n> \n> Before commit 33f2f9ab, 'commit -s' would populate the edit buffer with\n> a blank line before the Signed-off-by line. This provided a nice\n> hint to the user that something should be filled in. Let's restore that\n> behavior, but now let's ensure that the Signed-off-by line is preceded\n> by two blank lines to hint that something should be filled in, and that\n> a blank line should separate it from the Signed-off-by line.\n> \n> Plus, add a test for this behavior.\n> \n> Reported-by: John Keeping \n> Signed-off-by: Brandon Casey \n> ---\n> \n> How about something like this?\n\nFWIW, as a casual reader of this series, I find this to be way easier\nto follow than the previous round.\n\n-Peff\n"},{"id":"210066","messageId":"7v38woc6tf.fsf@alter.siamese.dyndns.org","threadId":"32888","inReplyTo":"20130222223513.GA21579@sigill.intra.peff.net","subject":"Re: [PATCH v2] git-commit: populate the edit buffer with 2 blank lines before s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-22T22:38:52Z","receivedAt":"2013-02-22T22:38:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> FWIW, as a casual reader of this series, I find this to be way easier\n> to follow than the previous round.\n\nIt is assuring to know that I am not the only one getting slow with\nage ;-)\n\nThanks.\n"},{"id":"319626","messageId":"CACBZZX543mhEDEZ78KH=GU++u0Gq=YGt-WXye1nSC=EirRiO-g@mail.gmail.com","threadId":"32888","inReplyTo":"1360664260-11803-4-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v4 03/12] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2017-05-13T17:41:20Z","receivedAt":"2017-05-13T17:42:13Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"On Tue, Feb 12, 2013 at 11:17 AM, Brandon Casey wrote:\n> The part of test_commit() may not be appropriate for a tag name.\n> So let's allow test_commit to accept a fourth argument to specify the tag\n> name.\n\n[Kind of late to notice, I know]\n\nI see nobody spotted in four rounds of reviews that this commit didn't\nupdate the corresponding t/README docs for test_commit.\n\n> Signed-off-by: Brandon Casey \n> Reviewed-by: Jonathan Nieder \n> ---\n> t/test-lib-functions.sh | 8 ++++----\n> 1 file changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> index 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> fi\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> notick= &&\n> @@ -168,7 +168,7 @@ test_commit () {\n> test_tick\n> fi &&\n> git commit $signoff -m \"$1\" &&\n> - git tag \"$1\"\n> + git tag \"${4:-$1}\"\n> }\n>\n> # Call test_merge with the arguments \" \", where \n> --\n> 1.8.1.3.579.gd9af3b6\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"}]}