[PATCH 0/2] Ignore trailing spaces in .gitignore

DORMANTno replies

Revision v1 of 2 in this series.

10 messages, 3 authors, 2016-06-15 · open the first message on its own page

[PATCH 0/2] Ignore trailing spaces in .gitignore

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

Trailing spaces are invisible in most standard editors (*). "git diff"
does show trailing spaces by default. But that does not help newly
written .gitignore files. And trailing spaces are the source of
frustration when writing .gitignore.

So let's ignore them. Nobody sane would put a trailing space in file
names. But we could be careful and do it in two steps: warn first,
then ignore trailing spaces. Another option is merge two patches in
one and be done with it.

People can still quote trailing spaces, which will not be ignored (and
much more visible). Quoting comes with a cost of doing fnmatch(). But
I won't optimize it until I see someone shows me they have a use case
for it.

(*) either that or my coworkers keep pissing me off on purpose when
    they never clean trailing spaces.

Nguyễn Thái Ngọc Duy (2):
  dir: warn about trailing spaces in exclude pattern
  dir: ignore trailing spaces in exclude patterns

 Documentation/gitignore.txt | 3 +++
 dir.c                       | 9 +++++++++
 2 files changed, 12 insertions(+)

-- 
1.8.5.2.240.g8478abd

[PATCH 1/2] dir: warn about trailing spaces in exclude pattern

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 dir.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dir.c b/dir.c
index b35b633..9edde44 100644
--- a/dir.c
+++ b/dir.c

@@ -491,6 +491,16 @@ void clear_exclude_list(struct exclude_list *el)
 	el->filebuf = NULL;
 }
 
+static void check_trailing_spaces(const char *fname, char *buf)
+{
+	int len = strlen(buf);
+	while (len && buf[len - 1] == ' ')
+		len--;
+	if (buf[len] != '\0')
+		warning(_("%s: trailing spaces in '%s'. Please quote them."),
+			fname, buf);
+}
+
 int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,

@@ -542,6 +552,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
+				check_trailing_spaces(fname, entry);
 				add_exclude(entry, base, baselen, el, lineno);
 			}
 			lineno++;

-- 
1.8.5.2.240.g8478abd

[PATCH 2/2] dir: ignore trailing spaces in exclude patterns

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Documentation/gitignore.txt | 3 +++
 dir.c                       | 4 +---
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index b08d34d..8734c15 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt

@@ -77,6 +77,9 @@ PATTERN FORMAT
    Put a backslash ("`\`") in front of the first hash for patterns
    that begin with a hash.
 
+ - Trailing spaces are ignored unless they are quoted with backlash
+   ("`\`").
+
  - An optional prefix "`!`" which negates the pattern; any
    matching file excluded by a previous pattern will become
    included again. It is not possible to re-include a file if a parent

diff --git a/dir.c b/dir.c
index 9edde44..7dee5ca 100644
--- a/dir.c
+++ b/dir.c

@@ -496,9 +496,7 @@ static void check_trailing_spaces(const char *fname, char *buf)
 	int len = strlen(buf);
 	while (len && buf[len - 1] == ' ')
 		len--;
-	if (buf[len] != '\0')
-		warning(_("%s: trailing spaces in '%s'. Please quote them."),
-			fname, buf);
+	buf[len] = '\0';
 }
 
 int add_excludes_from_file_to_list(const char *fname,

-- 
1.8.5.2.240.g8478abd

Re: [PATCH 1/2] dir: warn about trailing spaces in exclude pattern

From: Torsten Bögershausen <hidden>
Date: 2016-06-15 22:59:52

On 2014-02-08 09.10, Nguyễn Thái Ngọc Duy wrote:

quoted hunk

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 dir.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/dir.c b/dir.c
index b35b633..9edde44 100644
--- a/dir.c
+++ b/dir.c

@@ -491,6 +491,16 @@ void clear_exclude_list(struct exclude_list *el)
 	el->filebuf = NULL;
 }
 
+static void check_trailing_spaces(const char *fname, char *buf)
+{
+	int len = strlen(buf);
+	while (len && buf[len - 1] == ' ')
+		len--;
+	if (buf[len] != '\0')

Do we need the while loop here, (when we only warn) ?

+		warning(_("%s: trailing spaces in '%s'. Please quote them."),
+			fname, buf);
+}

This is nice. However we can hint the user that there are 2 choices: 
		warning(_("%s: trailing spaces in '%s'. Please remove them or quote them."),

quoted hunk

+
 int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,

@@ -542,6 +552,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
+				check_trailing_spaces(fname, entry);
 				add_exclude(entry, base, baselen, el, lineno);
 			}
 			lineno++;

Re: [PATCH 0/2] Ignore trailing spaces in .gitignore

From: Jeff King <hidden>
Date: 2016-06-15 22:59:52

On Sat, Feb 08, 2014 at 03:10:02PM +0700, Nguyễn Thái Ngọc Duy wrote:

Trailing spaces are invisible in most standard editors (*). "git diff"
does show trailing spaces by default. But that does not help newly
written .gitignore files. And trailing spaces are the source of
frustration when writing .gitignore.

I guess leading spaces are not as frustrating, but I wonder if it would
be more consistent to say that we soak up all whitespace. That is also a
regression, but I agree that these are exceptional cases, and as long as
we provide an "out" for people to cover them via quoting, ignoring the
whitespace seems like a sane default.

People can still quote trailing spaces, which will not be ignored (and
much more visible). Quoting comes with a cost of doing fnmatch(). But
I won't optimize it until I see someone shows me they have a use case
for it.

I naively expected that:

  echo 'trailing\ \ ' >.gitignore

would count as quoting, but your patch doesn't handle that. It _does_
seem to work currently (that is, the backslashes go away and we treat it
literally), but I am not sure if that is planned, or simply happens to
work.

I guess by quoting you meant:

  echo '"trailing  "' >.gitignore

Anyway, here are some tests I wrote up while playing with this. They do
not pass with your current patch for the reasons above, but maybe they
will be useful to squash in (either after tweaking the tests, or
tweaking the patch).

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index b4d98e6..4dde314 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh

@@ -775,4 +775,33 @@ test_expect_success PIPE 'streaming support for --stdin' '
 	echo "$response" | grep "^::	two"
 '
 
+############################################################################
+#
+# test whitespace handling
+
+test_expect_success 'leading/trailing whitespace is ignored' '
+	mkdir whitespace &&
+	>whitespace/leading &&
+	>whitespace/trailing &&
+	>whitespace/untracked &&
+	{
+		echo "    whitespace/leading" &&
+		echo "whitespace/trailing   "
+	} >ignore &&
+	echo whitespace/untracked >expect &&
+	git ls-files -o -X ignore whitespace >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'quoting allows trailing whitespace' '
+	rm -rf whitespace &&
+	mkdir whitespace &&
+	>"whitespace/trailing  " &&
+	>whitespace/untracked &&
+	echo "whitespace/trailing\\ \\ " >ignore &&
+	echo whitespace/untracked >expect &&
+	git ls-files -o -X ignore whitespace >actual &&
+	test_cmp expect actual
+'
+
 test_done

Re: [PATCH 0/2] Ignore trailing spaces in .gitignore

From: Duy Nguyen <hidden>
Date: 2016-06-15 22:59:52

On Sat, Feb 8, 2014 at 11:45 PM, Jeff King [off-list ref] wrote:

On Sat, Feb 08, 2014 at 03:10:02PM +0700, Nguyễn Thái Ngọc Duy wrote:

quoted

Trailing spaces are invisible in most standard editors (*). "git diff"
does show trailing spaces by default. But that does not help newly
written .gitignore files. And trailing spaces are the source of
frustration when writing .gitignore.

I guess leading spaces are not as frustrating, but I wonder if it would
be more consistent to say that we soak up all whitespace. That is also a
regression, but I agree that these are exceptional cases, and as long as
we provide an "out" for people to cover them via quoting, ignoring the
whitespace seems like a sane default.

Hm...

quoted

People can still quote trailing spaces, which will not be ignored (and
much more visible). Quoting comes with a cost of doing fnmatch(). But
I won't optimize it until I see someone shows me they have a use case
for it.

I naively expected that:

  echo 'trailing\ \ ' >.gitignore

would count as quoting, but your patch doesn't handle that. It _does_
seem to work currently (that is, the backslashes go away and we treat it
literally), but I am not sure if that is planned, or simply happens to
work.

No that's what I had in mind. But yeah my patches are flawed.

I guess by quoting you meant:

  echo '"trailing  "' >.gitignore

This makes " special. If we follow shell convention then things
between ".." should be literal (e.g. "*" is no longer a wildcard). We
don't support it yet. So I rather go with backslash as it adds less
code.

quoted hunk

Anyway, here are some tests I wrote up while playing with this. They do
not pass with your current patch for the reasons above, but maybe they
will be useful to squash in (either after tweaking the tests, or
tweaking the patch).

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index b4d98e6..4dde314 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh

@@ -775,4 +775,33 @@ test_expect_success PIPE 'streaming support for --stdin' '
        echo "$response" | grep "^::    two"
 '

+############################################################################
+#
+# test whitespace handling
+
+test_expect_success 'leading/trailing whitespace is ignored' '
+       mkdir whitespace &&
+       >whitespace/leading &&
+       >whitespace/trailing &&
+       >whitespace/untracked &&
+       {
+               echo "    whitespace/leading" &&
+               echo "whitespace/trailing   "
+       } >ignore &&
+       echo whitespace/untracked >expect &&
+       git ls-files -o -X ignore whitespace >actual &&
+       test_cmp expect actual
+'
+
+test_expect_success 'quoting allows trailing whitespace' '
+       rm -rf whitespace &&
+       mkdir whitespace &&
+       >"whitespace/trailing  " &&
+       >whitespace/untracked &&
+       echo "whitespace/trailing\\ \\ " >ignore &&
+       echo whitespace/untracked >expect &&
+       git ls-files -o -X ignore whitespace >actual &&
+       test_cmp expect actual
+'
+
 test_done



-- 
Duy

[PATCH v2 0/2] Ignore trailing spaces in .gitignore

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

This reroll now respects backslash quoting. Thanks Jeff and Torsten
for the comments.

Nguyễn Thái Ngọc Duy (2):
  dir: warn about trailing spaces in exclude patterns
  dir: ignore trailing spaces in exclude patterns

 Documentation/gitignore.txt |  3 +++
 dir.c                       | 20 ++++++++++++++++++++
 t/t0008-ignores.sh          | 31 +++++++++++++++++++++++++++++++
 3 files changed, 54 insertions(+)

-- 
1.8.5.2.240.g8478abd

[PATCH v2 2/2] dir: ignore trailing spaces in exclude patterns

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 Documentation/gitignore.txt |  3 +++
 dir.c                       | 21 ++++++++++++---------
 t/t0008-ignores.sh          |  8 ++++----
 3 files changed, 19 insertions(+), 13 deletions(-)

diff --git a/Documentation/gitignore.txt b/Documentation/gitignore.txt
index b08d34d..8734c15 100644
--- a/Documentation/gitignore.txt
+++ b/Documentation/gitignore.txt

@@ -77,6 +77,9 @@ PATTERN FORMAT
    Put a backslash ("`\`") in front of the first hash for patterns
    that begin with a hash.
 
+ - Trailing spaces are ignored unless they are quoted with backlash
+   ("`\`").
+
  - An optional prefix "`!`" which negates the pattern; any
    matching file excluded by a previous pattern will become
    included again. It is not possible to re-include a file if a parent

diff --git a/dir.c b/dir.c
index 6162209..70076b5 100644
--- a/dir.c
+++ b/dir.c

@@ -491,20 +491,23 @@ void clear_exclude_list(struct exclude_list *el)
 	el->filebuf = NULL;
 }
 
-static void check_trailing_spaces(const char *fname, char *buf)
+static void trim_trailing_spaces(char *buf)
 {
-	int i, last_space = -1, len = strlen(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] == ' ')
-			last_space = i;
-		else
+		else if (buf[i] == ' ') {
+			if (last_space == -1) {
+				last_space = i;
+				nr_spaces = 1;
+			} else
+				nr_spaces++;
+		} else
 			last_space = -1;
 
-	if (last_space == len - 1)
-		warning(_("%s: trailing spaces in '%s'. Please quote or remove them."),
-			fname, buf);
+	if (last_space != -1 && last_space + nr_spaces == len)
+		buf[last_space] = '\0';
 }
 
 int add_excludes_from_file_to_list(const char *fname,

@@ -558,7 +561,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
-				check_trailing_spaces(fname, entry);
+				trim_trailing_spaces(entry);
 				add_exclude(entry, base, baselen, el, lineno);
 			}
 			lineno++;

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index 9e1d64c..bbaf6b5 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh

@@ -779,18 +779,18 @@ test_expect_success PIPE 'streaming support for --stdin' '
 #
 # test whitespace handling
 
-test_expect_success 'trailing whitespace is warned' '
+test_expect_success 'trailing whitespace is ignored' '
 	mkdir whitespace &&
 	>whitespace/trailing &&
 	>whitespace/untracked &&
 	echo "whitespace/trailing   " >ignore &&
 	cat >expect <<EOF &&
-whitespace/trailing
 whitespace/untracked
 EOF
+	: >err.expect &&
 	git ls-files -o -X ignore whitespace >actual 2>err &&
-	grep "ignore:.*'\''whitespace/trailing   '\''" err &&
-	test_cmp expect actual
+	test_cmp expect actual &&
+	test_cmp err.expect err
 '
 
 test_expect_success 'quoting allows trailing whitespace' '

-- 
1.8.5.2.240.g8478abd

[PATCH v2 1/2] dir: warn about trailing spaces in exclude patterns

From: Nguyễn Thái Ngọc Duy <hidden>
Date: 2016-06-15 22:59:52

Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---
 dir.c              | 17 +++++++++++++++++
 t/t0008-ignores.sh | 31 +++++++++++++++++++++++++++++++
 2 files changed, 48 insertions(+)

diff --git a/dir.c b/dir.c
index b35b633..6162209 100644
--- a/dir.c
+++ b/dir.c

@@ -491,6 +491,22 @@ void clear_exclude_list(struct exclude_list *el)
 	el->filebuf = NULL;
 }
 
+static void check_trailing_spaces(const char *fname, char *buf)
+{
+	int i, last_space = -1, len = strlen(buf);
+	for (i = 0; i < len; i++)
+		if (buf[i] == '\\')
+			i++;
+		else if (buf[i] == ' ')
+			last_space = i;
+		else
+			last_space = -1;
+
+	if (last_space == len - 1)
+		warning(_("%s: trailing spaces in '%s'. Please quote or remove them."),
+			fname, buf);
+}
+
 int add_excludes_from_file_to_list(const char *fname,
 				   const char *base,
 				   int baselen,

@@ -542,6 +558,7 @@ int add_excludes_from_file_to_list(const char *fname,
 		if (buf[i] == '\n') {
 			if (entry != buf + i && entry[0] != '#') {
 				buf[i - (i && buf[i-1] == '\r')] = 0;
+				check_trailing_spaces(fname, entry);
 				add_exclude(entry, base, baselen, el, lineno);
 			}
 			lineno++;

diff --git a/t/t0008-ignores.sh b/t/t0008-ignores.sh
index b4d98e6..9e1d64c 100755
--- a/t/t0008-ignores.sh
+++ b/t/t0008-ignores.sh

@@ -775,4 +775,35 @@ test_expect_success PIPE 'streaming support for --stdin' '
 	echo "$response" | grep "^::	two"
 '
 
+############################################################################
+#
+# test whitespace handling
+
+test_expect_success 'trailing whitespace is warned' '
+	mkdir whitespace &&
+	>whitespace/trailing &&
+	>whitespace/untracked &&
+	echo "whitespace/trailing   " >ignore &&
+	cat >expect <<EOF &&
+whitespace/trailing
+whitespace/untracked
+EOF
+	git ls-files -o -X ignore whitespace >actual 2>err &&
+	grep "ignore:.*'\''whitespace/trailing   '\''" err &&
+	test_cmp expect actual
+'
+
+test_expect_success 'quoting allows trailing whitespace' '
+	rm -rf whitespace &&
+	mkdir whitespace &&
+	>"whitespace/trailing  " &&
+	>whitespace/untracked &&
+	echo "whitespace/trailing\\ \\ " >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.8.5.2.240.g8478abd

Re: [PATCH 0/2] Ignore trailing spaces in .gitignore

From: Jeff King <hidden>
Date: 2016-06-15 22:59:52

On Sun, Feb 09, 2014 at 06:48:18AM +0700, Duy Nguyen wrote:

quoted

I guess by quoting you meant:

  echo '"trailing  "' >.gitignore

This makes " special. If we follow shell convention then things
between ".." should be literal (e.g. "*" is no longer a wildcard). We
don't support it yet. So I rather go with backslash as it adds less
code.

For some reason I was thinking that we already handled double-quotes
here (as we do in other places where quoting is optional). But it looks
like we don't currently, so yeah, I don't think it is worth adding due
to the potential confusion.

Backslash-escaping was what I had originally assumed you meant, and it
was, so we are all on the same page (the patch was just broken. ;) ).

-Peff

`h`	back out one level
`j`	next message in thread
`k`	previous message in thread
`l`	drill in
`Esc`	close help / fold thread tree
`?`	toggle this help