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

Reply via email to