Merge lp:~jameinel/bzr/2.0.5-switch-unicode-317778 into lp:bzr

Proposed by John A Meinel
Status: Rejected
Rejected by: Robert Collins
Proposed branch: lp:~jameinel/bzr/2.0.5-switch-unicode-317778
Merge into: lp:bzr
Diff against target: 13 lines (+3/-0)
1 file modified
bzrlib/builtins.py (+3/-0)
To merge this branch: bzr merge lp:~jameinel/bzr/2.0.5-switch-unicode-317778
Reviewer Review Type Date Requested Status
Martin Pool Needs Fixing
Review via email: mp+19121@code.launchpad.net
To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

This is an attempt to propose the patch on bug #317778 as a merge request.

I think it has a lot of stuff missing (needs tests, some manual testing to make sure we haven't regressed for other cases, etc.)

get_transport() has a fair amount of DWIM code to allow users to specify whatever works the best for them, and I don't think that is also in urlutils.escape(). We use that mostly to allow people to specify a Unicode path, while internally we treat things as URLs. But users may also specify the url on the command line.

Anyway, getting this put up as a Merge Proposal in some form seemed better than leaving it as an abandoned patch.

Revision history for this message
Martin Pool (mbp) wrote :

It looks basically ok.

If there is an existing test we can amend to use a unicode name that would be nice.

It seems like really this should be lifted out of branch itself though.

review: Needs Fixing
Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin Pool wrote:
> Review: Needs Fixing
> It looks basically ok.
>
> If there is an existing test we can amend to use a unicode name that would be nice.
>
> It seems like really this should be lifted out of branch itself though.

so the issue is that if Branch.open(to_location) fails, then we fall back to

 urlutils.join(source, '..', to_location)

And at that point 'to_location' is expected to be a URL, but is actually
a Unicode string. This works for pure ascii, but not otherwise.

We could change the 'join' calls to be

urlutils.join(source, '..', DWIM(to_location)) (or just escape it).

I think there are 2 locations, though. also note that we probably want
to change the order of lookup. We set a lightweight location before a
heavyweight one, but we lookup the heavyweight neighbor before the
lightweight one.

John
=:->

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

iEYEARECAAYFAkt0VX8ACgkQJdeBCYSNAAOgzwCfQl6fMjOFyQYTwCKLpfAq+dif
E6QAn16WSWrgu5kQ3KUSa4HbYs6F3Oye
=9qBD
-----END PGP SIGNATURE-----

Revision history for this message
Robert Collins (lifeless) wrote :

John has said that he's not intending to complete this, so changing the MP to rejected for clarity; the bug and branch are still linked and the bug is still open.

Unmerged revisions

4730. By philyoon

Escape the to_location for 'bzr switch'
so that it can handle non-ascii sibling branch names.

Needs tests. Bug #317778

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/builtins.py'
2--- bzrlib/builtins.py 2010-02-11 09:27:55 +0000
3+++ bzrlib/builtins.py 2010-02-11 18:04:20 +0000
4@@ -5487,6 +5487,9 @@
5 except errors.NotBranchError:
6 branch = None
7 had_explicit_nick = False
8+ # This path will get joined with another URL, so we want to escape it
9+ # early.
10+ to_location = urlutils.escape(to_location)
11 if create_branch:
12 if branch is None:
13 raise errors.BzrCommandError('cannot create branch without'