{"thread":{"id":"36777","subject":"[PATCH] Improve function dir.c:trim_trailing_spaces()","startedAt":"2014-05-28T23:45:57Z","lastAt":"2014-05-30T18:45:36Z","messageCount":5,"participants":["Pasha Bolokhov","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"242920","messageId":"1401320757-9360-1-git-send-email-pasha.bolokhov@gmail.com","threadId":"36777","inReplyTo":null,"subject":"[PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-05-28T23:45:57Z","receivedAt":"2014-05-28T23:45:57Z","isPatch":true,"sender":{"key":"pasha.bolokhov@gmail.com","avatar":null},"body":"Move backwards from the end of the string (more efficient for\nlines which do not have trailing spaces or have just a couple).\nSlightly more rare occurrences of 'text  \\    ' with a backslash\nin between spaces are handled correctly.\nNamely, the code in 8ba87adad6 does not reset 'last_space' when\na backslash is encountered and the above line stays intact as\na result\n---\nHow about trailing tabs?\n\n dir.c | 25 +++++++++++--------------\n 1 file changed, 11 insertions(+), 14 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6f581..3315eea 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -508,21 +508,18 @@ void clear_exclude_list(struct exclude_list *el)\n \n static void trim_trailing_spaces(char *buf)\n {\n-\tint i, last_space = -1, nr_spaces, len = strlen(buf);\n-\tfor (i = 0; i < len; i++)\n-\t\tif (buf[i] == '\\\\')\n-\t\t\ti++;\n-\t\telse if (buf[i] == ' ') {\n-\t\t\tif (last_space == -1) {\n-\t\t\t\tlast_space = i;\n-\t\t\t\tnr_spaces = 1;\n-\t\t\t} else\n-\t\t\t\tnr_spaces++;\n-\t\t} else\n-\t\t\tlast_space = -1;\n-\n-\tif (last_space != -1 && last_space + nr_spaces == len)\n+\tint i, last_space, bslash = 0, len = strlen(buf);\n+\n+\tif (len == 0 || buf[len - 1] != ' ')\n+\t\treturn;\n+\tfor (i = len - 2; i >= 0 && buf[i] == ' '; i--) ;\n+\tlast_space = i + 1;\n+\tfor ( ; i >=0 && buf[i] == '\\\\'; i--) bslash ^= 1;\n+\n+\tif (!bslash)\n \t\tbuf[last_space] = '\\0';\n+\telse if (bslash && last_space < len - 1)\n+\t\tbuf[last_space + 1] = '\\0';\n }\n \n int add_excludes_from_file_to_list(const char *fname,\n-- \n1.9.1\n"},{"id":"242979","messageId":"20140529201334.GA17355@sigill.intra.peff.net","threadId":"36777","inReplyTo":"1401320757-9360-1-git-send-email-pasha.bolokhov@gmail.com","subject":"Re: [PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-29T20:13:34Z","receivedAt":"2014-05-29T20:13:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote:\n\n> Move backwards from the end of the string (more efficient for\n> lines which do not have trailing spaces or have just a couple).\n\nThe original code reads the string from left to right. In theory, that\nmeans we could get away with not calling strlen() at all, over a\nright-to-left that must start with a call to strlen().\n\nThat being said, I think we already have the length in the calling\nfunction, so you could probably avoid the strlen() altogether, which\nmakes a right-to-left function more efficient.\n\nHowever, I doubt it makes that much of a difference in practice, so\nunless it's measurable, I would certainly go with the version that is\nmore readable (and correct, of course).\n\n> Slightly more rare occurrences of 'text  \\    ' with a backslash\n> in between spaces are handled correctly.\n\nCan you add a test for this?\n\nAlso, if you are refactoring this function, I think it makes sense to\ncheck that:\n\n  \"foo\\\\ \"\n\nand\n\n  \"foo\\\\\\ \"\n\nmatch \"foo\\\" and \"foo\\ \", respectively (I think they do with your patch,\nbut it is a tricky case that is not immediately obvious).\n\n-Peff\n"},{"id":"242980","messageId":"CAKpPgveDfNq+kxWCukULPqvn7H2XsfS8maC0ExzvHzFGiEAPbg@mail.gmail.com","threadId":"36777","inReplyTo":"20140529201334.GA17355@sigill.intra.peff.net","subject":"Re: [PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-05-29T21:34:33Z","receivedAt":"2014-05-29T21:34:33Z","isPatch":true,"sender":{"key":"pasha.bolokhov@gmail.com","avatar":null},"body":"On Thu, May 29, 2014 at 1:13 PM, Jeff King <peff@peff.net> wrote:\n> On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote:\n>\n>> Move backwards from the end of the string (more efficient for\n>> lines which do not have trailing spaces or have just a couple).\n>\n> The original code reads the string from left to right. In theory, that\n> means we could get away with not calling strlen() at all, over a\n> right-to-left that must start with a call to strlen().\n>\n> That being said, I think we already have the length in the calling\n> function, so you could probably avoid the strlen() altogether, which\n> makes a right-to-left function more efficient.\n>\n> However, I doubt it makes that much of a difference in practice, so\n> unless it's measurable, I would certainly go with the version that is\n> more readable (and correct, of course).\n\n    Sorry, just to recap, you would go with the existing version\n(which needs correction), or with the one that is being suggested? (I\nagree I can format the style a tiny bit better in the latter one)\n\n     Tests should not be a big problem, although it's kind of clumsy\nto test an internal function which does not really give any output\n(you can only measure the outcome). Just again to stress, I have\ntested both implementation extensively and the suggested new\nimplementation gives the correct answers for all your examples below\nand all others. If I show this with explicit \"t/\" tests, will it\nsuffice then?\n\n    Basically what I suggest is\n\n-- either: improve the existing function such that it does correctly\nthat \"text  \\   \" case, and also does not use 'strlen' since it anyway\nmoves left to right\n\n-- or: use the new suggested implementation (and just brush the\nformatting a bit), and perhaps borrow 'len' from the calling routine\n\nAnd add tests in any case. What is the preference?\n\n\n>\n>> Slightly more rare occurrences of 'text  \\    ' with a backslash\n>> in between spaces are handled correctly.\n>\n> Can you add a test for this?\n>\n> Also, if you are refactoring this function, I think it makes sense to\n> check that:\n>\n>   \"foo\\\\ \"\n>\n> and\n>\n>   \"foo\\\\\\ \"\n>\n> match \"foo\\\" and \"foo\\ \", respectively (I think they do with your patch,\n> but it is a tricky case that is not immediately obvious).\n>\n> -Peff\n"},{"id":"243002","messageId":"20140530020444.GH28683@sigill.intra.peff.net","threadId":"36777","inReplyTo":"CAKpPgveDfNq+kxWCukULPqvn7H2XsfS8maC0ExzvHzFGiEAPbg@mail.gmail.com","subject":"Re: [PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-05-30T02:04:44Z","receivedAt":"2014-05-30T02:04:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, May 29, 2014 at 02:34:33PM -0700, Pasha Bolokhov wrote:\n\n> > However, I doubt it makes that much of a difference in practice, so\n> > unless it's measurable, I would certainly go with the version that is\n> > more readable (and correct, of course).\n> \n>     Sorry, just to recap, you would go with the existing version\n> (which needs correction), or with the one that is being suggested? (I\n> agree I can format the style a tiny bit better in the latter one)\n\nI actually think the original left-to-right is a little easier to\nfollow, but I do not feel strongly. I mainly meant \"argue based on\nreadability and correctness, do not argue based on speed\".\n\n>      Tests should not be a big problem, although it's kind of clumsy\n> to test an internal function which does not really give any output\n> (you can only measure the outcome). Just again to stress, I have\n> tested both implementation extensively and the suggested new\n> implementation gives the correct answers for all your examples below\n> and all others. If I show this with explicit \"t/\" tests, will it\n> suffice then?\n\nYes. I think specifically that you can extend the tests at the end of\nt0008.\n\n>     Basically what I suggest is\n> \n> -- either: improve the existing function such that it does correctly\n> that \"text  \\   \" case, and also does not use 'strlen' since it anyway\n> moves left to right\n> \n> -- or: use the new suggested implementation (and just brush the\n> formatting a bit), and perhaps borrow 'len' from the calling routine\n> \n> And add tests in any case. What is the preference?\n\nI'd be OK with either, though I have a slight preference for the first,\njust because I find the \"bslash ^= 1\" bit of yours, while clever, a bit\nhard to follow.\n\n-Peff\n"},{"id":"243037","messageId":"xmqqa99z9kbz.fsf@gitster.dls.corp.google.com","threadId":"36777","inReplyTo":"20140530020444.GH28683@sigill.intra.peff.net","subject":"Re: [PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-05-30T18:45:36Z","receivedAt":"2014-05-30T18:45:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Thu, May 29, 2014 at 02:34:33PM -0700, Pasha Bolokhov wrote:\n>\n>> > However, I doubt it makes that much of a difference in practice, so\n>> > unless it's measurable, I would certainly go with the version that is\n>> > more readable (and correct, of course).\n>> \n>>     Sorry, just to recap, you would go with the existing version\n>> (which needs correction), or with the one that is being suggested? (I\n>> agree I can format the style a tiny bit better in the latter one)\n>\n> I actually think the original left-to-right is a little easier to\n> follow, but I do not feel strongly. I mainly meant \"argue based on\n> readability and correctness, do not argue based on speed\".\n\nSensible.\n\n> I'd be OK with either, though I have a slight preference for the first,\n> just because I find the \"bslash ^= 1\" bit of yours, while clever, a bit\n> hard to follow.\n\nFWIW, I think I agree.\n"}]}