[PATCH alt-v2] Improve function dir.c:trim_trailing_spaces()
Discard the unnecessary 'nr_spaces' variable, remove 'strlen()' and improve the 'if' structure. Switch to pointers instead of integers Slightly more rare occurrences of 'text \' with a backslash in between spaces are handled correctly. Namely, the code in 8ba87adad6 does not reset 'last_space' when a backslash is encountered and the above line stays intact as a result. Add a test at the end of t/t0008-ignores.sh to exhibit this behavior Signed-off-by: Pasha Bolokhov pasha.bolok...@gmail.com --- After correcting for the trailing backslash 'text\', a switch() structure gives better readability than nested 'ifs', the way I see it. Add a test to show that the trailing backslash 'text\' is handled correctly dir.c | 35 --- t/t0008-ignores.sh | 23 +++ 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/dir.c b/dir.c index eb6f581..06483d4 100644 --- a/dir.c +++ b/dir.c @@ -508,21 +508,26 @@ void clear_exclude_list(struct exclude_list *el) static void trim_trailing_spaces(char *buf) { - int i, last_space = -1, nr_spaces, len = strlen(buf); - for (i = 0; i len; i++) - if (buf[i] == '\\') - i++; - else if (buf[i] == ' ') { - if (last_space == -1) { - last_space = i; - nr_spaces = 1; - } else - nr_spaces++; - } else - last_space = -1; - - if (last_space != -1 last_space + nr_spaces == len) - buf[last_space] = '\0'; + char *p, *last_space = NULL; + + for (p = buf; *p; p++) + switch (*p) { + case ' ': + if (!last_space) + last_space = p; + break; + + case '\\': + p++; + if (!*p) + return; + + default: + last_space = NULL; + } + + if (last_space) + *last_space = '\0'; } int add_excludes_from_file_to_list(const char *fname, diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 63beb99..4cea858 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -806,4 +806,27 @@ test_expect_success !MINGW 'quoting allows trailing whitespace' ' test_cmp err.expect err ' +test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' ' + rm -rf whitespace + mkdir whitespace + whitespace/trailing 1 + whitespace/trailing 2 + whitespace/trailing 3 + whitespace/trailing 4 \\ + whitespace/trailing 5 \\ \\ + whitespace/trailing 6 \\a\\ + whitespace/untracked + echo whitespace/trailing 1 \\ ignore + echo whitespace/trailing 2 ignore + echo whitespace/trailing 3 ignore + echo whitespace/trailing 4 ignore + echo whitespace/trailing 5 ignore + echo whitespace/trailing 6 a ignore + echo whitespace/untracked expect + : err.expect + git ls-files -o -X ignore whitespace actual 2err + test_cmp expect actual + test_cmp err.expect err +' + test_done -- 1.9.1 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH alt-v2] Improve function dir.c:trim_trailing_spaces()
Pasha Bolokhov pasha.bolok...@gmail.com writes: Discard the unnecessary 'nr_spaces' variable, remove 'strlen()' and improve the 'if' structure. Switch to pointers instead of integers Slightly more rare occurrences of 'text \' with a backslash in between spaces are handled correctly. Namely, the code in 8ba87adad6 does not reset 'last_space' when a backslash is encountered and the above line stays intact as a result. Add a test at the end of t/t0008-ignores.sh to exhibit this behavior Signed-off-by: Pasha Bolokhov pasha.bolok...@gmail.com --- After correcting for the trailing backslash 'text\', a switch() structure gives better readability than nested 'ifs', the way I see it. Add a test to show that the trailing backslash 'text\' is handled correctly dir.c | 35 --- t/t0008-ignores.sh | 23 +++ 2 files changed, 43 insertions(+), 15 deletions(-) diff --git a/dir.c b/dir.c index eb6f581..06483d4 100644 --- a/dir.c +++ b/dir.c @@ -508,21 +508,26 @@ void clear_exclude_list(struct exclude_list *el) static void trim_trailing_spaces(char *buf) { - int i, last_space = -1, nr_spaces, len = strlen(buf); - for (i = 0; i len; i++) - if (buf[i] == '\\') - i++; - else if (buf[i] == ' ') { - if (last_space == -1) { - last_space = i; - nr_spaces = 1; - } else - nr_spaces++; - } else - last_space = -1; - - if (last_space != -1 last_space + nr_spaces == len) - buf[last_space] = '\0'; + char *p, *last_space = NULL; + + for (p = buf; *p; p++) + switch (*p) { + case ' ': + if (!last_space) + last_space = p; + break; + + case '\\': + p++; + if (!*p) + return; + Write /* fallthru */ here. + default: + last_space = NULL; + } + + if (last_space) + *last_space = '\0'; } I think the loop structure is a lot simpler to follow (with or without switch/case) with this change. Good. int add_excludes_from_file_to_list(const char *fname, diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 63beb99..4cea858 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -806,4 +806,27 @@ test_expect_success !MINGW 'quoting allows trailing whitespace' ' test_cmp err.expect err ' +test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' ' + rm -rf whitespace + mkdir whitespace + whitespace/trailing 1 + whitespace/trailing 2 + whitespace/trailing 3 + whitespace/trailing 4 \\ + whitespace/trailing 5 \\ \\ + whitespace/trailing 6 \\a\\ Don't be cute and try to align with tabs. + whitespace/untracked + echo whitespace/trailing 1 \\ ignore + echo whitespace/trailing 2 ignore + echo whitespace/trailing 3 ignore + echo whitespace/trailing 4 ignore + echo whitespace/trailing 5 ignore + echo whitespace/trailing 6 a ignore + echo whitespace/untracked expect + : err.expect Other file truncation is done with whitespace/filename without an explicit : aka no-op command; I know this was copied from a previous test but perhaps we want to be consistent? + git ls-files -o -X ignore whitespace actual 2err + test_cmp expect actual + test_cmp err.expect err +' + test_done -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html