Merge lp:~vila/bzr/786980-url-aliases into lp:bzr/2.3

[Vincent Ladeuil]

Relax the URL comparison a bit when checking the master branch URL.

My understanding of the issue here is that we end up raising:

  ReadOnlyError: A write attempt was made in read only transaction

because, during a 'bzr update', we compare the source.user.url
(i.e. a '.base' transport attribute) to ``bound_location`` which
comes from a config file.

Between 2.3.1 and 2.3.3 a fix trigger this comparison and, as I
understand it, revealed bad urls in the config files. These urls
themselves have probably been created long before 2.3.1 itself as
inspecting te diff 2.3.1/2.3.3 gives absolutely no clue. This
theory seem to be confirmed by bug reporters being unable to
reproduce the issue when re-creating the checkout from scratch
with 2.3.3.

So I added a test based on this scenario and was able to
reproduce.

From there, I tried various approaches and ended up with a
conservative fix which avoid accessing the master branch itself
(some tests fail otherwise). This means that the fix still relies
on url comparison.

At this point I realize that we may want to more checks and issue
an error if the URL is really broken (because the config file was
edited by hand for example), but that's out of scope for 2.3

[Jonathan Riddell (jr)]

"# bond_location" typo

Suggestions to make the test more foolproof, add instances other than trailing slashes in URL such as including + or ~ characters. Add changes to the branch before the update.

the release note doesn't describe the bug very well, it should mention that it fixes the readonlyerror

"fron" typo

[Jonathan Riddell (jr)]

review: Needs Fixing

[Vincent Ladeuil (vila)]

> # bond_location" typo
> "fron" typo

Fixed.
> the release note doesn't describe the bug very well, it should mention that it fixes the readonlyerror

Bah, thanks. Fixed.

> Suggestions to make the test more foolproof, add instances other than trailing slashes in URL such as including + or ~ characters.

I limit myself to the reproducing test while targetting 2.3, for trunk I think we should even test that total garbage entries in the config file is handled cleanly. Can I get this proposal accepted and address this issue in a followup ?

> Add changes to the branch before the update.

That seems unrelated to the issue, what do you have in mind ?

review: Needs Information

[John A Meinel (jameinel)]

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

On 7/12/2011 6:49 PM, Vincent Ladeuil wrote:
> Review: Needs Information
>> # bond_location" typo
>> "fron" typo
>
> Fixed.
>> the release note doesn't describe the bug very well, it should mention that it fixes the readonlyerror
>
> Bah, thanks. Fixed.
>
>> Suggestions to make the test more foolproof, add instances other than trailing slashes in URL such as including + or ~ characters.
>
> I limit myself to the reproducing test while targetting 2.3, for trunk I think we should even test that total garbage entries in the config file is handled cleanly. Can I get this proposal accepted and address this issue in a followup ?
>
>> Add changes to the branch before the update.
>
> That seems unrelated to the issue, what do you have in mind ?
>

You run "checkout.update()" which did trigger the existing bug. However,
we've had issues with things like this in the past, so I'm recommending
being extra cautious here.

I was actually surprised that "checkout.update()" with nothing to-do
didn't short-cut and skip the fetch. So having 1 more commit on the
master branch so that update must update the local branch/checkout would
be good. It also allows you to add an assert after which checks that the
tree has been updated to the new revision.

I think your code fix is fine. I think history has shown that this is an
area we've had problems with in the past, so adding a few more edge-case
tests is prudent. (handling "+" vs "%2B", or "~" vs "%7E", etc.)
As is, you added "normalize_path" but have nothing that tests that code
path. You do test the trailing-slash because of your "relative_path()==''".

Put another way, you could remove "normalize" and the test would still
pass. Hence we have incomplete test coverage.

(And the other bit is that we could have made your test pass by just
changing 'checkout.update()' to not do a fetch when the last-revision
doesn't change. Which also hints at an incomplete test.)

I personally am ok with landing what you have, except I do feel it is
incomplete. It fixes the bug, and if we were under a crunch to get 2.3.4
out the door for SRU, we could land it. And under the "it is better than
what we have now" we can land it. I'm being extra cautious because we've
had regressions here before.

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

iEYEARECAAYFAk4dR40ACgkQJdeBCYSNAAMnTQCgpKzyrzSliMhTMBhMmolppqPU
B5MAoJDb2cmKHkra8mf9lP2MxmBbP21T
=R7Fv
-----END PGP SIGNATURE-----

[Vincent Ladeuil (vila)]

>>>>> John Arbash Meinel <email address hidden> writes:

    > On 7/12/2011 6:49 PM, Vincent Ladeuil wrote:
    >> Review: Needs Information
    >>> # bond_location" typo
    >>> "fron" typo
    >>
    >> Fixed.
    >>> the release note doesn't describe the bug very well, it should mention that it fixes the readonlyerror
    >>
    >> Bah, thanks. Fixed.
    >>
    >>> Suggestions to make the test more foolproof, add instances other than trailing slashes in URL such as including + or ~ characters.
    >>
    >> I limit myself to the reproducing test while targetting 2.3, for trunk I think we should even test that total garbage entries in the config file is handled cleanly. Can I get this proposal accepted and address this issue in a followup ?
    >>
    >>> Add changes to the branch before the update.
    >>
    >> That seems unrelated to the issue, what do you have in mind ?
    >>

    > You run "checkout.update()" which did trigger the existing
    > bug. However, we've had issues with things like this in the past,
    > so I'm recommending being extra cautious here.

    > I was actually surprised that "checkout.update()" with nothing
    > to-do didn't short-cut and skip the fetch. So having 1 more commit
    > on the master branch so that update must update the local
    > branch/checkout would be good.

That's what the test does.

    > It also allows you to add an assert after which checks that the
    > tree has been updated to the new revision.

I could add that.

    > I think your code fix is fine. I think history has shown that this
    > is an area we've had problems with in the past, so adding a few
    > more edge-case tests is prudent.

I agree with that but I don't think the stable branch is the place to
add more fixes (and I expect to encounter such cases when adding tests).

    > (handling "+" vs "%2B", or "~" vs "%7E", etc.) As is, you added
    > "normalize_path" but have nothing that tests that code path.

Indeed. While I switch away from get_master_branch() I looked a bit
which parts where still needed though. Since the url comes a config
file, it's received as unicode for once and then it should be
url-encoded and finally we should some fuzziness in the path matching, so
normalize_path() + transport.relpath() was the most expedient way to get
there without writing specific code (which would have make the proposal
even harder to review too).

    > You do test the trailing-slash because of your
    > "relative_path()==''".

Yup, but not only.

    > Put another way, you could remove "normalize" and the test would still
    > pass.

No, try it, you'll get an UnicodeError.

    > Hence we have incomplete test coverage.

Indeed, but it's a little less incomplete, *this* bug now has a test to
cover it ;)

    > (And the other bit is that we could have made your test pass by just
    > changing 'checkout.update()' to not do a fetch when the last-revision
    > doesn't change.

Far more intrusive so ruled out for a stable branch.

    > Which also hints at an incomplete test.)

Agreed too.

    > I personally am ok with landing what you have,

Ok.

    > except I do feel it is incomplete.

Which is why I said, le...

[Vincent Ladeuil (vila)]

> It fixes the bug, and if we were under a crunch to get 2.3.4 out

In fact 2.3.4 was planned for 2011-07-07, I just changed that to 2011-07-14 (i.e. today).

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

As agreed on IRC with jam, I'll land this and see for a follow-up targeted to bzr.dev

=== modified file 'bzrlib/branch.py'
--- bzrlib/branch.py	2011-03-18 06:36:57 +0000
+++ bzrlib/branch.py	2011-07-14 12:24:47 +0000
@@ -3466,7 +3466,16 @@
         if local and not bound_location:
             raise errors.LocalRequiresBoundBranch()
         master_branch = None
-        source_is_master = (self.source.user_url == bound_location)
+        source_is_master = False
+        if bound_location:
+            # bound_location comes from a config file, some care has to be
+            # taken to relate it to source.user_url
+            normalized = urlutils.normalize_url(bound_location)
+            try:
+                relpath = self.source.user_transport.relpath(normalized)
+                source_is_master = (relpath == '')
+            except (errors.PathNotChild, errors.InvalidURL):
+                source_is_master = False
         if not local and bound_location and not source_is_master:
             # not pulling from master, so we need to update master.
             master_branch = self.target.get_master_branch(possible_transports)

=== modified file 'bzrlib/tests/test_remote.py'
--- bzrlib/tests/test_remote.py	2011-03-01 07:15:38 +0000
+++ bzrlib/tests/test_remote.py	2011-07-14 12:24:47 +0000
@@ -3216,3 +3216,23 @@
         self.hpss_calls = []
         remote_branch.copy_content_into(local)
         self.assertFalse('Branch.revision_history' in self.hpss_calls)
+
+
+class TestUpdateBoundBranch(tests.TestCaseWithTransport):
+
+    def test_bug_786980(self):
+        self.transport_server = test_server.SmartTCPServer_for_testing
+        wt = self.make_branch_and_tree('master')
+        checkout = wt.branch.create_checkout('checkout')
+        wt.commit('add stuff')
+        last_revid = wt.commit('even more stuff')
+        bound_location = checkout.branch.get_bound_location()
+        # For unclear reasons some users have a bound_location without a final
+        # '/', simulate that by forcing such a value
+        self.assertEndsWith(bound_location, '/')
+        new_location = bound_location.rstrip('/')
+        checkout.branch.set_bound_location(new_location)
+        # bug 786980 was raising ReadOnlyError: A write attempt was made in a
+        # read only transaction during the update()
+        checkout.update()
+        self.assertEquals(last_revid, checkout.last_revision())

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2011-07-05 11:54:54 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2011-07-14 12:24:47 +0000
@@ -33,6 +33,10 @@
 .. Fixes for situations where bzr would previously crash or give incorrect
    or undesirable results.
 
+* Accept some differences for ``bound_location`` from the config files that
+  were leading to a 'ReadOnlyError: A write attempt was made in a read only
+  transaction' error.  (Vincent Ladeuil, #786980)
+
 * Don't fail with traceback if `bzr serve` is running as a service on Windows,
   and there is no USERNAME, nor BZR_EMAIL or other whoami-related environment
   variables set. (Alexander Belchenko, Bug #660174)