Merge lp:~vila/bzr/829237-location-matcher-misses into lp:bzr/2.4

[Vincent Ladeuil]

config.LocationMatcher properly excludes unrelated sections

config.LocationMatcher wasn't selecting the right sections when
unrelated sections were existing:

[/foo]
section=/foo
[/foo/baz]
section=/foo/baz
[/foo/bar]
section=/foo/bar

was selecting only /foo when queried for /foor/bar/quux

The fix is trivial.

I'm targetting 2.4 as at least the fdatasync options use
LocationStack.

[Jonathan Riddell (jr)]

I'm not convinced I understand the logic change here I'm afraid. However the test looks correct so I trust the change :)

review: Approve

[Vincent Ladeuil (vila)]

sent to pqm by email

[John A Meinel (jameinel)]

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

On 8/19/2011 10:18 AM, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr/829237-location-matcher-misses into lp:bzr/2.4.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #829237 in
> Bazaar: "config.LocationMatcher may miss"
> https://bugs.launchpad.net/bzr/+bug/829237
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr/829237-location-matcher-misses/+merge/72151
>
> config.LocationMatcher wasn't selecting the right sections when
> unrelated sections were existing:
>
> [/foo] section=/foo [/foo/baz] section=/foo/baz [/foo/bar]
> section=/foo/bar
>
> was selecting only /foo when queried for /foor/bar/quux
>
> The fix is trivial.
>
> I'm targetting 2.4 as at least the fdatasync options use
> LocationStack.

+ while True:
+ section = iter_all_sections.next()
+ if section_id == section.id:
+ matching_sections.append(
+ LocationSection(section, length, extra_path))
+ break

^- This seems like a good way to get either an infinite loop, or a
StopIteration exception. I think you want:

while True:
  try:
    section = ...
  except StopIteration:
    break

Do we already catch StopIteration somewhere else?

John
=:->

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

iEYEARECAAYFAk5SORcACgkQJdeBCYSNAAM2wgCg04d/TXBwFRH67gz/tNGOyYpJ
TEgAniKzw7ftU1lYEw3+Z6jy6SLBfMl6
=2FoX
-----END PGP SIGNATURE-----

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2011-08-11 09:03:18 +0000
+++ bzrlib/config.py	2011-08-19 08:18:23 +0000
@@ -2649,30 +2649,34 @@
         # We slightly diverge from LocalConfig here by allowing the no-name
         # section as the most generic one and the lower priority.
         no_name_section = None
-        sections = []
+        all_sections = []
         # Filter out the no_name_section so _iter_for_location_by_parts can be
         # used (it assumes all sections have a name).
         for section in self.store.get_sections():
             if section.id is None:
                 no_name_section = section
             else:
-                sections.append(section)
+                all_sections.append(section)
         # Unfortunately _iter_for_location_by_parts deals with section names so
         # we have to resync.
         filtered_sections = _iter_for_location_by_parts(
-            [s.id for s in sections], self.location)
-        iter_sections = iter(sections)
+            [s.id for s in all_sections], self.location)
+        iter_all_sections = iter(all_sections)
         matching_sections = []
         if no_name_section is not None:
             matching_sections.append(
                 LocationSection(no_name_section, 0, 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 iterate
-            # again
-            section = iter_sections.next()
-            if section_id == section.id:
-                matching_sections.append(
-                    LocationSection(section, length, extra_path))
+            # a section id is unique for a given store so it's safe to take the
+            # first matching section while iterating. Also, all filtered
+            # sections are part of 'all_sections' and will always be found
+            # there.
+            while True:
+                section = iter_all_sections.next()
+                if section_id == section.id:
+                    matching_sections.append(
+                        LocationSection(section, length, extra_path))
+                    break
         return matching_sections
 
     def get_sections(self):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2011-06-22 13:53:20 +0000
+++ bzrlib/tests/test_config.py	2011-08-19 08:18:23 +0000
@@ -2803,6 +2803,32 @@
     def get_store(self, file_name):
         return config.IniFileStore(self.get_readonly_transport(), file_name)
 
+    def test_unrelated_section_excluded(self):
+        store = self.get_store('foo.conf')
+        store._load_from_string('''
+[/foo]
+section=/foo
+[/foo/baz]
+section=/foo/baz
+[/foo/bar]
+section=/foo/bar
+[/foo/bar/baz]
+section=/foo/bar/baz
+[/quux/quux]
+section=/quux/quux
+''')
+        self.assertEquals(['/foo', '/foo/baz', '/foo/bar', '/foo/bar/baz',
+                           '/quux/quux'],
+                          [section.id for section in store.get_sections()])
+        matcher = config.LocationMatcher(store, '/foo/bar/quux')
+        sections = list(matcher.get_sections())
+        self.assertEquals([3, 2],
+                          [section.length for section in sections])
+        self.assertEquals(['/foo/bar', '/foo'],
+                          [section.id for section in sections])
+        self.assertEquals(['quux', 'bar/quux'],
+                          [section.extra_path for section in sections])
+
     def test_more_specific_sections_first(self):
         store = self.get_store('foo.conf')
         store._load_from_string('''

=== modified file 'doc/en/release-notes/bzr-2.4.txt'
--- doc/en/release-notes/bzr-2.4.txt	2011-08-15 07:29:01 +0000
+++ doc/en/release-notes/bzr-2.4.txt	2011-08-19 08:18:23 +0000
@@ -32,6 +32,9 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* ``config.LocationMatcher`` properly excludes unrelated sections.
+  (Vincent Ladeuil, #829237)
+
 Documentation
 *************