{"thread":{"id":"36816","subject":"[PATCH alt-v2] Improve function dir.c:trim_trailing_spaces()","startedAt":"2014-06-02T16:45:51Z","lastAt":"2014-06-02T20:23:41Z","messageCount":2,"participants":["Pasha Bolokhov","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":null},"messages":[{"id":"243137","messageId":"1401727551-8871-1-git-send-email-pasha.bolokhov@gmail.com","threadId":"36816","inReplyTo":null,"subject":"[PATCH alt-v2] Improve function dir.c:trim_trailing_spaces()","fromName":"Pasha Bolokhov","fromEmail":"pasha.bolokhov@gmail.com","sentAt":"2014-06-02T16:45:51Z","receivedAt":"2014-06-02T16:45:51Z","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---\nAfter correcting for the trailing backslash 'text\\', a switch()\nstructure gives better readability than nested 'ifs', the way I see it.\nAdd a test to show that the trailing backslash 'text\\' is handled\ncorrectly\n\n dir.c              | 35 ++++++++++++++++++++---------------\n t/t0008-ignores.sh | 23 +++++++++++++++++++++++\n 2 files changed, 43 insertions(+), 15 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex eb6f581..06483d4 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -508,21 +508,26 @@ 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+\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..4cea858 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  \"\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/trailing 6 \\\\a\\\\\"\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/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\"\t\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":"243155","messageId":"xmqqha433vsi.fsf@gitster.dls.corp.google.com","threadId":"36816","inReplyTo":"1401727551-8871-1-git-send-email-pasha.bolokhov@gmail.com","subject":"Re: [PATCH alt-v2] Improve function dir.c:trim_trailing_spaces()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-06-02T20:23:41Z","receivedAt":"2014-06-02T20:23:41Z","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> After correcting for the trailing backslash 'text\\', a switch()\n> structure gives better readability than nested 'ifs', the way I see it.\n> Add a test to show that the trailing backslash 'text\\' is handled\n> correctly\n>\n>  dir.c              | 35 ++++++++++++++++++++---------------\n>  t/t0008-ignores.sh | 23 +++++++++++++++++++++++\n>  2 files changed, 43 insertions(+), 15 deletions(-)\n>\n> diff --git a/dir.c b/dir.c\n> index eb6f581..06483d4 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -508,21 +508,26 @@ 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> +\n\nWrite\n\n\t\t\t/* fallthru */\n\nhere.\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\nI think the loop structure is a lot simpler to follow (with or\nwithout switch/case) with this change.  Good.\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..4cea858 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  \"\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/trailing 6 \\\\a\\\\\"\t&&\n\nDon't be cute and try to align with tabs.\n\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/trailing 6 \\\\\\\\\\\\\\\\a\\\\\\\\\\\\\\\\\"\t\t>>ignore &&\n> +\techo whitespace/untracked >expect &&\n> +\t: >err.expect &&\n\nOther file truncation is done with \">whitespace/filename\" without an\nexplicit \":\" aka no-op command; I know this was copied from a\nprevious test but perhaps we want to be consistent?\n\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"}]}