Re: [PATCH 0/2] Ignore trailing spaces in .gitignore
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 8, 2014, 23:48 UTC
- Message-ID
- <CACsJy8C_Lc+mcJxGc7S5AB47YKDrhmWPwZp2oxJ9Z1tONVT0kQ@mail.gmail.com>
- In-Reply-To
- <20140208164548.GA24600@sigill.intra.peff.net>
On Sat, Feb 8, 2014 at 11:45 PM, Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> 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...
Show 14 quoted lines
> >> 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.
Show 44 quoted lines
>
> 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