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

Revision history for this message
Martin Packman (gz) 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.

> 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?

« Back to merge proposal