{"thread":{"id":"36819","subject":"[PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","startedAt":"2014-06-02T22:36:56Z","lastAt":"2014-06-14T02:45:41Z","messageCount":5,"participants":["Pasha Bolokhov","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":3,"patchTotal":null},"messages":[{"id":"243169","messageId":"1401748616-14632-1-git-send-email-pasha.bolokhov@gmail.com","threadId":"36819","inReplyTo":null,"subject":"[PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-06-02T22:36:56Z","receivedAt":"2014-06-02T22:36:56Z","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---\nAdd /* fallthrough */ comment to switch(), remove TABs in test,\nand remove \":\" command in file truncation in the test\n\n dir.c              | 36 +++++++++++++++++++++---------------\n t/t0008-ignores.sh | 23 +++++++++++++++++++++++\n 2 files changed, 44 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6f581..c7dc846 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -508,21 +508,27 @@ 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\tswitch (*p) {\n+\t\tcase ' ':\n+\t\t\tif (!last_space)\n+\t\t\t\tlast_space = p;\n+\t\t\tbreak;\n+\n+\t\tcase '\\\\':\n+\t\t\tp++;\n+\t\t\tif (!*p)\n+\t\t\t\treturn;\n+\t\t\t/* fallthrough */\n+\n+\t\tdefault:\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..5ef5ad3 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -806,4 +806,27 @@ 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  \" &&\n+\t>\"whitespace/trailing 2 \\\\\\\\\" &&\n+\t>\"whitespace/trailing 3 \\\\\\\\\" &&\n+\t>\"whitespace/trailing 4   \\\\ \" &&\n+\t>\"whitespace/trailing 5 \\\\ \\\\ \" &&\n+\t>\"whitespace/trailing 6 \\\\a\\\\\" &&\n+\t>whitespace/untracked &&\n+\techo \"whitespace/trailing 1 \\\\    \" >ignore  &&\n+\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" >>ignore &&\n+\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \" >>ignore &&\n+\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \" >>ignore &&\n+\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \" >>ignore &&\n+\techo \"whitespace/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>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":"243170","messageId":"xmqqfvjm3ott.fsf@gitster.dls.corp.google.com","threadId":"36819","inReplyTo":"1401748616-14632-1-git-send-email-pasha.bolokhov@gmail.com","subject":"Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-02T22:54:06Z","receivedAt":"2014-06-02T22:54:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pasha Bolokhov <pasha.bolokhov@gmail.com> writes:\n\n> Discard the unnecessary 'nr_spaces' variable, remove 'strlen()' and\n> improve 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 a result.\n> Add a test at the end of t/t0008-ignores.sh to exhibit this behavior\n>\n> Signed-off-by: Pasha Bolokhov <pasha.bolokhov@gmail.com>\n> ---\n> Add /* fallthrough */ comment to switch(), remove TABs in test,\n> and remove \":\" command in file truncation in the test\n\n8ba87adad6 does not seem to do anything to do with this change,\nthough.  Tentatively I've queued the following (but not merged to\nanywhere nor pushed out).\n\nThanks.\n\n-- >8 --\nFrom: Pasha Bolokhov <pasha.bolokhov@gmail.com>\nDate: Mon, 2 Jun 2014 15:36:56 -0700\nSubject: [PATCH] dir.c:trim_trailing_spaces(): fix for \" \\ \" sequence\n\nDiscard the unnecessary 'nr_spaces' variable, remove 'strlen()' and\nimprove the 'if' structure.  Switch to pointers instead of integers\nto control the loop.\n\nSlightly more rare occurrences of 'text  \\    ' with a backslash\nin between spaces are handled correctly.  Namely, the code in\n7e2e4b37 (dir: ignore trailing spaces in exclude patterns, 2014-02-09)\ndoes not reset 'last_space' when a backslash is encountered and the above\nline stays intact as a result.\n\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>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n dir.c              | 34 +++++++++++++++++++---------------\n t/t0008-ignores.sh | 23 +++++++++++++++++++++++\n 2 files changed, 42 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6f581..797805d 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -508,21 +508,25 @@ 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\tswitch (*p) {\n+\t\tcase ' ':\n+\t\t\tif (!last_space)\n+\t\t\t\tlast_space = p;\n+\t\t\tbreak;\n+\t\tcase '\\\\':\n+\t\t\tp++;\n+\t\t\tif (!*p)\n+\t\t\t\treturn;\n+\t\t\t/* fallthrough */\n+\t\tdefault:\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..5ef5ad3 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -806,4 +806,27 @@ 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  \" &&\n+\t>\"whitespace/trailing 2 \\\\\\\\\" &&\n+\t>\"whitespace/trailing 3 \\\\\\\\\" &&\n+\t>\"whitespace/trailing 4   \\\\ \" &&\n+\t>\"whitespace/trailing 5 \\\\ \\\\ \" &&\n+\t>\"whitespace/trailing 6 \\\\a\\\\\" &&\n+\t>whitespace/untracked &&\n+\techo \"whitespace/trailing 1 \\\\    \" >ignore  &&\n+\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" >>ignore &&\n+\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \" >>ignore &&\n+\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \" >>ignore &&\n+\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \" >>ignore &&\n+\techo \"whitespace/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>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-- \n2.0.0-511-g1433423\n"},{"id":"244170","messageId":"xmqqoaxw1ru4.fsf@gitster.dls.corp.google.com","threadId":"36819","inReplyTo":"1401748616-14632-1-git-send-email-pasha.bolokhov@gmail.com","subject":"Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-13T20:23:47Z","receivedAt":"2014-06-13T20:23:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Pasha Bolokhov <pasha.bolokhov@gmail.com> writes:\n\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index 63beb99..5ef5ad3 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -806,4 +806,27 @@ 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  \" &&\n> +\t>\"whitespace/trailing 2 \\\\\\\\\" &&\n> +\t>\"whitespace/trailing 3 \\\\\\\\\" &&\n> +\t>\"whitespace/trailing 4   \\\\ \" &&\n> +\t>\"whitespace/trailing 5 \\\\ \\\\ \" &&\n> +\t>\"whitespace/trailing 6 \\\\a\\\\\" &&\n> +\t>whitespace/untracked &&\n> +\techo \"whitespace/trailing 1 \\\\    \" >ignore  &&\n> +\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" >>ignore &&\n> +\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \" >>ignore &&\n> +\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \" >>ignore &&\n> +\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \" >>ignore &&\n> +\techo \"whitespace/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>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\nThis passes with your shell set to dash but fails with bash.\n\nLet's fix it up like so.\n\n t/t0008-ignores.sh | 14 ++++++++------\n 1 file changed, 8 insertions(+), 6 deletions(-)\n\ndiff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\nindex 5ef5ad3..39e55a1 100755\n--- a/t/t0008-ignores.sh\n+++ b/t/t0008-ignores.sh\n@@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' '\n \t>\"whitespace/trailing 5 \\\\ \\\\ \" &&\n \t>\"whitespace/trailing 6 \\\\a\\\\\" &&\n \t>whitespace/untracked &&\n-\techo \"whitespace/trailing 1 \\\\    \" >ignore  &&\n-\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" >>ignore &&\n-\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \" >>ignore &&\n-\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \" >>ignore &&\n-\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \" >>ignore &&\n-\techo \"whitespace/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>ignore &&\n+\tsed -e \"s/Z$//\" >ignore <<-\\EOF &&\n+\twhitespace/trailing 1 \\    Z\n+\twhitespace/trailing 2 \\\\\\\\Z\n+\twhitespace/trailing 3 \\\\\\\\ Z\n+\twhitespace/trailing 4   \\\\\\    Z\n+\twhitespace/trailing 5 \\\\ \\\\\\   Z\n+\twhitespace/trailing 6 \\\\a\\\\Z\n+\tEOF\n \techo whitespace/untracked >expect &&\n \t>err.expect &&\n \tgit ls-files -o -X ignore whitespace >actual 2>err &&\n"},{"id":"244185","messageId":"20140613232537.GB23078@sigill","threadId":"36819","inReplyTo":"xmqqoaxw1ru4.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2014-06-13T23:25:38Z","receivedAt":"2014-06-13T23:25:38Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Jun 13, 2014 at 01:23:47PM -0700, Junio C Hamano wrote:\n\n> This passes with your shell set to dash but fails with bash.\n> \n> Let's fix it up like so.\n\nThanks, I should have been more suspicious of the combination of backslashes\nand echo during review. :)\n\n> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n> index 5ef5ad3..39e55a1 100755\n> --- a/t/t0008-ignores.sh\n> +++ b/t/t0008-ignores.sh\n> @@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' '\n>  \t>\"whitespace/trailing 5 \\\\ \\\\ \" &&\n>  \t>\"whitespace/trailing 6 \\\\a\\\\\" &&\n>  \t>whitespace/untracked &&\n> -\techo \"whitespace/trailing 1 \\\\    \" >ignore  &&\n> -\techo \"whitespace/trailing 2 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\" >>ignore &&\n> -\techo \"whitespace/trailing 3 \\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\\ \" >>ignore &&\n> -\techo \"whitespace/trailing 4   \\\\\\\\\\\\\\\\\\\\\\\\    \" >>ignore &&\n> -\techo \"whitespace/trailing 5 \\\\\\\\\\\\\\\\ \\\\\\\\\\\\\\\\\\\\\\\\   \" >>ignore &&\n> -\techo \"whitespace/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>ignore &&\n> +\tsed -e \"s/Z$//\" >ignore <<-\\EOF &&\n> +\twhitespace/trailing 1 \\    Z\n> +\twhitespace/trailing 2 \\\\\\\\Z\n> +\twhitespace/trailing 3 \\\\\\\\ Z\n> +\twhitespace/trailing 4   \\\\\\    Z\n> +\twhitespace/trailing 5 \\\\ \\\\\\   Z\n> +\twhitespace/trailing 6 \\\\a\\\\Z\n> +\tEOF\n\nThe end result is much more readable, too, IMHO.\n\n-Peff\n"},{"id":"244189","messageId":"CAKpPgvcwV_Zbd1W0gAGcy_A-bE6XZagXD-u4aMwOqhXMktUgWQ@mail.gmail.com","threadId":"36819","inReplyTo":"20140613232537.GB23078@sigill","subject":"Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-06-14T02:45:41Z","receivedAt":"2014-06-14T02:45:41Z","isPatch":true,"sender":{"key":"pasha.bolokhov@gmail.com","avatar":null},"body":"Thanks for the fix. I knew these backslashes could cause trouble. No\ndoubt the fixed version reads better than a hundred backslashes\n\n\nOn Fri, Jun 13, 2014 at 4:25 PM, Jeff King <peff@peff.net> wrote:\n> On Fri, Jun 13, 2014 at 01:23:47PM -0700, Junio C Hamano wrote:\n>\n>> This passes with your shell set to dash but fails with bash.\n>>\n>> Let's fix it up like so.\n>\n> Thanks, I should have been more suspicious of the combination of backslashes\n> and echo during review. :)\n>\n>> diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh\n>> index 5ef5ad3..39e55a1 100755\n>> --- a/t/t0008-ignores.sh\n>> +++ b/t/t0008-ignores.sh\n>> @@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' '\n>>       >\"whitespace/trailing 5 \\\\ \\\\ \" &&\n>>       >\"whitespace/trailing 6 \\\\a\\\\\" &&\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/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\" >>ignore &&\n>> +     sed -e \"s/Z$//\" >ignore <<-\\EOF &&\n>> +     whitespace/trailing 1 \\    Z\n>> +     whitespace/trailing 2 \\\\\\\\Z\n>> +     whitespace/trailing 3 \\\\\\\\ Z\n>> +     whitespace/trailing 4   \\\\\\    Z\n>> +     whitespace/trailing 5 \\\\ \\\\\\   Z\n>> +     whitespace/trailing 6 \\\\a\\\\Z\n>> +     EOF\n>\n> The end result is much more readable, too, IMHO.\n>\n> -Peff\n"}]}