Merge lp://staging/~jelmer/bzr/transport-segments into lp://staging/bzr

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

On 8/15/2011 6:46 PM, Jelmer Vernooij wrote:
> Jelmer Vernooij has proposed merging
> lp:~jelmer/bzr/transport-segments into lp:bzr.
>
> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #380871 in
> Bazaar: "support for colocated branches"
> https://bugs.launchpad.net/bzr/+bug/380871
>
> For more details, see:
> https://code.launchpad.net/~jelmer/bzr/transport-segments/+merge/71583
>
> Allow specifying the colocated branch to use in the branch URL, and
> retrieving the branch name using ControlDir._get_selected_branch.
>
> * Strip off path segment parameters when converting a URL to a path *
> Add ControlDir._get_selected_branch * Add
> Transport.get_segment_parameters * Use URL.__str__ rather than
> _unsplit_url in a couple of places
>

=== modified file 'bzrlib/tests/blackbox/test_upgrade.py'
- --- bzrlib/tests/blackbox/test_upgrade.py 2011-04-05 14:47:30 +0000
+++ bzrlib/tests/blackbox/test_upgrade.py 2011-08-15 16:46:28 +0000
@@ -254,7 +254,7 @@
         t = self.get_transport()
         url = t.base
         out, err = self.run_bzr(['upgrade', '--format=2a', url])
- - backup_dir = 'backup.bzr.~1~'
+ backup_dir = 'backup.bzr.%7E1%7E'
         self.assertEqualDiff("""Upgrading branch %s ...
 starting upgrade of %s
 making backup of %s.bzr
@@ -262,7 +262,7 @@
 starting repository conversion
 repository converted
 finished
- -""" % (url, url, url, url,backup_dir), out)
+""" % (url, url, url, url, backup_dir), out)
         self.assertEqual('', err)

^- Is it possible to not change this? We are informing users where the
old content is going, and nobody is going to parse "backup.bzr.%7E1%7E".

It is fine if we use URL escaped forms internally, but we should be
trying pretty hard to use the form people are going to understand when
writing messages.

         # Base should not keep track of the password
- - self.assertEquals(t.base, 'http://<email address hidden>:2222/path/')
+ self.assertEquals(t.base,
'http://ro%62ey@ex%41mple.com:2222/path/')

^- This seems odd. Given that it was a 'b', I don't see how that would
get escaped into %62. (Maybe it was originally escaped, and now that
form is just being preserved. The diff isn't very clear.)

I don't want to block on .~1~, but I do think it is something we should
be pretty careful about. Presenting escaped URLs to users is pretty high
on the "bad UI" list.

John
=:->

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

iEYEARECAAYFAk5KbIEACgkQJdeBCYSNAAMoGgCgu/FevwMJ0tU+EjaJ7fJMV/sF
dZ8An1dQnTw+K10gUxeRPv0oBU2d4Xw6
=124N
-----END PGP SIGNATURE-----

On 08/16/2011 03:11 PM, John Arbash Meinel wrote:
> On 8/15/2011 6:46 PM, Jelmer Vernooij wrote:
>> Jelmer Vernooij has proposed merging
>> lp:~jelmer/bzr/transport-segments into lp:bzr.
>>
>> Requested reviews: bzr-core (bzr-core) Related bugs: Bug #380871 in
>> Bazaar: "support for colocated branches"
>> https://bugs.launchpad.net/bzr/+bug/380871
>>
>> For more details, see:
>> https://code.launchpad.net/~jelmer/bzr/transport-segments/+merge/71583
>>
>> Allow specifying the colocated branch to use in the branch URL, and
>> retrieving the branch name using ControlDir._get_selected_branch.
>>
>> * Strip off path segment parameters when converting a URL to a path *
>> Add ControlDir._get_selected_branch * Add
>> Transport.get_segment_parameters * Use URL.__str__ rather than
>> _unsplit_url in a couple of places
>>
> === modified file 'bzrlib/tests/blackbox/test_upgrade.py'
> - --- bzrlib/tests/blackbox/test_upgrade.py 2011-04-05 14:47:30 +0000
> +++ bzrlib/tests/blackbox/test_upgrade.py 2011-08-15 16:46:28 +0000
> @@ -254,7 +254,7 @@
> t = self.get_transport()
> url = t.base
> out, err = self.run_bzr(['upgrade', '--format=2a', url])
> - - backup_dir = 'backup.bzr.~1~'
> + backup_dir = 'backup.bzr.%7E1%7E'
> self.assertEqualDiff("""Upgrading branch %s ...
> starting upgrade of %s
> making backup of %s.bzr
> @@ -262,7 +262,7 @@
> starting repository conversion
> repository converted
> finished
> - -""" % (url, url, url, url,backup_dir), out)
> +""" % (url, url, url, url, backup_dir), out)
> self.assertEqual('', err)
>
> ^- Is it possible to not change this? We are informing users where the
> old content is going, and nobody is going to parse "backup.bzr.%7E1%7E".
We should probably be calling urlutils.unescape_for_display() on it
inside of the upgrade code. I'll submit a follow up that does this .
> It is fine if we use URL escaped forms internally, but we should be
> trying pretty hard to use the form people are going to understand when
> writing messages.
>
> # Base should not keep track of the password
> - - self.assertEquals(t.base, 'http://<email address hidden>:2222/path/')
> + self.assertEquals(t.base,
> 'http://ro%62ey@ex%41mple.com:2222/path/')
>
>
> ^- This seems odd. Given that it was a 'b', I don't see how that would
> get escaped into %62. (Maybe it was originally escaped, and now that
> form is just being preserved. The diff isn't very clear.)
Yeah, it's indeed being preserved now, rather than unquoted as we did
previously.
> I don't want to block on .~1~, but I do think it is something we should
> be pretty careful about. Presenting escaped URLs to users is pretty high
> on the "bad UI" list.
Agreed.

Cheers,

Jelmer

I've submitted a fix for the upgrade issue as https://code.launchpad.net/~jelmer/bzr/upgrade-path-display/+merge/71698

sent to pqm by email