{"thread":{"id":"36797","subject":"[PATCH alt-v1] Improve function dir.c:trim_trailing_spaces()","startedAt":"2014-05-30T20:20:33Z","lastAt":"2014-05-31T01:12:09Z","messageCount":2,"participants":["Pasha Bolokhov","Duy Nguyen"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"243047","messageId":"1401481233-14500-1-git-send-email-pasha.bolokhov@gmail.com","threadId":"36797","inReplyTo":null,"subject":"[PATCH alt-v1] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-05-30T20:20:33Z","receivedAt":"2014-05-30T20:20:33Z","isPatch":true,"sender":{"key":"pasha.bolokhov@gmail.com","avatar":null},"body":"Discard unnecessary 'nr_spaces' variable and improve\nthe '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\na result. Add a test at the end of t/t0008-ignores.sh to\nexhibit this behavior\n\nSigned-off-by: Pasha Bolokhov <pasha.bolokhov@gmail.com>\n---\nInstead of the \"optimized\" version, which had complaints about\nreasonability, I am attaching the \"correction\" of the original version.\n'last_space' was not reset after encountering a backslash, and\n'nr_spaces' is an unnecessary variable.\n\nI myself am still leaning towards the optimized version (which scans\nbackwards instead of forward), since it completely ignores strings\nwhich do not have spaces at the end, while these forward implementations\n(both the one being attached and the original one) always scan\nall strings through. Again, I'm not attaching the optimized one\nbecause its readability is indeed less\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..e67dcc2 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 == NULL)\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 != NULL)\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":"243068","messageId":"CACsJy8DAgBms0=8JMfM_67DdURrKUXyLRYA-7v9rws2shyLPnw@mail.gmail.com","threadId":"36797","inReplyTo":"1401481233-14500-1-git-send-email-pasha.bolokhov@gmail.com","subject":"Re: [PATCH alt-v1] Improve function dir.c:trim_trailing_spaces()","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2014-05-31T01:12:09Z","receivedAt":"2014-05-31T01:12:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Sat, May 31, 2014 at 3:20 AM, Pasha Bolokhov\n<pasha.bolokhov@gmail.com> wrote:\n> Discard unnecessary 'nr_spaces' variable and improve\n> the 'if' structure. Switch to pointers instead of integers\n>\n> Slightly more rare occurrences of 'text  \\    ' with a backslash\n> in between spaces are handled correctly. Namely, the code in\n> 8ba87adad6 does not reset 'last_space' when a backslash is\n> encountered and the above line stays intact as\n> a result. Add a test at the end of t/t0008-ignores.sh to\n> exhibit this behavior\n\nDefinitely easier to read than the other version. Nit, we usually\nstrip \"!= NULL\" and \"== NULL\" out of the conditional expressions.\n\n>\n> Signed-off-by: Pasha Bolokhov <pasha.bolokhov@gmail.com>\n> ---\n> Instead of the \"optimized\" version, which had complaints about\n> reasonability, I am attaching the \"correction\" of the original version.\n> 'last_space' was not reset after encountering a backslash, and\n> 'nr_spaces' is an unnecessary variable.\n>\n> I myself am still leaning towards the optimized version (which scans\n> backwards instead of forward), since it completely ignores strings\n> which do not have spaces at the end, while these forward implementations\n> (both the one being attached and the original one) always scan\n> all strings through. Again, I'm not attaching the optimized one\n> because its readability is indeed less\n>\n>  dir.c              | 29 ++++++++++++++---------------\n>  t/t0008-ignores.sh | 21 +++++++++++++++++++++\n>  2 files changed, 35 insertions(+), 15 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index eb6f581..e67dcc2 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> -       int i, last_space = -1, nr_spaces, len = strlen(buf);\n> -       for (i = 0; i < len; i++)\n> -               if (buf[i] == '\\\\')\n> -                       i++;\n> -               else if (buf[i] == ' ') {\n> -                       if (last_space == -1) {\n> -                               last_space = i;\n> -                               nr_spaces = 1;\n> -                       } else\n> -                               nr_spaces++;\n> -               } else\n> -                       last_space = -1;\n> -\n> -       if (last_space != -1 && last_space + nr_spaces == len)\n> -               buf[last_space] = '\\0';\n> +       char *p, *last_space = NULL;\n> +\n> +       for (p = buf; *p; p++)\n> +               if (*p == ' ') {\n> +                       if (last_space == NULL)\n> +                               last_space = p;\n> +               } else {\n> +                       if (*p == '\\\\')\n> +                               p++;\n> +                       last_space = NULL;\n> +               }\n> +\n> +       if (last_space != NULL)\n> +               *last_space = '\\0';\n>  }\n>\n>  int add_excludes_from_file_to_list(const char *fname,\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index 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>         test_cmp err.expect err\n>  '\n>\n> +test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' '\n> +       rm -rf whitespace &&\n> +       mkdir whitespace &&\n> +       >\"whitespace/trailing 1  \"      &&\n> +       >\"whitespace/trailing 2 \\\\\\\\\"   &&\n> +       >\"whitespace/trailing 3 \\\\\\\\\"   &&\n> +       >\"whitespace/trailing 4   \\\\ \"  &&\n> +       >\"whitespace/trailing 5 \\\\ \\\\ \" &&\n> +       >whitespace/untracked &&\n> +       echo \"whitespace/trailing 1 \\\\    \"                     >ignore  &&\n> +       echo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\"           >>ignore &&\n> +       echo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \"          >>ignore &&\n> +       echo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \"         >>ignore &&\n> +       echo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \"   >>ignore &&\n> +       echo whitespace/untracked >expect &&\n> +       : >err.expect &&\n> +       git ls-files -o -X ignore whitespace >actual 2>err &&\n> +       test_cmp expect actual &&\n> +       test_cmp err.expect err\n> +'\n> +\n>  test_done\n> --\n> 1.9.1\n>\n\n\n\n-- \nDuy\n"}]}