{"thread":{"id":"36256","subject":"[PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","startedAt":"2014-03-20T19:39:44Z","lastAt":"2014-03-26T18:02:50Z","messageCount":9,"participants":["George Papanikolaou","Eric Sunshine","Michael Haggerty","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"237199","messageId":"1395344384-7975-1-git-send-email-g3orge.app@gmail.com","threadId":"36256","inReplyTo":null,"subject":"[PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"George Papanikolaou","fromEmail":"g3orge.app@gmail.com","sentAt":"2014-03-20T19:39:44Z","receivedAt":"2014-03-20T19:39:44Z","isPatch":true,"sender":{"key":"g3orge.app@gmail.com","avatar":"https://gravatar.com/avatar/57d9756bb8e51dd138b79f2276e0bb0be2bfdc9aa60f08704de80e96d3a1c0f8?d=mp&s=160"},"body":"Removing the bloat of checking for both '\\r' and '\\n' with the prettier\niswspace() function which checks for other characters as well. (read: \\f \\t \\v)\n---\n\nThis is one more try to clean up this fuzzy_matchlines() function as part of a\nmicroproject for GSOC. The rest more clarrified microprojects were taken.\nI'm obviously planning on applying.\n\nThanks\n\nSigned-of-by: George 'papanikge' Papanikolaou \n\n builtin/apply.c | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/apply.c b/builtin/apply.c\nindex b0d0986..912a53a 100644\n--- a/builtin/apply.c\n+++ b/builtin/apply.c\n@@ -295,9 +295,9 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n \tint result = 0;\n \n \t/* ignore line endings */\n-\twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n+\twhile (iswspace(*last1))\n \t\tlast1--;\n-\twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n+\twhile (iswspace(*last2))\n \t\tlast2--;\n \n \t/* skip leading whitespace */\n-- \n1.9.0\n"},{"id":"237253","messageId":"CAPig+cTw8pyRVOHToGRPBdxv+TX8Vcj5OrX-CmLWRCigZRS4MA@mail.gmail.com","threadId":"36256","inReplyTo":"1395344384-7975-1-git-send-email-g3orge.app@gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T02:48:52Z","receivedAt":"2014-03-21T02:48:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Mar 20, 2014 at 3:39 PM, George Papanikolaou\n wrote:\n> Removing the bloat of checking for both '\\r' and '\\n' with the prettier\n> iswspace() function which checks for other characters as well. (read: \\f \\t \\v)\n\nUse imperative mood. \"Remove\" rather than \"Removing\".\n\nBloat? Prettier? Subjective stuff.\n\nDid you verify that it is safe to strip all whitespace characters\nrather than only line-endings? Perhaps say so in the commit message.\n\nWhy the choice of iswspace()? These are normal-width character\nstrings, so why apply a wide-character function?\n\nMore below.\n\n> ---\n>\n> This is one more try to clean up this fuzzy_matchlines() function as part of a\n> microproject for GSOC. The rest more clarrified microprojects were taken.\n> I'm obviously planning on applying.\n>\n> Thanks\n>\n> Signed-of-by: George 'papanikge' Papanikolaou \n>\n> builtin/apply.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n>\n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index b0d0986..912a53a 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -295,9 +295,9 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n> int result = 0;\n>\n> /* ignore line endings */\n> - while ((*last1 == '\\r') || (*last1 == '\\n'))\n> + while (iswspace(*last1))\n> last1--;\n> - while ((*last2 == '\\r') || (*last2 == '\\n'))\n> + while (iswspace(*last2))\n> last2--;\n\nDoesn't this change turn the comment preceding this code into a\nhalf-truth? Perhaps update the comment?\n\n> /* skip leading whitespace */\n> --\n> 1.9.0\n"},{"id":"237307","messageId":"532C1EFA.3000109@alum.mit.edu","threadId":"36256","inReplyTo":"1395344384-7975-1-git-send-email-g3orge.app@gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-21T11:14:02Z","receivedAt":"2014-03-21T11:14:02Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/20/2014 08:39 PM, George Papanikolaou wrote:\n> Removing the bloat of checking for both '\\r' and '\\n' with the prettier\n> iswspace() function which checks for other characters as well. (read: \\f \\t \\v)\n> ---\n> \n> This is one more try to clean up this fuzzy_matchlines() function as part of a\n> microproject for GSOC. The rest more clarrified microprojects were taken.\n> I'm obviously planning on applying.\n> \n> Thanks\n> \n> Signed-of-by: George 'papanikge' Papanikolaou \n> \n> builtin/apply.c | 4 ++--\n> 1 file changed, 2 insertions(+), 2 deletions(-)\n> \n> diff --git a/builtin/apply.c b/builtin/apply.c\n> index b0d0986..912a53a 100644\n> --- a/builtin/apply.c\n> +++ b/builtin/apply.c\n> @@ -295,9 +295,9 @@ static int fuzzy_matchlines(const char *s1, size_t n1,\n> \tint result = 0;\n> \n> \t/* ignore line endings */\n> -\twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n> +\twhile (iswspace(*last1))\n> \t\tlast1--;\n> -\twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n> +\twhile (iswspace(*last2))\n> \t\tlast2--;\n> \n> \t/* skip leading whitespace */\n> \n\nIn addition to Eric's comments...\n\nWhat happens if the string consists *only* of whitespace?\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"237383","messageId":"CAPig+cRcW8jv7LNZmLfrSGLaqE7yHycbmfvtNETPo51QoM7N2g@mail.gmail.com","threadId":"36256","inReplyTo":"CAByyCQBmCTfW0HBL04MMqwm+bDe4Rb6n+MfWdYUQ6M6yW_u=yw@mail.gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-21T23:07:34Z","receivedAt":"2014-03-21T23:07:34Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[Please reply on-list to review comments. Other people may learn from\nthe discussion or have comments of their own.]\n\nOn Fri, Mar 21, 2014 at 6:00 PM, George Papanikolaou\n wrote:\n> On Fri, Mar 21, 2014 at 4:48 AM, Eric Sunshine wrote:\n>>\n>> Did you verify that it is safe to strip all whitespace characters\n>> rather than only line-endings? Perhaps say so in the commit message.\n>>\n>> Why the choice of iswspace()? These are normal-width character\n>> strings, so why apply a wide-character function?\n>>\n> why not?\n\nBecause it's unnecessary and invites confusion from people reading\ncode since they now have to wonder if there is something unusual and\nnon-obvious going on. Worse, the two loops immediately below the ones\nyou changed, as well as the rest of the function, use plain isspace(),\nwhich really ramps up the \"huh?\"-factor from the reader.\n\nThe original code has the asset of being clear and obvious. Changing\nthese two loops to use a wide-character function makes it less so.\n\n> since at this point it is checking for any non-readable\n> characters at the end of the buffer, I figured we should check for the\n> \"wide-character\" function that covers these.\n\nNeither the function comment nor the existing code implies that it is\nchecking for \"any non-readable characters\". (I'm not even sure what\nthat means.) The only thing the existing code says at that point is\nthat it is ignoring line-endings.\n\n> It is true that the\n> comment should change in that matter.\n>\n> Also why wouldn't it be safe? And how can I check?\n\nYou're changing the behavior of the function (assuming I'm reading\ncorrectly), which is why I asked if you verified that doing so was\nsafe. The existing code considers \"foo bar\" and \"foo bar \" to be\ndifferent. With your change, they are considered equal, which is\nactually more in line with what the function comment says.\nNevertheless, callers may be relying upon the existing behavior.\n\nAt the very least, the unit tests should be run as a quick check of\nwhether this behavior change introduces problems. Manual inspection of\ncallers also wouldn't hurt.\n\nThere's also the issue that Michael raised when he asked what would\nhappen if either string was composed of whitespace only. The existing\ncode is not robust and can crash, but your change may increase the\nlikelihood of the crash.\n\n> Thanks\n>\n> --\n> papanikge's surrogate email.\n> I may reply back.\n> http://www.5slingshots.com/\n"},{"id":"237388","messageId":"CAByyCQAqZnnc91ZgmxdKgc7T0POLqd+iXmKvaKEPMOx6CNQkKQ@mail.gmail.com","threadId":"36256","inReplyTo":"CAPig+cTct-42w5S=OUS_DQ2cD5X9nWa_eUVoFBGTT7nAEahi5g@mail.gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"George Papanikolaou","fromEmail":"g3orge.app@gmail.com","sentAt":"2014-03-22T09:33:56Z","receivedAt":"2014-03-22T09:33:56Z","isPatch":true,"sender":{"key":"g3orge.app@gmail.com","avatar":"https://gravatar.com/avatar/57d9756bb8e51dd138b79f2276e0bb0be2bfdc9aa60f08704de80e96d3a1c0f8?d=mp&s=160"},"body":"On Sat, Mar 22, 2014 at 12:46 AM, Eric Sunshine wrote:\n>\n> Because it's unnecessary and invites confusion from people reading the\n> code since they now have to wonder if there is something unusual and\n> non-obvious going. Worse, the two loops immediately below the ones you\n> changed, as well as the rest of the function, use plain isspace(),\n> which really ramps up the \"huh?\"-factor from the reader.\n>\n> The original code has the asset of being clear and obvious. Changing\n> these two loops to use a wide-character function makes it less so.\n>\n\nYes I understand it does add a factor of ambiguity.\n\n>\n> Neither the function comment nor the existing code implies that it is\n> checking for \"any non-readable characters\". (I'm not even sure what\n> that means.) The only thing the existing code says at that point is\n> that it is ignoring line-endings.\n>\n\nI mean characters that are not printable like letters, numbers, dots etc\n\n>\n> You're changing the behavior of the function (assuming I'm reading it\n> correctly), which is why I asked if you verified that doing so was\n> safe. The existing code considers \"foo bar\" and \"foo bar \" to be\n> different. With your change, they are considered equal, which is\n> actually more in line with what the function comment says.\n> Nevertheless, callers may be relying upon the existing behavior.\n>\n> At the very least, the unit tests should be run as a quick check of\n> whether if this behavior change introduces problems. Manual inspection\n> of callers also wouldn't hurt.\n>\n\nI did not think about that possibility, because I ran `make` and the\ntests passed so I thought that that would be ok.\n\nAnyway, do you have any ideas on how to improve that function?\n\nThanks again for the feedback.\n\n-- \npapanikge's surrogate email.\nI may reply back.\nhttp://www.5slingshots.com/I did not think about that possibility.\n"},{"id":"237415","messageId":"CAPig+cTFNsmQPmUpax-rbqkk5JzgAw4fK0tM4U013Z_x7o-ZyA@mail.gmail.com","threadId":"36256","inReplyTo":"CAByyCQAqZnnc91ZgmxdKgc7T0POLqd+iXmKvaKEPMOx6CNQkKQ@mail.gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-23T09:35:43Z","receivedAt":"2014-03-23T09:35:43Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 22, 2014 at 5:33 AM, George Papanikolaou\n wrote:\n> On Sat, Mar 22, 2014 at 12:46 AM, Eric Sunshine wrote:\n>> Because it's unnecessary and invites confusion from people reading the\n>> code since they now have to wonder if there is something unusual and\n>> non-obvious going. Worse, the two loops immediately below the ones you\n>> changed, as well as the rest of the function, use plain isspace(),\n>> which really ramps up the \"huh?\"-factor from the reader.\n>>\n>> The original code has the asset of being clear and obvious. Changing\n>> these two loops to use a wide-character function makes it less so.\n>>\n> Yes I understand it does add a factor of ambiguity.\n>\n>> Neither the function comment nor the existing code implies that it is\n>> checking for \"any non-readable characters\". (I'm not even sure what\n>> that means.) The only thing the existing code says at that point is\n>> that it is ignoring line-endings.\n>>\n> I mean characters that are not printable like letters, numbers, dots etc\n\nIt's still not clear how this answer relates to my question about why\nyou used iswspace() rather than isspace().\n\nNothing in the code or comments indicates that it wants to ignore\nnon-printing characters. Even if the intention of your change had\nindeed been to ignore such characters, you would have used !isprint()\nor !iswprint().\n\n>> You're changing the behavior of the function (assuming I'm reading it\n>> correctly), which is why I asked if you verified that doing so was\n>> safe. The existing code considers \"foo bar\" and \"foo bar \" to be\n>> different. With your change, they are considered equal, which is\n>> actually more in line with what the function comment says.\n>> Nevertheless, callers may be relying upon the existing behavior.\n>>\n>> At the very least, the unit tests should be run as a quick check of\n>> whether if this behavior change introduces problems. Manual inspection\n>> of callers also wouldn't hurt.\n>>\n> I did not think about that possibility, because I ran `make` and the\n> tests passed so I thought that that would be ok.\n\nUnit tests may cover a lot of functionality, but there will always be\nholes in the coverage. Thus, it's a good idea to examine callers and\nsurrounding code manually, as well.\n\nSince this is a behavior change, it deserves mention in the commit\nmessage, as well as assurance that you verified (as best you can) that\nit did not break existing callers. (It also wouldn't hurt to mention\nthat it brings the code more in line with the function documentation.)\n\n> Anyway, do you have any ideas on how to improve that function?\n\nMichael gave you a strong clue when he asked what would happen, with\nyour change in place, if the string consisted only of whitespace. The\nloops you touched are already fragile, even without your change.\nMaking them more robust would likely be considered an improvement.\n\n> Thanks again for the feedback.\n>\n> --\n> papanikge's surrogate email.\n> I may reply back.\n> http://www.5slingshots.com/I did not think about that possibility.\n"},{"id":"237523","messageId":"7vd2haq3n5.fsf@alter.siamese.dyndns.org","threadId":"36256","inReplyTo":"532C1EFA.3000109@alum.mit.edu","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-25T04:54:38Z","receivedAt":"2014-03-25T04:54:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n>> -\twhile ((*last1 == '\\r') || (*last1 == '\\n'))\n>> +\twhile (iswspace(*last1))\n>> \t\tlast1--;\n>> -\twhile ((*last2 == '\\r') || (*last2 == '\\n'))\n>> +\twhile (iswspace(*last2))\n>> \t\tlast2--;\n>> \n>> \t/* skip leading whitespace */\n>> \n>\n> In addition to Eric's comments...\n>\n> What happens if the string consists *only* of whitespace?\n\nAlso, why would casting char to wchar_t without any conversion be\nsafe and/or sane?\n\nI would sort-of understand if the change were to use isspace(), but\nI do not think that is a correct conversion, either. Isn't a pair\nof strings \"a bc\" and \"a bc \" supposed not to match?\n\nMy understanding is that two strings that differ only at places\nwhere they have runs of whitespaces whose length differ are to\ncompare the same, e.g. \"a_bc__\" and \"a__bc_\" (SP replaced with _ to\nmake them stand out). Ignoring whitespace change is very different\nfrom ignoring all whitespaces (the latter of which would make \"a b\"\nand \"ab\" match).\n\nAs a tangent, I have a suspicion that the current implementation may\nbe wrong at the beginning of the string. Wouldn't it match \" abc\"\nand \"abc\", even though these two strings shouldn't match?\n"},{"id":"237818","messageId":"CAByyCQBX+xfDdMwFOE2bZg8W2S0jwj2nLV36JwH1N3D4Fn2BUw@mail.gmail.com","threadId":"36256","inReplyTo":"7vd2haq3n5.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"George Papanikolaou","fromEmail":"g3orge.app@gmail.com","sentAt":"2014-03-26T16:58:19Z","receivedAt":"2014-03-26T16:58:19Z","isPatch":true,"sender":{"key":"g3orge.app@gmail.com","avatar":"https://gravatar.com/avatar/57d9756bb8e51dd138b79f2276e0bb0be2bfdc9aa60f08704de80e96d3a1c0f8?d=mp&s=160"},"body":"On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano wrote:\n> As a tangent, I have a suspicion that the current implementation may\n> be wrong at the beginning of the string. Wouldn't it match \" abc\"\n> and \"abc\", even though these two strings shouldn't match?\n\nWouldn't that be accomplished by just removing the leading whitespace check?\n\nI'm somewhat confused about what the function should match. I haven't\ngrasped it.\n\n--\npapanikge's surrogate email.\nI may reply back.\nhttp://www.5slingshots.com/\n"},{"id":"237821","messageId":"xmqqzjkcj0s5.fsf@gitster.dls.corp.google.com","threadId":"36256","inReplyTo":"CAByyCQBX+xfDdMwFOE2bZg8W2S0jwj2nLV36JwH1N3D4Fn2BUw@mail.gmail.com","subject":"Re: [PATCH] builtin/apply.c: use iswspace() to detect line-ending-like chars","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-26T18:02:50Z","receivedAt":"2014-03-26T18:02:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"George Papanikolaou writes:\n\n> On Tue, Mar 25, 2014 at 6:54 AM, Junio C Hamano wrote:\n>> As a tangent, I have a suspicion that the current implementation may\n>> be wrong at the beginning of the string. Wouldn't it match \" abc\"\n>> and \"abc\", even though these two strings shouldn't match?\n>\n> Wouldn't that be accomplished by just removing the leading whitespace check?\n\nYes. I was wondering *what* semantics we want in the first place;\nhow to implement what I suggested is so trivial that it goes without\nsaying for the intended audiences of that comment ;-).\n\n> I'm somewhat confused about what the function should match. I haven't\n> grasped it.\n\nThis function is used when attempting to resurrect a patch that is\nwhitespace-damaged. The patch may want to change a line \"a_bc\" in\nthe original into something else [*1*], and we may not find \"a_bc\"\nin the current source, but there may be \"a__bc\" (two spaces instead\nof one the whitespace-damaged patch claims to expect). By ignoring\nthe amount of whitespaces, it forces \"git apply\" to consider that\n\"a_bc\" in the broken patch meant to refer to \"a__bc\" in reality.\n\nI _think_ the original motivation of ignore_ws_change was to match\nthe \"--ignore-space-change\" option of \"diff\", i.e. \"ignore changes\nin the amount of white space\". I just checked the source\n(xdiff/xutils.c) and made sure that \"git diff\" does not treat the\nbeginning of line any differently hence \"_a_bc\" and \"a_bc\" are not\nconsidered a match under its --ignore-space-change option.\n\nThe current implementation of \"apply --ignore-space-change\" that\nignores leading whitespaces (not \"ignore changes in the amount of\nleading whitespaces\") is likely to be a bug from this point of view.\n\nBut I wanted to hear opinions from other Git experts [*2*]. Hence\nmy \"As a tangent, I have a suspicion\".\n\n\n[Footnote]\n\n*1* This mode is not enabled by default. I am not even sure if\n anybody sane would (or should) use this option. Sure, the fuzzy\n match may be able to find the original line that the patch\n author may meant to patch even when it is whiltespace-damaged\n because it does not fully trust what the original lines exactly\n say (i.e. context lines prefixed by \" \" and old lines prefixed\n by \"-\"). What makes it sane for us to trust what the\n replacement lines (i.e. new lines prefixed by \"+\") in such a\n mangled patch says?\n\n*2* For example, somebody may be able to point out that \"this is\n meant to match the option of the same name 'diff' has\", which is\n my assumption that leads to the above discussion, may not be\n true.\n"}]}