Merge lp:~vila/bzr/832046-globs-store-ordered into lp:bzr

[Vincent Ladeuil]

Provide a config section matcher respecting the file order.

This fixes http://pad.lv/832046 by introducing a new section matcher that
respect the order specified by the user in the file.

That's the matcher I want to use for the new config files (defaults,
system-wide, etc) instead of locations.conf for which the order is less
predictable.

Once http://pad.lv/832042 is also fixed, that would address my concerns
about https://code.launchpad.net/~thomir/bzr/add-global-config/+merge/69592
which could then be taken into account.

[Vincent Ladeuil (vila)]

Hit send too fast.

While I was there, I fixed a docstring issue in IniFileStore (which was intended for TransportIniFileStore) and also added an external_url to IniFileStore or we get weird NotImplemented exceptions when trying to %r such a store during debug.

I'm not sold on the StartingPathMatcher name (which I think capture the most important use case) and used GlobOrderedMatcher in a previous version (which I find uninformative). Suggestions welcome.

[Jelmer Vernooij (jelmer)]

Rather than this:

55 + if location.startswith('file://'):
56 + location = urlutils.local_path_from_url(location)

shouldn't this be using urlutils.unescape_for_display() ?

[Vincent Ladeuil (vila)]

> Rather than this:
>
> 55 + if location.startswith('file://'):
> 56 + location = urlutils.local_path_from_url(location)
>
> shouldn't this be using urlutils.unescape_for_display() ?

We've been using the same for locations.conf so I'm quite sure it's correct :)

We want to get a local path if possible and unescape_for_display won't do that AFAIK.

[Jelmer Vernooij (jelmer)]

On 12/23/2011 08:05 AM, Vincent Ladeuil wrote:
>> Rather than this:
>>
>> 55 + if location.startswith('file://'):
>> 56 + location = urlutils.local_path_from_url(location)
>>
>> shouldn't this be using urlutils.unescape_for_display() ?
>
> We've been using the same for locations.conf so I'm quite sure it's
correct :)
>
> We want to get a local path if possible and unescape_for_display won't
do that AFAIK.
unescape_for_display also handles local paths. I guess this is fine for
now if it was already there, it just seems to duplicate some code
unnecessarily.

Cheers,

Jelmer

[Vincent Ladeuil (vila)]

> unescape_for_display also handles local paths. I guess this is fine for
> now if it was already there, it just seems to duplicate some code
> unnecessarily.

I don't get it, what is duplicated here ?

[Jelmer Vernooij (jelmer)]

> > unescape_for_display also handles local paths. I guess this is fine for
> > now if it was already there, it just seems to duplicate some code
> > unnecessarily.
>
> I don't get it, what is duplicated here ?
The code which converts a URL to a section name in locations.conf, as it seems to be essentially the same thing as urlutils.unescape_for_display.

This all seems a bit magical - special casing local file paths. I wonder if it would make more sense to just convert everything to normalized URLs (using location_to_url) and then just compare those? That removes the need for special casing, and should also take care of any issues with characters that are e.g. escaped in the locations.conf file and not in the specified URL and vice versa.

[Vincent Ladeuil (vila)]

> > > unescape_for_display also handles local paths. I guess this is fine for
> > > now if it was already there, it just seems to duplicate some code
> > > unnecessarily.
> >
> > I don't get it, what is duplicated here ?
> The code which converts a URL to a section name in locations.conf, as it seems
> to be essentially the same thing as urlutils.unescape_for_display.

With no encoding involved :-/

>
> This all seems a bit magical - special casing local file paths. I wonder if it
> would make more sense to just convert everything to normalized URLs (using
> location_to_url) and then just compare those?

Well, that's old magic which works, I'd rather not fix it.

> That removes the need for
> special casing, and should also take care of any issues with characters that
> are e.g. escaped in the locations.conf file and not in the specified URL and
> vice versa.

So far, I mimic what is done in locations.conf, we can revisit that for both StartingPathMatcher and LocationMatcher but that seems out of scope for this proposal.

[lp:~vila/bzr/832046-globs-store-ordered]

6409. By Vincent Ladeuil on 2012-01-03

Cleanup a bit and make sure we use at least one file:// url in the tests.

[Jelmer Vernooij (jelmer)]

review: Abstain

[Vincent Ladeuil (vila)]

To clarify: I looked at using unescape_for_display but its API requires 7bits urls which is fine for location (or should be) but not for sections ids (they are already unicode) and there is not a lot of tests for file:/// urls either :-/

[Jelmer Vernooij (jelmer)]

I think at the very least this should have tests for URLs in locations.conf, and for matching local file URLs against paths and vice versa.

[Martin Packman (gz)]

The StartingPathMatcher logic is the confusing part, particularly what 'location' is expected to be. Stating it's inherited from what existing function, and listing limitations would help I think.

Using unescape_for_display wouldn't really help I don't think, and doesn't make much sense - the result needs operating on, not displaying. Normalising the location is wanted, but to a certain extent we can expect people to write their rules consistently. Likewise, splitting components in a sane manner would be nice but mostly people just want path segments handled correctly.

[lp:~vila/bzr/832046-globs-store-ordered]

6410. By Vincent Ladeuil on 2012-01-04

Feedback from review.

6411. By Vincent Ladeuil on 2012-01-05

Tweak news entry to conflict on merge.

[Martin Packman (gz)]

Changes look good provided the new local_path tests don't break on windows from using nix-style paths.

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2012-01-05 14:10:32 +0000
+++ bzrlib/config.py	2012-01-05 16:49:28 +0000
@@ -3114,10 +3114,6 @@
 class IniFileStore(Store):
     """A config Store using ConfigObj for storage.
 
-    :ivar transport: The transport object where the config file is located.
-
-    :ivar file_name: The config file basename in the transport directory.
-
     :ivar _config_obj: Private member to hold the ConfigObj instance used to
         serialize/deserialize the config file.
     """
@@ -3253,9 +3249,19 @@
             value = self._config_obj._unquote(value)
         return value
 
+    def external_url(self):
+        # Since an IniFileStore can be used without a file (at least in tests),
+        # it's better to provide something than raising a NotImplementedError.
+        # All daughter classes are supposed to provide an implementation
+        # anyway.
+        return 'In-Process Store, no URL'
 
 class TransportIniFileStore(IniFileStore):
     """IniFileStore that loads files from a transport.
+
+    :ivar transport: The transport object where the config file is located.
+
+    :ivar file_name: The config file basename in the transport directory.
     """
 
     def __init__(self, transport, file_name):
@@ -3435,9 +3441,8 @@
 
 class LocationSection(Section):
 
-    def __init__(self, section, length, extra_path):
+    def __init__(self, section, extra_path):
         super(LocationSection, self).__init__(section.id, section.options)
-        self.length = length
         self.extra_path = extra_path
         self.locals = {'relpath': extra_path,
                        'basename': urlutils.basename(extra_path)}
@@ -3465,6 +3470,53 @@
         return value
 
 
+class StartingPathMatcher(SectionMatcher):
+    """Select sections for a given location respecting the Store order."""
+
+    # FIXME: Both local paths and urls can be used for section names as well as
+    # ``location`` to stay consistent with ``LocationMatcher`` which itself
+    # inherited the fuzziness from the previous ``LocationConfig``
+    # implementation. We probably need to revisit which encoding is allowed for
+    # both ``location`` and section names and how we normalize
+    # them. http://pad.lv/85479, http://pad.lv/437009 and http://359320 are
+    # related too. -- vila 2012-01-04
+
+    def __init__(self, store, location):
+        super(StartingPathMatcher, self).__init__(store)
+        if location.startswith('file://'):
+            location = urlutils.local_path_from_url(location)
+        self.location = location
+
+    def get_sections(self):
+        """Get all sections matching ``location`` in the store.
+
+        The most generic sections are described first in the store, then more
+        specific ones can be provided for reduced scopes.
+
+        The returned section are therefore returned in the reversed order so
+        the most specific ones can be found first.
+        """
+        location_parts = self.location.rstrip('/').split('/')
+        store = self.store
+        sections = []
+        # Later sections are more specific, they should be returned first
+        for _, section in reversed(list(store.get_sections())):
+            if section.id is None:
+                # The no-name section is always included if present
+                yield store, LocationSection(section, self.location)
+                continue
+            section_path = section.id
+            if section_path.startswith('file://'):
+                # the location is already a local path or URL, convert the
+                # section id to the same format
+                section_path = urlutils.local_path_from_url(section_path)
+            if (self.location.startswith(section_path)
+                or fnmatch.fnmatch(self.location, section_path)):
+                section_parts = section_path.rstrip('/').split('/')
+                extra_path = '/'.join(location_parts[len(section_parts):])
+                yield store, LocationSection(section, extra_path)
+
+
 class LocationMatcher(SectionMatcher):
 
     def __init__(self, store, location):
@@ -3494,7 +3546,7 @@
         matching_sections = []
         if no_name_section is not None:
             matching_sections.append(
-                LocationSection(no_name_section, 0, self.location))
+                (0, LocationSection(no_name_section, self.location)))
         for section_id, extra_path, length in filtered_sections:
             # a section id is unique for a given store so it's safe to take the
             # first matching section while iterating. Also, all filtered
@@ -3504,7 +3556,7 @@
                 section = iter_all_sections.next()
                 if section_id == section.id:
                     matching_sections.append(
-                        LocationSection(section, length, extra_path))
+                        (length, LocationSection(section, extra_path)))
                     break
         return matching_sections
 
@@ -3513,10 +3565,10 @@
         matching_sections = self._get_matching_sections()
         # We want the longest (aka more specific) locations first
         sections = sorted(matching_sections,
-                          key=lambda section: (section.length, section.id),
+                          key=lambda (length, section): (length, section.id),
                           reverse=True)
         # Sections mentioning 'ignore_parents' restrict the selection
-        for section in sections:
+        for _, section in sections:
             # FIXME: We really want to use as_bool below -- vila 2011-04-07
             ignore = section.get('ignore_parents', None)
             if ignore is not None:

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2012-01-05 10:39:49 +0000
+++ bzrlib/tests/test_config.py	2012-01-05 16:49:28 +0000
@@ -3336,8 +3336,7 @@
 
     def get_section(self, options, extra_path):
         section = config.Section('foo', options)
-        # We don't care about the length so we use '0'
-        return config.LocationSection(section, 0, extra_path)
+        return config.LocationSection(section, extra_path)
 
     def test_simple_option(self):
         section = self.get_section({'foo': 'bar'}, '')
@@ -3380,9 +3379,7 @@
                            '/quux/quux'],
                           [section.id for _, section in store.get_sections()])
         matcher = config.LocationMatcher(store, '/foo/bar/quux')
-        sections = [section for s, section in matcher.get_sections()]
-        self.assertEquals([3, 2],
-                          [section.length for section in sections])
+        sections = [section for _, section in matcher.get_sections()]
         self.assertEquals(['/foo/bar', '/foo'],
                           [section.id for section in sections])
         self.assertEquals(['quux', 'bar/quux'],
@@ -3399,9 +3396,7 @@
         self.assertEquals(['/foo', '/foo/bar'],
                           [section.id for _, section in store.get_sections()])
         matcher = config.LocationMatcher(store, '/foo/bar/baz')
-        sections = [section for s, section in matcher.get_sections()]
-        self.assertEquals([3, 2],
-                          [section.length for section in sections])
+        sections = [section for _, section in matcher.get_sections()]
         self.assertEquals(['/foo/bar', '/foo'],
                           [section.id for section in sections])
         self.assertEquals(['baz', 'bar/baz'],
@@ -3432,6 +3427,90 @@
         self.assertEquals(expected_location, matcher.location)
 
 
+class TestStartingPathMatcher(TestStore):
+
+    def setUp(self):
+        super(TestStartingPathMatcher, self).setUp()
+        # Any simple store is good enough
+        self.store = config.IniFileStore()
+
+    def assertSectionIDs(self, expected, location, content):
+        self.store._load_from_string(content)
+        matcher = config.StartingPathMatcher(self.store, location)
+        sections = list(matcher.get_sections())
+        self.assertLength(len(expected), sections)
+        self.assertEqual(expected, [section.id for _, section in sections])
+        return sections
+
+    def test_empty(self):
+        self.assertSectionIDs([], self.get_url(), '')
+
+    def test_url_vs_local_paths(self):
+        # The matcher location is an url and the section names are local paths
+        sections = self.assertSectionIDs(['/foo/bar', '/foo'],
+                                         'file:///foo/bar/baz', '''\
+[/foo]
+[/foo/bar]
+''')
+
+    def test_local_path_vs_url(self):
+        # The matcher location is a local path and the section names are urls
+        sections = self.assertSectionIDs(['file:///foo/bar', 'file:///foo'],
+                                         '/foo/bar/baz', '''\
+[file:///foo]
+[file:///foo/bar]
+''')
+
+
+    def test_no_name_section_included_when_present(self):
+        # Note that other tests will cover the case where the no-name section
+        # is empty and as such, not included.
+        sections = self.assertSectionIDs(['/foo/bar', '/foo', None],
+                                         '/foo/bar/baz', '''\
+option = defined so the no-name section exists
+[/foo]
+[/foo/bar]
+''')
+        self.assertEquals(['baz', 'bar/baz', '/foo/bar/baz'],
+                          [s.locals['relpath'] for _, s in sections])
+
+    def test_order_reversed(self):
+        self.assertSectionIDs(['/foo/bar', '/foo'], '/foo/bar/baz', '''\
+[/foo]
+[/foo/bar]
+''')
+
+    def test_unrelated_section_excluded(self):
+        self.assertSectionIDs(['/foo/bar', '/foo'], '/foo/bar/baz', '''\
+[/foo]
+[/foo/qux]
+[/foo/bar]
+''')
+
+    def test_glob_included(self):
+        sections = self.assertSectionIDs(['/foo/*/baz', '/foo/b*', '/foo'],
+                                         '/foo/bar/baz', '''\
+[/foo]
+[/foo/qux]
+[/foo/b*]
+[/foo/*/baz]
+''')
+        # Note that 'baz' as a relpath for /foo/b* is not fully correct, but
+        # nothing really is... as far using {relpath} to append it to something
+        # else, this seems good enough though.
+        self.assertEquals(['', 'baz', 'bar/baz'],
+                          [s.locals['relpath'] for _, s in sections])
+
+    def test_respect_order(self):
+        self.assertSectionIDs(['/foo', '/foo/b*', '/foo/*/baz'],
+                              '/foo/bar/baz', '''\
+[/foo/*/baz]
+[/foo/qux]
+[/foo/b*]
+[/foo]
+''')
+
+
 class TestNameMatcher(TestStore):
 
     def setUp(self):

=== modified file 'doc/developers/configuration.txt'
--- doc/developers/configuration.txt	2011-12-22 18:37:21 +0000
+++ doc/developers/configuration.txt	2012-01-05 16:49:28 +0000
@@ -301,12 +301,15 @@
 
 ``bzrlib`` provides:
 
-* ``LocationMatcher(store, location)``: To select all sections that match
-  ``location``.
-
 * ``NameMatcher(store, unique_id)``: To select a single section matching
   ``unique_id``.
 
+* ``LocationMatcher(store, location)``: To select all sections that match
+  ``location`` sorted by decreasing number of path components.
+
+* ``StartingPathMatcher(store, location)``: To select all sections that
+  match ``location`` in the order they appear in the ``store``.
+
 Stacks
 ------
 

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2012-01-05 11:39:43 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2012-01-05 16:49:28 +0000
@@ -82,11 +82,19 @@
   of revisions whose ancestry is not obviously on the same developement
   line. (Vincent Ladeuil, #904744)
 
+<<<<<<< TREE
 * Make lazy imports resilient when resolved concurrently from multiple
   threads. Now the stand-in object will behave as a proxy for the real object
   after the initial access, rather than throwing. Assigning the object to
   multiple names should still be avoided. (Martin von Gagern, #396819)
 
+=======
+* Configuration stacks can now use ``StartingPathMatcher`` to select the
+  sections matching a location while respecting the order chosen by the user
+  in the configuration file: from generic sections to specific
+  sections. (Vincent Ladeuil, #832046).
+
+>>>>>>> MERGE-SOURCE
 * Not setting ``gpg_signing_key`` or setting it to ``default`` will use the
   user email (obtained from the ``email`` configuration option or its
   default value). (Vincent Ladeuil, Jelmer Vernooij, #904550)