{"thread":{"id":"40278","subject":"More builtin git-am issues..","startedAt":"2015-09-04T23:47:10Z","lastAt":"2015-09-08T18:14:16Z","messageCount":30,"participants":["Linus Torvalds","Jeff King","Junio C Hamano","Johannes Sixt","Paul Tan","Christian Couder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"269458","messageId":"CA+55aFzN4SnenchxPScn61_apzitGAPtoYEd49iLZPxgK0KQGw@mail.gmail.com","threadId":"40278","inReplyTo":null,"subject":"More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-04T23:47:10Z","receivedAt":"2015-09-04T23:47:10Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"Ok, this may not be new either, but I'm trying to be careful when\nusing \"git am\" these days, because I know it got rewritten.\n\nAnd I _think_ the whitespace handling for adding sign-offs got scrogged.\n\nI just applied the usual patch-bomb from Andrew, and several of the\ncommits (but not all) end up looking like this:\n\n Cc: [3.15+]\n Signed-off-by: Andrew Morton \n\n Signed-off-by: Linus Torvalds \n\nnote the extraneous whitespace line between Andrew's sign-off and mine.\n\nWhat's odd is that the emails I'm applying literally don't have that\nextra empty line, so it's git that somehow decides to add it. Only\nfor a few cases, though.\n\nThe pattern *seems* to be that git now looks at the *first* line of\nthe sign-off block and decides that \"this is a sign-off block if that\nfirst line has a sign-ff on it, ie this is fine:\n\n Signed-off-by: Andrea Arcangeli \n Cc: Pavel Emelyanov \n Cc: Dave Hansen \n Cc: Linus Torvalds \n Signed-off-by: Andrew Morton \n Signed-off-by: Linus Torvalds \n\nbut the failing cases have a comment by Andrew:\n\n [akpm@linux-foundation.org: coding-style fixes]\n Signed-off-by: Tang Chen \n Cc: Xishi Qiu \n Cc: Yasuaki Ishimatsu \n Cc: Kamezawa Hiroyuki \n Cc: Taku Izumi \n Cc: Gu Zheng \n Cc: Naoya Horiguchi \n Cc: Vlastimil Babka \n Cc: Mel Gorman \n Cc: David Rientjes \n Cc: [4.2.x]\n Signed-off-by: Andrew Morton \n\n Signed-off-by: Linus Torvalds \n\nie that \"[akpm@linux-foundation.org: coding-style fixes]\" makes git am\nnow decide that the previous block of text was not a sign-off block,\nso it adds an empty line before adding my sign-off. But very obviously\nit *was* a sign-off block.\n\nMaybe this isn't new at all, and it's just that I notice because I'm\nlooking for \"git am\" oddities. Something is clearly wrong in\n\"has_conforming_footer()\".\n\nI *think* it's this part:\n\n if (!(found_rfc2822 ||\n is_cherry_picked_from_line(buf + i, k - i - 1)))\n return 0;\n\nwhich basically returns 0 for _any_ line in the footer that doesn't\nmatch the found_rfc2822 format.\n\nI really think that if we find any \"Signed-off-by:\" in that last\nchunk, we should not add a whitespace.\n\nComments?\n\n Linus\n"},{"id":"269459","messageId":"CA+55aFw2bnhSQYk4FaHfp4ED0Y611NWyQs05TMQtFj=2As1=nA@mail.gmail.com","threadId":"40278","inReplyTo":"CA+55aFzN4SnenchxPScn61_apzitGAPtoYEd49iLZPxgK0KQGw@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-04T23:52:42Z","receivedAt":"2015-09-04T23:52:42Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 4, 2015 at 4:47 PM, Linus Torvalds\n wrote:\n>\n> I *think* it's this part:\n>\n> if (!(found_rfc2822 ||\n> is_cherry_picked_from_line(buf + i, k - i - 1)))\n> return 0;\n>\n> which basically returns 0 for _any_ line in the footer that doesn't\n> match the found_rfc2822 format.\n\nConfirmed. I hacked up a version that just doesn't do that check at\nall, and it works fine (but obviously only on well-formatted emails\nthat really do have a sign-off).\n\nSo I think that logic should basically be extended to saying\n\n - if any line in the last chunk has a \"Signed-off-by:\", set a flag.\n\n - at the end of the loop, if that flag wasn't set, return 0.\n\nInstead of that thing that basically returns zero immediately when it\nsees a line it doesn't like.\n\nI'm in the middle of my merge window, so I'm not going to get around\nto writing a patch until that's over. Hopefully somebody will step up\nin the meantime. Hint, hint.\n\n Linus\n"},{"id":"269460","messageId":"20150905000745.GC11443@sigill.intra.peff.net","threadId":"40278","inReplyTo":"CA+55aFw2bnhSQYk4FaHfp4ED0Y611NWyQs05TMQtFj=2As1=nA@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-05T00:07:45Z","receivedAt":"2015-09-05T00:07:45Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 04, 2015 at 04:52:42PM -0700, Linus Torvalds wrote:\n\n> On Fri, Sep 4, 2015 at 4:47 PM, Linus Torvalds\n> wrote:\n> >\n> > I *think* it's this part:\n> >\n> > if (!(found_rfc2822 ||\n> > is_cherry_picked_from_line(buf + i, k - i - 1)))\n> > return 0;\n> >\n> > which basically returns 0 for _any_ line in the footer that doesn't\n> > match the found_rfc2822 format.\n> \n> Confirmed. I hacked up a version that just doesn't do that check at\n> all, and it works fine (but obviously only on well-formatted emails\n> that really do have a sign-off).\n> \n> So I think that logic should basically be extended to saying\n> \n> - if any line in the last chunk has a \"Signed-off-by:\", set a flag.\n> \n> - at the end of the loop, if that flag wasn't set, return 0.\n\nDo we want to make Signed-off-by a special token here? What about \"Cc:\",\nand other project-specific trailers? You wouldn't want to end up with:\n\n [some comment]\n Cc: somebody\n\n Signed-off-by: somebody else\n\nIt's not a problem for git-am, which should be taking in patches that\nare already signed-off (and after all, if your project does not use\nsignoffs, why are you using \"am -s\"?). But won't \"git commit -s\" run\ninto the same problem?\n\nWe could look for an arbitrary rfc2822-ish string, but I'd be worried\nslightly about false positives, like:\n\n This is a paragraph with arbitrary text. But one\n of the lines will use a colon, and that a causes a\n problem: because of wrapping, this line kind of looks\n like a trailer.\n\nMaybe only the final line needs to look rfc2822-ish? Or maybe we can\nconstrain the content that _doesn't_ look like a trailer line. Is it\nsufficient to allow a block encased in \"[]\", rather than arbitrary text?\n\nAll that being said, I think we can punt on the harder issues for now,\nand just restore the original git-am.sh behavior (which it looks\nlike was fairly naive) for the upcoming release, so that there is no\nregression.\n\n-Peff\n"},{"id":"269462","messageId":"CA+55aFzR2kZuS=b5D0sSV7u7Wm3a2_FENOD1_kYmr4gqdcVS2w@mail.gmail.com","threadId":"40278","inReplyTo":"20150905000745.GC11443@sigill.intra.peff.net","subject":"Re: More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-05T00:36:43Z","receivedAt":"2015-09-05T00:36:43Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 4, 2015 at 5:07 PM, Jeff King wrote:\n>\n> Do we want to make Signed-off-by a special token here? What about \"Cc:\",\n> and other project-specific trailers? You wouldn't want to end up with:\n>\n> [some comment]\n> Cc: somebody\n>\n> Signed-off-by: somebody else\n>\n> It's not a problem for git-am, which should be taking in patches that\n> are already signed-off (and after all, if your project does not use\n> signoffs, why are you using \"am -s\"?). But won't \"git commit -s\" run\n> into the same problem?\n\nSo I'm a *bit* worried about taking anything else than Signed-off-by:\nexactly because of the problem you mention:\n\n> We could look for an arbitrary rfc2822-ish string, but I'd be worried\n> slightly about false positives, like:\n>\n> This is a paragraph with arbitrary text. But one\n> of the lines will use a colon, and that a causes a\n> problem: because of wrapping, this line kind of looks\n> like a trailer.\n\nwhich clearly needs an empty line before the sign-off.\n\nAnd even if we limit it to just the last line, like you suggest:\n\n> Maybe only the final line needs to look rfc2822-ish?\n\nI'd still worry, for the same exact reason. The \"rfc2822-ish\" check is\n_so_ weak that it can be trivially triggered by normal text.\n\nSo maybe it doesn't require a strict \"Signed-off-by:\" match, but I\nthink it needs something stricter than the found_rfc2822 format (like\nmaybe \"looks like a name/email\"). We just don't want to require that\n*all* the lines are that way, because at least in the kernel we often\ndo end up adding small comments in that section.\n\nThe git project sign-off rules seem to be stricter, and it looks like\nit's almost universally of the form \"Signed-off-by:\" with a few\n\"Helped-by:\" and \"Reviewed-by:\". In the kernel, we really do tend to\nbe messier in that area, with the sign-off chunk not just having the\nsign-offs and cc's etc, but also tends to have those occasional small\nnotes left by the people forwarding the emails.\n\nAnd we also often don't have _strictly_ just an email. We tend to have\nthings like\n\n Cc: # v3.13+\n\netc, and sometimes we don't have have any email at all, but things like\n\n Reported-by: coverity\n Tested-by: MysterX on #openelec\n Generated-by: Coccinelle SmPL\n\nso it's hard to give a really strict rule. It's fairly free-format.\n\nThat said, I would argue that when you apply a patch with sign-off,\npretty much by definition that patch _should_ have a sign-off from the\noriginator. So I suspect that the \"there is an existing sign-off in\nthat last chunk\" is a fairly good rule. Even if there are lots of\nother lines too.\n\nIf you're using \"git commit -s\" to just commit your own work, you\npresumably would normally want the extra sign-off. Or at least you can\ndo a \"git commit --amend\" fairly easily to fix things up. Doing the\namending later for \"git am\" is rather impractical, when it might have\nbeen a series of 100+ emails..\n\n Linus\n"},{"id":"269463","messageId":"xmqqh9n9ele4.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFw2bnhSQYk4FaHfp4ED0Y611NWyQs05TMQtFj=2As1=nA@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T00:54:11Z","receivedAt":"2015-09-05T00:54:11Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> So I think that logic should basically be extended to saying\n>\n> - if any line in the last chunk has a \"Signed-off-by:\", set a flag.\n>\n> - at the end of the loop, if that flag wasn't set, return 0.\n\nI am reluctant to special case S-o-b: too much, even though this is\nabout \"am -s\" and by definition S-o-b: is special, as that is what\nwe are adding after all.\n\nHow about a bit looser rule like this?\n\n A block of text at the end of the message, each and every\n line in which must match \"^[^: \t]+:[ ]\" (that is,\n a \"keyword\" that does not contain a whitespace nor a colon,\n followed by a colon and whitespace, and arbitrary value thru\n the end of line) is a signature block.\n"},{"id":"269465","messageId":"xmqqd1xxekt8.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFzN4SnenchxPScn61_apzitGAPtoYEd49iLZPxgK0KQGw@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T01:06:43Z","receivedAt":"2015-09-05T01:06:43Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> but the failing cases have a comment by Andrew:\n>\n> [akpm@linux-foundation.org: coding-style fixes]\n> Signed-off-by: Tang Chen \n> Cc: Xishi Qiu \n> Cc: Yasuaki Ishimatsu \n> Cc: Kamezawa Hiroyuki \n> Cc: Taku Izumi \n> Cc: Gu Zheng \n> Cc: Naoya Horiguchi \n> Cc: Vlastimil Babka \n> Cc: Mel Gorman \n> Cc: David Rientjes \n> Cc: [4.2.x]\n> Signed-off-by: Andrew Morton \n>\n> Signed-off-by: Linus Torvalds \n>\n> ie that \"[akpm@linux-foundation.org: coding-style fixes]\" makes git am\n> now decide that the previous block of text was not a sign-off block,\n> so it adds an empty line before adding my sign-off. But very obviously\n> it *was* a sign-off block.\n\nAhh, OK, scratch what I said earlier. The user intended this to be\nsign-off block, but the convention append_signoff() was taught from\nvery earlier days is that the sign-off block must consist of block\nof text all of which look like rfc2822 \"keyword: value\" header lines,\nand the comment thing makes it a non-conforming header.\n\nPerhaps\n\n A block of text at the end of the existing text could be a\n signature block. If all its lines that are rfc2822-like are at\n its end, then it is a sign-off block. Otherwise it is not.\n\nwould allow the leading non-signature lines in the above example.\n\nIf the comment line (which I would say should have been separated by\na blank line from the signature block if only to make it easier to\nread the whole thing) were in the middle, e.g.\n\n> Signed-off-by: Tang Chen \n> Cc: Xishi Qiu \n> Cc: Yasuaki Ishimatsu \n> Cc: Vlastimil Babka \n> Cc: Mel Gorman \n> [akpm@linux-foundation.org: coding-style fixes]\n> Cc: David Rientjes \n> Cc: [4.2.x]\n> Signed-off-by: Andrew Morton \n\nthat rule would still not think this is a signature block, but at\nthat point, do we really want to consider such a block of text a\nsignature block?\n"},{"id":"269464","messageId":"CA+55aFw-PObyj2boqpeA9DDcKeAuCGouT7Qd2Zw02+sxvd2CgA@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqh9n9ele4.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-05T01:06:44Z","receivedAt":"2015-09-05T01:06:44Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 4, 2015 at 5:54 PM, Junio C Hamano wrote:\n>\n> How about a bit looser rule like this?\n>\n> A block of text at the end of the message, each and every\n> line in which must match \"^[^: ]+:[ ]\" (that is,\n> a \"keyword\" that does not contain a whitespace nor a colon,\n> followed by a colon and whitespace, and arbitrary value thru\n> the end of line) is a signature block.\n\nNo. That's still broken.\n\nThe thing is, and that was what the report was all about, not every\nline _is_ of that format. We have commetns from the sign-off people.\nThings like this:\n\n Signed-off-by: Noam Camus \n Acked-by: Vineet Gupta \n [ Also removed pointless cast from \"void *\". - Linus ]\n Signed-off-by: Linus Torvalds \n\nor\n\n Signed-off-by: Andi Kleen \n [ Updated comments and changelog a bit. ]\n Signed-off-by: Peter Zijlstra (Intel) \n Link: http://lkml.kernel.org/r/1424225886-18652-3-git-send-email-andi@firstfloor.org\n Signed-off-by: Ingo Molnar \n\nso no, it is simply not true that \"every line must match\".\n\nI'm not even seeing why you argue for that, since clearly having a\nsign-off-line is actually a safer choice too. The \"every line must\nmatch\" rule is bad, not just because it's not true like above, but\nalso because it can be true without it being a sign-off block.\n\nFor example, it's not at all unlikely that you have perfectly normal\ncomments that just list some subsystem and their changes. Which could\neasily look like\n\n Trivial fixes all over the tree\n\n drm: fix whitespace\n mm: speeling errors\n kernel: indentation and codign style\n\nThe above looks like a perfectly sane commit log to me.\n\nDo you seriously think that it makes for a better \"sign-off block\ntest\" than one that actually checks for \"is there a sign-off line\"?\n\nI'd much rather have special cases like testing for specific keywords\nor looking for things that look like emails, than make it about being\n\"every line has this very generic format\".\n\n Linus\n"},{"id":"269466","messageId":"CA+55aFwcv+iXnUtMhZrtx9hFrJ_xjfxvoinLmKTyEfLwrDoJvQ@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqd1xxekt8.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-05T01:20:37Z","receivedAt":"2015-09-05T01:20:37Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 4, 2015 at 6:06 PM, Junio C Hamano wrote:\n>\n> that rule would still not think this is a signature block, but at\n> that point, do we really want to consider such a block of text a\n> signature block?\n\nSo exactly why are you arguing for these rules that are known to break\nin real life, that I gave actual examples for existing, and that I\nalso gave an actual example for not just giving a false negative, but\nalso a false positive?\n\nI'm also pretty sure that what you are arguing for is a regression.\n\nNow, as mentioned, it may well be true that we've had this odd\nbehavior before, and it's not a real regression - I may just have\npicked up on this problem because I've been more careful. Maybe I\ndidn't notice these problems before.\n\nBut looking at the old git-am.sh script, it does simply seem to look\nfor that '^Signed-off-by:' pattern. It did\n\n ADD_SIGNOFF=$(\n test \"$LAST_SIGNED_OFF_BY\" = \"$SIGNOFF\" || {\n test '' = \"$LAST_SIGNED_OFF_BY\" && echo\n echo \"$SIGNOFF\"\n })\n\nwhich seems to literally just check the last sign-off line it found.\nIf it matches the new sign-off, it doesn't do anything (and doesn't\nadd the new one either), and if it doesn't exist at all (so it's\nempty) it adds teh empty line.\n\nQuite frankly, that not only worked for a long time, it's simply less\nambiguous than your made up rule.\n\nIt's very simple. \"if you find a sign-off in the commit message, don't\nadd an new empty line before the new signoff\".\n\nMuch better than \"every line in the last set of lines must match some\nweak format that isn't even true, and is too non-specific anyway\".\n\n Linus\n"},{"id":"269467","messageId":"xmqq613pek1b.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFw-PObyj2boqpeA9DDcKeAuCGouT7Qd2Zw02+sxvd2CgA@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T01:23:28Z","receivedAt":"2015-09-05T01:23:28Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> The thing is, and that was what the report was all about, not every\n> line _is_ of that format. We have commetns from the sign-off people.\n> Things like this:\n>\n> Signed-off-by: Noam Camus \n> Acked-by: Vineet Gupta \n> [ Also removed pointless cast from \"void *\". - Linus ]\n> Signed-off-by: Linus Torvalds \n\nOK, I didn't know that was acceptable in the kernel community\nto have random comments like that inside the block. Can these\ncomments span multiple paragraphs? What I am wondering is what\nyou want to see in a case like this:\n\n Signed-off-by: Noam Camus \n Acked-by: Vineet Gupta \n [ Also removed pointless cast from \"void *\". - Linus]\n Signed-off-by: Linus Torvalds \n [ Ahh, we have to tell at least these people\n\n - stakeholder class 1\n - staeholder class 2\n ]\n Cc: foo\n Cc: bar\n"},{"id":"269468","messageId":"xmqq1tedejzh.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFwcv+iXnUtMhZrtx9hFrJ_xjfxvoinLmKTyEfLwrDoJvQ@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T01:24:34Z","receivedAt":"2015-09-05T01:24:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> On Fri, Sep 4, 2015 at 6:06 PM, Junio C Hamano wrote:\n>>\n>> that rule would still not think this is a signature block, but at\n>> that point, do we really want to consider such a block of text a\n>> signature block?\n>\n> So exactly why are you arguing for these rules that are known to break\n> in real life, that I gave actual examples for existing,...\n\nOur mails crossed. I was working from the [akpm] comment at the\nvery beginning while you were giving more examples that inserts\ncomments in the middle which I didn't see.\n"},{"id":"269469","messageId":"CA+55aFw9BmBKEbibwVs+LuVQR2=PKt+hh5LAhdmVC-OkgBB3eg@mail.gmail.com","threadId":"40278","inReplyTo":"xmqq613pek1b.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-05T01:35:46Z","receivedAt":"2015-09-05T01:35:46Z","isPatch":false,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Fri, Sep 4, 2015 at 6:23 PM, Junio C Hamano wrote:\n>\n> OK, I didn't know that was acceptable in the kernel community\n> to have random comments like that inside the block. Can these\n> comments span multiple paragraphs? What I am wondering is what\n> you want to see in a case like this:\n>\n> Signed-off-by: Noam Camus \n> Acked-by: Vineet Gupta \n> [ Also removed pointless cast from \"void *\". - Linus]\n> Signed-off-by: Linus Torvalds \n> [ Ahh, we have to tell at least these people\n>\n> - stakeholder class 1\n> - staeholder class 2\n> ]\n> Cc: foo\n> Cc: bar\n\nSo quite frankly, at that point if git doesn't recognize it as a\nsign-off block, I don't think it's a big deal.\n\nThat said, the original \"git am\" rules actually seem to be rather\nstraightforward: it's never an issue about \"last block of text\", and\nit's simply an issue of \"is there a sign-ff _anywhere_ in the text\".\n\nThat simplicity has a certain appeal to me. I don't think it was\nnecessarily written that way because it was \"well designed\" - I\nsuspect it is more an issue of \"easy to implement in a shell-script\".\n\nAnd it's possible that I'm mis-reading the scripts too. It's not like\nI _remember_ what the exact behavior was, I just think it used to work\nreally well for us (ie I don't recall seeing lots of those empty lines\nin the middle of the sign-off block before, and this current merge\nwindow it happened for four of the emails I applied from Andrew's\n119-patch series..\n\nFour out of 119 emails may not be a big percentage, but it does mean\nthat it's not horribly unusual either..\n\n Linus\n"},{"id":"269471","messageId":"55EA9A13.2050108@kdbg.org","threadId":"40278","inReplyTo":"xmqqh9n9ele4.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2015-09-05T07:30:27Z","receivedAt":"2015-09-05T07:30:27Z","isPatch":false,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 05.09.2015 um 02:54 schrieb Junio C Hamano:\n> Linus Torvalds writes:\n>\n>> So I think that logic should basically be extended to saying\n>>\n>> - if any line in the last chunk has a \"Signed-off-by:\", set a flag.\n>>\n>> - at the end of the loop, if that flag wasn't set, return 0.\n>\n> I am reluctant to special case S-o-b: too much, even though this is\n> about \"am -s\" and by definition S-o-b: is special, as that is what\n> we are adding after all.\n>\n> How about a bit looser rule like this?\n>\n> A block of text at the end of the message, each and every\n> line in which must match \"^[^: \t]+:[ ]\" (that is,\n> a \"keyword\" that does not contain a whitespace nor a colon,\n> followed by a colon and whitespace, and arbitrary value thru\n> the end of line) is a signature block.\n\nWhy do we need a new rule? The old git-am had a logic that pleased \neveryone, and it must have been implemented somewhere. Shouldn't it be \nsufficient to just re-implement or re-use that logic?\n\n-- Hannes\n"},{"id":"269472","messageId":"20150905080325.GA25039@sigill.intra.peff.net","threadId":"40278","inReplyTo":"55EA9A13.2050108@kdbg.org","subject":"Re: More builtin git-am issues..","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-05T08:03:26Z","receivedAt":"2015-09-05T08:03:26Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 05, 2015 at 09:30:27AM +0200, Johannes Sixt wrote:\n\n> >How about a bit looser rule like this?\n> >\n> > A block of text at the end of the message, each and every\n> > line in which must match \"^[^: \t]+:[ ]\" (that is,\n> > a \"keyword\" that does not contain a whitespace nor a colon,\n> > followed by a colon and whitespace, and arbitrary value thru\n> > the end of line) is a signature block.\n> \n> Why do we need a new rule? The old git-am had a logic that pleased everyone,\n> and it must have been implemented somewhere. Shouldn't it be sufficient to\n> just re-implement or re-use that logic?\n\nThat was my thought, too; if there is a regression, let's start by\nfixing that for the upcoming 2.6.0, and then we can worry about doing\nsomething fancier[1] later.\n\nAnd I think the original behavior really is what Linus is asking for: we\nconsider the final block, even with a \"[]\" comment, as a S-o-b block if\nit has a Signed-off-by in it. So if we have the final block:\n\n [some comment]\n Signed-off-by: Somebody Else \n\nRunning \"am -s\" with the current master yields:\n\n [some comment]\n Signed-off-by: Somebody Else \n\n Signed-off-by: Jeff King \n\nwhich is wrong. Running the same with v2.5.0 gets:\n\n [some comment]\n Signed-off-by: Somebody Else \n Signed-off-by: Jeff King \n\nSo far so good. Now let's change our input to:\n\n [some comment]\n Reviewed-by: Somebody Else \n\nand run \"am -s\". Current \"master\" continues to stick the extra line in\nthere. No surprise. But now so does v2.5.0!\n\nSo I don't think the old behavior covered all cases well, either, and\nthere's room for improvement. But likewise, I don't recall seeing a lot\nof complaints about it in practice. It seems like a sane thing to\nrestore it for the upcoming release, and then build from there.\n\n-Peff\n\n[1] I think part of the reason people are interested in \"fancy\" here is\n that this topic extends beyond \"git am\". There's \"commit -s\", of\n course, but there's also the generic \"interpret-trailers\" command,\n which is supposed to be a generalization of the \"--signoff\" option.\n It would be nice if the rules remained consistent for when we add a\n trailer to an existing block, rather than special-casing signoffs.\n\n But again, I think that's something to shoot for in the long run.\n It's more important in the short term not to regress \"am\".\n"},{"id":"269494","messageId":"xmqqwpw4dh4a.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"55EA9A13.2050108@kdbg.org","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T15:24:05Z","receivedAt":"2015-09-05T15:24:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Sixt writes:\n\n> Why do we need a new rule? The old git-am had a logic that pleased\n> everyone, and it must have been implemented somewhere. Shouldn't it be\n> sufficient to just re-implement or re-use that logic?\n\nIf you look at the helper the rewritten \"am\" calls, you will notice\nthat it is used by other things that wants to know whether a log\nmessage has the final \"trailer\" block, and the reason why the\ncallers want to know is not limited to \"I want to add a sign-off\".\n\nWhat your \"just re-implement\" means is \"change the world order\ndrastically to other callers who do not want such a special casing\nfor sign-off\".\n\nThat was why I tried to see if a slight tweak to the rule shared by\nall callers that would be much less likely to break these other\ncallers can satisfy the need of \"am\".\n\nPerhaps we would need to tell has_conforming_footer() function who\nthe caller is, and use a different logic (i.e. \"just re-implement\")\nwhen the caller is append_signoff().\n"},{"id":"269495","messageId":"xmqqsi6sdgnm.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"xmqqwpw4dh4a.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T15:34:05Z","receivedAt":"2015-09-05T15:34:05Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Johannes Sixt writes:\n>\n>> Why do we need a new rule? The old git-am had a logic that pleased\n>> everyone, and it must have been implemented somewhere. Shouldn't it be\n>> sufficient to just re-implement or re-use that logic?\n> ...\n> Perhaps we would need to tell has_conforming_footer() function who\n> the caller is, and use a different logic (i.e. \"just re-implement\")\n> when the caller is append_signoff().\n\nAh, I think I misunderstood you. \"just re-implement\" could be \"not\nto use the existing helper used by other callers and instead do it\nmanually all inside builtin/am.c\", and I agree that it would be the\nsafest way to go.\n"},{"id":"269496","messageId":"xmqqoahgdfec.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFw9BmBKEbibwVs+LuVQR2=PKt+hh5LAhdmVC-OkgBB3eg@mail.gmail.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T16:01:15Z","receivedAt":"2015-09-05T16:01:15Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus Torvalds writes:\n\n> That said, the original \"git am\" rules actually seem to be rather\n> straightforward: it's never an issue about \"last block of text\", and\n> it's simply an issue of \"is there a sign-ff _anywhere_ in the text\".\n>\n> That simplicity has a certain appeal to me. I don't think it was\n> necessarily written that way because it was \"well designed\" - I\n> suspect it is more an issue of \"easy to implement in a shell-script\".\n\nGuilty as pointed out.\n\n> Four out of 119 emails may not be a big percentage, but it does mean\n> that it's not horribly unusual either..\n\nSure. Thanks for these examples. I was aware that people do\nstrange things with the footer, but with the first example of [akpm]\ncomment at the very beginning alone, I wouldn't have guessed why\nintermixing one-liner comments directly in the chain of\nSigned-of-by: lines made any sense. Call it lack of imagination,\nbut each sign-off optionally prefixed by a single-liner summary of\nwhat change was done makes sense why people do want to use these\nlines that way.\n"},{"id":"269497","messageId":"xmqqk2s4deyo.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"20150905080325.GA25039@sigill.intra.peff.net","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T16:10:39Z","receivedAt":"2015-09-05T16:10:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> [1] I think part of the reason people are interested in \"fancy\" here is\n> that this topic extends beyond \"git am\". There's \"commit -s\", of\n> course, but there's also the generic \"interpret-trailers\" command,\n> which is supposed to be a generalization of the \"--signoff\" option.\n> It would be nice if the rules remained consistent for when we add a\n> trailer to an existing block, rather than special-casing signoffs.\n>\n> But again, I think that's something to shoot for in the long run.\n> It's more important in the short term not to regress \"am\".\n\nYes. I personally think the global append_signoff() everybody else\nuses can and should implement the same logic (whichever the exact\nlogic is) as whatever is used by \"am\" because the caller that makes\na call to that function knows it is adding a \"Signed-off-by:\" line,\nso existing \"Signed-off-by\" lines are already special in that context.\n\nBut for the purpose of 2.6-rc period, I think we should start from\ndoing a separate implementation inside builtin/am.c without touching\nappend_signoff().\n\nWe can do a more thoughtful refactoring for append_signoff() in the\nnext cycle. Another thing that needs consideration is the other\nuser of the has_conforming_footer() helper append_signoff() uses,\nwhich is the codepath to add \"cherry-picked-from\"; it is less\nobvious that \"find Signed-off-by: anywhere in the text to decide if\nthe new footer needs its own paragraph\" is a good logic in that\ncontext.\n\nTo salvage \"interpret-trailers\" needs a lot more, as we are\nrealizing that the definition that led to its external design does\nnot match the way users use footers in the real world. This affects\nthe internal data representation and the whole thing needs to be\nrethought.\n\nHere is a quick attempt to do the \"just fix am regression without\nchanging anything else\".\n\n\n builtin/am.c | 17 ++++++++++++++++-\n 1 file changed, 16 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 83b3d86..c63d238 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1213,9 +1213,24 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n static void am_append_signoff(struct am_state *state)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n+\tchar *cp;\n \n \tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n-\tappend_signoff(&sb, 0, 0);\n+\tstrbuf_complete_line(&sb);\n+\n+\tfor (cp = sb.buf;\n+\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n+\t cp = strchr(cp, '\\n')) {\n+\t\tif (sb.buf < cp && cp[-1] == '\\n')\n+\t\t\tbreak;\n+\t}\n+\tif (!cp)\n+\t\tstrbuf_addch(&sb, '\\n');\n+\tstrbuf_addf(&sb, \"%s%s\\n\",\n+\t\t sign_off_header,\n+\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n+\tstrbuf_addch(&sb, '\\n');\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n \n"},{"id":"269498","messageId":"xmqqbndgddy0.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"xmqqk2s4deyo.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T16:32:39Z","receivedAt":"2015-09-05T16:32:39Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> But for the purpose of 2.6-rc period, I think we should start from\n> doing a separate implementation inside builtin/am.c without touching\n> append_signoff().\n> ...\n> Here is a quick attempt to do the \"just fix am regression without\n> changing anything else\".\n\nAnd a quick attempt without even compilation testing has flaws as\nexpected X-<.\n\nSecond attempt.\n\n builtin/am.c | 25 +++++++++++++++++++++++--\n 1 file changed, 23 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 83b3d86..3a65d09 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1207,6 +1207,27 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n \texit(128);\n }\n \n+static void am_signoff(struct strbuf *sb)\n+{\n+\tchar *cp;\n+\n+\tstrbuf_complete_line(sb);\n+\n+\tfor (cp = sb->buf;\n+\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n+\t cp = strchr(cp, '\\n')) {\n+\t\tif (sb->buf == cp || cp[-1] == '\\n')\n+\t\t\tbreak;\n+\t}\n+\tif (!cp)\n+\t\tstrbuf_addch(sb, '\\n');\n+\tstrbuf_addf(sb, \"%s%s\\n\",\n+\t\t sign_off_header,\n+\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n+\tstrbuf_addch(sb, '\\n');\n+}\n+\n /**\n * Appends signoff to the \"msg\" field of the am_state.\n */\n@@ -1215,7 +1236,7 @@ static void am_append_signoff(struct am_state *state)\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n-\tappend_signoff(&sb, 0, 0);\n+\tam_signoff(&sb);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n \n@@ -1319,7 +1340,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstripspace(&msg, 0);\n \n \tif (state->signoff)\n-\t\tappend_signoff(&msg, 0, 0);\n+\t\tam_signoff(&msg);\n \n \tassert(!state->author_name);\n \tstate->author_name = strbuf_detach(&author_name, NULL);\n"},{"id":"269499","messageId":"xmqq7fo4dcsk.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"xmqqbndgddy0.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T16:57:31Z","receivedAt":"2015-09-05T16:57:31Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> And a quick attempt without even compilation testing has flaws as\n> expected X-<.\n>\n> Second attempt.\n\n... and I forget the de-dup logic. The third attempt.\n\n builtin/am.c | 32 ++++++++++++++++++++++++++++++--\n 1 file changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 83b3d86..30ffdde 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1207,6 +1207,34 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n \texit(128);\n }\n \n+static void am_signoff(struct strbuf *sb)\n+{\n+\tchar *cp;\n+\tstruct strbuf mine = STRBUF_INIT;\n+\n+\t/* Does it end with our own sign-off? */\n+\tstrbuf_addf(&mine, \"%s%s\\n\",\n+\t\t sign_off_header,\n+\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n+\tif (mine.len < sb->len &&\n+\t !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n+\t\tgoto exit; /* no need to duplicate */\n+\n+\t/* Does it have any Signed-off-by: in the text */\n+\tfor (cp = sb->buf;\n+\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n+\t cp = strchr(cp, '\\n')) {\n+\t\tif (sb->buf == cp || cp[-1] == '\\n')\n+\t\t\tbreak;\n+\t}\n+\tif (!cp)\n+\t\tstrbuf_addch(sb, '\\n');\n+\tstrbuf_addbuf(sb, &mine);\n+exit:\n+\tstrbuf_release(&mine);\n+}\n+\n /**\n * Appends signoff to the \"msg\" field of the am_state.\n */\n@@ -1215,7 +1243,7 @@ static void am_append_signoff(struct am_state *state)\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n-\tappend_signoff(&sb, 0, 0);\n+\tam_signoff(&sb);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n \n@@ -1319,7 +1347,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstripspace(&msg, 0);\n \n \tif (state->signoff)\n-\t\tappend_signoff(&msg, 0, 0);\n+\t\tam_signoff(&msg);\n \n \tassert(!state->author_name);\n \tstate->author_name = strbuf_detach(&author_name, NULL);\n"},{"id":"269518","messageId":"xmqqsi6sbqqm.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"xmqqk2s4deyo.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-05T19:39:13Z","receivedAt":"2015-09-05T19:39:13Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> To salvage \"interpret-trailers\" needs a lot more, as we are\n> realizing that the definition that led to its external design does\n> not match the way users use footers in the real world. This affects\n> the internal data representation and the whole thing needs to be\n> rethought.\n\nNote that I am not saying that you personally did any bad job while\nworking on the interpret-trailers topic. We collectively designed\nits feature based on a much narrower definition of what the trailer\nblock is than what is used in the real world in practice, so we do\nnot have a good way to locate an existing entry that is not a (key,\nvalue), no matter what the syntax to denote which is key and which\nis value is, insert a new entry that is not a (key, value), or\nremove an existing entry that is not a (key, value), all of which\nwill be necessary to mutate trailer blocks people use in the real\nlife.\n\nI think a good way forward would be to go like this:\n\n * a helper function that starts from a flat (or a\n strbuf) and identifies the end of the body of the message,\n i.e. find \"^---$\", \"^Conflicts:$\", etc. and skip blank lines\n backwards. That is what ignore_non_trailer() in commit.c does,\n and that can be shared across everybody that mutates a log\n message.\n\n * a helper function that takes the result of the above as a flat\n (or a strbuf) and identifies the beginning of a\n trailer block. That may be just the matter of scanning backwards\n from the end of the trailer block ignore_non_trailer() identified\n for the first blank line, as I agree with Linus's \"So quite\n frankly, at that point if git doesn't recognize it as a sign-off\n block, I don't think it's a big deal\" in the thread.\n\n Not having that and not calling that function can reintroduce the\n recent \"interpret-trailers corner case\" bug Matthieu brought up.\n\nWith these, everybody except interpret-trailers that mutates a log\nmessage can add a new signoff consistently. And then, building on\nthese, \"interpret-trailers\" can be written like this:\n\n (1) starting from a flat (or a strbuf), using the above\n helpers, identify the parts of the log message that is the\n trailer block (and you will know what is before and what is\n after the trailer block).\n\n (2) keep the part before the trailer block and the part after the\n trailer block (this could be empty) in one strbuf each; we do\n not want to mutate these parts, and it is pointless to split\n them further into individual lines.\n\n (3) express the lines in the trailer block in a richer data\n structure that is easier to manipulate (i.e. reorder the lines,\n insert, delete, etc.) and work on it.\n\n (4) when manipulation of the trailer block is finished, reconstruct\n the resulting message by concatenating the \"before trailer\"\n part, \"trailer\" part, and \"after trailer\" part.\n\nAs to the exact design of \"a richer data structure\" and the\nmanipulation we may want on the trailer, I currently do not have a\nstrong \"it should be this way\" opinion yet, but after looking at\nvarious examples Linus gave us in the discussion, my gut feelig is\nthat it would be best to keep the operation simple and generic,\ne.g. \"find a line that matches this regexp and replace it with this\nline\", \"insert this line at the end\", \"delete all lines that match\nthis regexp\", etc.\n"},{"id":"269524","messageId":"xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CA+55aFzN4SnenchxPScn61_apzitGAPtoYEd49iLZPxgK0KQGw@mail.gmail.com","subject":"[PATCH] am: match --signoff to the original scripted version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-06T04:56:27Z","receivedAt":"2015-09-06T04:56:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Linus noticed that the recently reimplementated \"git am -s\" defines\nthe trailer block too rigidly, resulting an unnecessary blank line\nbetween the existing sign-offs and his new sign-off. An e-mail\nsubmission sent to Linus in real life ends with mixture of sign-offs\nand commentaries, e.g.\n\n\ttitle here\n\n\tmessage here\n\n\tSigned-off-by: Original Author \n\t[rv: tweaked frotz and nitfol]\n\tSigned-off-by: Re Viewer \n\tSigned-off-by: Other Reviewer \n\t---\n\tpatch here\n\nBecause the reimplementation reused append_signoff() helper that is\nused by other codepaths, which is unaware that people intermix such\ncomments with their sign-offs in the trailer block, such a message\nwas judged to end with a non-trailer, resulting in an extra blank\nbefore adding a new sign-off.\n\nThe original scripted version of \"git am\" used a lot looser\ndefinition, i.e. \"if and only if there is no line that begins with\nSigned-off-by:, add a blank line before adding a new sign-off\". For\nthe upcoming release, stop using the append_signoff() in \"git am\"\nand reimplement the looser definition used by the scripted version\nto use only in \"git am\" to fix this regression in \"am\" while\navoiding new regressions to other users of append_signoff().\n\nIn the longer term, we should look into loosening append_signoff()\nso that other codepaths that add a new sign-off behave the same way\nas \"git am -s\", but that is a task for post-release.\n\nReported-by: Linus Torvalds \nSigned-off-by: Junio C Hamano \n---\n\n * So this is essentially that \"third try\", but with a bit of test\n to cast in stone that we expect things that are not \"key: value\"\n in the trailer block. Let's have this (or a fix along this line)\n as a regression fix for the upcoming release.\n\n builtin/am.c | 31 +++++++++++++++++++++++++++++--\n t/t4150-am.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 77 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex 634f7a7..e7828e5 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1191,6 +1191,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n \texit(128);\n }\n \n+static void am_signoff(struct strbuf *sb)\n+{\n+\tchar *cp;\n+\tstruct strbuf mine = STRBUF_INIT;\n+\n+\t/* Does it end with our own sign-off? */\n+\tstrbuf_addf(&mine, \"\\n%s%s\\n\",\n+\t\t sign_off_header,\n+\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n+\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n+\tif (mine.len < sb->len &&\n+\t !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n+\t\tgoto exit; /* no need to duplicate */\n+\n+\t/* Does it have any Signed-off-by: in the text */\n+\tfor (cp = sb->buf;\n+\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n+\t cp = strchr(cp, '\\n')) {\n+\t\tif (sb->buf == cp || cp[-1] == '\\n')\n+\t\t\tbreak;\n+\t}\n+\n+\tstrbuf_addstr(sb, mine.buf + !!cp);\n+exit:\n+\tstrbuf_release(&mine);\n+}\n+\n /**\n * Appends signoff to the \"msg\" field of the am_state.\n */\n@@ -1199,7 +1226,7 @@ static void am_append_signoff(struct am_state *state)\n \tstruct strbuf sb = STRBUF_INIT;\n \n \tstrbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n-\tappend_signoff(&sb, 0, 0);\n+\tam_signoff(&sb);\n \tstate->msg = strbuf_detach(&sb, &state->msg_len);\n }\n \n@@ -1303,7 +1330,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n \tstripspace(&msg, 0);\n \n \tif (state->signoff)\n-\t\tappend_signoff(&msg, 0, 0);\n+\t\tam_signoff(&msg);\n \n \tassert(!state->author_name);\n \tstate->author_name = strbuf_detach(&author_name, NULL);\ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex dd627c4..5e48555 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -873,4 +873,52 @@ test_expect_success 'am --message-id -s signs off after the message id' '\n \ttest_cmp expected actual\n '\n \n+test_expect_success 'am -s unexpected trailer block' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\techo signed >file &&\n+\tgit add file &&\n+\tcat >msg <<-EOF &&\n+\tsubject here\n+\n+\tSigned-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\n+\t[jc: tweaked log message]\n+\tSigned-off-by: J C H \n+\tEOF\n+\tgit commit -F msg &&\n+\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit format-patch --stdout -1 >patch &&\n+\n+\tgit reset --hard HEAD^ &&\n+\tgit am -s patch &&\n+\t(\n+\t\tcat original &&\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expect &&\n+\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\ttest_cmp expect actual &&\n+\n+\tcat >msg <<-\\EOF &&\n+\tsubject here\n+\n+\tWe make sure that there is a blank line between the log\n+\tmessage proper and Signed-off-by: line added.\n+\tEOF\n+\tgit reset HEAD^ &&\n+\tgit commit -F msg file &&\n+\tgit cat-file commit HEAD | sed -e '1,/^$/d' >original &&\n+\tgit format-patch --stdout -1 >patch &&\n+\n+\tgit reset --hard HEAD^ &&\n+\tgit am -s patch &&\n+\n+\t(\n+\t\tcat original &&\n+\t\techo &&\n+\t\techo \"Signed-off-by: $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL>\"\n+\t) >expect &&\n+\tgit cat-file commit HEAD | sed -e '1,/^$/d' >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.6.0-rc0-130-gfaaaedc\n"},{"id":"269527","messageId":"CACRoPnTUzeF=RUVv_0+2Ej422Vh-NH+KpYaKH0VKNmGbuVQ_8g@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-09-06T09:04:13Z","receivedAt":"2015-09-06T09:04:13Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"Hi,\n\nThanks for handling this.\n\nOn Sun, Sep 6, 2015 at 12:56 PM, Junio C Hamano wrote:\n> Linus noticed that the recently reimplementated \"git am -s\" defines\n\ns/reimplementated/reimplemented/ ?\n\n> the trailer block too rigidly, resulting an unnecessary blank line\n\ns/resulting an/resulting in an/ ?\n\n> between the existing sign-offs and his new sign-off. An e-mail\n> submission sent to Linus in real life ends with mixture of sign-offs\n> and commentaries, e.g.\n>\n> title here\n>\n> message here\n>\n> Signed-off-by: Original Author \n> [rv: tweaked frotz and nitfol]\n> Signed-off-by: Re Viewer \n> Signed-off-by: Other Reviewer \n> ---\n> patch here\n>\n> Because the reimplementation reused append_signoff() helper that is\n> used by other codepaths, which is unaware that people intermix such\n> comments with their sign-offs in the trailer block, such a message\n> was judged to end with a non-trailer, resulting in an extra blank\n\ns/extra blank/extra blank line/ ?\n\n> before adding a new sign-off.\n>\n> The original scripted version of \"git am\" used a lot looser\n> definition, i.e. \"if and only if there is no line that begins with\n> Signed-off-by:, add a blank line before adding a new sign-off\". For\n> the upcoming release, stop using the append_signoff() in \"git am\"\n> and reimplement the looser definition used by the scripted version\n> to use only in \"git am\" to fix this regression in \"am\" while\n> avoiding new regressions to other users of append_signoff().\n>\n> In the longer term, we should look into loosening append_signoff()\n> so that other codepaths that add a new sign-off behave the same way\n> as \"git am -s\", but that is a task for post-release.\n>\n> Reported-by: Linus Torvalds \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/am.c | 31 +++++++++++++++++++++++++++++--\n> t/t4150-am.sh | 48 ++++++++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 77 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 634f7a7..e7828e5 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1191,6 +1191,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n> exit(128);\n> }\n>\n> +static void am_signoff(struct strbuf *sb)\n> +{\n\nHmm, okay, but now we have two similarly named functions am_signoff()\nand am_append_signoff() which both do nearly similar things, the only\ndifference being am_signoff() operates on a strbuf while\nam_append_signoff() operates on the \"msg\" char* field in the am_state,\nwhich seems a bit iffy to me. I wonder if the logic could be\nimplemented in am_append_signoff() instead so we have only one\nfunction?\n\n> + char *cp;\n> + struct strbuf mine = STRBUF_INIT;\n> +\n> + /* Does it end with our own sign-off? */\n> + strbuf_addf(&mine, \"\\n%s%s\\n\",\n> + sign_off_header,\n> + fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n> + getenv(\"GIT_COMMITTER_EMAIL\")));\n\nMaybe use git_committer_info() here?\n\n> + if (mine.len < sb->len &&\n> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n\nPerhaps use ends_with()?\n\n> + goto exit; /* no need to duplicate */\n> +\n> + /* Does it have any Signed-off-by: in the text */\n> + for (cp = sb->buf;\n> + cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n> + cp = strchr(cp, '\\n')) {\n> + if (sb->buf == cp || cp[-1] == '\\n')\n> + break;\n> + }\n> +\n> + strbuf_addstr(sb, mine.buf + !!cp);\n> +exit:\n> + strbuf_release(&mine);\n> +}\n> +\n> /**\n> * Appends signoff to the \"msg\" field of the am_state.\n> */\n> @@ -1199,7 +1226,7 @@ static void am_append_signoff(struct am_state *state)\n> struct strbuf sb = STRBUF_INIT;\n>\n> strbuf_attach(&sb, state->msg, state->msg_len, state->msg_len);\n> - append_signoff(&sb, 0, 0);\n> + am_signoff(&sb);\n> state->msg = strbuf_detach(&sb, &state->msg_len);\n> }\n>\n> @@ -1303,7 +1330,7 @@ static int parse_mail(struct am_state *state, const char *mail)\n> stripspace(&msg, 0);\n>\n> if (state->signoff)\n> - append_signoff(&msg, 0, 0);\n> + am_signoff(&msg);\n>\n> assert(!state->author_name);\n> state->author_name = strbuf_detach(&author_name, NULL);\n\nThanks again,\nPaul\n"},{"id":"269534","messageId":"CACRoPnQvNowZGbqvRLuhS8cC1EaTQX3vsyaESSQVXpuRToWLOQ@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Paul Tan","fromEmail":"pyokagan@gmail.com","sentAt":"2015-09-06T14:21:09Z","receivedAt":"2015-09-06T14:21:09Z","isPatch":true,"sender":{"key":"pyokagan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/109479?v=4"},"body":"On Sun, Sep 6, 2015 at 12:56 PM, Junio C Hamano wrote:\n> diff --git a/builtin/am.c b/builtin/am.c\n> index 634f7a7..e7828e5 100644\n> --- a/builtin/am.c\n> +++ b/builtin/am.c\n> @@ -1191,6 +1191,33 @@ static void NORETURN die_user_resolve(const struct am_state *state)\n> exit(128);\n> }\n>\n> +static void am_signoff(struct strbuf *sb)\n> +{\n> + char *cp;\n> + struct strbuf mine = STRBUF_INIT;\n> +\n> + /* Does it end with our own sign-off? */\n> + strbuf_addf(&mine, \"\\n%s%s\\n\",\n> + sign_off_header,\n> + fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n> + getenv(\"GIT_COMMITTER_EMAIL\")));\n> + if (mine.len < sb->len &&\n> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n> + goto exit; /* no need to duplicate */\n> +\n> + /* Does it have any Signed-off-by: in the text */\n> + for (cp = sb->buf;\n> + cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n> + cp = strchr(cp, '\\n')) {\n> + if (sb->buf == cp || cp[-1] == '\\n')\n> + break;\n> + }\n> +\n> + strbuf_addstr(sb, mine.buf + !!cp);\n\nTo add on, I wonder if the above \"add a blank line if there is no\nSigned-off-by: at the beginning of a line\" logic could be expressed\nmore succinctly like this:\n\n if (!starts_with(sb->buf, \"Signed-off-by: \") &&\n !strstr(sb->buf, \"\\nSigned-off-by: \"))\n strbuf_addch(sb, '\\n');\n\n strbuf_addstr(sb, mine.buf + 1);\n\n> +exit:\n> + strbuf_release(&mine);\n> +}\n> +\n> /**\n> * Appends signoff to the \"msg\" field of the am_state.\n> */\n\nRegards,\nPaul\n"},{"id":"269535","messageId":"CA+55aFx+rwGwHr-pdo=yjgzysfJ_CVir7=dTV0GTjeboCAwjrw@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Linus Torvalds","fromEmail":"torvalds@linux-foundation.org","sentAt":"2015-09-06T16:16:27Z","receivedAt":"2015-09-06T16:16:27Z","isPatch":true,"sender":{"key":"torvalds@linux-foundation.org","avatar":"https://avatars.githubusercontent.com/u/1024025?v=4"},"body":"On Sat, Sep 5, 2015 at 9:56 PM, Junio C Hamano wrote:\n>\n> For\n> the upcoming release, stop using the append_signoff() in \"git am\"\n> and reimplement the looser definition used by the scripted version\n> to use only in \"git am\" to fix this regression in \"am\" while\n> avoiding new regressions to other users of append_signoff().\n\nI've tested this by re-doing the same patch-bomb from Andrew that I\nhad problems with originally, and it WorksForMe(tm).\n\nThanks,\n\n Linus\n"},{"id":"269538","messageId":"xmqqd1xvbgw3.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CACRoPnTUzeF=RUVv_0+2Ej422Vh-NH+KpYaKH0VKNmGbuVQ_8g@mail.gmail.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-06T17:24:12Z","receivedAt":"2015-09-06T17:24:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n> s/reimplementated/reimplemented/ ?\n> s/resulting an/resulting in an/ ?\n> s/extra blank/extra blank line/ ?\n\nThanks.\n\n>> +static void am_signoff(struct strbuf *sb)\n>> +{\n>\n> Hmm, okay, but now we have two similarly named functions am_signoff()\n> and am_append_signoff() which both do nearly similar things, the only\n> difference being am_signoff() operates on a strbuf while\n> am_append_signoff() operates on the \"msg\" char* field in the am_state,\n> which seems a bit iffy to me.\n\nI do recall advising you not to overuse strbuf in am_state, and\nspecifically not to use strbuf for a field like author_name, and the\ncriteria to tell why a field should not be a strbuf in am_state is\nif the code used its strbuf-ness only because it is initially read\nwith strbuf_read_file(), but afterwards it is necessary to use the\nfield as a strbuf because the field is not modified later and is not\npassed to a helper function that takes a strbuf.\n\nI think the final code ended up following that piece of advice a bit\ntoo aggressively. The pair in am_state did need to\nbe modified with sign-off, and it was done by passing it as a strbuf\nto append_signoff() in the code we are fixing here; keeping msg\nfield that you wrote as a strbuf in the original would have made\nam_append_signoff() unnecessary.\n\nBut this patch you are commenting on is purely for regression fix,\nand I didn't want to change other things like data representation at\nthe same time.\n\n>> + /* Does it end with our own sign-off? */\n>> + strbuf_addf(&mine, \"\\n%s%s\\n\",\n>> + sign_off_header,\n>> + fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n>> + getenv(\"GIT_COMMITTER_EMAIL\")));\n>\n> Maybe use git_committer_info() here?\n\nPerhaps, but I wanted to make sure I am doing the same thing as the\ncodepath of sequencer.c::append_signoff(), which the original ended\nup calling. git_committer_info() does way more than that, no?\n\n>> + if (mine.len < sb->len &&\n>> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n>\n> Perhaps use ends_with()?\n\nThis caller already _knows_ how long the sb->buf string is; it is\npointless to have ends_with() run strlen() on it.\n"},{"id":"269539","messageId":"xmqq8u8jbg79.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"CACRoPnQvNowZGbqvRLuhS8cC1EaTQX3vsyaESSQVXpuRToWLOQ@mail.gmail.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-06T17:39:06Z","receivedAt":"2015-09-06T17:39:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Paul Tan writes:\n\n>> + /* Does it have any Signed-off-by: in the text */\n>> + for (cp = sb->buf;\n>> + cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n>> + cp = strchr(cp, '\\n')) {\n>> + if (sb->buf == cp || cp[-1] == '\\n')\n>> + break;\n>> + }\n>> +\n>> + strbuf_addstr(sb, mine.buf + !!cp);\n>\n> To add on, I wonder if the above \"add a blank line if there is no\n> Signed-off-by: at the beginning of a line\" logic could be expressed\n> more succinctly like this:\n>\n> if (!starts_with(sb->buf, \"Signed-off-by: \") &&\n> !strstr(sb->buf, \"\\nSigned-off-by: \"))\n> strbuf_addch(sb, '\\n');\n>\n> strbuf_addstr(sb, mine.buf + 1);\n\nThat may be more obvious, but I wanted to reuse the existing\nsign_off_header[]; there is no variant that has a leading \"end of\nprevious line\".\n\nI actually think it is not an uncommon thing to ask \"Give me the\nfirst occurrence of the string NEEDLE that appears at the beginning\nof lines in BUF\", so the right way to address the \"is it more\nreadable\" question would probably be to have a helper function to do\njust that, and use it here and other places that answer that\nquestion by themselves due to lack of such a helper.\n"},{"id":"269579","messageId":"CAP8UFD1AoSsP303NEMLf39o-RHEwRMt-GvUv-87kCGtpA56Piw@mail.gmail.com","threadId":"40278","inReplyTo":"xmqqsi6sbqqm.fsf@gitster.mtv.corp.google.com","subject":"Re: More builtin git-am issues..","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2015-09-07T19:27:09Z","receivedAt":"2015-09-07T19:27:09Z","isPatch":false,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Sat, Sep 5, 2015 at 9:39 PM, Junio C Hamano wrote:\n> Junio C Hamano writes:\n>\n>> To salvage \"interpret-trailers\" needs a lot more, as we are\n>> realizing that the definition that led to its external design does\n>> not match the way users use footers in the real world. This affects\n>> the internal data representation and the whole thing needs to be\n>> rethought.\n>\n> Note that I am not saying that you personally did any bad job while\n> working on the interpret-trailers topic. We collectively designed\n> its feature based on a much narrower definition of what the trailer\n> block is than what is used in the real world in practice, so we do\n> not have a good way to locate an existing entry that is not a (key,\n> value), no matter what the syntax to denote which is key and which\n> is value is, insert a new entry that is not a (key, value), or\n> remove an existing entry that is not a (key, value), all of which\n> will be necessary to mutate trailer blocks people use in the real\n> life.\n>\n> I think a good way forward would be to go like this:\n>\n> * a helper function that starts from a flat (or a\n> strbuf) and identifies the end of the body of the message,\n> i.e. find \"^---$\", \"^Conflicts:$\", etc. and skip blank lines\n> backwards. That is what ignore_non_trailer() in commit.c does,\n> and that can be shared across everybody that mutates a log\n> message.\n>\n> * a helper function that takes the result of the above as a flat\n> (or a strbuf) and identifies the beginning of a\n> trailer block. That may be just the matter of scanning backwards\n> from the end of the trailer block ignore_non_trailer() identified\n> for the first blank line, as I agree with Linus's \"So quite\n> frankly, at that point if git doesn't recognize it as a sign-off\n> block, I don't think it's a big deal\" in the thread.\n>\n> Not having that and not calling that function can reintroduce the\n> recent \"interpret-trailers corner case\" bug Matthieu brought up.\n>\n> With these, everybody except interpret-trailers that mutates a log\n> message can add a new signoff consistently. And then, building on\n> these, \"interpret-trailers\" can be written like this:\n>\n> (1) starting from a flat (or a strbuf), using the above\n> helpers, identify the parts of the log message that is the\n> trailer block (and you will know what is before and what is\n> after the trailer block).\n>\n> (2) keep the part before the trailer block and the part after the\n> trailer block (this could be empty) in one strbuf each; we do\n> not want to mutate these parts, and it is pointless to split\n> them further into individual lines.\n>\n> (3) express the lines in the trailer block in a richer data\n> structure that is easier to manipulate (i.e. reorder the lines,\n> insert, delete, etc.) and work on it.\n>\n> (4) when manipulation of the trailer block is finished, reconstruct\n> the resulting message by concatenating the \"before trailer\"\n> part, \"trailer\" part, and \"after trailer\" part.\n>\n> As to the exact design of \"a richer data structure\" and the\n> manipulation we may want on the trailer, I currently do not have a\n> strong \"it should be this way\" opinion yet, but after looking at\n> various examples Linus gave us in the discussion, my gut feelig is\n> that it would be best to keep the operation simple and generic,\n> e.g. \"find a line that matches this regexp and replace it with this\n> line\", \"insert this line at the end\", \"delete all lines that match\n> this regexp\", etc.\n\nI will see what I can do about this.\n\nThanks,\nChristian.\n"},{"id":"269594","messageId":"20150908061836.GD26331@sigill.intra.peff.net","threadId":"40278","inReplyTo":"xmqqd1xvbgw3.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T06:18:37Z","receivedAt":"2015-09-08T06:18:37Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Sep 06, 2015 at 10:24:12AM -0700, Junio C Hamano wrote:\n\n> >> + /* Does it end with our own sign-off? */\n> >> + strbuf_addf(&mine, \"\\n%s%s\\n\",\n> >> + sign_off_header,\n> >> + fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n> >> + getenv(\"GIT_COMMITTER_EMAIL\")));\n> >\n> > Maybe use git_committer_info() here?\n> \n> Perhaps, but I wanted to make sure I am doing the same thing as the\n> codepath of sequencer.c::append_signoff(), which the original ended\n> up calling. git_committer_info() does way more than that, no?\n\nNot really. I think git_committer_info(IDENT_STRICT | IDENT_NO_DATE)\nruns the exact same code, with one exception: we would also set the\nident_explicitly_given variables. But nobody in builtin/am.c calls\ncommitter_ident_sufficiently_given(). And if they did, I think the\nchange would be an improvement.\n\n> >> + if (mine.len < sb->len &&\n> >> + !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n> >\n> > Perhaps use ends_with()?\n> \n> This caller already _knows_ how long the sb->buf string is; it is\n> pointless to have ends_with() run strlen() on it.\n\nThat actually goes double. We know sb.len. The ends_with() function is\nbuilt around strip_suffix(), which both operate on strings. But we do\nnot have ends_with_mem() built around strip_suffix_mem().\n\nBut we also know mine.len. Even strip_suffix_mem() assumes the suffix\nitself is a string. So what you really want is:\n\n int strip_suffix_mem_mem(const char *buf, size_t *len,\n const char *suffix, size_t suffix_len);\n\nand then you can trivially build the existing strip_suffix_mem() around\nit, build strip_suffix() around that, and then build ends_with(),\nends_with_mem() and ends_with_mem_mem() around those. And don't forget\nstrbuf_ends_with(), strbuf_ends_with_mem(), and strbuf_ends_with_strbuf()\n\n:) I am only half tongue in cheek. The proliferation of names is tedious\n(and not appropriate for an -rc regression fix), but I do think the\nresulting code is a lot more obvious as:\n\n if (strbuf_ends_with_strbuf(&sb, &mine))\n ...\n\nor even:\n\n if (ends_with_mem_mem(sb->buf, sb->len, mine.buf, mine.len))\n ...\n\nOf course given that this is run only once per program, and that these\n_are_ in fact strings, we can probably not bother to optimize it and\njust accept:\n\n if (ends_with(sb->buf, mine.buf))\n ...\n\nBut if you want to go all-out on optimization, I think you can replace\nyour strcmp with memcmp:\n\n if (mine.len < sb->len &&\n !memcmp(mine.buf, sb->buf + sb->len - mine.len, mine.len))\n\n(assuming that memcmp is in fact faster than strcmp).\n\n-Peff\n"},{"id":"269595","messageId":"20150908062523.GE26331@sigill.intra.peff.net","threadId":"40278","inReplyTo":"xmqqio7ob0xw.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-09-08T06:25:23Z","receivedAt":"2015-09-08T06:25:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 05, 2015 at 09:56:27PM -0700, Junio C Hamano wrote:\n\n> +static void am_signoff(struct strbuf *sb)\n> +{\n> +\tchar *cp;\n> +\tstruct strbuf mine = STRBUF_INIT;\n> +\n> +\t/* Does it end with our own sign-off? */\n> +\tstrbuf_addf(&mine, \"\\n%s%s\\n\",\n> +\t\t sign_off_header,\n> +\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n> +\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n> +\tif (mine.len < sb->len &&\n> +\t !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n> +\t\tgoto exit; /* no need to duplicate */\n\nHere you insert the \"\\n\" directly at the start of \"mine\", so the test\n\"does it contain S-o-b at the beginning of a line\" does not count the\nfirst line. Probably fine, as somebody putting a S-o-b in their subject\ndeserves whatever they get.\n\nBut...\n\n> +\t/* Does it have any Signed-off-by: in the text */\n> +\tfor (cp = sb->buf;\n> +\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n> +\t cp = strchr(cp, '\\n')) {\n> +\t\tif (sb->buf == cp || cp[-1] == '\\n')\n> +\t\t\tbreak;\n> +\t}\n\nHere you are more careful about finding S-o-b at sb->buf.\n\nI don't think it really matters in practice, but it's an interesting\ninconsistency.\n\nOther than that (and I do not think it is worth re-rolling for this;\njust an interesting observation), the patch looks OK to me.\n\n-Peff\n"},{"id":"269614","messageId":"xmqqlhcg93t3.fsf@gitster.mtv.corp.google.com","threadId":"40278","inReplyTo":"20150908062523.GE26331@sigill.intra.peff.net","subject":"Re: [PATCH] am: match --signoff to the original scripted version","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-09-08T18:14:16Z","receivedAt":"2015-09-08T18:14:16Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Sat, Sep 05, 2015 at 09:56:27PM -0700, Junio C Hamano wrote:\n>\n>> +static void am_signoff(struct strbuf *sb)\n>> +{\n>> +\tchar *cp;\n>> +\tstruct strbuf mine = STRBUF_INIT;\n>> +\n>> +\t/* Does it end with our own sign-off? */\n>> +\tstrbuf_addf(&mine, \"\\n%s%s\\n\",\n>> +\t\t sign_off_header,\n>> +\t\t fmt_name(getenv(\"GIT_COMMITTER_NAME\"),\n>> +\t\t\t getenv(\"GIT_COMMITTER_EMAIL\")));\n>> +\tif (mine.len < sb->len &&\n>> +\t !strcmp(mine.buf, sb->buf + sb->len - mine.len))\n>> +\t\tgoto exit; /* no need to duplicate */\n>\n> Here you insert the \"\\n\" directly at the start of \"mine\", so the test\n> \"does it contain S-o-b at the beginning of a line\" does not count the\n> first line. Probably fine, as somebody putting a S-o-b in their subject\n> deserves whatever they get.\n\nYup.\n\n>> +\t/* Does it have any Signed-off-by: in the text */\n>> +\tfor (cp = sb->buf;\n>> +\t cp && *cp && (cp = strstr(cp, sign_off_header)) != NULL;\n>> +\t cp = strchr(cp, '\\n')) {\n>> +\t\tif (sb->buf == cp || cp[-1] == '\\n')\n>> +\t\t\tbreak;\n>> +\t}\n>\n> Here you are more careful about finding S-o-b at sb->buf.\n\nIt is not being careful for that, actually. It just is avoiding to\naccess sb->buf[-1], which would be a realproblem. \"The beginning of\nbuffer is also at the beginning of a line\" is merely a happy side\neffect ;-).\n"}]}