{"thread":{"id":"36802","subject":"[PATCH] Improve function dir.c:trim_trailing_spaces()","startedAt":"2014-05-31T15:21:31Z","lastAt":"2014-06-02T06:47:06Z","messageCount":2,"participants":["Pasha Bolokhov","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243079","messageId":"1401549691-20788-1-git-send-email-pasha.bolokhov@gmail.com","threadId":"36802","inReplyTo":null,"subject":"[PATCH] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-05-31T15:21:31Z","receivedAt":"2014-05-31T15:21:31Z","isPatch":true,"sender":{"key":"pasha.bolokhov@gmail.com","avatar":null},"body":"Discard the unnecessary 'nr_spaces' variable, remove 'strlen()' and\nimprove the 'if' structure. Switch to pointers instead of integers\n\nSlightly more rare occurrences of 'text  \\    ' with a backslash\nin between spaces are handled correctly. Namely, the code in\n8ba87adad6 does not reset 'last_space' when a backslash is\nencountered and the above line stays intact as a result.\nAdd a test at the end of t/t0008-ignores.sh to exhibit this behavior\n\nSigned-off-by: Pasha Bolokhov <pasha.bolokhov@gmail.com>\n---\nCorrect 'if()' statements to conform to the general style\nwhich implies using 'if(ptr)' as an equivalent of 'if(ptr != NULL)'\n\n dir.c              | 29 ++++++++++++++---------------\n t/t0008-ignores.sh | 21 +++++++++++++++++++++\n 2 files changed, 35 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6f581..98c81a8 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -508,21 +508,20 @@ 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-\t\tbuf[last_space] = '\\0';\n+\tchar *p, *last_space = NULL;\n+\n+\tfor (p = buf; *p; p++)\n+\t\tif (*p == ' ') {\n+\t\t\tif (!last_space)\n+\t\t\t\tlast_space = p;\n+\t\t} else {\n+\t\t\tif (*p == '\\\\')\n+\t\t\t\tp++;\n+\t\t\tlast_space = NULL;\n+\t\t}\n+\n+\tif (last_space)\n+\t\t*last_space = '\\0';\n }\n \n int add_excludes_from_file_to_list(const char *fname,\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 63beb99..7becf96 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -806,4 +806,25 @@ test_expect_success !MINGW 'quoting allows trailing whitespace' '\n \ttest_cmp err.expect err\n '\n \n+test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' '\n+\trm -rf whitespace &&\n+\tmkdir whitespace &&\n+\t>\"whitespace/trailing 1  \"\t&&\n+\t>\"whitespace/trailing 2 \\\\\\\\\"\t&&\n+\t>\"whitespace/trailing 3 \\\\\\\\\"\t&&\n+\t>\"whitespace/trailing 4   \\\\ \"\t&&\n+\t>\"whitespace/trailing 5 \\\\ \\\\ \"\t&&\n+\t>whitespace/untracked &&\n+\techo \"whitespace/trailing 1 \\\\    \"\t\t\t>ignore  &&\n+\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\"\t\t>>ignore &&\n+\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \"\t\t>>ignore &&\n+\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \"\t\t>>ignore &&\n+\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \"\t>>ignore &&\n+\techo whitespace/untracked >expect &&\n+\t: >err.expect &&\n+\tgit ls-files -o -X ignore whitespace >actual 2>err &&\n+\ttest_cmp expect actual &&\n+\ttest_cmp err.expect err\n+'\n+\n test_done\n-- \n1.9.1\n"},{"id":"243120","messageId":"20140602064706.GB27254@sigill.intra.peff.net","threadId":"36802","inReplyTo":"1401549691-20788-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-06-02T06:47:06Z","receivedAt":"2014-06-02T06:47:06Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, May 31, 2014 at 08:21:31AM -0700, Pasha Bolokhov wrote:\n\n> +\tchar *p, *last_space = NULL;\n> +\n> +\tfor (p = buf; *p; p++)\n> +\t\tif (*p == ' ') {\n> +\t\t\tif (!last_space)\n> +\t\t\t\tlast_space = p;\n> +\t\t} else {\n> +\t\t\tif (*p == '\\\\')\n> +\t\t\t\tp++;\n> +\t\t\tlast_space = NULL;\n> +\t\t}\n\nYour backslash-escape works here by incrementing \"p\" an extra time. So\nwe move past the backslash to the next character (which is escaped), and\nthen the for-loop increments it again to the character beyond that,\nwhich is the next one worth considering.\n\nWhat happens if we are parsing a string with an unmatched backslash at\nthe end of the string, like:\n\n  foo\\\n\nWe consider the end-of-string NUL to be escaped, skip it, and then keep\nreading whatever random bytes are in memory after the string.\n\nThe original version did not have a problem with this because it used\na length, which meant that  \"i < len\" caught this case.\n\nI think you either need to insert an extra check for \"!p[1]\" when moving\npast the escaped character, or move back to a length-based check.\n\n-Peff\n"}]}