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

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

[Nguyễn Thái Ngọc Duy]

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Torsten Bögershausen]

On 2014-02-08 09.10, Nguyễn Thái Ngọc Duy wrote:
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  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."),
> +
>  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++;
> 

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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 0/2] Ignore trailing spaces in .gitignore

[Jeff King]

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

[Duy Nguyen]

On Sat, Feb 8, 2014 at 11:45 PM, Jeff King <peff@peff.net> wrote:
> 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.

Hm...

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

>
> 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

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

[Jeff King]

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

> > 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

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

[Nguyễn Thái Ngọc Duy]

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 1/2] dir: warn about trailing spaces in exclude patterns

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 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

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> 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

Hmph, sorry but I fail to see why we need to incur cost for
fnmatch().  We read and parse the file and keep them as internal
strings, so your unquoting (and complaining the unquoted trailng
spaces) can be done at the parse time, while keeping the trailing
spaces the user explicitly told us to keep by quoting in the
internal string that we eventually feed fnmatch() with _after_
unquoting, no?

Puzzled...

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

[Duy Nguyen]

On Mon, Feb 10, 2014 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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
>
> Hmph, sorry but I fail to see why we need to incur cost for
> fnmatch().  We read and parse the file and keep them as internal
> strings, so your unquoting (and complaining the unquoted trailng
> spaces) can be done at the parse time, while keeping the trailing
> spaces the user explicitly told us to keep by quoting in the
> internal string that we eventually feed fnmatch() with _after_
> unquoting, no?

That's the optimization in the "but" sentence. Another (off topic)
opt. we could do is make "*.[ch]" behave more like "*.c", where we
just try to match the tail part.
-- 
Duy

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

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> 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
>
> Hmph, sorry but I fail to see why we need to incur cost for
> fnmatch().  We read and parse the file and keep them as internal
> strings, so your unquoting (and complaining the unquoted trailng
> spaces) can be done at the parse time, while keeping the trailing
> spaces the user explicitly told us to keep by quoting in the
> internal string that we eventually feed fnmatch() with _after_
> unquoting, no?
>
> Puzzled...

Heh, silly me. Yes, we _could_ parse and strip "\ " into " " if we
wanted to, but we can just give "\ " and let fnmatch(3) do its
thing, and that is the right thing to do for a rare corner case like
this (i.e. deliberate trailing spaces).

So I think I understand your reasoning, and I agree with it.