Code review comment for lp://staging/~jelmer/bzr/transport-segments

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