{"thread":{"id":"50850","subject":"[PATCH] mailinfo: support Unicode scissors","startedAt":"2019-03-31T22:01:10Z","lastAt":"2021-06-09T05:11:36Z","messageCount":16,"participants":["Andrei Rybak","SZEDER Gábor","Junio C Hamano","Duy Nguyen","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"372846","messageId":"20190331220104.31628-1-rybak.a.v@gmail.com","threadId":"50850","inReplyTo":null,"subject":"[PATCH] mailinfo: support Unicode scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-03-31T22:01:04Z","receivedAt":"2019-03-31T22:01:10Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"'git am --scissors' allows cutting a patch from an email at a scissors\nline. Such a line should contain perforation, i.e. hyphens, and a\nscissors symbol. Only ASCII graphics scissors '8<' '>8' '%<' '>%' are\nrecognized by 'git am --scissors' command at the moment.\n\nUnicode character 'BLACK SCISSORS' (U+2702) has been a part of Unicode\nsince version 1.0.0 [1]. Since then 'BLACK SCISSORS' also became part\nof character set Emoji 1.0, published in 2015 [2]. With its adoption as\nan emoji, availability of this character on keyboards has increased.\n\nSupport UTF-8 encoding of '✂' in function is_scissors_line, for 'git am\n--scissors' to be able to cut at Unicode perforation lines in emails.\nNote, that Unicode character '✂' is three bytes in UTF-8 encoding.\n\n1. https://www.unicode.org/versions/Unicode1.0.0/CodeCharts1.pdf\n https://www.unicode.org/Public/reconstructed/1.0.0/UnicodeData.txt\n2. https://unicode.org/Public/emoji/1.0/emoji-data.txt\n\nSigned-off-by: Andrei Rybak \n---\n\nThis applies on top of ar/t4150-remove-cruft merged into next in \ncommit a0106a8d5c, which also edited the test setup in t4150.\n\n mailinfo.c | 7 +++++++\n t/t4150-am.sh | 26 +++++++++++++++++++++++++-\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..4ef6cdee85 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n \t\t\tc++;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (!memcmp(c, \"✂\", 3)) {\n+\t\t\tin_perforation = 1;\n+\t\t\tperforation += 3;\n+\t\t\tscissors += 3;\n+\t\t\tc++;\n+\t\t\tcontinue;\n+\t\t}\n \t\tin_perforation = 0;\n \t}\n \ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 3f7f750cc8..3ea8e8a2cf 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -77,12 +77,20 @@ test_expect_success 'setup: messages' '\n \n \tprintf \"Subject: \" >subject-prefix &&\n \n-\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n \n \tEOF\n+\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-unicode-scissors <<-\\EOF\n+\tLines above unicode scissors line should not be included in the commit\n+\tmessage with --scissors enabled.\n+\n+\t- - - ✂ - - - ✂ - - -\n+\n+\tEOF\n '\n \n test_expect_success setup '\n@@ -161,6 +169,12 @@ test_expect_success setup '\n \tgit format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-with-unicode-scissors &&\n+\tgit format-patch --stdout HEAD^ >patch-with-unicode-scissors.eml &&\n+\tgit reset --hard HEAD^ &&\n+\n \tsed -n -e \"3,\\$p\" msg >file &&\n \tgit add file &&\n \ttest_tick &&\n@@ -421,6 +435,16 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \ttest_cmp_rev expected-for-scissors HEAD\n '\n \n+test_expect_success 'am --scissors cuts the message at the unicode scissors line' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout second &&\n+\tgit am --scissors patch-with-unicode-scissors.eml &&\n+\ttest_path_is_missing .git/rebase-apply &&\n+\tgit diff --exit-code expected-for-scissors &&\n+\ttest_cmp_rev expected-for-scissors HEAD\n+'\n+\n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n-- \n2.21.0\n\n"},{"id":"372849","messageId":"20190331230947.GI32732@szeder.dev","threadId":"50850","inReplyTo":"20190331220104.31628-1-rybak.a.v@gmail.com","subject":"Re: [PATCH] mailinfo: support Unicode scissors","fromName":"SZEDER Gábor","fromEmail":"szeder.dev@gmail.com","sentAt":"2019-03-31T23:09:47Z","receivedAt":"2019-03-31T23:09:52Z","isPatch":true,"sender":{"key":"szeder.dev@gmail.com","avatar":"https://avatars.githubusercontent.com/u/116324?v=4"},"body":"On Mon, Apr 01, 2019 at 12:01:04AM +0200, Andrei Rybak wrote:\n> diff --git a/mailinfo.c b/mailinfo.c\n> index b395adbdf2..4ef6cdee85 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n> \t\t\tc++;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (!memcmp(c, \"✂\", 3)) {\n\nThis character is tiny. Please add a comment that it's supposed to be\na Unicode scissors character.\n\nShould we worry about this memcmp() potentially reading past the end\nof the string when 'c' points to the last character?\n\n> +\t\t\tin_perforation = 1;\n> +\t\t\tperforation += 3;\n> +\t\t\tscissors += 3;\n> +\t\t\tc++;\n\nHere you should jump past the three byte long Unicode character, so\nthis should be c += 2.\n\n> +\t\t\tcontinue;\n> +\t\t}\n> \t\tin_perforation = 0;\n> \t}\n> \n"},{"id":"372861","messageId":"xmqqimvyxfed.fsf@gitster-ct.c.googlers.com","threadId":"50850","inReplyTo":"20190331230947.GI32732@szeder.dev","subject":"Re: [PATCH] mailinfo: support Unicode scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-01T09:07:22Z","receivedAt":"2019-04-01T09:07:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"SZEDER Gábor writes:\n\n> On Mon, Apr 01, 2019 at 12:01:04AM +0200, Andrei Rybak wrote:\n>> diff --git a/mailinfo.c b/mailinfo.c\n>> index b395adbdf2..4ef6cdee85 100644\n>> --- a/mailinfo.c\n>> +++ b/mailinfo.c\n>> @@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n>> \t\t\tc++;\n>> \t\t\tcontinue;\n>> \t\t}\n>> +\t\tif (!memcmp(c, \"✂\", 3)) {\n>\n> This character is tiny. Please add a comment that it's supposed to be\n> a Unicode scissors character.\n>\n> Should we worry about this memcmp() potentially reading past the end\n> of the string when 'c' points to the last character?\n\nQuite honestly, I'd rather document what \"scissors\" line looks like\nexactly and make sure no readers would mistake that we'd accept any\nUnicode character whose name has substring \"scissors\" in it.\n\nAh, wait, we already do. It is very clear that scissors are either\n\">8\" (for right handers) or \"8<\" (for lefties) and nothing else.\n\nUnless you are sure that you are (and more importantly, can stay to\nbe) exhaustive, adding allowed representations for a thing will\nforce users to learn more non-essential things (\"we allow only 8<\nand >8\" vs \"we allow only these 7, even though we are aware that\nthere are at least 14 more that we do not allow\"---the end-user\nneeds to remember which 7 are allowed) and does not help users.\n\nTaking only \"black scissors\" U+2702 but not all of U+2700 - U+2704\nwill be a cause for unnecessary end-user complaints \"why do you take\nthis but not that one?\" Then the next noise would be \"why is '-'\nthe only perforation and not U+2014 Em Dash or U+2013 En Dash?\"\n\nLet's try not to be cute in non-essential things like how a pair of\nscissors ought to be spelled. If \"8<\" had worked well for us for\nthe past 10 years, we should just stick to it.\n\n\n\n\n"},{"id":"372862","messageId":"CACsJy8As7_qS-JRdMSNJco_=F4r21sjPR32thVrAB_CTrRdz8Q@mail.gmail.com","threadId":"50850","inReplyTo":"20190331220104.31628-1-rybak.a.v@gmail.com","subject":"Re: [PATCH] mailinfo: support Unicode scissors","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2019-04-01T09:27:02Z","receivedAt":"2019-04-01T09:27:31Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Mon, Apr 1, 2019 at 5:03 AM Andrei Rybak wrote:\n>\n> 'git am --scissors' allows cutting a patch from an email at a scissors\n> line. Such a line should contain perforation, i.e. hyphens, and a\n> scissors symbol. Only ASCII graphics scissors '8<' '>8' '%<' '>%' are\n> recognized by 'git am --scissors' command at the moment.\n>\n> Unicode character 'BLACK SCISSORS' (U+2702) has been a part of Unicode\n> since version 1.0.0 [1]. Since then 'BLACK SCISSORS' also became part\n> of character set Emoji 1.0, published in 2015 [2]. With its adoption as\n> an emoji, availability of this character on keyboards has increased.\n>\n> Support UTF-8 encoding of '✂' in function is_scissors_line, for 'git am\n> --scissors' to be able to cut at Unicode perforation lines in emails.\n> Note, that Unicode character '✂' is three bytes in UTF-8 encoding.\n\nOn top of what was already said in this thread. For some reason (bad\nfont?) these scissors are drawn cutting _down_ for me instead of left\nor right. It looks a bit strange.\n-- \nDuy\n"},{"id":"372866","messageId":"20190401101156.GA1131@sigill.intra.peff.net","threadId":"50850","inReplyTo":"20190331230947.GI32732@szeder.dev","subject":"Re: [PATCH] mailinfo: support Unicode scissors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-01T10:11:57Z","receivedAt":"2019-04-01T10:12:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2019 at 01:09:47AM +0200, SZEDER Gábor wrote:\n\n> On Mon, Apr 01, 2019 at 12:01:04AM +0200, Andrei Rybak wrote:\n> > diff --git a/mailinfo.c b/mailinfo.c\n> > index b395adbdf2..4ef6cdee85 100644\n> > --- a/mailinfo.c\n> > +++ b/mailinfo.c\n> > @@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n> > \t\t\tc++;\n> > \t\t\tcontinue;\n> > \t\t}\n> > +\t\tif (!memcmp(c, \"✂\", 3)) {\n> \n> This character is tiny. Please add a comment that it's supposed to be\n> a Unicode scissors character.\n\nI think it might also be the first raw UTF-8 character in our source,\nwhich is otherwise ASCII. Usually we'd spell out the binary (with a\ncomment).\n\nI think I agree with Junio's response, tough, that this is probably not\na road we want to go down, unless this micro-format is being actively\nused in the wild (I have no idea, but I have never seen it).\n\n> Should we worry about this memcmp() potentially reading past the end\n> of the string when 'c' points to the last character?\n\nI also wondered if the existing memcmps for \">8\", etc, would have this\nproblem. They don't, but it's somewhat subtle. They are only 2\ncharacters long, and the outer loop guarantees we have at least 1\ncharacter. So at most we will look at the NUL. But obviously a 3-byte\nsequence like this may invoke undefined behavior, and the existing\nmemcmps encourage anybody adding code to do it wrong.\n\nI wonder if it's worth re-writing it like:\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..46b1b2a4a8 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -693,8 +693,8 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n-\t\t !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n+\t\tif ((starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n+\t\t starts_with(c, \">%\") || starts_with(c, \"%<\"))) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n\n-Peff\n"},{"id":"372948","messageId":"20190401215334.18678-1-rybak.a.v@gmail.com","threadId":"50850","inReplyTo":"20190331220104.31628-1-rybak.a.v@gmail.com","subject":"[PATCH v2 1/2] mailinfo: use starts_with() for clarity","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-04-01T21:53:33Z","receivedAt":"2019-04-01T21:53:39Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Existing checks using memcmp(3) never read past the end of the line,\nbecause all substrings we are interested in are two characters long, and\nthe outer loop guarantees we have at least one character. So at most we\nwill look at the NUL.\n\nHowever, this is too subtle and may lead to bugs in code which copies\nthis behavior without realizing substring length requirement. So use\nstarts_with() instead, which will stop at NUL regardless of the length\nof the prefix. Remove extra pair of parentheses while we are here.\n\nHelped-by: Jeff King \nSigned-off-by: Andrei Rybak \n---\n\nOn Mon, Apr 01, 2019 at 06:11:57 -0400, Jeff King wrote:\n> I wonder if it's worth re-writing it like:\n\nTurned Peff's suggestion into a patch.\n\n mailinfo.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..f4aaa89788 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -693,8 +693,8 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n-\t\t !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n+\t\tif (starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n+\t\t starts_with(c, \">%\") || starts_with(c, \"%<\")) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n-- \n2.21.0\n\n"},{"id":"372949","messageId":"20190401215334.18678-2-rybak.a.v@gmail.com","threadId":"50850","inReplyTo":"20190401215334.18678-1-rybak.a.v@gmail.com","subject":"[PATCH v2 2/2] mailinfo: support Unicode scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-04-01T21:53:34Z","receivedAt":"2019-04-01T21:53:42Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Thank you all for review. Below is the second version of original patch,\naddressing comments by Gábor and Peff.\n\nWhile preparing v2 I found out that U+2702 was already suggested on the list\neight months before cutting at perforation lines was implemented:\nhttps://public-inbox.org/git/200901181656.37813.markus.heidelberg@web.de/T/#m3856d2e5c5f3e1900210b74bf2be8851b92d2271\n\n---- >8 ----\nSubject: [PATCH v2 2/2] mailinfo: support Unicode scissors\nDate: Mon, 1 Apr 2019 00:00:00 +0000\n\n'git am --scissors' allows cutting a patch from an email at a scissors\nline. Such a line should contain perforation, i.e. hyphens, and a\nscissors symbol. Only ASCII graphics scissors '8<' '>8' '%<' '>%' are\nrecognized by 'git am --scissors' command at the moment.\n\nUnicode character 'BLACK SCISSORS' (U+2702) has been a part of Unicode\nsince version 1.0.0 [1]. Since then 'BLACK SCISSORS' also became part\nof character set Emoji 1.0, published in 2015 [2]. With its adoption as\nan emoji, availability of this character on keyboards has increased.\n\nSupport UTF-8 encoding of '✂' in function is_scissors_line, for 'git am\n--scissors' to be able to cut at Unicode perforation lines in emails.\nNote, that Unicode character '✂' is three bytes in UTF-8 encoding and is\nspelled out using hexadecimal escape sequence.\n\n1. https://www.unicode.org/versions/Unicode1.0.0/CodeCharts1.pdf\n https://www.unicode.org/Public/reconstructed/1.0.0/UnicodeData.txt\n2. https://unicode.org/Public/emoji/1.0/emoji-data.txt\n\nSigned-off-by: Andrei Rybak \n---\n mailinfo.c | 7 +++++++\n t/t4150-am.sh | 26 +++++++++++++++++++++++++-\n 2 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex f4aaa89788..804b07cd8a 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n \t\t\tc++;\n \t\t\tcontinue;\n \t\t}\n+\t\tif (starts_with(c, \"\\xE2\\x9C\\x82\" /* U-2702 ✂ in UTF-8 */)) {\n+\t\t\tin_perforation = 1;\n+\t\t\tperforation += 3;\n+\t\t\tscissors += 3;\n+\t\t\tc += 2;\n+\t\t\tcontinue;\n+\t\t}\n \t\tin_perforation = 0;\n \t}\n \ndiff --git a/t/t4150-am.sh b/t/t4150-am.sh\nindex 3f7f750cc8..3ea8e8a2cf 100755\n--- a/t/t4150-am.sh\n+++ b/t/t4150-am.sh\n@@ -77,12 +77,20 @@ test_expect_success 'setup: messages' '\n \n \tprintf \"Subject: \" >subject-prefix &&\n \n-\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-scissors-line <<-\\EOF &&\n \tThis line should not be included in the commit message with --scissors enabled.\n \n \t - - >8 - - remove everything above this line - - >8 - -\n \n \tEOF\n+\n+\tcat - subject-prefix msg-without-scissors-line >msg-with-unicode-scissors <<-\\EOF\n+\tLines above unicode scissors line should not be included in the commit\n+\tmessage with --scissors enabled.\n+\n+\t- - - ✂ - - - ✂ - - -\n+\n+\tEOF\n '\n \n test_expect_success setup '\n@@ -161,6 +169,12 @@ test_expect_success setup '\n \tgit format-patch --stdout expected-for-no-scissors^ >patch-with-scissors-line.eml &&\n \tgit reset --hard HEAD^ &&\n \n+\techo file >file &&\n+\tgit add file &&\n+\tgit commit -F msg-with-unicode-scissors &&\n+\tgit format-patch --stdout HEAD^ >patch-with-unicode-scissors.eml &&\n+\tgit reset --hard HEAD^ &&\n+\n \tsed -n -e \"3,\\$p\" msg >file &&\n \tgit add file &&\n \ttest_tick &&\n@@ -421,6 +435,16 @@ test_expect_success 'am --scissors cuts the message at the scissors line' '\n \ttest_cmp_rev expected-for-scissors HEAD\n '\n \n+test_expect_success 'am --scissors cuts the message at the unicode scissors line' '\n+\trm -fr .git/rebase-apply &&\n+\tgit reset --hard &&\n+\tgit checkout second &&\n+\tgit am --scissors patch-with-unicode-scissors.eml &&\n+\ttest_path_is_missing .git/rebase-apply &&\n+\tgit diff --exit-code expected-for-scissors &&\n+\ttest_cmp_rev expected-for-scissors HEAD\n+'\n+\n test_expect_success 'am --no-scissors overrides mailinfo.scissors' '\n \trm -fr .git/rebase-apply &&\n \tgit reset --hard &&\n-- \n2.21.0\n\n"},{"id":"372950","messageId":"9481b5c1-5fe0-2d83-a721-72cb2a9e6d18@gmail.com","threadId":"50850","inReplyTo":"CACsJy8As7_qS-JRdMSNJco_=F4r21sjPR32thVrAB_CTrRdz8Q@mail.gmail.com","subject":"Re: [PATCH] mailinfo: support Unicode scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2019-04-01T21:54:44Z","receivedAt":"2019-04-01T21:54:49Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On Mon, 1 Apr 2019 16:27:02 +0700, Duy Nguyen wrote:\n> On Mon, Apr 1, 2019 at 5:03 AM Andrei Rybak wrote:\n>> Support UTF-8 encoding of '✂' in function is_scissors_line, for 'git am\n>> --scissors' to be able to cut at Unicode perforation lines in emails.\n>> Note, that Unicode character '✂' is three bytes in UTF-8 encoding.\n> \n> On top of what was already said in this thread. For some reason (bad\n> font?) these scissors are drawn cutting _down_ for me instead of left\n> or right. It looks a bit strange.\n\nThis might be an indication that the font used is rendering the\nsymbol as an emoji. Most scissors in emoji fonts are vertical:\nhttps://emojipedia.org/black-scissors/\n"},{"id":"372981","messageId":"20190402142846.GB10564@sigill.intra.peff.net","threadId":"50850","inReplyTo":"20190401215334.18678-1-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 1/2] mailinfo: use starts_with() for clarity","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-02T14:28:47Z","receivedAt":"2019-04-02T14:28:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2019 at 11:53:33PM +0200, Andrei Rybak wrote:\n\n> Existing checks using memcmp(3) never read past the end of the line,\n> because all substrings we are interested in are two characters long, and\n> the outer loop guarantees we have at least one character. So at most we\n> will look at the NUL.\n> \n> However, this is too subtle and may lead to bugs in code which copies\n> this behavior without realizing substring length requirement. So use\n> starts_with() instead, which will stop at NUL regardless of the length\n> of the prefix. Remove extra pair of parentheses while we are here.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Andrei Rybak \n> ---\n> \n> On Mon, Apr 01, 2019 at 06:11:57 -0400, Jeff King wrote:\n> > I wonder if it's worth re-writing it like:\n> \n> Turned Peff's suggestion into a patch.\n\nThanks. I think this may be worth doing regardless of what happens with\npatch 2.\n\n-Peff\n"},{"id":"372982","messageId":"20190402143615.GC10564@sigill.intra.peff.net","threadId":"50850","inReplyTo":"20190401215334.18678-2-rybak.a.v@gmail.com","subject":"Re: [PATCH v2 2/2] mailinfo: support Unicode scissors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-02T14:36:15Z","receivedAt":"2019-04-02T14:36:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Apr 01, 2019 at 11:53:34PM +0200, Andrei Rybak wrote:\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index f4aaa89788..804b07cd8a 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -701,6 +701,13 @@ static int is_scissors_line(const char *line)\n> \t\t\tc++;\n> \t\t\tcontinue;\n> \t\t}\n> +\t\tif (starts_with(c, \"\\xE2\\x9C\\x82\" /* U-2702 ✂ in UTF-8 */)) {\n> +\t\t\tin_perforation = 1;\n> +\t\t\tperforation += 3;\n> +\t\t\tscissors += 3;\n> +\t\t\tc += 2;\n> +\t\t\tcontinue;\n> +\t\t}\n\nIt might be worth using skip_prefix() instead of starts_with() to\ncompute the size automatically. E.g.:\n\n if (skip_prefix(c, \"\\xE2\\x9C\\x82\", &end)) {\n\tsize_t len = end - c; /* no magic number needed! */\n }\n\nIn fact, I think you could then combine this with the previous\nconditional and get:\n\n if (skip_prefix(c, \">8\", &end) ||\n skip_prefix(c, \"8<\", &end) ||\n skip_prefix(c, \">%\", &end) ||\n skip_prefix(c, \"%<\", &end) ||\n /* U-2702 in UTF-8 */\n skip_prefix(c, \"\\xE2\\x9C\\x82\", &end)) {\n in_perforation = 1;\n\t perforation += end - c;\n\t scissors += end - c;\n\t c = end - 1; /* minus one to account for loop increment */\n }\n\n(Though I'm still on the fence regarding the whole idea, so do not take\nthis as an endorsement ;) ).\n\n-Peff\n"},{"id":"373001","messageId":"xmqq8swrtwj0.fsf@gitster-ct.c.googlers.com","threadId":"50850","inReplyTo":"20190402143615.GC10564@sigill.intra.peff.net","subject":"Re: [PATCH v2 2/2] mailinfo: support Unicode scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-03T06:47:47Z","receivedAt":"2019-04-03T06:47:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> In fact, I think you could then combine this with the previous\n> conditional and get:\n>\n> if (skip_prefix(c, \">8\", &end) ||\n> skip_prefix(c, \"8<\", &end) ||\n> skip_prefix(c, \">%\", &end) ||\n> skip_prefix(c, \"%<\", &end) ||\n> /* U-2702 in UTF-8 */\n> skip_prefix(c, \"\\xE2\\x9C\\x82\", &end)) {\n> in_perforation = 1;\n> \t perforation += end - c;\n> \t scissors += end - c;\n> \t c = end - 1; /* minus one to account for loop increment */\n> }\n>\n> (Though I'm still on the fence regarding the whole idea, so do not take\n> this as an endorsement ;) ).\n\nI do not think we want to add more, but use of skip_prefix does\nsound sensible. I was very tempted to suggest\n\n\tstatic const char *scissors[] = {\n\t\t\">8\", \"8<\", \">%\", \"%<\",\n NULL,\n\t};\n const char **s;\n\n\tfor (s = scissors; *s; s++)\n\t\tif (skip_prefix, c, *s, &end) {\n\t\t\tin_perforation = 1;\n\t\t\t...\n\t\t\tbreak;\n\t\t}\n\t}\n if (!s)\n\t\t... we are not looking at any of the scissors[] ...\n\nbut that would encourage adding more random entries to the array,\nwhich we would want to avoid in order to help reduce the cognirive\nload of end-users.\n\nIn hindsight, addition of an undocumented '%' was already a mistake.\nI wonder how widely it is in use (yes, I am tempted to deprecate and\nremove these two to match the code to the docs).\n"},{"id":"426811","messageId":"20210608204841.2793591-1-rybak.a.v@gmail.com","threadId":"50850","inReplyTo":"20190401215334.18678-1-rybak.a.v@gmail.com","subject":"[PATCH] mailinfo: use starts_with() when checking scissors","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2021-06-08T20:48:41Z","receivedAt":"2021-06-08T20:49:03Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"Existing checks for scissors characters using memcmp(3) never read past\nthe end of the line, because all substrings we are interested in are two\ncharacters long, and the outer loop guarantees we have at least one\ncharacter. So at most we will look at the NUL.\n\nHowever, this is too subtle and may lead to bugs in code which copies\nthis behavior without realizing substring length requirement. So use\nstarts_with() instead, which will stop at NUL regardless of the length\nof the prefix. Remove extra pair of parentheses while we are here.\n\nHelped-by: Jeff King \nSigned-off-by: Andrei Rybak \n---\n mailinfo.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\nThis patch was originally part of:\nhttps://public-inbox.org/git/20190401215334.18678-1-rybak.a.v@gmail.com/\nI've finally gotten around to sending it on its own.\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ccc6beb27e..8013e5c566 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -705,8 +705,8 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n-\t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n-\t\t !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n+\t\tif (starts_with(c, \">8\") || starts_with(c, \"8<\") ||\n+\t\t starts_with(c, \">%\") || starts_with(c, \"%<\")) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n-- \n2.31.1\n\n"},{"id":"426815","messageId":"YL/n1b641m+wuVHG@coredump.intra.peff.net","threadId":"50850","inReplyTo":"20210608204841.2793591-1-rybak.a.v@gmail.com","subject":"Re: [PATCH] mailinfo: use starts_with() when checking scissors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-08T21:57:41Z","receivedAt":"2021-06-08T21:57:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jun 08, 2021 at 10:48:41PM +0200, Andrei Rybak wrote:\n\n> Existing checks for scissors characters using memcmp(3) never read past\n> the end of the line, because all substrings we are interested in are two\n> characters long, and the outer loop guarantees we have at least one\n> character. So at most we will look at the NUL.\n> \n> However, this is too subtle and may lead to bugs in code which copies\n> this behavior without realizing substring length requirement. So use\n> starts_with() instead, which will stop at NUL regardless of the length\n> of the prefix. Remove extra pair of parentheses while we are here.\n> \n> Helped-by: Jeff King \n> Signed-off-by: Andrei Rybak \n> ---\n> mailinfo.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> This patch was originally part of:\n> https://public-inbox.org/git/20190401215334.18678-1-rybak.a.v@gmail.com/\n> I've finally gotten around to sending it on its own.\n\nWell, after seeing the date I don't feel too bad for not remembering it. :)\n\nI think this is an improvement in safety and readability, and worth\napplying. I'd also be fine going further and using skip_prefix(), which\nwould let us drop the magic-number \"2\", though:\n\n - as Junio said in that earlier thread, we hope people aren't\n encouraged to add more versions of scissors here anyway).\n\n - I am not 100% sure that the increment of \"c\" in the conditional\n is not also relying on that \"2\", as well (i.e., it is incrementing\n one, and then the for-loop increments one). There's a non-trivial\n risk of introducing off-by-one errors or even more subtlety here. :)\n\nSo I'm OK leaving that. Thanks for resurrecting this cleanup.\n\n-Peff\n"},{"id":"426833","messageId":"xmqq35trn5f1.fsf@gitster.g","threadId":"50850","inReplyTo":"YL/n1b641m+wuVHG@coredump.intra.peff.net","subject":"Re: [PATCH] mailinfo: use starts_with() when checking scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-09T02:22:10Z","receivedAt":"2021-06-09T02:22:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think this is an improvement in safety and readability, and worth\n> applying. I'd also be fine going further and using skip_prefix(), which\n> would let us drop the magic-number \"2\", though:\n>\n> - as Junio said in that earlier thread, we hope people aren't\n> encouraged to add more versions of scissors here anyway).\n>\n> - I am not 100% sure that the increment of \"c\" in the conditional\n> is not also relying on that \"2\", as well (i.e., it is incrementing\n> one, and then the for-loop increments one). There's a non-trivial\n> risk of introducing off-by-one errors or even more subtlety here. :)\n>\n> So I'm OK leaving that. Thanks for resurrecting this cleanup.\n\nYou are correct to point out that the increment of \"c\" relies on 2.\nWe increment by one here, and let the loop control to increment\nanother, to skip over these two bytes.\n\nThe posted patch _might_ make it easier to read, but I do not think\nit improves safety at all. At the point of doing memcmp(), we know\nthat *c is not NUL and because we are dealing with NUL-terminated\nstring, we know we can check c[1] (otherwise we wouldn't even be\nable to see if c is pointing at the end of string), so we check c[0]\nand c[1] against four variants of two-byte scissors patterns. The\ncurrent code uses memcmp() of 2 bytes, which is perfectly safe under\nthe condition, and starts_with() would also be equally safe.\n\nIf we were to teach new scissors sequence that is longer than two\nbytes, then starts_with() would start becoming safer, but that will\nnot happen, so...\n\n\n"},{"id":"426839","messageId":"YMA8sDr/uuJ9Tst6@coredump.intra.peff.net","threadId":"50850","inReplyTo":"xmqq35trn5f1.fsf@gitster.g","subject":"Re: [PATCH] mailinfo: use starts_with() when checking scissors","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2021-06-09T03:59:44Z","receivedAt":"2021-06-09T03:59:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jun 09, 2021 at 11:22:10AM +0900, Junio C Hamano wrote:\n\n> The posted patch _might_ make it easier to read, but I do not think\n> it improves safety at all. At the point of doing memcmp(), we know\n> that *c is not NUL and because we are dealing with NUL-terminated\n> string, we know we can check c[1] (otherwise we wouldn't even be\n> able to see if c is pointing at the end of string), so we check c[0]\n> and c[1] against four variants of two-byte scissors patterns. The\n> current code uses memcmp() of 2 bytes, which is perfectly safe under\n> the condition, and starts_with() would also be equally safe.\n> \n> If we were to teach new scissors sequence that is longer than two\n> bytes, then starts_with() would start becoming safer, but that will\n> not happen, so...\n\nRight, I agree the current code is safe. The main value would be trying\nto make the correctness of the code more apparent. Perhaps a comment\nwould be better there, like:\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ccc6beb27e..25b606db28 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -705,12 +705,17 @@ static int is_scissors_line(const char *line)\n \t\t\tperforation++;\n \t\t\tcontinue;\n \t\t}\n+\t\t/*\n+\t\t * passing 2 bytes to memcmp is safe here; we have at least\n+\t\t * one non-NUL character from the loop condition, plus a\n+\t\t * terminating NUL\n+\t\t */\n \t\tif ((!memcmp(c, \">8\", 2) || !memcmp(c, \"8<\", 2) ||\n \t\t !memcmp(c, \">%\", 2) || !memcmp(c, \"%<\", 2))) {\n \t\t\tin_perforation = 1;\n \t\t\tperforation += 2;\n \t\t\tscissors += 2;\n-\t\t\tc++;\n+\t\t\tc++; /* only 1 here to account for loop increment */\n \t\t\tcontinue;\n \t\t}\n \t\tin_perforation = 0;\n\nThough since this is code we'd not plan to modify, and presumably\nanybody touching it would have to fully grok the loop anyway, it might\nnot be that important.\n\nI dunno. I offer it as an alternative (and am happy to add a commit\nmessage). But I'm fine with leaving it as-is, too.\n\n-Peff\n"},{"id":"426852","messageId":"xmqq1r9blj0g.fsf@gitster.g","threadId":"50850","inReplyTo":"YMA8sDr/uuJ9Tst6@coredump.intra.peff.net","subject":"Re: [PATCH] mailinfo: use starts_with() when checking scissors","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-06-09T05:11:27Z","receivedAt":"2021-06-09T05:11:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I dunno. I offer it as an alternative (and am happy to add a commit\n> message). But I'm fine with leaving it as-is, too.\n\nI am fine with Andrei's version as posted, which I've queued.\n\nThanks, both.\n"}]}