Bazaar

Merge lp:~maxb/bzr/normalize-lp into lp:bzr/2.3

  1. normalize-lp
  2. Merge into 2.3
Proposed by Max Bowsher
Status: Work in progress
Proposed branch: lp:~maxb/bzr/normalize-lp
Merge into: lp:bzr/2.3
Diff against target: 41 lines (+10/-14)
1 file modified
bzrlib/urlutils.py (+10/-14)
To merge this branch: bzr merge lp:~maxb/bzr/normalize-lp
Reviewer Review Type Date Requested Status
Andrew Bennetts Needs Fixing
Review via email: mp+61729@code.launchpad.net

Description of the change

Fixes the bzrlib.urlutils.normalize_url function to not mangle lp:foo URLs for lp:foolp:foo

One way to demonstrate the bug is:

bzr config -d lp:bzr wibble=blah

and then check your ~/.bazaar/locations.conf

To post a comment you must log in.
Revision history for this message
Andrew Bennetts (spiv) wrote :

 review needs-fixing

Max Bowsher wrote:
[…]
> - path = _url_hex_escapes_re.sub(_unescape_safe_chars, path)
> - return str(prefix + ''.join(path))
> + url = _url_hex_escapes_re.sub(_unescape_safe_chars, url)
> + return str(url)

Hmm. This will change what we unescape, including in the netloc part of
a URL. I'm not sure that's correct (I'm not sure it's not either). I
wonder particularly about URLs with usernames with unusual characters in
them.

I *think* what you have is correct, but it's a slightly worrying change
to make in a stable branch as it appears it might affect more cases than
<lp:bzr>.

I like that your change makes the code a little simpler. I wonder if we
should land this change on trunk, and a more conservative fix on 2.3:

=== modified file 'bzrlib/urlutils.py'
--- bzrlib/urlutils.py 2010-10-21 22:27:43 +0000
+++ bzrlib/urlutils.py 2011-05-24 03:48:53 +0000
@@ -308,7 +308,10 @@
     if scheme_end is None:
         return local_path_to_url(url)
     prefix = url[:path_start]
- path = url[path_start:]
+ if path_start is None:
+ path = ''
+ else:
+ path = url[path_start:]
     if not isinstance(url, unicode):
         for c in url:
             if c not in _url_safe_characters:

Either way, you should add at least one test to
bzrlib/tests/test_urlutils.py.

review: Needs Fixing
Reply

Unmerged revisions

5650. By Max Bowsher

Fix bzrlib.urlutils.normalize_url when processing URLs with a scheme, but no
slash in the path - before this, lp:bzr was 'normalized' to lp:bzrlp:bzr (sic).

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/urlutils.py'
2--- bzrlib/urlutils.py 2010-10-21 22:27:43 +0000
3+++ bzrlib/urlutils.py 2011-05-20 10:00:58 +0000
4@@ -307,27 +307,23 @@
5 scheme_end, path_start = _find_scheme_and_separator(url)
6 if scheme_end is None:
7 return local_path_to_url(url)
8- prefix = url[:path_start]
9- path = url[path_start:]
10 if not isinstance(url, unicode):
11 for c in url:
12 if c not in _url_safe_characters:
13 raise errors.InvalidURL(url, 'URLs can only contain specific'
14 ' safe characters (not %r)' % c)
15- path = _url_hex_escapes_re.sub(_unescape_safe_chars, path)
16- return str(prefix + ''.join(path))
17+ url = _url_hex_escapes_re.sub(_unescape_safe_chars, url)
18+ return str(url)
19
20 # We have a unicode (hybrid) url
21- path_chars = list(path)
22-
23- for i in xrange(len(path_chars)):
24- if path_chars[i] not in _url_safe_characters:
25- chars = path_chars[i].encode('utf-8')
26- path_chars[i] = ''.join(
27- ['%%%02X' % ord(c) for c in path_chars[i].encode('utf-8')])
28- path = ''.join(path_chars)
29- path = _url_hex_escapes_re.sub(_unescape_safe_chars, path)
30- return str(prefix + path)
31+ url_chars = list(url)
32+ for i in xrange(len(url_chars)):
33+ if url_chars[i] not in _url_safe_characters:
34+ url_chars[i] = ''.join(
35+ ['%%%02X' % ord(c) for c in url_chars[i].encode('utf-8')])
36+ url = ''.join(url_chars)
37+ url = _url_hex_escapes_re.sub(_unescape_safe_chars, url)
38+ return str(url)
39
40
41 def relative_url(base, other):

Subscribers

People subscribed via source and target branches