Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()
Pasha Bolokhov pasha.bolok...@gmail.com writes: diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 63beb99..5ef5ad3 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 +' This passes with your shell set to dash but fails with bash. Let's fix it up like so. t/t0008-ignores.sh | 14 -- 1 file changed, 8 insertions(+), 6 deletions(-) diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 5ef5ad3..39e55a1 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' ' 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 + sed -e s/Z$// ignore -\EOF + whitespace/trailing 1 \Z + whitespace/trailing 2 Z + whitespace/trailing 3 Z + whitespace/trailing 4 \\\Z + whitespace/trailing 5 \\ \\\ Z + whitespace/trailing 6 \\a\\Z + EOF echo whitespace/untracked expect err.expect git ls-files -o -X ignore whitespace actual 2err -- 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-v3] Improve function dir.c:trim_trailing_spaces()
On Fri, Jun 13, 2014 at 01:23:47PM -0700, Junio C Hamano wrote: This passes with your shell set to dash but fails with bash. Let's fix it up like so. Thanks, I should have been more suspicious of the combination of backslashes and echo during review. :) diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 5ef5ad3..39e55a1 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' ' 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 + sed -e s/Z$// ignore -\EOF + whitespace/trailing 1 \Z + whitespace/trailing 2 Z + whitespace/trailing 3 Z + whitespace/trailing 4 \\\Z + whitespace/trailing 5 \\ \\\ Z + whitespace/trailing 6 \\a\\Z + EOF The end result is much more readable, too, IMHO. -Peff -- 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-v3] Improve function dir.c:trim_trailing_spaces()
Thanks for the fix. I knew these backslashes could cause trouble. No doubt the fixed version reads better than a hundred backslashes On Fri, Jun 13, 2014 at 4:25 PM, Jeff King p...@peff.net wrote: On Fri, Jun 13, 2014 at 01:23:47PM -0700, Junio C Hamano wrote: This passes with your shell set to dash but fails with bash. Let's fix it up like so. Thanks, I should have been more suspicious of the combination of backslashes and echo during review. :) diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh index 5ef5ad3..39e55a1 100755 --- a/t/t0008-ignores.sh +++ b/t/t0008-ignores.sh @@ -816,12 +816,14 @@ test_expect_success NOT_MINGW,NOT_CYGWIN 'correct handling of backslashes' ' 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 + sed -e s/Z$// ignore -\EOF + whitespace/trailing 1 \Z + whitespace/trailing 2 Z + whitespace/trailing 3 Z + whitespace/trailing 4 \\\Z + whitespace/trailing 5 \\ \\\ Z + whitespace/trailing 6 \\a\\Z + EOF The end result is much more readable, too, IMHO. -Peff -- 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
[PATCH alt-v3] 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
---
Add /* fallthrough */ comment to switch(), remove TABs in test,
and remove : command in file truncation in the test
dir.c | 36 +---
t/t0008-ignores.sh | 23 +++
2 files changed, 44 insertions(+), 15 deletions(-)
diff --git a/dir.c b/dir.c
index eb6f581..c7dc846 100644
--- a/dir.c
+++ b/dir.c
@@ -508,21 +508,27 @@ 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;
+ /* fallthrough */
+
+ 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..5ef5ad3 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-v3] 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
---
Add /* fallthrough */ comment to switch(), remove TABs in test,
and remove : command in file truncation in the test
8ba87adad6 does not seem to do anything to do with this change,
though. Tentatively I've queued the following (but not merged to
anywhere nor pushed out).
Thanks.
-- 8 --
From: Pasha Bolokhov pasha.bolok...@gmail.com
Date: Mon, 2 Jun 2014 15:36:56 -0700
Subject: [PATCH] dir.c:trim_trailing_spaces(): fix for \ sequence
Discard the unnecessary 'nr_spaces' variable, remove 'strlen()' and
improve the 'if' structure. Switch to pointers instead of integers
to control the loop.
Slightly more rare occurrences of 'text \' with a backslash
in between spaces are handled correctly. Namely, the code in
7e2e4b37 (dir: ignore trailing spaces in exclude patterns, 2014-02-09)
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
Signed-off-by: Junio C Hamano gits...@pobox.com
---
dir.c | 34 +++---
t/t0008-ignores.sh | 23 +++
2 files changed, 42 insertions(+), 15 deletions(-)
diff --git a/dir.c b/dir.c
index eb6f581..797805d 100644
--- a/dir.c
+++ b/dir.c
@@ -508,21 +508,25 @@ 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;
+ /* fallthrough */
+ 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..5ef5ad3 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
--
2.0.0-511-g1433423
--
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
