{"thread":{"id":"32196","subject":"[PATCH 00/11] alternative unify appending of sob","startedAt":"2012-11-26T01:45:48Z","lastAt":"2012-11-29T05:56:45Z","messageCount":18,"participants":["Brandon Casey","Nguyen Thai Ngoc Duy","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":11},"messages":[{"id":"203831","messageId":"1353894359-6733-1-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":null,"subject":"[PATCH 00/11] alternative unify appending of sob","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:48Z","receivedAt":"2012-11-26T01:45:48Z","isPatch":true,"sender":{"key":"drafnel@gmail.com","avatar":"https://avatars.githubusercontent.com/u/921167?v=4"},"body":"From: Brandon Casey \n\nI hate to have the battle of the patches, but I kinda prefer the\nappend_signoff code in sequencer.c over the code in log-tree.c as a\nfoundation to build on.\n\nSo, this series is similar to Duy's \"unification\" series, but it goes in the\nopposite direction and builds on top of sequencer.c and additionally adds the\nelements from my original series to treat the \"(cherry picked from...\" line\nadded by 'cherry-pick -x' in the same way that other footer elements are\ntreated (after addressing Junio's comments about rfc2822 continuation lines\nand duplicate s-o-b's).\n\nI've integrated Duy's series with a few minor tweaks. I added a couple\nof additional tests to t4014 and corrected one of the tests which had\nincorrect behavior. I think his sign-off's should still be valid, so I\nkept them in. Sorry that I've been slow, and now the two of us are stepping\non each other's toes, but Duy please take a look and let me know if there's\nanything you dislike.\n\nThe main difference between this series and Duy's, aside from the change in\nbehavior with respect to a \"(cherry picked from...)\" line, is that the\ndetection of a s-o-b footer is done in a somewhat more generic way. The\nlog-tree.c code expects to find something that looks like \"[-A-Za-z]+:\" on\nthe left-hand side, and something that looks like an email address on the\nright-hand side. The sequencer.c code relaxes this definition so that an\nemail address is not required on the right hand side. This allows lines like\n\"Change-id: IXXXX\" or \"Bug: XXX\" to be considered valid footer elements.\n\nSo, this series should result in s-o-b and \"(cherry picked from...)\" being\nappended to commit messages in a more consistent and deterministic way. For\nexample, the decision about whether to insert a blank line before appending\na s-o-b. As discussed, cherry-pick and commit will only refrain from\nappending a s-o-b if the committer's s-o-b appears as the last one in a\nconforming footer, while format-patch will refrain from appending it if it\nappears anywhere within the footer.\n\n-Brandon\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 (3):\n t4014: more tests about appending s-o-b lines\n format-patch: stricter S-o-b detection\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 | 133 +++++++++++++++---------\n sequencer.h | 2 +-\n t/t3511-cherry-pick-x.sh | 219 +++++++++++++++++++++++++++++++++++++++\n t/t4014-format-patch.sh | 262 +++++++++++++++++++++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 9 +-\n 9 files changed, 580 insertions(+), 154 deletions(-)\n create mode 100755 t/t3511-cherry-pick-x.sh\n\n-- \n1.8.0.284.g959048a\n"},{"id":"203832","messageId":"1353894359-6733-2-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 01/11] sequencer.c: remove broken support for rfc2822 continuation in footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:49Z","receivedAt":"2012-11-26T01:45:49Z","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 2260490..656df6b 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1023,7 +1023,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@@ -1040,11 +1039,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.0.284.g959048a\n"},{"id":"203833","messageId":"1353894359-6733-3-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 02/11] t/test-lib-functions.sh: allow to specify the tag name to test_commit","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:50Z","receivedAt":"2012-11-26T01:45:50Z","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 8889ba5..9e2b8b8 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.0.284.g959048a\n"},{"id":"203834","messageId":"1353894359-6733-4-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 03/11] t/t3511: add some tests of 'cherry-pick -s' functionality","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:51Z","receivedAt":"2012-11-26T01:45:51Z","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.0.284.g959048a\n"},{"id":"203835","messageId":"1353894359-6733-5-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 04/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:52Z","receivedAt":"2012-11-26T01:45:52Z","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 Mitter \n\ninstead of this:\n\n Signed-off-by: A U Thor \n (cherry picked from da39a3ee5e6b4b0d3255bfef95601890afd80709)\n\n Signed-off-by: C O Mitter \n\nSigned-off-by: Brandon Casey \n---\n sequencer.c | 42 ++++++++++++++++++++++++------------\n t/t3511-cherry-pick-x.sh | 55 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 84 insertions(+), 13 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 656df6b..19eaf11 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@@ -492,7 +493,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@@ -1017,11 +1018,32 @@ 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_pick_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+}\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@@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))\n \t\t\treturn 0;\n-\t\t}\n \t}\n \treturn 1;\n }\n@@ -1064,7 +1080,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.0.284.g959048a\n"},{"id":"203836","messageId":"1353894359-6733-6-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 05/11] sequencer.c: always separate \"(cherry picked from\" from commit body","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:53Z","receivedAt":"2012-11-26T01:45:53Z","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 | 106 +++++++++++++++++++++++++----------------------\n t/t3511-cherry-pick-x.sh | 53 ++++++++++++++++++++++++\n 2 files changed, 109 insertions(+), 50 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 19eaf11..7c0852a 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -20,6 +20,60 @@\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_pick_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+}\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_pick_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@@ -493,6 +547,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@@ -1018,56 +1074,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_pick_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-}\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_pick_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.0.284.g959048a\n"},{"id":"203837","messageId":"1353894359-6733-7-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:54Z","receivedAt":"2012-11-26T01:45:54Z","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 | 43 +++++++++++++++++++++++++++++++------------\n sequencer.h | 2 +-\n t/t3511-cherry-pick-x.sh | 2 +-\n 4 files changed, 34 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 1dd2ec5..7b9e2ac 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 7c0852a..3062ad4 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -42,12 +42,19 @@ static int is_cherry_pick_from_line(const char *buf, int len)\n \t\t!prefixcmp(buf, cherry_picked_prefix);\n }\n \n-static int has_conforming_footer(struct strbuf *sb, int ignore_footer)\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 i, k = 0;\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@@ -63,14 +70,24 @@ 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\t\tis_cherry_pick_from_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\tfound_sob = k;\n+\n+\t\tif (!(found_rfc2822 || is_cherry_pick_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@@ -291,7 +308,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@@ -547,7 +564,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@@ -1074,9 +1091,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@@ -1085,10 +1103,11 @@ 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+\tif (!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+\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 \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.0.284.g959048a\n"},{"id":"203838","messageId":"1353894359-6733-8-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 07/11] sequencer.c: teach append_signoff to avoid adding a duplicate newline","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:55Z","receivedAt":"2012-11-26T01:45:55Z","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 | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 3062ad4..eb93dd6 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -1103,8 +1103,8 @@ 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 (!i || !(has_footer =\n-\t\thas_conforming_footer(msgbuf, &sob, ignore_footer)))\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 \tif (has_footer != 3 && (!no_dup_sob || has_footer != 2))\n \t\tstrbuf_splice(msgbuf, msgbuf->len - ignore_footer, 0,\n-- \n1.8.0.284.g959048a\n"},{"id":"203839","messageId":"1353894359-6733-9-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 08/11] t4014: more tests about appending s-o-b lines","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:56Z","receivedAt":"2012-11-26T01:45:56Z","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: 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\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n t/t4014-format-patch.sh | 146 ++++++++++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 146 insertions(+)\n\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 16a4ca1..dfe9209 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -963,4 +963,150 @@ test_expect_success 'format patch ignores color.ui' '\n \ttest_cmp expect actual\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+\techo -n 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_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_done\n-- \n1.8.0.284.g959048a\n"},{"id":"203840","messageId":"1353894359-6733-10-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 09/11] format-patch: stricter S-o-b detection","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:57Z","receivedAt":"2012-11-26T01:45:57Z","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\nS-o-b in the middle of a sentence, at the beginning of the sentence\nbut it is just because of text wrapping, or a full paragraph of valid\nS-o-b in the middle of the message. All these are not S-o-b, but\ndetected as is. Fix them.\n\n[bc: add two additional tests to demonstrate shortcomings of the current\n code:\n\n 1. failure to detect non-conforming elements in the footer when last\n line matches committer's s-o-b.\n 2. failure to handle various s-o-b -like elements in the footer as\n conforming. e.g. \"Change-id: IXXXX or Bug: 1234\"\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Brandon Casey \n---\n log-tree.c | 37 +++++++++++++++++--\n t/t4014-format-patch.sh | 95 +++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 130 insertions(+), 2 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex 4f86def..14ebf69 100644\n--- a/log-tree.c\n+++ b/log-tree.c\n@@ -260,14 +260,47 @@ static void append_signoff(struct strbuf *sb, const char *signoff)\n \tint has_signoff = 0;\n \tchar *cp;\n \n-\tcp = sb->buf;\n+\t/*\n+\t * Only search in the last paragrah, don't be fooled by a\n+\t * paragraph full of valid S-o-b in the middle.\n+\t */\n+\tcp = sb->buf + sb->len - 1;\n+\twhile (cp > sb->buf) {\n+\t\tif (cp[0] == '\\n' && cp[1] == '\\n') {\n+\t\t\tcp += 2;\n+\t\t\tbreak;\n+\t\t}\n+\t\tcp--;\n+\t}\n \n \t/* First see if we already have the sign-off by the signer */\n \twhile ((cp = strstr(cp, signed_off_by))) {\n+\t\tconst char *s = cp;\n+\t\tcp += strlen(signed_off_by);\n+\n+\t\tif (!has_signoff && s > sb->buf) {\n+\t\t\t/*\n+\t\t\t * S-o-b in the middle of a sentence is not\n+\t\t\t * really S-o-b\n+\t\t\t */\n+\t\t\tif (s[-1] != '\\n')\n+\t\t\t\tcontinue;\n+\n+\t\t\tif (s - 1 > sb->buf && s[-2] == '\\n')\n+\t\t\t\t; /* the first S-o-b */\n+\t\t\telse if (!detect_any_signoff(sb->buf, s - sb->buf))\n+\t\t\t\t/*\n+\t\t\t\t * The previous line looks like an\n+\t\t\t\t * S-o-b. There's still no guarantee\n+\t\t\t\t * that it's an actual S-o-b. For that\n+\t\t\t\t * we need to look back until we find\n+\t\t\t\t * a blank line, which is too costly.\n+\t\t\t\t */\n+\t\t\t\tcontinue;\n+\t\t}\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))\ndiff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex dfe9209..30e37a7 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1062,6 +1062,60 @@ EOF\n \ttest_cmp expected actual\n '\n \n+test_expect_success '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_success '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_success '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@@ -1109,4 +1163,45 @@ 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_failure 'signoff: footer begins with non-signoff without @ sign' '\n+\tappend_signoff <<\\EOF >actual &&\n+subject\n+\n+body\n+\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+13:Signed-off-by: C O Mitter \n+EOF\n+\ttest_cmp expected actual\n+'\n+\n test_done\n-- \n1.8.0.284.g959048a\n"},{"id":"203841","messageId":"1353894359-6733-11-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 10/11] format-patch: update append_signoff prototype","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:58Z","receivedAt":"2012-11-26T01:45:58Z","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 | 21 +++++++++++++--------\n revision.h | 2 +-\n 3 files changed, 15 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex e7b7db1..bb48344 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1058,7 +1058,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@@ -1154,16 +1153,6 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \t\t\t PARSE_OPT_KEEP_ARGV0 | PARSE_OPT_KEEP_UNKNOWN |\n \t\t\t PARSE_OPT_KEEP_DASHDASH);\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@@ -1354,7 +1343,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 14ebf69..be8e7ab 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 flags)\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+\tstatic const char sign_off_header[] = \"Signed-off-by: \";\n \tsize_t signoff_len = strlen(signoff);\n \tint has_signoff = 0;\n \tchar *cp;\n@@ -274,9 +276,9 @@ static void append_signoff(struct strbuf *sb, const char *signoff)\n \t}\n \n \t/* First see if we already have the sign-off by the signer */\n-\twhile ((cp = strstr(cp, signed_off_by))) {\n+\twhile ((cp = strstr(cp, sign_off_header))) {\n \t\tconst char *s = cp;\n-\t\tcp += strlen(signed_off_by);\n+\t\tcp += strlen(sign_off_header);\n \n \t\tif (!has_signoff && s > sb->buf) {\n \t\t\t/*\n@@ -317,9 +319,10 @@ static void append_signoff(struct strbuf *sb, const char *signoff)\n \tif (!has_signoff)\n \t\tstrbuf_addch(sb, '\\n');\n \n-\tstrbuf_addstr(sb, signed_off_by);\n+\tstrbuf_addstr(sb, sign_off_header);\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@@ -695,8 +698,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@@ -707,7 +712,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);\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 059bfff..435a60b 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-\tconst char\t*add_signoff;\n+\tint add_signoff;\n \tconst char\t*extra_headers;\n \tconst char\t*log_reencode;\n \tconst char\t*subject_prefix;\n-- \n1.8.0.284.g959048a\n"},{"id":"203842","messageId":"1353894359-6733-12-git-send-email-drafnel@gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"[PATCH 11/11] Unify appending signoff in format-patch, commit and sequencer","fromName":"Brandon Casey","fromEmail":"drafnel@gmail.com","sentAt":"2012-11-26T01:45:59Z","receivedAt":"2012-11-26T01:45:59Z","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 | 122 +-----------------------------------------------\n t/t4014-format-patch.sh | 27 +++++++++--\n 2 files changed, 26 insertions(+), 123 deletions(-)\n\ndiff --git a/log-tree.c b/log-tree.c\nindex be8e7ab..1fb0a16 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,125 +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 flags)\n-{\n-\tchar* signoff = xstrdup(fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n-\t\t\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n-\tstatic const char sign_off_header[] = \"Signed-off-by: \";\n-\tsize_t signoff_len = strlen(signoff);\n-\tint has_signoff = 0;\n-\tchar *cp;\n-\n-\t/*\n-\t * Only search in the last paragrah, don't be fooled by a\n-\t * paragraph full of valid S-o-b in the middle.\n-\t */\n-\tcp = sb->buf + sb->len - 1;\n-\twhile (cp > sb->buf) {\n-\t\tif (cp[0] == '\\n' && cp[1] == '\\n') {\n-\t\t\tcp += 2;\n-\t\t\tbreak;\n-\t\t}\n-\t\tcp--;\n-\t}\n-\n-\t/* First see if we already have the sign-off by the signer */\n-\twhile ((cp = strstr(cp, sign_off_header))) {\n-\t\tconst char *s = cp;\n-\t\tcp += strlen(sign_off_header);\n-\n-\t\tif (!has_signoff && s > sb->buf) {\n-\t\t\t/*\n-\t\t\t * S-o-b in the middle of a sentence is not\n-\t\t\t * really S-o-b\n-\t\t\t */\n-\t\t\tif (s[-1] != '\\n')\n-\t\t\t\tcontinue;\n-\n-\t\t\tif (s - 1 > sb->buf && s[-2] == '\\n')\n-\t\t\t\t; /* the first S-o-b */\n-\t\t\telse if (!detect_any_signoff(sb->buf, s - sb->buf))\n-\t\t\t\t/*\n-\t\t\t\t * The previous line looks like an\n-\t\t\t\t * S-o-b. There's still no guarantee\n-\t\t\t\t * that it's an actual S-o-b. For that\n-\t\t\t\t * we need to look back until we find\n-\t\t\t\t * a blank line, which is too costly.\n-\t\t\t\t */\n-\t\t\t\tcontinue;\n-\t\t}\n-\n-\t\thas_signoff = 1;\n-\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\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, sign_off_header);\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;\n@@ -712,7 +594,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, 0);\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/t/t4014-format-patch.sh b/t/t4014-format-patch.sh\nindex 30e37a7..1dea58b 100755\n--- a/t/t4014-format-patch.sh\n+++ b/t/t4014-format-patch.sh\n@@ -1046,7 +1046,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@@ -1163,7 +1184,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@@ -1184,7 +1205,7 @@ EOF\n \ttest_cmp expected actual\n '\n \n-test_expect_failure 'signoff: footer begins with non-signoff without @ sign' '\n+test_expect_success 'signoff: footer begins with non-signoff without @ sign' '\n \tappend_signoff <<\\EOF >actual &&\n subject\n \n-- \n1.8.0.284.g959048a\n"},{"id":"203859","messageId":"CACsJy8AfaETfikvB+zON4iiX2HBehJg-xX2GGmKYHueX8rv3qw@mail.gmail.com","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 00/11] alternative unify appending of sob","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2012-11-26T07:56:49Z","receivedAt":"2012-11-26T07:56:49Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey wrote:\n> From: Brandon Casey \n>\n> I hate to have the battle of the patches, but I kinda prefer the\n> append_signoff code in sequencer.c over the code in log-tree.c as a\n> foundation to build on.\n>\n> So, this series is similar to Duy's \"unification\" series, but it goes in the\n> opposite direction and builds on top of sequencer.c and additionally adds the\n> elements from my original series to treat the \"(cherry picked from...\" line\n> added by 'cherry-pick -x' in the same way that other footer elements are\n> treated (after addressing Junio's comments about rfc2822 continuation lines\n> and duplicate s-o-b's).\n>\n> I've integrated Duy's series with a few minor tweaks. I added a couple\n> of additional tests to t4014 and corrected one of the tests which had\n> incorrect behavior. I think his sign-off's should still be valid, so I\n> kept them in. Sorry that I've been slow, and now the two of us are stepping\n> on each other's toes, but Duy please take a look and let me know if there's\n> anything you dislike.\n\nI'm still not sure whether format-patch should follow cherry-pick's\nrule in appending sob. If it does, it probably makes more sense to fix\nthe sequencer.c code then delete log-tree.c (not fixes on log-tree.c\nthen delete it). I see that your changes pass all the new tests I\nadded in format-patch so sequencer.c is probably good enough,\nlog-tree.c changes are probably not needed. Feel free take over the\nseries :-)\n-- \nDuy\n"},{"id":"203939","messageId":"7v1ufgqb1s.fsf@alter.siamese.dyndns.org","threadId":"32196","inReplyTo":"1353894359-6733-1-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 00/11] alternative unify appending of sob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-26T22:00:47Z","receivedAt":"2012-11-26T22:00:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> So, this series should result in s-o-b and \"(cherry picked from...)\" being\n> appended to commit messages in a more consistent and deterministic way. For\n> example, the decision about whether to insert a blank line before appending\n> a s-o-b. As discussed, cherry-pick and commit will only refrain from\n> appending a s-o-b if the committer's s-o-b appears as the last one in a\n> conforming footer, while format-patch will refrain from appending it if it\n> appears anywhere within the footer.\n\nSounds sensible; we may want to fix format-patch later, but with\nsomething like this series, it is just the matter of flipping one\nbit.\n\nWill queue. Thanks.\n"},{"id":"204228","messageId":"7v8v9ltgxn.fsf@alter.siamese.dyndns.org","threadId":"32196","inReplyTo":"1353894359-6733-5-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 04/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T00:02:12Z","receivedAt":"2012-11-29T00:02:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\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_pick_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> +}\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> @@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))\n> \t\t\treturn 0;\n> -\t\t}\n> \t}\n> \treturn 1;\n> }\n\nRefactored code looks vastly more readable, but I think the\nis_cherry_pick_from_line() function (by the way, shouldn't it be\nnamed is_cherry_picked_from_line()?) shows its ambivalence. It\ninsists that the line has to be long enough to hold 40-hex object\nname plus a closing parenthesis, but it only makes sure that the\nprefix matches, without checking if the line has 40-hex object name,\nor the object name is immediately followed by a closing parenthesis.\nIt also does not care if there are other garbage after it.\n\nIf the code is trying to be strict to avoid misidentification, then\nthe check should be tightened (i.e. require the known fixed length,\nmake sure get_sha1_hex() is happy, 41st byte is a close parenthesis\nthat is followed by the end of line). If the code is trying to be\nmore lenient to allow people hand-editing the cherry-picked-from\nline that was generated, the check could be loosened (people may\ntruncate the 40-hex down to 12-hex or something).\n\nI cannot read from this code which one was intended; the code must\nmake up its mind, whichever way (I do not have a strong preference).\n\n> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '\n\nIs the title of this test appropriate? It looks like it is making\nsure we do not add an extra blank line after the existing footer to\nme.\n\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\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\nFor people on the sideline, $mesg_with_footer_sob ends with s-o-b by\nthe same \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" we are adding\nhere. This is probably a sensible thing to do.\n\nOne thing I am not so happy about this whole \"(cherry picked from\"\nthing (and I am again agreeing with Linus who made me change the\ndefault long time ago not to add this line by default) is this. If\nyou use \"cherry-pick -s -x $commit\" to transplant a commit from an\nunrelated part of the history, you will get the object name in the\nresulting commit. But you can do the same in at least two different\nways. The easiest is:\n\n git format-patch -1 --stdout $commit | git am -s3\n\nand a bit closer to what \"cherry-pick\" does is:\n\n git checkout $commit^0 &&\n git rebase --onto @{-1} HEAD^ &&\n git checkout -B @{-1}\n\ni.e. rebase the single commit on top of the branch you were on.\n\nIn either way, you don't get \"cherry picked from\", even though you\njust did the moral equivalent of it. Also we need to realize that\nthe first one is essentially what happens all the time here; the \"|\"\nis implemented by the mailing list. And nobody misses \"cherry\npicked from\" to point at the original commit object, which is\nuseless for the recipient of the resulting commit.\n\nBut that is an orthogonal issue.\n"},{"id":"204229","messageId":"7vzk21s0tg.fsf@alter.siamese.dyndns.org","threadId":"32196","inReplyTo":"1353894359-6733-7-git-send-email-drafnel@gmail.com","subject":"Re: [PATCH 06/11] sequencer.c: teach append_signoff how to detect duplicate s-o-b","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T00:35:39Z","receivedAt":"2012-11-29T00:35:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> +/* Returns 0 for non-conforming footer\n\nPlease format it like this:\n\n/*\n * Returns 0 for ...\n\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 i, k = 0;\n\nThis is not wrong per-se, but I do not think it is necessary ('k' is\nalways initialized at least to 'i' before it gets used). If we need\nto do this, I'd prefer to see it done in 1/11 that cleaned up the\ncontaining function as a preparation for this series.\n\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> @@ -63,14 +70,24 @@ 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\t\tis_cherry_pick_from_line(buf+i, k-i)))\n> +\t\tfound_rfc2822 = is_rfc2822_line(buf+i, k-i);\n\nNot limited to this place but please have SP around binary operators, i.e.\n\n\t\tfound_rfc2822 = is_rfc2822_line(buf + i, k - i);\n\n> +\t\tif (found_rfc2822 && sob &&\n> +\t\t\t!strncasecmp(buf+i, sob->buf, sob->len))\n> +\t\t\tfound_sob = k;\n\nAre we sure we want strncasecmp() here? I *think* you are trying to\ncatch \"signed-off-By:\" and other misspellings, and even though I\nknow in practice we know \"Brandon Casey\" and \"brandon casey\" are the\nsame person, it still feels somewhat sloppy. Perhaps it is just me.\n\n> +\n> +\t\tif (!(found_rfc2822 || is_cherry_pick_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> @@ -291,7 +308,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> @@ -547,7 +564,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> @@ -1074,9 +1091,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> @@ -1085,10 +1103,11 @@ 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> +\tif (!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> +\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\nAvoid assignment inside if () conditional. It is not immediately\nobvious what value is compared against 3 in the second one, as the\nabove makes it appear as if has_footer is uninitialized when i is\nzero.\n\nThanks.\n"},{"id":"204232","messageId":"50B6BA41.7070402@nvidia.com","threadId":"32196","inReplyTo":"7v8v9ltgxn.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 04/11] sequencer.c: recognize \"(cherry picked from ...\" as part of s-o-b footer","fromName":"Brandon Casey","fromEmail":"bcasey@nvidia.com","sentAt":"2012-11-29T01:28:33Z","receivedAt":"2012-11-29T01:28:33Z","isPatch":true,"sender":{"key":"bcasey@nvidia.com","avatar":null},"body":"On 11/28/2012 4:02 PM, Junio C Hamano wrote:\n> Brandon Casey writes:\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_pick_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>> +}\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>> @@ -1039,15 +1061,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_pick_from_line(buf+i, k-i)))\n>> \t\t\treturn 0;\n>> -\t\t}\n>> \t}\n>> \treturn 1;\n>> }\n>\n> Refactored code looks vastly more readable, but I think the\n> is_cherry_pick_from_line() function (by the way, shouldn't it be\n> named is_cherry_picked_from_line()?\n\nYes.\n\n> ) shows its ambivalence. It\n> insists that the line has to be long enough to hold 40-hex object\n> name plus a closing parenthesis, but it only makes sure that the\n> prefix matches, without checking if the line has 40-hex object name,\n> or the object name is immediately followed by a closing parenthesis.\n> It also does not care if there are other garbage after it.\n>\n> If the code is trying to be strict to avoid misidentification, then\n> the check should be tightened (i.e. require the known fixed length,\n> make sure get_sha1_hex() is happy, 41st byte is a close parenthesis\n> that is followed by the end of line). If the code is trying to be\n> more lenient to allow people hand-editing the cherry-picked-from\n> line that was generated, the check could be loosened (people may\n> truncate the 40-hex down to 12-hex or something).\n>\n> I cannot read from this code which one was intended; the code must\n> make up its mind, whichever way (I do not have a strong preference).\n\nThe intention was a stricter-type match, but implemented somewhat\nlazily. Part of me doesn't think that anyone should need to modify\nthe string that 'cherry-pick -x' adds and that a strict match is\nappropriate. The other part of me doesn't want to reject a line that\nlooks like \"(cherry picked from ...)\" line that has a trimmed sha1\ninside.\n\nI think I'll submit a somewhat loosened check.\n\n>> +test_expect_success 'cherry-pick -x -s adds sob when last sob doesnt match committer' '\n>\n> Is the title of this test appropriate? It looks like it is making\n> sure we do not add an extra blank line after the existing footer to\n> me.\n\nIt's really just part of the series of tests that checks for correct\noperation when \"the last sob doesn't match committer\". This test\nhas a few elements in it:\n\n * existing sob footer contains the committer's sob, but not last.\n ensure that we don't refrain from appending a sob just because\n one already exists in the footer.\n * ensure extra blank isn't inserted after existing footer and\n \"(cherry picked from...\"\n * ensure a blank isn't inserted between \"(cherry picked from...\"\n and new sob\n\nThe title of the test is me trying to fit \"correctly adds \\\"(cherry\npicked from...\\\" and sob when footer contains committer's sob but\nnot last\" within a single line.\n\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>\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> For people on the sideline, $mesg_with_footer_sob ends with s-o-b by\n> the same \"$GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\" we are adding\n> here. This is probably a sensible thing to do.\n>\n> One thing I am not so happy about this whole \"(cherry picked from\"\n> thing (and I am again agreeing with Linus who made me change the\n> default long time ago not to add this line by default) is this. If\n> you use \"cherry-pick -s -x $commit\" to transplant a commit from an\n> unrelated part of the history, you will get the object name in the\n> resulting commit. But you can do the same in at least two different\n> ways. The easiest is:\n>\n> git format-patch -1 --stdout $commit | git am -s3\n>\n> and a bit closer to what \"cherry-pick\" does is:\n>\n> git checkout $commit^0 &&\n> git rebase --onto @{-1} HEAD^ &&\n> git checkout -B @{-1}\n>\n> i.e. rebase the single commit on top of the branch you were on.\n>\n> In either way, you don't get \"cherry picked from\", even though you\n> just did the moral equivalent of it. Also we need to realize that\n> the first one is essentially what happens all the time here; the \"|\"\n> is implemented by the mailing list. And nobody misses \"cherry\n> picked from\" to point at the original commit object, which is\n> useless for the recipient of the resulting commit.\n>\n> But that is an orthogonal issue.\n\nAre you suggesting that people shouldn't use 'cherry-pick -x' at all?\n\nI agree that it is useless to use -x when the commit that will be\nreferenced will not be available as part of the permanent history\nof the project.\n\nI do however think it is useful to add a reference to the original\ncommit when back- or forward- porting a commit. I think it adds\na little visibility to the commit to say that it was not originally\nimplemented on top of the base it is built on. I find it helpful\nto examine the original implementation (the referenced commit) when\nverifying the correctness or attempting to understand the ported\ncommit.\n\n-Brandon\n\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":"204233","messageId":"7vvccprlya.fsf@alter.siamese.dyndns.org","threadId":"32196","inReplyTo":"CACsJy8AfaETfikvB+zON4iiX2HBehJg-xX2GGmKYHueX8rv3qw@mail.gmail.com","subject":"Re: [PATCH 00/11] alternative unify appending of sob","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-11-29T05:56:45Z","receivedAt":"2012-11-29T05:56:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nguyen Thai Ngoc Duy writes:\n\n> On Mon, Nov 26, 2012 at 8:45 AM, Brandon Casey wrote:\n>\n>> I've integrated Duy's series with a few minor tweaks. I added a couple\n>> of additional tests to t4014 and corrected one of the tests which had\n>> incorrect behavior. I think his sign-off's should still be valid, so I\n>> kept them in. Sorry that I've been slow, and now the two of us are stepping\n>> on each other's toes, but Duy please take a look and let me know if there's\n>> anything you dislike.\n>\n> I'm still not sure whether format-patch should follow cherry-pick's\n> rule in appending sob. If it does, it probably makes more sense to fix\n> the sequencer.c code then delete log-tree.c (not fixes on log-tree.c\n> then delete it). I see that your changes pass all the new tests I\n> added in format-patch so sequencer.c is probably good enough,\n> log-tree.c changes are probably not needed. Feel free take over the\n> series :-)\n\nAfter reading the series over, I agree with the above.\n\nPatch #9 that fixes the copy in log-tree.c only to discard it in\npatch #11 does not seem to be the best organization of the series.\nInstead, perhaps we can salvage the tests in patch #9 (but mark\nfailing ones as expecting failure) without updating the one in\nlog-tree.c, adjust prototype in patch #10 (still broken in\nlog-tree.c) to avoid having to make changes to the callers in patch\n#11, and then conclude the series with #11?\n\nOther than the code in patches #06 and #07 that I already commented\non, i.e. assignments in if () condition that make it harder to\nfollow the logic, I did not find anything majorly objectionable in\nthe series.\n\nThanks for a pleasant read.\n"}]}