Code review comment for lp:~gz/bzr/url_unquote_unreserved_842223

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

On 11/16/2011 3:16 PM, Martin Packman wrote:
>> Have you tried using this with a real branch? The test was
>> enforcing that we preserve the existing formatting, so that users
>> doing "bzr up" don't get "failed-to-lock-master-branch" failures
>> when the URL mismatches.
>
> This is what I'm worried about. I went ahead and changed the test
> and sent again anyway because:
>
> * The underlying URL handling is changed, and overall this branch
> is closer to the old behaviour * Backing out parts or doing further
> changes to get closer to the old handling is still possible * For
> serious breakage, I'd hope we'd have more than one failing test.
>
> What manual testing is needed on top of the test suite for those
> old issues? I've just tried a few things with a remote http branch,
> bind, manually editing the branch.conf and so on, and apart from no
> longer getting a notice about a http redirect from '...%7E' ->
> '...~' haven't seen any changes.

$ cat .bzr/branch/branch.conf
bound_location = bzr+ssh://bazaar.launchpad.net/%2Bbranch/bzr/
bound = True
...

$ cd $checkout-of-lp-from-older-bzr
$ bzr up

Interestingly, when I switch to another checkout the new default
branch paths help prevent this from happening:
$ cat .bzr/branch/branch.conf
bound_location = bzr+ssh://bazaar.launchpad.net/+branch/u1db/
bound = True

Also interestingly, if I use:

$ bzr uncommit --local --force
$ bzr up
Updated to revision 6265 of branch
bzr+ssh://bazaar.launchpad.net/%2Bbranch/bzr

Note that your patch did *not* change the final value. Even weirder,
it was still +branch but one version of bzr used %2Bbranch and a newer
version of bzr used +branch.

>
>> What would be nice if we could update the matching logic to
>> normalize-but-preserve. So it would know that "~a" == "%7Ea", but
>> it would leave the URL at whatever it was when it read it.
>
> There is new equivalence handling (not entirely correct perhaps,
> but an improvement) in URL.__eq__ but code is generally still doing
> string comparisons.
>
>> We've flip-flopped at least once on this, and got a bunch of bug
>> reports both times. I really don't want us to do that again.
>
> I agree completely, but the current state of play isn't clear to
> me. What tests were added for the fallout previously?

Things are still working for me, so maybe URL.__eq__ is happy. I don't
have much more time to dig into this, but basically, we want to check
that a checkout created by bzr-2.4 of a readonly branch can be updated
by bzr.dev.

So something like:

$ bzr2.4 co -r -2 lp:~user/project/branch test
$ cd test
$ bzr.dev up

Make sure that ~user isn't yourself, so that you're sure you won't
have write permission on the source branch.

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

iEYEARECAAYFAk7D2PQACgkQJdeBCYSNAAMocgCfcHiAJ6tTJfOMNtRr9IDErFzp
QAkAoK/rZl6bEbsU+PFbpOEWiRCZXTmh
=cdXs
-----END PGP SIGNATURE-----