eol filter silently ignores unknown settings

[Brian de Alwis]

It would be nice if the eol functionality warned if a rule is configured with an invalid value.

--- bzrlib/filters/eol.py.orig 2009-04-08 22:56:50.000000000 -0600
+++ bzrlib/filters/eol.py 2009-04-08 22:50:46.000000000 -0600
@@ -66,5 +66,9 @@
             [ContentFilter(_to_crlf_converter, _to_crlf_converter)],
         }
     def eol_lookup(key):
- return _eol_filter_stack_map.get(key)
+ filter = _eol_filter_stack_map.get(key)
+ if filter == None:
+ from bzrlib.trace import warning
+ warning("WARNING: Unknown eol filter type '%s'" % key)
+ return filter
     register_filter_stack_map('eol', eol_lookup)

[Ian Clatworthy (ian-clatworthy)]

Brian,

Thanks for the bug report and fix. I'd really like to see this land. Can I ask you to package it as patch (complete with a short test confirming the error gets reported on a bad value) and send it to the mailing list?

[Brian de Alwis (slyguy)]

Attached is a patch to bzr.dev to emit a warning when encountering an
unknown eol setting. This patch fixed bug 358199.

Brian.

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Brian de Alwis wrote:
> Attached is a patch to bzr.dev to emit a warning when encountering an
> unknown eol setting. This patch fixed bug 358199.
>
> Brian.
>

+ filter = _eol_filter_stack_map.get(key)
+ if filter == None:
+ from bzrlib.trace import warning
+ warning("Unknown eol filter type '%s'" % key)

^- as a 'pythonic' thing, you should always use 'is' to compare with
None, so it would be:

if filter is None:
  ...

I'm also a bit concerned about filling the screens with warnings.

Consider someone who does something like:

[name *.c]
eol = crlff

At that point when the do 'bzr co' it will give 10,000 warnings about
unknown eol filter type (I believe).

So I think we would want a 'debounce' in it with something like:

_unknown_eol = set()

def eol_lookup(key):
  ...
  if filter is None and key not in _unknown_eol:
    _unknown_eol.add(key)
    warning("Unknown eol filter type '%s'", key)

(note that 'warning' already has the right code to do % *args, so you
can just pass 'key' as another argument.)

...

+class TestEolRulesSpecifications(TestCase):
+
+ def setUp(self):
+ self.warnings = []
+ self._warning = trace.warning
+ def warning(*args):
+ self.warnings.append(args[0] % args[1:])
+ trace.warning = warning
+ super(TestEolRulesSpecifications, self).setUp()

^- The spacing on this shows that you have some 'tab' characters mixed
with plain space characters. We only allow plain space characters in our
python source code.

I'm not 100% sure about adding a debounce to 'unknown eol', but I'm
guessing that scrolling by 1000s of messages is undesirable.

Anyway, because of tabs in the source:
BB:resubmit

for now. I think the change is otherwise reasonable.

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkno1F0ACgkQJdeBCYSNAAN5rACgqUPFTeORS7v97HQhDMus9HO8
wSwAoJTW5Q3zKEsMhEs8/bFiiiXpXx5R
=H65n
-----END PGP SIGNATURE-----

[Brian de Alwis (slyguy)]

Thanks for the review.

I wonder now if a warning is the right behaviour; I now believe that
bzr should actually throw an error on an unknown setting. Otherwise
the user might have different content stored in the repository than
she might have desired. When using a checkout or bound branch, such a
commit { can't | likely shouldn't } be uncommitted.

Brian.

On 17-Apr-2009, at 1:11 PM, John Arbash Meinel wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Brian de Alwis wrote:
>> Attached is a patch to bzr.dev to emit a warning when encountering an
>> unknown eol setting. This patch fixed bug 358199.
>>
>> Brian.
>>
>
> + filter = _eol_filter_stack_map.get(key)
> + if filter == None:
> + from bzrlib.trace import warning
> + warning("Unknown eol filter type '%s'" % key)
>
> ^- as a 'pythonic' thing, you should always use 'is' to compare with
> None, so it would be:
>
> if filter is None:
> ...
>
> I'm also a bit concerned about filling the screens with warnings.
>
> Consider someone who does something like:
>
> [name *.c]
> eol = crlff
>
> At that point when the do 'bzr co' it will give 10,000 warnings about
> unknown eol filter type (I believe).
>
> So I think we would want a 'debounce' in it with something like:
>
> _unknown_eol = set()
>
> def eol_lookup(key):
> ...
> if filter is None and key not in _unknown_eol:
> _unknown_eol.add(key)
> warning("Unknown eol filter type '%s'", key)
>
> (note that 'warning' already has the right code to do % *args, so you
> can just pass 'key' as another argument.)
>
>
> ...
>
> +class TestEolRulesSpecifications(TestCase):
> +
> + def setUp(self):
> + self.warnings = []
> + self._warning = trace.warning
> + def warning(*args):
> + self.warnings.append(args[0] % args[1:])
> + trace.warning = warning
> + super(TestEolRulesSpecifications, self).setUp()
>
> ^- The spacing on this shows that you have some 'tab' characters mixed
> with plain space characters. We only allow plain space characters in
> our
> python source code.
>
>
> I'm not 100% sure about adding a debounce to 'unknown eol', but I'm
> guessing that scrolling by 1000s of messages is undesirable.
>
> Anyway, because of tabs in the source:
> BB:resubmit
>
> for now. I think the change is otherwise reasonable.
>
> John
> =:->
> -----BEGIN PGP SIGNATURE-----
> Version: GnuPG v1.4.9 (Cygwin)
> Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org
>
> iEYEARECAAYFAkno1F0ACgkQJdeBCYSNAAN5rACgqUPFTeORS7v97HQhDMus9HO8
> wSwAoJTW5Q3zKEsMhEs8/bFiiiXpXx5R
> =H65n
> -----END PGP SIGNATURE-----

--
"Amusement to an observing mind is study." - Benjamin Disraeli

[Aaron Bentley (abentley)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Brian de Alwis wrote:
> Thanks for the review.
>
> I wonder now if a warning is the right behaviour; I now believe that bzr
> should actually throw an error on an unknown setting.

That sounds like a good choice. If it turns out that people want to
commit anyway, we can look at changing it later.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (GNU/Linux)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iEYEARECAAYFAkno1zUACgkQ0F+nu1YWqI3DYgCfWZc8pP9eWomN9Dh10LKKWWv6
mPYAn3LtTk/r6/VhUq34FeSUk8AvsW1A
=LvvU
-----END PGP SIGNATURE-----

[Brian de Alwis (slyguy)]

Changed the patch to now raise an error on an unknown eol setting.
And fixed up tabbing issues.

Brian.

--
"Amusement to an observing mind is study." - Benjamin Disraeli

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: <email address hidden>
# target_branch: file:///scratch/dev/bzr.dev/
# testament_sha1: 1d73ba513cc3615ae5b627e590030cd0e65f1c63
# timestamp: 2009-04-17 13:56:07 -0600
# source_branch: bzr+ssh://bazaar.launchpad.net/%7Ebzr/bzr/trunk/
# base_revision_id: <email address hidden>
#
# Begin patch
=== modified file 'bzrlib/filters/eol.py'
--- bzrlib/filters/eol.py 2009-04-09 01:08:14 +0000
+++ bzrlib/filters/eol.py 2009-04-17 19:53:42 +0000
@@ -66,5 +66,9 @@
              [ContentFilter(_to_crlf_converter, _to_crlf_converter)],
          }
      def eol_lookup(key):
- return _eol_filter_stack_map.get(key)
+ filter = _eol_filter_stack_map.get(key)
+ if filter is None:
+ from bzrlib.errors import BzrError
+ raise BzrError("Unknown eol filter type '%s'" % key)
+ return filter
      register_filter_stack_map('eol', eol_lookup)

=== modified file 'bzrlib/tests/test_eol_filters.py'
--- bzrlib/tests/test_eol_filters.py 2009-04-02 04:12:11 +0000
+++ bzrlib/tests/test_eol_filters.py 2009-04-17 19:53:42 +0000
@@ -37,3 +37,31 @@
      def test_to_crlf(self):
          result = _to_crlf_converter([_sample_file1])
          self.assertEqual(["hello\r\nworld\r\n"], result)
+
+from bzrlib import (errors, rules)
+from bzrlib.filters import _get_filter_stack_for
+
+class TestEolRulesSpecifications(TestCase):
+
+ def test_exact_setting(self):
+ """'eol = exact' should have no content filters"""
+ prefs = (('eol','exact'),)
+ self.assertEqual([], _get_filter_stack_for(prefs))
+
+ def test_other_known_settings(self):
+ """These known eol settings have corresponding filters."""
+ known_settings = ('lf', 'crlf', 'native',
+ 'native-with-crlf-in-repo', 'lf-with-crlf-in-repo',
+ 'crlf-with-crlf-in-repo')
+ for setting in known_settings:
+ prefs = (('eol',setting),)
+ self.assertNotEqual([], _get_filter_stack_for(prefs))
+
+ def test_unknown_setting(self):
+ """
+ Unknown settings should emit a warning, and result in no
+ content filters, equivalent to 'exact'.
+ """
+ prefs = (('eol','unknown-setting'),)
+ self.assertRaises(errors.BzrError, _get_filter_stack_for,
prefs)
+

# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWf0au9sAAvffgERQU
+f//3sB
Hoq////
wYAcPvebpXewKZ3W6jbfQ06Yj0ElCJkTyam0TaeqeSeUDJpk9QaNA0ANBJREwBpop5Q0a
BtQaHpANAAAIpmQSehPSaAGgAAAAaNAASKaJok8mk9UeSep6mg2oaaNqPUaPUNDQGQZSmPVGE0AM
IA0ADIAAABJEI00BTYhTyNCekZNQ2kzSaDTEGiABXnsHxYsmVe5tpHvqiuqTIwnu1TDkxZCEldFf
UiYKMzxwj8JAsD7PV1D5yxan6fHaAgrAWC2fbDPRjzhx31XA4xjbfZ1rZWlR7KZSgsm5GnENf47
+
V7lQDNKFyb0AUqkScaUri1EijEdASLQJGBLpweIDvQMBjWDLT+qJ9MUogJTDIWZ/
CaBZmAkDkLqk
A2FPxJ0AEMSltLWu8a1SwNcLIGU4UvLyoLe70b/Ln19eI/OcmO
+LloaOadqcrEob9dyCHU1Jck+F
tETeJ1jaKmsyqG5pllVcDWEEOqKiF0x3aci4ieBjwKLhJRKssVBH0KSqdolE0yLFsrQ7...

[Jelmer Vernooij (jelmer)]

Hi Brian,

Brian de Alwis wrote:
> Changed the patch to now raise an error on an unknown eol setting. And
> fixed up tabbing issues.

Thanks for working on this. I've hit this as well (trying to figure out
why my eol setting didn't work).

> === modified file 'bzrlib/filters/eol.py'
> --- bzrlib/filters/eol.py 2009-04-09 01:08:14 +0000
> +++ bzrlib/filters/eol.py 2009-04-17 19:53:42 +0000
> @@ -66,5 +66,9 @@
> [ContentFilter(_to_crlf_converter, _to_crlf_converter)],
> }
> def eol_lookup(key):
> - return _eol_filter_stack_map.get(key)
> + filter = _eol_filter_stack_map.get(key)
> + if filter is None:
> + from bzrlib.errors import BzrError
Please add imports on top of the file (per HACKING).

Any reason for not using _eol_filter_stack_map[key] here, and catching
and handling any KeyErrors?

> === modified file 'bzrlib/tests/test_eol_filters.py'
> --- bzrlib/tests/test_eol_filters.py 2009-04-02 04:12:11 +0000
> +++ bzrlib/tests/test_eol_filters.py 2009-04-17 19:53:42 +0000
> @@ -37,3 +37,31 @@
> def test_to_crlf(self):
> result = _to_crlf_converter([_sample_file1])
> self.assertEqual(["hello\r\nworld\r\n"], result)
> +
> +from bzrlib import (errors, rules)
> +from bzrlib.filters import _get_filter_stack_for

See above. Please use the same style for imports as the rest of the
file, where each module is on a separate line (to help merge, which is
line-based).

Other than that, looks good.

bb:tweak

Cheers,

Jelmer

[Brian de Alwis (slyguy)]

On 17-Apr-2009, at 3:04 PM, Jelmer Vernooij wrote:
> Please add imports on top of the file (per HACKING).

I've moved the imports, though I didn't see anything in HACKING.

> Any reason for not using _eol_filter_stack_map[key] here, and catching
> and handling any KeyErrors?

I was just following the original code. I suppose it allows the
possibility of a content-filter being None.

I also fixed up a comment and tweaked the code to use "values" rather
than "settings" to match the eol docs.

Brian.

--
"Amusement to an observing mind is study." - Benjamin Disraeli

# Bazaar merge directive format 2 (Bazaar 0.90)
# revision_id: <email address hidden>
# target_branch: file:///scratch/dev/bzr.dev/
# testament_sha1: e6b65efecc9cddc6a6ffeace220a9de2a36ecf30
# timestamp: 2009-04-17 16:49:39 -0600
# source_branch: bzr+ssh://bazaar.launchpad.net/%7Ebzr/bzr/trunk/
# base_revision_id: <email address hidden>
#
# Begin patch
=== modified file 'bzrlib/filters/eol.py'
--- bzrlib/filters/eol.py 2009-04-09 01:08:14 +0000
+++ bzrlib/filters/eol.py 2009-04-17 22:49:24 +0000
@@ -22,6 +22,7 @@

  import re, sys

+from bzrlib.errors import BzrError

  # Real Linux/Unix/OSX newline - \n without \r before it
  _LINUX_NL_RE = re.compile(r'(?<!\r)\n')
@@ -66,5 +67,8 @@
              [ContentFilter(_to_crlf_converter, _to_crlf_converter)],
          }
      def eol_lookup(key):
- return _eol_filter_stack_map.get(key)
+ filter = _eol_filter_stack_map.get(key)
+ if filter is None:
+ raise BzrError("Unknown eol value '%s'" % key)
+ return filter
      register_filter_stack_map('eol', eol_lookup)

=== modified file 'bzrlib/tests/test_eol_filters.py'
--- bzrlib/tests/test_eol_filters.py 2009-04-02 04:12:11 +0000
+++ bzrlib/tests/test_eol_filters.py 2009-04-17 22:49:24 +0000
@@ -22,6 +22,11 @@
      _to_lf_converter,
      )
  from bzrlib.tests import TestCase
+from bzrlib import (
+ errors,
+ rules,
+ )
+from bzrlib.filters import _get_filter_stack_for

  # Sample files
@@ -37,3 +42,27 @@
      def test_to_crlf(self):
          result = _to_crlf_converter([_sample_file1])
          self.assertEqual(["hello\r\nworld\r\n"], result)
+
+class TestEolRulesSpecifications(TestCase):
+
+ def test_exact_value(self):
+ """'eol = exact' should have no content filters"""
+ prefs = (('eol','exact'),)
+ self.assertEqual([], _get_filter_stack_for(prefs))
+
+ def test_other_known_values(self):
+ """These known eol values have corresponding filters."""
+ known_values = ('lf', 'crlf', 'native',
+ 'native-with-crlf-in-repo', 'lf-with-crlf-in-repo',
+ 'crlf-with-crlf-in-repo')
+ for value in known_values:
+ prefs = (('eol',value),)
+ self.assertNotEqual([], _get_filter_stack_for(prefs))
+
+ def test_unknown_value(self):
+ """
+ Unknown eol values should raise an error.
+ """
+ prefs = (('eol','unknown-value'),)
+ self.assertRaises(errors.BzrError, _get_filter_stack_for,
prefs)
+

# Begin bundle
IyBCYXphYXIgcmV2aXNpb24gYnVuZGxlIHY0CiMKQlpoOTFBWSZTWeJu...