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

Proposed by Vincent Ladeuil on 2011-07-08
Status: Merged
Approved by: Vincent Ladeuil on 2011-07-14
Approved revision: 5656
Merged at revision: 5655
Proposed branch: lp:~vila/bzr/786980-url-aliases
Merge into: lp:bzr/2.3
Diff against target: 64 lines (+34/-1)
3 files modified
bzrlib/branch.py (+10/-1)
bzrlib/tests/test_remote.py (+20/-0)
doc/en/release-notes/bzr-2.3.txt (+4/-0)
To merge this branch: bzr merge lp:~vila/bzr/786980-url-aliases
Reviewer Review Type Date Requested Status
Vincent Ladeuil Needs Information on 2011-07-12
Jonathan Riddell (community) 2011-07-08 Needs Fixing on 2011-07-12
Review via email: mp+67351@code.launchpad.net

Commit message

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

Description of the change

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

To post a comment you must log in.
Jonathan Riddell (jr) wrote :

"# 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) wrote :

> # 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
lp:~vila/bzr/786980-url-aliases updated on 2011-07-12
5655. By Vincent Ladeuil on 2011-07-12

Fix typos caught by Jonathan.

John A Meinel (jameinel) wrote :

-----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) wrote :
Download full text (3.7 KiB)

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

Read more...

Vincent Ladeuil (vila) wrote :

> 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).

lp:~vila/bzr/786980-url-aliases updated on 2011-07-14
5656. By Vincent Ladeuil on 2011-07-14

Ensure the checkout is updated as expected.

Vincent Ladeuil (vila) wrote :

sent to pqm by email

Vincent Ladeuil (vila) wrote :

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

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/branch.py'
2--- bzrlib/branch.py 2011-03-18 06:36:57 +0000
3+++ bzrlib/branch.py 2011-07-14 12:24:47 +0000
4@@ -3466,7 +3466,16 @@
5 if local and not bound_location:
6 raise errors.LocalRequiresBoundBranch()
7 master_branch = None
8- source_is_master = (self.source.user_url == bound_location)
9+ source_is_master = False
10+ if bound_location:
11+ # bound_location comes from a config file, some care has to be
12+ # taken to relate it to source.user_url
13+ normalized = urlutils.normalize_url(bound_location)
14+ try:
15+ relpath = self.source.user_transport.relpath(normalized)
16+ source_is_master = (relpath == '')
17+ except (errors.PathNotChild, errors.InvalidURL):
18+ source_is_master = False
19 if not local and bound_location and not source_is_master:
20 # not pulling from master, so we need to update master.
21 master_branch = self.target.get_master_branch(possible_transports)
22
23=== modified file 'bzrlib/tests/test_remote.py'
24--- bzrlib/tests/test_remote.py 2011-03-01 07:15:38 +0000
25+++ bzrlib/tests/test_remote.py 2011-07-14 12:24:47 +0000
26@@ -3216,3 +3216,23 @@
27 self.hpss_calls = []
28 remote_branch.copy_content_into(local)
29 self.assertFalse('Branch.revision_history' in self.hpss_calls)
30+
31+
32+class TestUpdateBoundBranch(tests.TestCaseWithTransport):
33+
34+ def test_bug_786980(self):
35+ self.transport_server = test_server.SmartTCPServer_for_testing
36+ wt = self.make_branch_and_tree('master')
37+ checkout = wt.branch.create_checkout('checkout')
38+ wt.commit('add stuff')
39+ last_revid = wt.commit('even more stuff')
40+ bound_location = checkout.branch.get_bound_location()
41+ # For unclear reasons some users have a bound_location without a final
42+ # '/', simulate that by forcing such a value
43+ self.assertEndsWith(bound_location, '/')
44+ new_location = bound_location.rstrip('/')
45+ checkout.branch.set_bound_location(new_location)
46+ # bug 786980 was raising ReadOnlyError: A write attempt was made in a
47+ # read only transaction during the update()
48+ checkout.update()
49+ self.assertEquals(last_revid, checkout.last_revision())
50
51=== modified file 'doc/en/release-notes/bzr-2.3.txt'
52--- doc/en/release-notes/bzr-2.3.txt 2011-07-05 11:54:54 +0000
53+++ doc/en/release-notes/bzr-2.3.txt 2011-07-14 12:24:47 +0000
54@@ -33,6 +33,10 @@
55 .. Fixes for situations where bzr would previously crash or give incorrect
56 or undesirable results.
57
58+* Accept some differences for ``bound_location`` from the config files that
59+ were leading to a 'ReadOnlyError: A write attempt was made in a read only
60+ transaction' error. (Vincent Ladeuil, #786980)
61+
62 * Don't fail with traceback if `bzr serve` is running as a service on Windows,
63 and there is no USERNAME, nor BZR_EMAIL or other whoami-related environment
64 variables set. (Alexander Belchenko, Bug #660174)

Subscribers

People subscribed via source and target branches