threads / patch / 36819

v3Improve function dir.c:trim_trailing_spaces()

Subject: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()

## tl;dr

5 messages between Jun 2, 2014 and Jun 14, 2014. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Pasha Bolokhov· Jun 2, 2014, 22:36 UTC · lore

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.bolokhov@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(-)
Show changes to 2 files +44 −15

dir.c, t/t0008-ignores.sh

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 2>err &&
+	test_cmp expect actual &&
+	test_cmp err.expect err
+'
+
 test_done
-- 
1.9.1
Junio C Hamano· Jun 2, 2014, 22:54 UTC · re: Pasha Bolokhov · lore

Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()

Pasha Bolokhov <pasha.bolokhov@gmail.com> writes:
Show 13 quoted lines
> 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.bolokhov@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.bolokhov@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.bolokhov@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 dir.c              | 34 +++++++++++++++++++---------------
 t/t0008-ignores.sh | 23 +++++++++++++++++++++++
 2 files changed, 42 insertions(+), 15 deletions(-)
Show changes to 2 files +42 −15

dir.c, t/t0008-ignores.sh

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 2>err &&
+	test_cmp expect actual &&
+	test_cmp err.expect err
+'
+
 test_done
-- 
2.0.0-511-g1433423
Junio C Hamano· Jun 13, 2014, 20:23 UTC · re: Pasha Bolokhov · lore

Re: [PATCH alt-v3] Improve function dir.c:trim_trailing_spaces()

Pasha Bolokhov <pasha.bolokhov@gmail.com> writes:
Show 30 quoted lines
> 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 2>err &&
> +	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(-)
Show changes to t/t0008-ignores.sh +8 −6
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 2>err &&
Jeff King· Jun 13, 2014, 23:25 UTC · re: Junio C Hamano · lore

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. :)

Show 22 quoted lines
> 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
Pasha Bolokhov· Jun 14, 2014, 02:45 UTC · re: Jeff King · lore

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 <peff@peff.net> wrote:
Show 35 quoted lines
> 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

← back to recent threads