{"thread":{"id":"32682","subject":"[PATCH v2 00/10] unify appending of sob","startedAt":"2013-01-21T08:40:17Z","lastAt":"2013-02-09T23:49:19Z","messageCount":31,"participants":["Brandon Casey","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":10},"messages":[{"id":"207373","messageId":"1358757627-16682-1-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":null,"subject":"[PATCH v2 00/10] unify appending of sob","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:17Z","receivedAt":"2013-01-21T08:40:17Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Here's version 2 of the unify-appending-of-sob series. Hopefully this\naddresses the comments made on the first series:\n\n http://thread.gmane.org/gmane.comp.version-control.git/210390\n\nThe main difference is that the detection of the \"(cherry picked from ...)\"\nline has been relaxed, and the modifications to log-tree.c have been dropped.\n\nHere's the inter-diff of this series against the original series, both built\non top of 2d242fb3fc19fc9ba046accdd9210be8b9913f64 (the actual series in the\nfollowing emails is of course built on top of master).\n\ndiff --git a/revision.h b/revision.h\nindex 435a60b..d20defa 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -137,7 +137,7 @@ struct rev_info {\n \tint\t\tnumbered_files;\n \tchar\t\t*message_id;\n \tstruct string_list *ref_message_ids;\n-\tint 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;\ndiff --git a/sequencer.c b/sequencer.c\nindex eb93dd6..54b3cb9 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -36,13 +36,18 @@ static int is_rfc2822_line(const char *buf, int len)\n \treturn 1;\n }\n \n-static int is_cherry_pick_from_line(const char *buf, int len)\n+static int is_cherry_picked_from_line(const char *buf, int len)\n {\n-\treturn (strlen(cherry_picked_prefix) + 41) <= len &&\n-\t\t!prefixcmp(buf, cherry_picked_prefix);\n+\t/*\n+\t * We only care that it looks roughly like (cherry picked from ...)\n+\t */\n+\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n+\t\t(buf[len - 1] == ')' ||\n+\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n }\n \n-/* Returns 0 for non-conforming 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@@ -51,7 +56,7 @@ static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n \tint ignore_footer)\n {\n \tint hit = 0;\n-\tint i, k = 0;\n+\tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \tint found_sob = 0;\n@@ -76,12 +81,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);\n \t\tif (found_rfc2822 && sob &&\n-\t\t\t!strncasecmp(buf+i, sob->buf, sob->len))\n+\t\t\t!strncmp(buf + i, sob->buf, sob->len))\n \t\t\tfound_sob = k;\n \n-\t\tif (!(found_rfc2822 || is_cherry_pick_from_line(buf+i, k-i)))\n+\t\tif (!(found_rfc2822 ||\n+\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n \t\t\treturn 0;\n \t}\n \tif (found_sob == i)\n@@ -1103,11 +1109,20 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)\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 (msgbuf->buf[i] != '\\n' && (!i || !(has_footer =\n-\t\thas_conforming_footer(msgbuf, &sob, ignore_footer))))\n-\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\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+\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/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex c53dc4b..6d00e43 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1211,16 +1211,17 @@ 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+Bug: 1234\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:Signed-off-by: C O Mitter \n EOF\n \ttest_cmp expected actual\n '\n\nBrandon Casey (8):\n sequencer.c: remove broken support for rfc2822 continuation in footer\n t/test-lib-functions.sh: allow to specify the tag name to test_commit\n t/t3511: add some tests of 'cherry-pick -s' functionality\n sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b\n footer\n sequencer.c: always separate \"(cherry picked from\" from commit body\n sequencer.c: teach append_signoff how to detect duplicate s-o-b\n sequencer.c: teach append_signoff to avoid adding a duplicate newline\n Unify appending signoff in format-patch, commit and sequencer\n\nNguyễn Thái Ngọc Duy (2):\n t4014: more tests about appending s-o-b lines\n format-patch: update append_signoff prototype\n\n builtin/commit.c | 2 +-\n builtin/log.c | 13 +--\n log-tree.c | 92 ++---------------\n revision.h | 2 +-\n sequencer.c | 146 +++++++++++++++++---------\n sequencer.h | 2 +-\n t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 263 +++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 9 +-\n 9 files changed, 595 insertions(+), 153 deletions(-)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207374","messageId":"1358757627-16682-2-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:18Z","receivedAt":"2013-01-21T08:40:18Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Commit c1e01b0c generalized the detection of the last paragraph\nsigned-off-by footer and used rfc2822 as a guideline. Support for rfc2822\nstyle continuation lines was also implemented, but not correctly, so it has\nnever detected a line beginning with space or tab as a continuation of the\nprevious line.\n\nSince a commit message is not governed by the line length limits imposed\nby rfc2822 for email messages, and it does not seem like this functionality\nwould produce \"better\" commit messages anyway, let's remove this broken\nfunctionality.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 6 ------\n 1 file changed, 6 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex aef5e8a..fe25ef4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1027,7 +1027,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \tint hit = 0;\n \tint i, j, k;\n \tint len = sb->len - ignore_footer;\n-\tint first = 1;\n \tconst char *buf = sb->buf;\n \n \tfor (i = len - 1; i > 0; i--) {\n@@ -1044,11 +1043,6 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n-\t\t\tcontinue;\n-\n-\t\tfirst = 0;\n-\n \t\tfor (j = 0; i + j < len; j++) {\n \t\t\tch = buf[i + j];\n \t\t\tif (ch == ':')\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207377","messageId":"1358757627-16682-3-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:19Z","receivedAt":"2013-01-21T08:40:19Z","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 \n---\n t/test-lib-functions.sh | 9 +++++----\n 1 file changed, 5 insertions(+), 4 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fa62d01..c601918 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -135,12 +135,13 @@ 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. It will also add a tag with as name unless is\n+# given.\n #\n-# Both and default to .\n+# , , and all default to .\n \n test_commit () {\n \tnotick= &&\n@@ -168,7 +169,7 @@ test_commit () {\n \t\ttest_tick\n \tfi &&\n \tgit commit $signoff -m \"$1\" &&\n-\tgit tag \"$1\"\n+\tgit tag \"${4:-$1}\"\n }\n \n # Call test_merge with the arguments \" \", where \n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207384","messageId":"1358757627-16682-4-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:20Z","receivedAt":"2013-01-21T08:40:20Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Add some tests to ensure that 'cherry-pick -s' operates in the following\nmanner:\n\n * Inserts a blank line before appending a s-o-b to a commit message that\n does not contain a s-o-b footer\n\n * Does not mistake first line \"subject: description\" as a s-o-b footer\n\n * Does not mistake single word message body as conforming to rfc2822\n\n * Appends a s-o-b when last s-o-b in footer does not match committer\n s-o-b, even when committer's s-o-b exists elsewhere in footer.\n\n * Does not append a s-o-b when last s-o-b matches committer s-o-b\n\n * Correctly detects a non-conforming footer containing a mix of s-o-b\n like elements and s-o-b elements. (marked \"expect failure\")\n\nSigned-off-by: Brandon Casey \n---\n t/t3511-cherry-pick-x.sh | 111 +++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 111 insertions(+)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nnew file mode 100755\nindex 0000000..a6c4168\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+Signed-off-by: B.U. Thor \n+Not a valid footer element\n+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\n+mesg_with_footer_sob=\"$mesg_with_footer\n+Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\n+\n+test_expect_success setup '\n+\tgit config advice.detachedhead false &&\n+\techo unrelated >unrelated &&\n+\tgit add unrelated &&\n+\ttest_commit initial foo a &&\n+\ttest_commit \"$mesg_one_line\" foo b mesg-one-line &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_no_footer\" foo b mesg-no-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_broken_footer\" foo b mesg-broken-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_footer\" foo b mesg-with-footer &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_footer_sob\" foo b mesg-with-footer-sob &&\n+\tpristine_detach initial &&\n+\ttest_commit conflicting unrelated\n+'\n+\n+test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-one-line &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_line\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-broken-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_broken_footer\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-footer-sob &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer_sob\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207378","messageId":"1358757627-16682-5-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 04/10] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:21Z","receivedAt":"2013-01-21T08:40:21Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"When 'cherry-pick -s' is used to append a signed-off-by line to a cherry\npicked commit, it does not currently detect the \"(cherry picked from...\"\nthat may have been appended by a previous 'cherry-pick -x' as part of the\ns-o-b footer and it will insert a blank line before appending a new s-o-b.\n\nLet's detect \"(cherry picked from...)\" as part of the footer so that we\nwill produce this:\n\n Signed-off-by: A U Thor \n (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n Signed-off-by: C O Mmitter \n\ninstead of this:\n\n Signed-off-by: A U Thor \n (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n\n Signed-off-by: C O Mmitter \n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 46 ++++++++++++++++++++++++++++------------\n t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 88 insertions(+), 13 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fe25ef4..fe76a1d 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -18,6 +18,7 @@\n #define GIT_REFLOG_ACTION \"GIT_REFLOG_ACTION\"\n \n const char sign_off_header[] = \"Signed-off-by: \";\n+static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n static void remove_sequencer_state(void)\n {\n@@ -496,7 +497,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)\n \t\t}\n \n \t\tif (opts->record_origin) {\n-\t\t\tstrbuf_addstr(&msgbuf, \"(cherry picked from commit \");\n+\t\t\tstrbuf_addstr(&msgbuf, cherry_picked_prefix);\n \t\t\tstrbuf_addstr(&msgbuf, sha1_to_hex(commit->object.sha1));\n \t\t\tstrbuf_addstr(&msgbuf, \")\\n\");\n \t\t}\n@@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n-static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n+static int is_rfc2822_line(const char *buf, int len)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tint ch = buf[i];\n+\t\tif (ch == ':')\n+\t\t\tbreak;\n+\t\tif (isalnum(ch) || (ch == '-'))\n+\t\t\tcontinue;\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_cherry_picked_from_line(const char *buf, int len)\n+{\n+\t/*\n+\t * We only care that it looks roughly like (cherry picked from ...)\n+\t */\n+\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n+\t\t(buf[len - 1] == ')' ||\n+\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n {\n-\tint ch;\n \tint hit = 0;\n-\tint i, j, k;\n+\tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n \n@@ -1043,15 +1069,9 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tfor (j = 0; i + j < len; j++) {\n-\t\t\tch = buf[i + j];\n-\t\t\tif (ch == ':')\n-\t\t\t\tbreak;\n-\t\t\tif (isalnum(ch) ||\n-\t\t\t (ch == '-'))\n-\t\t\t\tcontinue;\n+\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n \t\t\treturn 0;\n-\t\t}\n \t}\n \treturn 1;\n }\n@@ -1068,7 +1088,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 a6c4168..32c0bb1 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -32,6 +32,10 @@ Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n mesg_with_footer_sob=\"$mesg_with_footer\n Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n \n+mesg_with_cherry_footer=\"$mesg_with_footer_sob\n+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Tested-by: C.U. Thor \"\n+\n \n test_expect_success setup '\n \tgit config advice.detachedhead false &&\n@@ -47,6 +51,8 @@ test_expect_success setup '\n \ttest_commit \"$mesg_with_footer\" foo b mesg-with-footer &&\n \tgit reset --hard initial &&\n \ttest_commit \"$mesg_with_footer_sob\" foo b mesg-with-footer-sob &&\n+\tgit reset --hard initial &&\n+\ttest_commit \"$mesg_with_cherry_footer\" foo b mesg-with-cherry-footer &&\n \tpristine_detach initial &&\n \ttest_commit conflicting unrelated\n '\n@@ -98,6 +104,19 @@ test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committe\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-footer^0` &&\n+\tgit cherry-pick -x -s mesg-with-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-footer-sob &&\n@@ -108,4 +127,40 @@ test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob'\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s adds sob even when trailing sob exists for committer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-footer-sob^0` &&\n+\tgit cherry-pick -x -s mesg-with-footer-sob &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_footer_sob\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -x treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-cherry-footer^0` &&\n+\tgit cherry-pick -x mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tgit cherry-pick -s mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207376","messageId":"1358757627-16682-6-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 05/10] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:22Z","receivedAt":"2013-01-21T08:40:22Z","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 \n---\n sequencer.c | 114 +++++++++++++++++++++++++----------------------\n t/t3511-cherry-pick-x.sh | 53 ++++++++++++++++++++++\n 2 files changed, 113 insertions(+), 54 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex fe76a1d..163dc12 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -20,6 +20,64 @@\n const char sign_off_header[] = \"Signed-off-by: \";\n static const char cherry_picked_prefix[] = \"(cherry picked from commit \";\n \n+static int is_rfc2822_line(const char *buf, int len)\n+{\n+\tint i;\n+\n+\tfor (i = 0; i < len; i++) {\n+\t\tint ch = buf[i];\n+\t\tif (ch == ':')\n+\t\t\tbreak;\n+\t\tif (isalnum(ch) || (ch == '-'))\n+\t\t\tcontinue;\n+\t\treturn 0;\n+\t}\n+\n+\treturn 1;\n+}\n+\n+static int is_cherry_picked_from_line(const char *buf, int len)\n+{\n+\t/*\n+\t * We only care that it looks roughly like (cherry picked from ...)\n+\t */\n+\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n+\t\t(buf[len - 1] == ')' ||\n+\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n+}\n+\n+static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+{\n+\tint hit = 0;\n+\tint i, k;\n+\tint len = sb->len - ignore_footer;\n+\tconst char *buf = sb->buf;\n+\n+\tfor (i = len - 1; i > 0; i--) {\n+\t\tif (hit && buf[i] == '\\n')\n+\t\t\tbreak;\n+\t\thit = (buf[i] == '\\n');\n+\t}\n+\n+\t/* require at least one blank line */\n+\tif (!hit || buf[i] != '\\n')\n+\t\treturn 0;\n+\n+\twhile (i < len - 1 && buf[i] == '\\n')\n+\t\ti++;\n+\n+\tfor (; i < len; i = k) {\n+\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n+\t\t\t; /* do nothing */\n+\t\tk++;\n+\n+\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n+\t\t\treturn 0;\n+\t}\n+\treturn 1;\n+}\n+\n static void remove_sequencer_state(void)\n {\n \tstruct strbuf seq_dir = STRBUF_INIT;\n@@ -497,6 +555,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,60 +1082,6 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n \treturn pick_commits(todo_list, opts);\n }\n \n-static int is_rfc2822_line(const char *buf, int len)\n-{\n-\tint i;\n-\n-\tfor (i = 0; i < len; i++) {\n-\t\tint ch = buf[i];\n-\t\tif (ch == ':')\n-\t\t\tbreak;\n-\t\tif (isalnum(ch) || (ch == '-'))\n-\t\t\tcontinue;\n-\t\treturn 0;\n-\t}\n-\n-\treturn 1;\n-}\n-\n-static int is_cherry_picked_from_line(const char *buf, int len)\n-{\n-\t/*\n-\t * We only care that it looks roughly like (cherry picked from ...)\n-\t */\n-\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n-\t\t(buf[len - 1] == ')' ||\n-\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n-}\n-\n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n-{\n-\tint hit = 0;\n-\tint i, k;\n-\tint len = sb->len - ignore_footer;\n-\tconst char *buf = sb->buf;\n-\n-\tfor (i = len - 1; i > 0; i--) {\n-\t\tif (hit && buf[i] == '\\n')\n-\t\t\tbreak;\n-\t\thit = (buf[i] == '\\n');\n-\t}\n-\n-\twhile (i < len - 1 && buf[i] == '\\n')\n-\t\ti++;\n-\n-\tfor (; i < len; i = k) {\n-\t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n-\t\t\t; /* do nothing */\n-\t\tk++;\n-\n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n-\t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n-\t\t\treturn 0;\n-\t}\n-\treturn 1;\n-}\n-\n void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n {\n \tstruct strbuf sob = STRBUF_INIT;\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 32c0bb1..9dd6d5d 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -57,6 +57,19 @@ test_expect_success setup '\n \ttest_commit conflicting unrelated\n '\n \n+test_expect_success 'cherry-pick -x inserts blank line after one line subject' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-one-line^0` &&\n+\tgit cherry-pick -x mesg-one-line &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_one_line\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-one-line &&\n@@ -81,6 +94,19 @@ test_expect_failure 'cherry-pick -s inserts blank line after non-conforming foot\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-no-footer^0` &&\n+\tgit cherry-pick -x mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\t(cherry picked from commit $sha1)\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-no-footer &&\n@@ -93,6 +119,20 @@ test_expect_success 'cherry-pick -s inserts blank line when conforming footer no\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s inserts blank line when conforming footer not found' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-no-footer^0` &&\n+\tgit cherry-pick -x -s mesg-no-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_no_footer\n+\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-with-footer &&\n@@ -163,4 +203,17 @@ test_expect_success 'cherry-pick -s treats \"(cherry picked from...\" line as part\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'cherry-pick -x -s treats \"(cherry picked from...\" line as part of footer' '\n+\tpristine_detach initial &&\n+\tsha1=`git rev-parse mesg-with-cherry-footer^0` &&\n+\tgit cherry-pick -x -s mesg-with-cherry-footer &&\n+\tcat <<-EOF >expect &&\n+\t\t$mesg_with_cherry_footer\n+\t\t(cherry picked from commit $sha1)\n+\t\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\tEOF\n+\tgit log -1 --pretty=format:%B >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207379","messageId":"1358757627-16682-7-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:23Z","receivedAt":"2013-01-21T08:40:23Z","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 | 46 ++++++++++++++++++++++++++++++++++++----------\n sequencer.h | 2 +-\n t/t3511-cherry-pick-x.sh | 2 +-\n 4 files changed, 39 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 7c2a3d4..081ff66 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 163dc12..d4a2ece 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -46,12 +46,20 @@ static int is_cherry_picked_from_line(const char *buf, int len)\n \t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n }\n \n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n+/*\n+ * Returns 0 for non-conforming footer\n+ * Returns 1 for conforming footer\n+ * Returns 2 when sob exists within conforming footer\n+ * Returns 3 when sob exists within conforming footer as last entry\n+ */\n+static int has_conforming_footer(struct strbuf *sb, struct strbuf *sob,\n+\tint ignore_footer)\n {\n \tint hit = 0;\n \tint i, k;\n \tint len = sb->len - ignore_footer;\n \tconst char *buf = sb->buf;\n+\tint found_sob = 0;\n \n \tfor (i = len - 1; i > 0; i--) {\n \t\tif (hit && buf[i] == '\\n')\n@@ -67,14 +75,25 @@ static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\n \t\ti++;\n \n \tfor (; i < len; i = k) {\n+\t\tint found_rfc2822;\n+\n \t\tfor (k = i; k < len && buf[k] != '\\n'; k++)\n \t\t\t; /* do nothing */\n \t\tk++;\n \n-\t\tif (!(is_rfc2822_line(buf + i, k - i) ||\n+\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i);\n+\t\tif (found_rfc2822 && sob &&\n+\t\t\t!strncmp(buf + i, sob->buf, sob->len))\n+\t\t\tfound_sob = k;\n+\n+\t\tif (!(found_rfc2822 ||\n \t\t\tis_cherry_picked_from_line(buf + i, k - i)))\n \t\t\treturn 0;\n \t}\n+\tif (found_sob == i)\n+\t\treturn 3;\n+\tif (found_sob)\n+\t\treturn 2;\n \treturn 1;\n }\n \n@@ -296,7 +315,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@@ -555,7 +574,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@@ -1082,9 +1101,10 @@ 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, int no_dup_sob)\n {\n \tstruct strbuf sob = STRBUF_INIT;\n+\tint has_footer = 0;\n \tint i;\n \n \tstrbuf_addstr(&sob, sign_off_header);\n@@ -1093,10 +1113,16 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer)\n \tstrbuf_addch(&sob, '\\n');\n \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n \t\t; /* do nothing */\n-\tif (prefixcmp(msgbuf->buf + i, sob.buf)) {\n-\t\tif (!i || !has_conforming_footer(msgbuf, ignore_footer))\n-\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n-\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, sob.buf, sob.len);\n-\t}\n+\n+\tif (i)\n+\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n+\n+\tif (!has_footer)\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n+\n+\tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n+\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\tsob.buf, sob.len);\n+\n \tstrbuf_release(&sob);\n }\ndiff --git a/sequencer.h b/sequencer.h\nindex 9d57d57..c4c7132 100644\n--- a/sequencer.h\n+++ b/sequencer.h\n@@ -48,6 +48,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, int no_dup_sob);\n \n #endif\ndiff --git a/t/t3511-cherry-pick-x.sh b/t/t3511-cherry-pick-x.sh\nindex 9dd6d5d..4b67343 100755\n--- a/t/t3511-cherry-pick-x.sh\n+++ b/t/t3511-cherry-pick-x.sh\n@@ -82,7 +82,7 @@ test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n \ttest_cmp expect actual\n '\n \n-test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n+test_expect_success 'cherry-pick -s inserts blank line after non-conforming footer' '\n \tpristine_detach initial &&\n \tgit cherry-pick -s mesg-broken-footer &&\n \tcat <<-EOF >expect &&\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207380","messageId":"1358757627-16682-8-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 07/10] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:24Z","receivedAt":"2013-01-21T08:40:24Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"Teach append_signoff to detect whether a blank line exists at the position\nthat the signed-off-by line will be added, and avoid adding an additional\none if one already exists. This is necessary to allow format-patch to add a\ns-o-b to a patch with no commit message without adding an extra newline. A\nfollowing patch will make format-patch use this function rather than the\nappend_signoff implementation inside log-tree.c.\n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 14 +++++++++-----\n 1 file changed, 9 insertions(+), 5 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex d4a2ece..d51e6f2 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1114,11 +1114,15 @@ void append_signoff(struct strbuf *msgbuf, int ignore_footer, int no_dup_sob)\n \tfor (i = msgbuf->len - 1 - ignore_footer; i > 0 && msgbuf->buf[i - 1] != '\\n'; i--)\n \t\t; /* do nothing */\n \n-\tif (i)\n-\t\thas_footer = has_conforming_footer(msgbuf, &sob, ignore_footer);\n-\n-\tif (!has_footer)\n-\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0, \"\\n\", 1);\n+\tif (msgbuf->buf[i] != '\\n') {\n+\t\tif (i)\n+\t\t\thas_footer = has_conforming_footer(msgbuf, &sob,\n+\t\t\t\t\tignore_footer);\n+\n+\t\tif (!has_footer)\n+\t\t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n+\t\t\t\t\t\"\\n\", 1);\n+\t}\n \n \tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n \t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207381","messageId":"1358757627-16682-9-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 08/10] t4014: more tests about appending s-o-b lines","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:25Z","receivedAt":"2013-01-21T08:40:25Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\n[bc: Squash the tests from Duy's original unify-appending-sob series.\n\n Fix test 90 \"signoff: some random signoff-alike\" and mark as failing.\n Correct behavior should insert a blank line after message body and\n signed-off-by.\n\n Add two additional tests:\n\n 1. failure to detect non-conforming elements in the footer when last\n line matches committer's s-o-b.\n 2. ensure various s-o-b -like elements in the footer are handled as\n conforming. e.g. \"Change-id: IXXXX or Bug: 1234\"\n]\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n t/t4014-format-patch.sh | 242 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 242 insertions(+)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 7fa3647..3868cef 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1021,4 +1021,246 @@ test_expect_success 'cover letter using branch description (6)' '\n \tgrep hello actual >/dev/null\n '\n \n+append_signoff()\n+{\n+\tC=`git commit-tree HEAD^^{tree} -p HEAD` &&\n+\tgit format-patch --stdout --signoff ${C}^..${C} |\n+\t\ttee append_signoff.patch |\n+\t\tsed -n \"1,/^---$/p\" |\n+\t\tgrep -n -E \"^Subject|Sign|^$\"\n+}\n+\n+test_expect_success 'signoff: commit with no body' '\n+\tappend_signoff actual &&\n+\tcat <<\\EOF | sed \"s/EOL$//\" >expected &&\n+4:Subject: [PATCH] EOL\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: commit with only subject' '\n+\techo subject | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: commit with only subject that does not end with NL' '\n+\tprintf subject | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: no existing signoffs' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: no existing signoffs and no trailing NL' '\n+\tprintf \"subject\\n\\nbody\" | append_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: some random signoff' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: my@house\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: some random signoff-alike' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+Fooled-by-me: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: not really a signoff' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+I want to mention about Signed-off-by: here.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:I want to mention about Signed-off-by: here.\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: not really a signoff (2)' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+My unfortunate\n+Signed-off-by: example happens to be wrapped here.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:Signed-off-by: example happens to be wrapped here.\n+11:\n+12:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+Signed-off-by: my@house\n+Signed-off-by: your@house\n+\n+A lot of houses.\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: my@house\n+10:Signed-off-by: your@house\n+11:\n+13:\n+14:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff at the end' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: C O Mitter \n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff at the end, no trailing NL' '\n+\tprintf \"subject\\n\\nSigned-off-by: C O Mitter \" |\n+\t\tappend_signoff >actual &&\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+9:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: the same signoff NOT at the end' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: C O Mitter \n+Signed-off-by: my@house\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: C O Mitter \n+12:Signed-off-by: my@house\n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_failure 'signoff: detect garbage in non-conforming footer' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Tested-by: my@house\n+Some Trash\n+Signed-off-by: C O Mitter \n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+13:Signed-off-by: C O Mitter \n+14:\n+15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: footer begins with non-signoff without @ sign' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Reviewed-id: Noone\n+Tested-by: my@house\n+Change-id: Ideadbeef\n+Signed-off-by: C O Mitter \n+Bug: 1234\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+14:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207383","messageId":"1358757627-16682-10-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 09/10] format-patch: update append_signoff prototype","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:26Z","receivedAt":"2013-01-21T08:40:26Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Nguyễn Thái Ngọc Duy \n\nThis is a preparation step for merging with append_signoff from\nsequencer.c\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n builtin/log.c | 13 +------------\n log-tree.c | 14 ++++++++++----\n revision.h | 2 +-\n 3 files changed, 12 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..83f33f4 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -253,9 +253,11 @@ 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, int no_dup_sob)\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 +277,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 +290,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 +676,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 +692,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, 1);\n \n \tif ((ctx.fmt != CMIT_FMT_USERFORMAT) &&\n \t ctx.notes_message && *ctx.notes_message) {\ndiff --git a/revision.h b/revision.h\nindex 5da09ee..01bd2b7 100644\n--- a/revision.h\n+++ b/revision.h\n@@ -138,7 +138,7 @@ struct rev_info {\n \tint\t\treroll_count;\n \tchar\t\t*message_id;\n \tstruct string_list *ref_message_ids;\n-\tconst char\t*add_signoff;\n+\tint\t\tadd_signoff;\n \tconst char\t*extra_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207382","messageId":"1358757627-16682-11-git-send-email-drafnel@gmail.com","threadId":"32682","inReplyTo":"1358757627-16682-1-git-send-email-drafnel@gmail.com","subject":"[PATCH v2 10/10] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-21T08:40:27Z","receivedAt":"2013-01-21T08:40:27Z","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 | 88 +------------------------------------------------\n t/t4014-format-patch.sh | 31 ++++++++++++++---\n 2 files changed, 27 insertions(+), 92 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 83f33f4..299dad3 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -9,6 +9,7 @@\n #include \"string-list.h\"\n #include \"color.h\"\n #include \"gpg-interface.h\"\n+#include \"sequencer.h\"\n \n struct decoration name_decoration = { \"object names\" };\n \n@@ -206,93 +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, int no_dup_sob)\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-\n-\tcp = sb->buf;\n-\n-\t/* First see if we already have the sign-off by the signer */\n-\twhile ((cp = strstr(cp, signed_off_by))) {\n-\n-\t\thas_signoff = 1;\n-\n-\t\tcp += strlen(signed_off_by);\n-\t\tif (cp + signoff_len >= sb->buf + sb->len)\n-\t\t\tbreak;\n-\t\tif (strncmp(cp, signoff, signoff_len))\n-\t\t\tcontinue;\n-\t\tif (!isspace(cp[signoff_len]))\n-\t\t\tcontinue;\n-\t\t/* we already have him */\n-\t\tfree(signoff);\n-\t\treturn;\n-\t}\n-\n-\tif (!has_signoff)\n-\t\thas_signoff = detect_any_signoff(sb->buf, sb->len);\n-\n-\tif (!has_signoff)\n-\t\tstrbuf_addch(sb, '\\n');\n-\n-\tstrbuf_addstr(sb, signed_off_by);\n-\tstrbuf_add(sb, signoff, signoff_len);\n-\tstrbuf_addch(sb, '\\n');\n-\tfree(signoff);\n-}\n-\n static unsigned int digits_in_number(unsigned int number)\n {\n \tunsigned int i = 10, result = 1;\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 3868cef..d0ec097 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1104,7 +1104,28 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: some random signoff-alike' '\n+test_expect_success 'signoff: misc conforming footer elements' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\n+Signed-off-by: my@house\n+(cherry picked from commit da39a3ee5e6b4b0d3255bfef95601890afd80709)\n+Tested-by: Some One \n+Bug: 1234\n+EOF\n+\tcat >expected <<\\EOF &&\n+4:Subject: [PATCH] subject\n+8:\n+10:\n+11:Signed-off-by: my@house\n+15:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n+test_expect_success 'signoff: some random signoff-alike' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1120,7 +1141,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: not really a signoff' '\n+test_expect_success 'signoff: not really a signoff' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1136,7 +1157,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: not really a signoff (2)' '\n+test_expect_success 'signoff: not really a signoff (2)' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1153,7 +1174,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: valid S-o-b paragraph in the middle' '\n+test_expect_success 'signoff: valid S-o-b paragraph in the middle' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n@@ -1221,7 +1242,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: detect garbage in non-conforming footer' '\n+test_expect_success 'signoff: detect garbage in non-conforming footer' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n-- \n1.8.1.1.252.gdb33759\n"},{"id":"207487","messageId":"20130122075413.GB6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-2-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T07:54:13Z","receivedAt":"2013-01-22T07:54:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Casey wrote:\n\n> --- a/sequencer.c\n> +++ b/sequencer.c\n[...]\n> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n\nGit is checking if (sb->buf) ends with a \"Signed-off-by:\" style\nline. If it doesn't, it will need to add an extra blank line\nbefore adding a new sign-off.\n\nFirst (snipped), it seeks back two newlines from the end and then\nforward to the next non-newline character, so (buf + i) is at the\nstart of the last line of (the interesting part of) sb. Now:\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(buf + k) points to the end of this line.\n\n> -\t\tif ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n> -\t\t\tcontinue;\n\nThis is always the first line examined, so this \"continue\" never\ntriggers.\n\n> -\n> -\t\tfirst = 0;\n> -\n> \t\tfor (j = 0; i + j < len; j++) {\n\nIf the line matches /^[[:alnum:]-]*:/, it passes and git moves on to\nthe (nonexistent) next line. Otherwise, it fails.\n\nDo I understand correctly? If so, this patch should be a no-op, which\nis good, I guess.\n\nBut in that case, couldn't this function be made much simpler? As far\nas I can tell, all the function needs to do is the following:\n\n\t1. Find the last line.\n\t2. Check if it is blank or matches /^[[:alnum:]-]*:/\n\t3. There is no step 3. That's it.\n\nIn other words, something like:\n\n\tconst char *eol, *p;\n\n\t/* End of line */\n\teol = memrchr(sb->buf, '\\n', sb->len - ignore_footer);\n\tif (!eol)\n\t\teol = sb->buf;\n\n\t/* Start of line */\n\tp = memrchr(sb->buf, '\\n', eol - sb->buf);\n\tif (p)\n\t\tp++;\n\telse\n\t\tp = sb->buf;\n\n\tif (p == eol)\t/* Blank line? */\n\t\treturn 1;\n\n\t/* \"Signed-off-by\"-style field */\n\twhile ((isalnum(*p) || *p == '-') && p < eol)\n\t\tp++;\n\treturn *p == ':';\n\nwhere memrchr is defined roughly as follows[1]:\n\n\t#ifdef __GLIBC_PREREQ\n\t#if __GLIBC_PREREQ(2, 2)\n\t#define HAVE_MEMRCHR\n\t#endif\n\t#endif\n\n\t#ifndef HAVE_MEMRCHR\n\t#define memrchr gitmemrchr\n\tstatic inline void *gitmemrchr(const void *s, int c, size_t n)\n\t{\n\t\tconst unsigned char *p = s;\n\t\tp += n;\n\t\twhile (p != s)\n\t\t\tif (*--p == (unsigned char) c)\n\t\t\t\treturn p;\n\t\treturn NULL;\n\t}\n\t#endif\n\nDoes that look right?\n\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/159081/focus=159121\n"},{"id":"207488","messageId":"20130122080216.GC6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-3-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T08:02:16Z","receivedAt":"2013-01-22T08:02:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\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\nYes!\n\n[...]\n> --- a/t/test-lib-functions.sh\n> +++ b/t/test-lib-functions.sh\n> @@ -135,12 +135,13 @@ 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. It will also add a tag with as name unless is\n> +# given.\n> #\n> -# Both and default to .\n> +# , , and all default to .\n\nSimpler:\n\n # This will commit a file with the given contents and the given commit\n # message and tag the resulting commit with the given tag name.\n #\n # , , and all default to .\n\nWith or without that change,\nReviewed-by: Jonathan Nieder \n"},{"id":"207490","messageId":"20130122081758.GD6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-4-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T08:17:59Z","receivedAt":"2013-01-22T08:17:59Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> --- /dev/null\n> +++ b/t/t3511-cherry-pick-x.sh\n> @@ -0,0 +1,111 @@\n[...]\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\nNeat.\n\n[...]\n> +test_expect_success 'cherry-pick -s inserts blank line after one line subject' '\n\nIn particular, a blank line after a one-line subject that starts with\nthe usual \"subsystem:\" convention is not mistaken for an RFC2822-style\nheader. Good.\n\n[...]\n> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n\nIIUC this is an illustration of false-positives from messages like\nthis one:\n\n\tbase: do something great without a sign-off\n\n\tIf he does that, it will be the best thing in the\n\tworld: or so I think.\n\nA worthy cause. Could the example broken message be tweaked to\nemphasize that use case? With the current example, I'd consider\neither result (blank line or no blank line) to be ok behavior by git.\n\n[...]\n> +test_expect_success 'cherry-pick -s inserts blank line when conforming footer not found' '\n\nThis is a simpler version of the previous test, without the distracting\ncolon.\n\n[...]\n> +test_expect_success 'cherry-pick -s adds sob when last sob doesnt match committer' '\n\nNice test for basic \"-s\" functionality.\n\n[...]\n> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n\nAnd the other side of basic \"-s\" functionality.\n\nOne more test would be interesting: what does \"-s\" do when asked to\nproduce a duplicate signoff with an interspersed signoff by someone else?\n\n\ttest: a patch with a more complicated life\n\n\tThis patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for\n\ttweaking, then back to $GIT_COMMITTER_NAME who will be\n\trecording it in permanent history.\n\n\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n\tSigned-off-by: A U Thor \n\nWith the changes suggested above or similar ones,\nReviewed-by: Jonathan Nieder \n"},{"id":"207493","messageId":"20130122082701.GE6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-5-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 04/10] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T08:27:01Z","receivedAt":"2013-01-22T08:27:01Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Let's detect \"(cherry picked from...)\" as part of the footer so that we\n> will produce this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n> Signed-off-by: C O Mmitter \n>\n> instead of this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n>\n> Signed-off-by: C O Mmitter \n\nYes, looks sane.\n\nA downside is that this produces an arguably worse result when using\n\"-x -s\" for a commit that does not already have a sign-off. Before,\nwe had:\n\n\ttest: do something great\n\n\tDo something fantastic in a clean and elegant way that\n\tonly takes two lines of explanation.\n\t(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n\n\tSigned-off-by: C H Errypicker \n\nAfterwards, we will have:\n\n\ttest: do something great\n\n\tDo something fantastic in a clean and elegant way that\n\tonly takes two lines of explanation.\n\t(cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n\tSigned-off-by: C H Errypicker \n\nAn ideal result would be completely different:\n\n\ttest: do something great\n\n\tcommit da39a3ee5e6b4b0d3255bfef95601890afd80709 upstream.\n\n\tDo something fantastic in a clean and elegant way that\n\tonly takes two lines of explanation.\n\n\tSigned-off-by: C H Errypicker \n\nIn other words, the -x output format that puts the commit id at the\nend with odd spacing seems to be of questionable taste anyway. But\ngiven the constraint of leaving that alone, cramming together the\nsign-off like this seems like the best we can do, so for what it's\nworth,\nAcked-by: Jonathan Nieder \n"},{"id":"207494","messageId":"20130122083216.GF6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-6-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 05/10] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T08:32:16Z","receivedAt":"2013-01-22T08:32:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Start treating the \"(cherry picked from\" line added by cherry-pick -x\n> the same way that the s-o-b lines are treated. Namely, separate them\n> from the main commit message body with an empty line.\n\nHeh, you read my mind.\n"},{"id":"207496","messageId":"20130122083825.GG6085@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-7-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T08:38:25Z","receivedAt":"2013-01-22T08:38:25Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n> This is in preparation to unify the append_signoff implementations in\n> log-tree.c and sequencer.c.\n[...]\n> --- a/sequencer.c\n> +++ b/sequencer.c\n> @@ -1082,9 +1101,10 @@ 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, int no_dup_sob)\n\nIsn't the behavior of passing '1' here just a bug in \"format-patch -s\"?\n\nStyle: callers will be easier to read if the function takes a flag\nword with named bits, as in:\n\n\t#define APPEND_SIGNOFF_DEDUP (1 << 0)\n\tvoid append_signoff(..., int ignore_footer, unsigned flag)\n"},{"id":"207500","messageId":"CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com","threadId":"32682","inReplyTo":"20130122075413.GB6085@elie.Belkin","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-22T09:35:51Z","receivedAt":"2013-01-22T09:35:51Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder wrote:\n> Hi,\n>\n> Brandon Casey wrote:\n>\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n> [...]\n>> @@ -1042,13 +1041,8 @@ static int ends_rfc2822_footer(struct strbuf *sb, int ignore_footer)\n>\n> Git is checking if (sb->buf) ends with a \"Signed-off-by:\" style\n> line. If it doesn't, it will need to add an extra blank line\n> before adding a new sign-off.\n>\n> First (snipped), it seeks back two newlines from the end and then\n> forward to the next non-newline character, so (buf + i) is at the\n> start of the last line of (the interesting part of) sb.\n\nDid you catch that the two newlines have to be adjacent to each other?\n i.e. it seeks back until it finds a blank line. Then it seeks\nforward so that buf + i points at the first line of the last paragraph\nof sb.\n\n> Now:\n>\n>> for (; i < len; i = k) {\n>> for (k = i; k < len && buf[k] != '\\n'; k++)\n>> ; /* do nothing */\n>> k++;\n>\n> (buf + k) points to the end of this line.\n\nbuf + k points to the start of the next line. Not sure if that is the\nsame thing as what you said.\n\n>> - if ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n>> - continue;\n>\n> This is always the first line examined, so this \"continue\" never\n> triggers.\n\nThis is just totally broken and always has been. The index variable\nshould be 'i' not 'k'. It is supposed to check whether the current\nline is a continuation of the previously inspected line. An rfc2822\ncontinuation line begins with space or tab. The first line of the\nparagraph obviously can't be a continuation line, so the /first/\nvariable is used as a guard against matching the first line.\n\n>> -\n>> - first = 0;\n>> -\n>> for (j = 0; i + j < len; j++) {\n>\n> If the line matches /^[[:alnum:]-]*:/, it passes and git moves on to\n> the (nonexistent) next line. Otherwise, it fails.\n>\n> Do I understand correctly?\n\nNo, I think you misread the for loop that you snipped out. It seeks\nback to the beginning of the last paragraph, not the last line. The\nfor loop that you included above, inspects each line of that\nparagraph. If any line does not match /^[[:alnum:]-]*:/, then the\nfunction returns false (0). If every line matches, it returns true\n(1).\n\n> If so, this patch should be a no-op, which\n> is good, I guess.\n\nYou're correct here though. The patch is a no-op. This patch removes\nthe code that was supposed to parse rfc2822 continuation lines, but it\nwas broken. Thankfully it was broken utterly and completely so it\nnever allowed anything through that wasn't /^[[:alnum:]-]*:/.\n\n-Brandon\n"},{"id":"207501","messageId":"CA+sFfMeZ0VoySUtpYFZcjSxGQithhKUHarOr3TuioyjaYddsXg@mail.gmail.com","threadId":"32682","inReplyTo":"20130122080216.GC6085@elie.Belkin","subject":"Re: [PATCH v2 02/10] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-22T09:43:05Z","receivedAt":"2013-01-22T09:43:05Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Jan 22, 2013 at 12:02 AM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n\n>> --- a/t/test-lib-functions.sh\n>> +++ b/t/test-lib-functions.sh\n>> @@ -135,12 +135,13 @@ 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. It will also add a tag with as name unless is\n>> +# given.\n>> #\n>> -# Both and default to .\n>> +# , , and all default to .\n>\n> Simpler:\n>\n> # This will commit a file with the given contents and the given commit\n> # message and tag the resulting commit with the given tag name.\n> #\n> # , , and all default to .\n\nYes, a nice improvement.\n\nThanks,\n-Brandon\n"},{"id":"207503","messageId":"20130122094720.GA8908@elie.Belkin","threadId":"32682","inReplyTo":"CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T09:47:20Z","receivedAt":"2013-01-22T09:47:20Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder wrote:\n\n>> First (snipped), it seeks back two newlines from the end and then\n>> forward to the next non-newline character, so (buf + i) is at the\n>> start of the last line of (the interesting part of) sb.\n>\n> Did you catch that the two newlines have to be adjacent to each other?\n[...]\n\nHere is the loop in master:\n\n\tint hit = 0;\n[...]\n\n\tfor (i = len - 1; i > 0; i--) {\n\t\tif (hit && buf[i] == '\\n')\n\t\t\tbreak;\n\t\thit = (buf[i] == '\\n');\n\t}\n\nI don't see any adjacency check. I agree with you that \"two adjacent\nnewlines\" was probably the intent, though.\n"},{"id":"207504","messageId":"20130122094916.GB8908@elie.Belkin","threadId":"32682","inReplyTo":"20130122094720.GA8908@elie.Belkin","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T09:49:16Z","receivedAt":"2013-01-22T09:49:16Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> Here is the loop in master:\n>\n> \tint hit = 0;\n> [...]\n> \n> \tfor (i = len - 1; i > 0; i--) {\n> \t\tif (hit && buf[i] == '\\n')\n> \t\t\tbreak;\n> \t\thit = (buf[i] == '\\n');\n> \t}\n>\n> I don't see any adjacency check.\n\nOf course that's because I can't read. :) Checking again.\n"},{"id":"207505","messageId":"20130122101208.GC8908@elie.Belkin","threadId":"32682","inReplyTo":"CA+sFfMfERgGbf58LjOfAhhO_-YLu+yo+L-vYMuAuOUaP90yvgA@mail.gmail.com","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T10:12:08Z","receivedAt":"2013-01-22T10:12:08Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n> On Mon, Jan 21, 2013 at 11:54 PM, Jonathan Nieder wrote:\n\n>>> - if ((buf[k] == ' ' || buf[k] == '\\t') && !first)\n>>> - continue;\n>>\n>> This is always the first line examined, so this \"continue\" never\n>> triggers.\n>\n> This is just totally broken and always has been. The index variable\n> should be 'i' not 'k'.\n\nYes, now I see.\n\nThis test trips when the *next* line starts with ' ' or '\\t'.\n\n\tcommit, cherry-pick -s: remove broken support for multiline rfc2822 fields\n\n\tStarting with c1e01b0c (commit: More generous accepting of RFC-2822\n\tfooter lines, 2009-10-28), \"git commit -s\" carefully parses the last\n\tparagraph of each commit message to check if it consists only of\n\tRFC2822-style headers, in which case the signoff will be added as a\n\tnew line in the same list:\n\n\t\tReported-by: Reporter \n\t\tSigned-off-by: Author \n\t\tAcked-by: Lieutenant \n\n\tIt even included support for accepting indented continuation lines for\n\tmultiline fields. Unfortunately the multiline field support is broken\n\tbecause it checks whether buf[k] (the first character of the *next*\n\tline) instead of buf[i] is a whitespace character. So for example, it\n\tconfuses the following for a well-formed RFC2822 footer:\n\n\t\tReported-by: Reporter \n\t\tLINE NOISE\n\t\t continuation\n\t\t continuation\n\n\tA typical well-formed footer with multiline fields is not accepted:\n\n\t\tReported-by: Re Porter\n\t\t \n\t\tSigned-off-by: Author \n\n\tThat this has remained broken for so long is a good sign that nobody\n\tactually needed continuation lines. Rip out the broken continuation\n\tsupport.\n"},{"id":"207506","messageId":"20130122102147.GD8908@elie.Belkin","threadId":"32682","inReplyTo":"20130122101208.GC8908@elie.Belkin","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-22T10:21:47Z","receivedAt":"2013-01-22T10:21:47Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jonathan Nieder wrote:\n\n> \tline) instead of buf[i] is a whitespace character. So for example, it\n> \tconfuses the following for a well-formed RFC2822 footer:\n\nLuckily it doesn't, since the final continuation line is not followed\nby whitespace. I should have said:\n\n\t\"... is a whitespace character. The result is that any\n\tfooter with a continuation line is not accepted, since the last\n\tcontinuation line neither starts with an RFC2822 field name nor is\n\tfollowed by a continuation line.\n\n\tThat this has remained broken for so long is good evidence that nobody\n\tactually needed multiline fields. Rip out the broken continuation\n\tsupport.\n\n\tNo functional change intended.\"\n\nI have no excuse, since you explained this all out loud to me a couple of\nmonths ago. :) Sorry for the nonsense.\n\nThanks,\nJonathan\n"},{"id":"207507","messageId":"CA+sFfMeB_PEgoykPj-RX6zZ-hOD1106aO_KwkWF7BBiJvG-1AA@mail.gmail.com","threadId":"32682","inReplyTo":"20130122094916.GB8908@elie.Belkin","subject":"Re: [PATCH v2 01/10] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-22T11:08:40Z","receivedAt":"2013-01-22T11:08:40Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Jan 22, 2013 at 1:49 AM, Jonathan Nieder wrote:\n> Jonathan Nieder wrote:\n>\n>> Here is the loop in master:\n>>\n>> int hit = 0;\n>> [...]\n>>\n>> for (i = len - 1; i > 0; i--) {\n>> if (hit && buf[i] == '\\n')\n>> break;\n>> hit = (buf[i] == '\\n');\n>> }\n>>\n>> I don't see any adjacency check.\n>\n> Of course that's because I can't read. :) Checking again.\n\nBlame the code, not your eyes. The use of the term 'hit' is what\nmakes this loop confusing. It gives the impression that 'hit' gets\nset once, after the first newline is /hit/.\n\nIt would be much easier to read if it was written like this:\n\n int last_char_was_nl = 0;\n for (i = len - 1; i > 0; i--) {\n this_char_is_nl = (buf[i] == '\\n');\n if (last_char_was_nl && this_char_is_nl)\n break;\n last_char_was_nl = this_char_is_nl;\n }\n\nI'll slide this in when I resubmit this series with your suggestions.\n\n-Brandon\n"},{"id":"208007","messageId":"CA+sFfMc0_QxwtJ6YS6H+GOW+Pzh3tSZs3rW-VSDf-GWeL=3tDA@mail.gmail.com","threadId":"32682","inReplyTo":"20130122081758.GD6085@elie.Belkin","subject":"Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-27T23:33:33Z","receivedAt":"2013-01-27T23:33:33Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Jan 22, 2013 at 12:17 AM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n>\n>> --- /dev/null\n>> +++ b/t/t3511-cherry-pick-x.sh\n\n> [...]\n>> +test_expect_failure 'cherry-pick -s inserts blank line after non-conforming footer' '\n>\n> IIUC this is an illustration of false-positives from messages like\n> this one:\n>\n> base: do something great without a sign-off\n>\n> If he does that, it will be the best thing in the\n> world: or so I think.\n>\n> A worthy cause. Could the example broken message be tweaked to\n> emphasize that use case? With the current example, I'd consider\n> either result (blank line or no blank line) to be ok behavior by git.\n\nThe primary motivation for this test was to exercise an existing\nbehavior which fails to append a newline and sob if the last line of\nthe last paragraph matches the sob of the committer regardless of\nwhether the entire paragraph would be interpreted as a conforming\nfooter. Your example is tested as a side-effect of that. I'll tweak\nthe string so it looks like this:\n\nThe signed-off-by string should begin with the words Signed-off-by followed\nby a colon and space, and then the signers name and email address. e.g.\nSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n\n> [...]\n>> +test_expect_success 'cherry-pick -s refrains from adding duplicate trailing sob' '\n>\n> And the other side of basic \"-s\" functionality.\n>\n> One more test would be interesting: what does \"-s\" do when asked to\n> produce a duplicate signoff with an interspersed signoff by someone else?\n>\n> test: a patch with a more complicated life\n>\n> This patch bounced from $GIT_COMMITTER_NAME to Ms. Thor for\n> tweaking, then back to $GIT_COMMITTER_NAME who will be\n> recording it in permanent history.\n>\n> Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n> Signed-off-by: A U Thor \n\nThis one exists as \"adds sob when last sob doesn't match committer\".\nIn this case an additional sob should be appended to the footer.\n\n-Brandon\n"},{"id":"208006","messageId":"20130127234014.GB8206@elie.Belkin","threadId":"32682","inReplyTo":"CA+sFfMc0_QxwtJ6YS6H+GOW+Pzh3tSZs3rW-VSDf-GWeL=3tDA@mail.gmail.com","subject":"Re: [PATCH v2 03/10] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-27T23:40:14Z","receivedAt":"2013-01-27T23:40:14Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Brandon Casey wrote:\n\n> I'll tweak\n> the string so it looks like this:\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\nYep, that looks better than the example I suggested. :)\n\n[...]\n> On Tue, Jan 22, 2013 at 12:17 AM, Jonathan Nieder wrote:\n\n>> One more test would be interesting: what does \"-s\" do when asked to\n>> produce a duplicate signoff with an interspersed signoff by someone else?\n[...]\n> This one exists as \"adds sob when last sob doesn't match committer\".\n\nSo it does. Thanks for the sanity check.\n"},{"id":"208010","messageId":"CA+sFfMcyupLGPt8-3PWMzwgGR3zrm4ZfA-7KwBh5VkMXmhuTKQ@mail.gmail.com","threadId":"32682","inReplyTo":"20130122083825.GG6085@elie.Belkin","subject":"Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2013-01-28T00:36:23Z","receivedAt":"2013-01-28T00:36:23Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder wrote:\n> Brandon Casey wrote:\n>\n>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n>> This is in preparation to unify the append_signoff implementations in\n>> log-tree.c and sequencer.c.\n> [...]\n>> --- a/sequencer.c\n>> +++ b/sequencer.c\n>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>> return 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, int no_dup_sob)\n>\n> Isn't the behavior of passing '1' here just a bug in \"format-patch -s\"?\n\nI think that is an open question.\n\n> Style: callers will be easier to read if the function takes a flag\n> word with named bits, as in:\n>\n> #define APPEND_SIGNOFF_DEDUP (1 << 0)\n> void append_signoff(..., int ignore_footer, unsigned flag)\n\nCan't I just be lazy!!! Ok, you win. :b\n\n-Brandon\n"},{"id":"208032","messageId":"20130128012419.GC8206@elie.Belkin","threadId":"32682","inReplyTo":"1358757627-16682-5-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH v2 04/10] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-01-28T01:24:19Z","receivedAt":"2013-01-28T01:24:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nBrandon Casey wrote:\n\n> Let's detect \"(cherry picked from...)\" as part of the footer so that we\n> will produce this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n> Signed-off-by: C O Mmitter \n>\n> instead of this:\n>\n> Signed-off-by: A U Thor \n> (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n>\n> Signed-off-by: C O Mmitter \n\nMy last review was based on a misunderstanding of\nends_rfc2822_footer(). This time I can unreservedly say I like this.\n\n[...]\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\nThe static/nonstatic mismatch looks strange. Not about this patch,\nbut probably rest_is_empty() from builtin-commit.c should move here\nsome day.\n\n[...]\n> @@ -1021,11 +1022,36 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n[...]\n> +static int is_cherry_picked_from_line(const char *buf, int len)\n> +{\n> +\t/*\n> +\t * We only care that it looks roughly like (cherry picked from ...)\n> +\t */\n> +\treturn !prefixcmp(buf, cherry_picked_prefix) &&\n> +\t\t(buf[len - 1] == ')' ||\n> +\t\t (buf[len - 1] == '\\n' && buf[len - 2] == ')'));\n\nThese two cases are based on whether the commit message ended with\na '(cherry picked from' with no trailing newline, I guess?\n\nI wonder if switching the convention for 'k' would make this simpler:\n\n\tconst char *line, *eol;\n\n\tline = buf + i;\n\teol = memchr(buf, '\\n', len - i);\n\tif (!eol)\n\t\teol = buf + len;\n\n\tif (!is_rfc2822_field(line, eol - line) &&\n\t !is_cherry_picked_from_line(line, eol - line))\n\t\treturn 0;\n\nI notice you just sent a new version of the series. I'll try this out\non top of that.\n\nThanks,\nJonathan\n"},{"id":"208035","messageId":"7v7gmy12op.fsf@alter.siamese.dyndns.org","threadId":"32682","inReplyTo":"CA+sFfMcyupLGPt8-3PWMzwgGR3zrm4ZfA-7KwBh5VkMXmhuTKQ@mail.gmail.com","subject":"Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-28T02:06:30Z","receivedAt":"2013-01-28T02:06:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder wrote:\n>> Brandon Casey wrote:\n>>\n>>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n>>> This is in preparation to unify the append_signoff implementations in\n>>> log-tree.c and sequencer.c.\n>> [...]\n>>> --- a/sequencer.c\n>>> +++ b/sequencer.c\n>>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>>> return 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, int no_dup_sob)\n>>\n>> Isn't the behavior of passing '1' here just a bug in \"format-patch -s\"?\n>\n> I think that is an open question.\n\nYes. as I said in a previous review round, I think it was a mistake\nthat format-patch chose to pay attention to any S-o-b in the patch\ntrail, not only the last one, and we may want to correcct it once\nthis series solidifies as a separate \"bugfix\" change on top.\n"},{"id":"209100","messageId":"7v7gmhulya.fsf@alter.siamese.dyndns.org","threadId":"32682","inReplyTo":"7v7gmy12op.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-02-09T23:06:05Z","receivedAt":"2013-02-09T23:06:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Brandon Casey writes:\n>\n>> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder wrote:\n>>> Brandon Casey wrote:\n>>>\n>>>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n>>>> This is in preparation to unify the append_signoff implementations in\n>>>> log-tree.c and sequencer.c.\n>>> [...]\n>>>> --- a/sequencer.c\n>>>> +++ b/sequencer.c\n>>>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>>>> return 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, int no_dup_sob)\n>>>\n>>> Isn't the behavior of passing '1' here just a bug in \"format-patch -s\"?\n>>\n>> I think that is an open question.\n>\n> Yes. as I said in a previous review round, I think it was a mistake\n> that format-patch chose to pay attention to any S-o-b in the patch\n> trail, not only the last one, and we may want to correcct it once\n> this series solidifies as a separate \"bugfix\" change on top.\n\nThis is a tangent, but I _think_ (didn't check, though) \"git am -s\"\nimplements this incorrrectly. Just another LHF somebody may want to\ntake a look.\n"},{"id":"209105","messageId":"5116E07F.9020607@nvidia.com","threadId":"32682","inReplyTo":"7v7gmhulya.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 06/10] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2013-02-09T23:49:19Z","receivedAt":"2013-02-09T23:49:19Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"On 2/9/2013 3:06 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n>> Brandon Casey writes:\n>>\n>>> On Tue, Jan 22, 2013 at 12:38 AM, Jonathan Nieder wrote:\n>>>> Brandon Casey wrote:\n>>>>\n>>>>> Teach append_signoff how to detect a duplicate s-o-b in the commit footer.\n>>>>> This is in preparation to unify the append_signoff implementations in\n>>>>> log-tree.c and sequencer.c.\n>>>> [...]\n>>>>> --- a/sequencer.c\n>>>>> +++ b/sequencer.c\n>>>>> @@ -1082,9 +1101,10 @@ int sequencer_pick_revisions(struct replay_opts *opts)\n>>>>> return 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, int no_dup_sob)\n>>>>\n>>>> Isn't the behavior of passing '1' here just a bug in \"format-patch -s\"?\n>>>\n>>> I think that is an open question.\n>>\n>> Yes. as I said in a previous review round, I think it was a mistake\n>> that format-patch chose to pay attention to any S-o-b in the patch\n>> trail, not only the last one, and we may want to correcct it once\n>> this series solidifies as a separate \"bugfix\" change on top.\n> \n> This is a tangent, but I _think_ (didn't check, though) \"git am -s\"\n> implements this incorrrectly. Just another LHF somebody may want to\n> take a look.\n\n(I haven't checked the 'git am -s' behavior, but ...)\n\nOne distinction between commit, cherry-pick, am, and format-patch is\nthat the first three are related to importing commits _into_ your own\nrepository while format-patch is related to exporting patches _from_\nyour repository.\n\nNot sure if an \"exporting\" operation should be treated differently from\nan \"importing\" operation and mean \"add my sign-off only if I did not\npreviously do so when I did commit, cherry-pick, or am\" or whether it\nshould mean the same thing i.e. \"append my sign-off, unless it's already\nlast\".\n\nJust pointing something out that may or may not be useful.\n\nHmm, actually, this series could provide a data point. When I submitted\nthe first series, I used 'format-patch -s' to append my sob. When\nJonathan reviewed the series, he provided Acked-by and Reviewed-by which\nI collected and added as I reworked the series. When I added Jonathan's\nAck's/Rev-by's, I _appended_ them to the commits that I had already\nsubmitted, so they appeared _after_ my own sob. When I submitted the\nlater iterations of the series, I probably used 'format-patch -s' again\nand I of course did not want an additional sob to be appended after\nJonathan's Acks.\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"}]}