Merge lp:~gz/bzr/split_segment_params_not_url_842233 into lp:bzr

[Martin Packman]

Avoid using URL specific join and split when seperating segment parameters

Correctly split segment parameters from a url, without depending on url handling functions. This branch keeps the current semantics, except for a couple of assertions on relative paths which are invalid.

For background, see the merge proposal fixing the main initial fallout from the addition colo branch addressing:

<https://code.launchpad.net/~gz/bzr/root_drive_file_url_841322/+merge/74034>

The salient point is that to make a colo address, the join code roughly uses "%(url)s,branch=name" with whatever the base url is. Blindly sticking a comma and segment params on the end of any old url means the result is no longer a url - depending on the base format it may still be parsable as one, but it may not.

Such as:
    <http://example.com> is valid
    <http://example.com,branch=name> is invalid
    <file:///> is special-cased to valid in bzrlib on windows
    <file:///,branch=name> is invalid on windows

So, either we must be careful to make sure to always remove the segment parameters smuggled on before using any url handling functions or passing anywhere that needs a url, or a different approach is needed.

This is further complicated by the laxness of the various Transport implementations when it comes to url handling, which involves a lot of plain string operations without checking inputs. Particularly the adding terminal slashes in some places and removing them in others is troublesome.

[Vincent Ladeuil (vila)]

Thanks for your patience on that.

Overall, I agree that url handling in the various transports classes may receive some love especially about which encoding is used (including url-escaped) is/should/should not be used by which methods.

It's both amazing and scary that you've been able to fix this bug without touching a single transport test... I understand why but it's still amazing.

A couple of nits:

8 + # Check relative references with absolute paths
17 + # Check relative references with relative paths
29 + # TODO: Check full URLs as well as relative references

33 + # Check relative references with absolute paths
42 + # Check relative references with relative paths
51 + # TODO: Check full URLs as well as relative references

Hmm, do I see a pattern there ?

This screams: split these tests to my ears.

70 + # Segements begin at first comma after last forward slash, if one exists

s/Segements/Segments/

71 + segment_start = lurl.find(",", lurl.rfind("/")+1)

'+1'... Cunning ;) and commented !

74 - return (join(parent_url, subsegments[0]), subsegments[1:])
75 + return lurl[:segment_start], lurl[segment_start+1:].split(",")

The comma is easy to miss, the leading paren was clearer to indicate that two values are returned.

Two final notes:
- there has been discussion in the past about never removing the final path from 't.base' as it *is* a directory anyway (and then stop adding it blindly),
- I'd really like to see your mail summarising your thoughts about url handling across the code base ;-D

.. and one more thing: news entry

review: Approve

[Martin Packman (gz)]

> Overall, I agree that url handling in the various transports classes may
> receive some love especially about which encoding is used (including url-
> escaped) is/should/should not be used by which methods.

Yes, Jelmer has gone some way towards sorting this out, but there's still a lot of confusion in the code.

> It's both amazing and scary that you've been able to fix this bug without
> touching a single transport test... I understand why but it's still amazing.

Well, that's due to the cheat with split_trailing_slash which lets me replicate the existing (somewhat bogus) behaviour but without tickling the main problem by trying to split/join as a url.

> Hmm, do I see a pattern there ?
>
> This screams: split these tests to my ears.

Yes, and given it should really cover the case with schemes as well, it almost wants parametrizing. I resisted that as part of this change as I'm unconvinced by the current interfaces.

> Two final notes:
> - there has been discussion in the past about never removing the final path
> from 't.base' as it *is* a directory anyway (and then stop adding it blindly),
> - I'd really like to see your mail summarising your thoughts about url
> handling across the code base ;-D

Being far more careful about what are valid operations on different kinds of url scheme would help, which perhaps the URL class can assist with though I'm a little leery of the added weight there.

> .. and one more thing: news entry

Added, and nits addressed.

[Martin Packman (gz)]

sent to pqm by email

=== modified file 'bzrlib/tests/test_urlutils.py'
--- bzrlib/tests/test_urlutils.py	2011-09-04 23:18:43 +0000
+++ bzrlib/tests/test_urlutils.py	2011-11-24 12:10:31 +0000
@@ -501,6 +501,7 @@
 
     def test_split_segment_parameters_raw(self):
         split_segment_parameters_raw = urlutils.split_segment_parameters_raw
+        # Check relative references with absolute paths
         self.assertEquals(("/some/path", []),
             split_segment_parameters_raw("/some/path"))
         self.assertEquals(("/some/path", ["tip"]),
@@ -511,19 +512,22 @@
             split_segment_parameters_raw("/somedir/path,heads%2Ftip"))
         self.assertEquals(("/somedir/path", ["heads%2Ftip", "bar"]),
             split_segment_parameters_raw("/somedir/path,heads%2Ftip,bar"))
-        self.assertEquals(("/", ["key1=val1"]),
+        # Check relative references with relative paths
+        self.assertEquals(("", ["key1=val1"]),
             split_segment_parameters_raw(",key1=val1"))
         self.assertEquals(("foo/", ["key1=val1"]),
             split_segment_parameters_raw("foo/,key1=val1"))
-        self.assertEquals(("/foo", ["key1=val1"]),
+        self.assertEquals(("foo", ["key1=val1"]),
             split_segment_parameters_raw("foo,key1=val1"))
         self.assertEquals(("foo/base,la=bla/other/elements", []),
             split_segment_parameters_raw("foo/base,la=bla/other/elements"))
         self.assertEquals(("foo/base,la=bla/other/elements", ["a=b"]),
             split_segment_parameters_raw("foo/base,la=bla/other/elements,a=b"))
+        # TODO: Check full URLs as well as relative references
 
     def test_split_segment_parameters(self):
         split_segment_parameters = urlutils.split_segment_parameters
+        # Check relative references with absolute paths
         self.assertEquals(("/some/path", {}),
             split_segment_parameters("/some/path"))
         self.assertEquals(("/some/path", {"branch": "tip"}),
@@ -538,7 +542,8 @@
                 "/somedir/path,ref=heads%2Ftip,key1=val1"))
         self.assertEquals(("/somedir/path", {"ref": "heads%2F=tip"}),
             split_segment_parameters("/somedir/path,ref=heads%2F=tip"))
-        self.assertEquals(("/", {"key1": "val1"}),
+        # Check relative references with relative paths
+        self.assertEquals(("", {"key1": "val1"}),
             split_segment_parameters(",key1=val1"))
         self.assertEquals(("foo/", {"key1": "val1"}),
             split_segment_parameters("foo/,key1=val1"))
@@ -547,6 +552,7 @@
         self.assertEquals(("foo/base,key1=val1/other/elements",
             {"key2": "val2"}), split_segment_parameters(
                 "foo/base,key1=val1/other/elements,key2=val2"))
+        # TODO: Check full URLs as well as relative references
 
     def test_win32_strip_local_trailing_slash(self):
         strip = urlutils._win32_strip_local_trailing_slash

=== modified file 'bzrlib/urlutils.py'
--- bzrlib/urlutils.py	2011-11-16 10:36:31 +0000
+++ bzrlib/urlutils.py	2011-11-24 12:10:31 +0000
@@ -441,11 +441,15 @@
     :param url: A relative or absolute URL
     :return: (url, subsegments)
     """
-    (parent_url, child_dir) = split(url)
-    subsegments = child_dir.split(",")
-    if len(subsegments) == 1:
+    # GZ 2011-11-18: Dodgy removing the terminal slash like this, function
+    #                operates on urls not url+segments, and Transport classes
+    #                should not be blindly adding slashes in the first place. 
+    lurl = strip_trailing_slash(url)
+    # Segments begin at first comma after last forward slash, if one exists
+    segment_start = lurl.find(",", lurl.rfind("/")+1)
+    if segment_start == -1:
         return (url, [])
-    return (join(parent_url, subsegments[0]), subsegments[1:])
+    return (lurl[:segment_start], lurl[segment_start+1:].split(","))
 
 
 def split_segment_parameters(url):

=== modified file 'doc/en/release-notes/bzr-2.5.txt'
--- doc/en/release-notes/bzr-2.5.txt	2011-11-23 14:11:03 +0000
+++ doc/en/release-notes/bzr-2.5.txt	2011-11-24 12:10:31 +0000
@@ -50,6 +50,12 @@
 * Resolve regression from colocated branch path handling, by ensuring that
   unreserved characters are unquoted in URLs. (Martin Packman, #842223)
 
+* Split segments from URLs for colocated branches without assuming the
+  combined form is a valid. (Martin Packman, #842233)
+
+* Support looking up revision numbers by revision id in empty branches.
+  (Jelmer Vernooij, #535031)
+
 * Support verifying signatures on remote repositories.
   (Jelmer Vernooij, #889694)