{"thread":{"id":"31101","subject":"[PATCH] builtin-commit.c: Not add duplicate S-o-b when commit","startedAt":"2012-07-26T06:01:19Z","lastAt":"2012-07-27T01:07:22Z","messageCount":5,"participants":["Jiang Xin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"195814","messageId":"ee08efceaa3f015732a19d49eb96bdeeaaf3d609.1343282283.git.worldhello.net@gmail.com","threadId":"31101","inReplyTo":null,"subject":"[PATCH] builtin-commit.c: Not add duplicate S-o-b when commit","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-26T06:01:19Z","receivedAt":"2012-07-26T06:01:19Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"Scan the whole rfc2822 footer for duplicate S-o-b, not just the last\nline of the commit message.\n\nA commit may have multiple S-o-bs, or other tags, such as:\n\n some commit log...\n\n Signed-off-by: C O Mitter \n Reported-by: R E Porter \n\nBecause the S-o-b is not located at the last line in the above commit,\nwhen the above commit is amended by the original committer, a\nduplicated S-o-b may appended by accident. New commit log may looks\nlike:\n\n some commit log...\n\n Signed-off-by: C O Mitter \n Reported-by: R E Porter \n Signed-off-by: C O Mitter \n\nThis hack scans the whole rfc2822 footer for duplicate S-o-b, and only\nappend a S-o-b when necessary. Also add testcases in 't/t7502-commit.sh'\nfor this.\n\nSigned-off-by: Jiang Xin \n---\n builtin/commit.c | 28 ++++++++++++++++++++++++----\n t/t7502-commit.sh | 19 +++++++++++++++++++\n 2 files changed, 43 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 20cef..1a3da 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -704,15 +704,35 @@ static int prepare_to_commit(const char *index_file, const char *prefix,\n \tif (signoff) {\n \t\tstruct strbuf sob = STRBUF_INIT;\n \t\tint i;\n+\t\tint hit_footer = 0;\n+\t\tint hit_sob = 0;\n \n \t\tstrbuf_addstr(&sob, sign_off_header);\n \t\tstrbuf_addstr(&sob, fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n \t\t\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n \t\tstrbuf_addch(&sob, '\\n');\n-\t\tfor (i = sb.len - 1; i > 0 && sb.buf[i - 1] != '\\n'; i--)\n-\t\t\t; /* do nothing */\n-\t\tif (prefixcmp(sb.buf + i, sob.buf)) {\n-\t\t\tif (!i || !ends_rfc2822_footer(&sb))\n+\t\tfor (i = sb.len - 1; i > 0; i--) {\n+\t\t\tif (hit_footer && sb.buf[i] == '\\n') {\n+\t\t\t\thit_footer = 2;\n+\t\t\t\ti += 2;\n+\t\t\t\tbreak;\n+\t\t\t}\n+\t\t\thit_footer = (sb.buf[i] == '\\n');\n+\t\t}\n+\t\thit_footer = (2 == hit_footer);\n+\t\tif (hit_footer) {\n+\t\t\twhile (i < sb.len)\n+\t\t\t{\n+\t\t\t\tif (!prefixcmp(sb.buf + i, sob.buf)) {\n+\t\t\t\t\thit_sob = 1;\n+\t\t\t\t\tbreak;\n+\t\t\t\t}\n+\t\t\t\twhile (i < sb.len && sb.buf[i++] != '\\n')\n+\t\t\t\t\t; /* do nothing */\n+\t\t\t}\n+\t\t}\n+\t\tif (!hit_sob) {\n+\t\t\tif (!hit_footer || !ends_rfc2822_footer(&sb))\n \t\t\t\tstrbuf_addch(&sb, '\\n');\n \t\t\tstrbuf_addbuf(&sb, &sob);\n \t\t}\ndiff --git a/t/t7502-commit.sh b/t/t7502-commit.sh\nindex 18145..8198f 100755\n--- a/t/t7502-commit.sh\n+++ b/t/t7502-commit.sh\n@@ -336,7 +336,26 @@ test_expect_success 'A single-liner subject with a token plus colon is not a foo\n \tgit commit -s -m \"hello: kitty\" --allow-empty &&\n \tgit cat-file commit HEAD | sed -e \"1,/^$/d\" >actual &&\n \ttest_line_count = 3 actual\n+'\n+\n+cat > expect << EOF\n+Footer-like: commit log\n+\n+Signed-off-by: C O Mitter \n+EOF\n+\n+test_expect_success 'S-o-b after footer-like commit message' '\n+\thead -1 expect | git commit -s --allow-empty -F - &&\n+\tgit cat-file commit HEAD | sed \"1,/^\\$/d\" > output &&\n+\ttest_cmp expect output\n+'\n+\n+echo \"Reported-by: R E Porter \" >> expect\n \n+test_expect_success 'no duplicate S-o-b when signoff' '\n+\tcat expect | git commit -s --allow-empty -F - &&\n+\tgit cat-file commit HEAD | sed \"1,/^\\$/d\" > output &&\n+\ttest_cmp expect output\n '\n \n cat >.git/FAKE_EDITOR < writes:\n\n> Scan the whole rfc2822 footer for duplicate S-o-b, not just the last\n> line of the commit message.\n>\n> A commit may have multiple S-o-bs, or other tags, such as:\n>\n> some commit log...\n>\n> Signed-off-by: C O Mitter \n> Reported-by: R E Porter \n>\n> Because the S-o-b is not located at the last line in the above commit,\n> when the above commit is amended by the original committer, a\n> duplicated S-o-b may appended by accident. New commit log may looks\n> like:\n>\n> some commit log...\n>\n> Signed-off-by: C O Mitter \n> Reported-by: R E Porter \n> Signed-off-by: C O Mitter \n>\n\nAfter stating the observation like the above, please make it a habit\nto say \"which is bad because...\", if you think it is a bad behaviour\nand the patch is about fixing it.\n\nBecause a chain of S-o-b is used to record the flow of a patch, it\nis entirely normal if developer A writes the patch (she signs it\noff), reviewer B picks it up and sends it back with a minor fix-up\nto the list, and developer A again picks it up from the list and\nforwards it to the uplevel maintainer, in which case you may see\nS-o-b by A, then B (it may be S-o-b or something else,\ne.g. Reviewed-by) and then S-o-b by A again.\n\nThe above observation is correct (a commit log may look like so),\nbut your untold conclusion (it is a bad thing because there are\nS-o-b from the same person twice) is not necessarily correct.\n"},{"id":"195819","messageId":"CANYiYbH0Jfk0LLp8HXVEamD0uRzM2OGJCE8W4h_UoDLh1pMVHA@mail.gmail.com","threadId":"31101","inReplyTo":"7vy5m781j5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-26T07:15:24Z","receivedAt":"2012-07-26T07:15:24Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"2012/7/26 Junio C Hamano :\n> After stating the observation like the above, please make it a habit\n> to say \"which is bad because...\", if you think it is a bad behaviour\n> and the patch is about fixing it.\n\nIndead before I start, I examine git-commit and git-am, and find\nthe behaviours of the two commands are different.\n\n\"git commit -s\" checks the last line of the footer, while \"git -am\"\nchecks the last S-o-b. E.g. original commit X:\n\n commit log...\n\n Signed-off-by: A\n Signed-off-by: B\n Reported-by: C\n\nWhen user B amend the commit, the amended commit Y looks like:\n\n commit log...\n\n Signed-off-by: A\n Signed-off-by: B\n Reported-by: C\n Signed-off-by: B\n\nWhile if the original commit X send to user B by patch, and\nuser B run command \"git am -s\", the commit would be:\n\n Signed-off-by: A\n Signed-off-by: B\n Reported-by: C\n\nSo I guess duplicate S-o-b is not intentional.\n\nI use an alias for commit:\n\n git config --global alias.ci \"commit -s\"\n\nAnd will encounter duplicate S-o-b issues frequently, especially\nformat-patch/send-email workflow.\n\n-- \nJiang Xin\n"},{"id":"195866","messageId":"7vpq7i8oag.fsf@alter.siamese.dyndns.org","threadId":"31101","inReplyTo":"CANYiYbH0Jfk0LLp8HXVEamD0uRzM2OGJCE8W4h_UoDLh1pMVHA@mail.gmail.com","subject":"Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T16:44:55Z","receivedAt":"2012-07-26T16:44:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jiang Xin writes:\n\n> 2012/7/26 Junio C Hamano :\n>> After stating the observation like the above, please make it a habit\n>> to say \"which is bad because...\", if you think it is a bad behaviour\n>> and the patch is about fixing it.\n>\n> Indead before I start, I examine git-commit and git-am, and find\n> the behaviours of the two commands are different.\n>\n> \"git commit -s\" checks the last line of the footer, while \"git -am\"\n> checks the last S-o-b.\n\nI think \"git am -s\" (which I think you meant) is wrong, then.\n\n> E.g. original commit X:\n>\n> commit log...\n>\n> Signed-off-by: A\n> Signed-off-by: B\n> Reported-by: C\n\nThe order in this \"original\" is already wrong, isn't it, though?\nDidn't the change result by first C reporting an issue, fixes done\nby A and forwarded by B?\n\n\n> When user B amend the commit, the amended commit Y looks like:\n>\n> commit log...\n>\n> Signed-off-by: A\n> Signed-off-by: B\n> Reported-by: C\n> Signed-off-by: B\n>\n> While if the original commit X send to user B by patch, and\n> user B run command \"git am -s\", the commit would be:\n>\n> Signed-off-by: A\n> Signed-off-by: B\n> Reported-by: C\n>\n> So I guess duplicate S-o-b is not intentional.\n\nI think the two commands are doing randomly different things on\ngarbage input. The order in the input (i.e. your \"original\") does\nnot make sense. C is not the person who handled the patch the last.\n\nIf you start from\n\n\tReported-by: X\n\tS-o-b: A\n\tReviewed-by: Y\n S-o-b: B\n\ni.e. the last person who touched this patch is B, tweaking the patch\nand amending a commit will add S-o-b for the person who amends, iff\nthat person is not B, which is what you usually want.\n\n> I use an alias for commit:\n>\n> git config --global alias.ci \"commit -s\"\n>\n> And will encounter duplicate S-o-b issues frequently, especially\n> format-patch/send-email workflow.\n"},{"id":"195920","messageId":"CANYiYbFOS6Qw+vrF6_amzKinHwULrqJJKjm9SSy7i3rCtB10aw@mail.gmail.com","threadId":"31101","inReplyTo":"7vpq7i8oag.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] builtin-commit.c: Not add duplicate S-o-b when commit","fromName":"Jiang Xin","fromEmail":"worldhello.net@gmail.com","sentAt":"2012-07-27T01:07:22Z","receivedAt":"2012-07-27T01:07:22Z","isPatch":true,"sender":{"key":"worldhello.net@gmail.com","avatar":"https://avatars.githubusercontent.com/u/183860?v=4"},"body":"2012/7/27 Junio C Hamano :\n>> Signed-off-by: A\n>> Signed-off-by: B\n>> Reported-by: C\n>>\n>> So I guess duplicate S-o-b is not intentional.\n>\n> I think the two commands are doing randomly different things on\n> garbage input. The order in the input (i.e. your \"original\") does\n> not make sense. C is not the person who handled the patch the last.\n\nI am clear, thanks. Please reword the commit 3c58c1a in branch\n\"jx/i18n-1.7.11\". Change the tags from:\n\n Signed-off-by: Jiang Xin \n Signed-off-by: Ævar Arnfjörð Bjarmason \n Reported-by: Vincent van Ravesteijn \n Reviewed-by: Stefano Lattarini \n Signed-off-by: Junio C Hamano \n\nto\n\n Reported-by: Vincent van Ravesteijn \n Signed-off-by: Jiang Xin \n Signed-off-by: Ævar Arnfjörð Bjarmason \n Reviewed-by: Stefano Lattarini \n Signed-off-by: Junio C Hamano \n\n\n-- \nJiang Xin\n"}]}