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
=== 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 @@
3466 if local and not bound_location:3466 if local and not bound_location:
3467 raise errors.LocalRequiresBoundBranch()3467 raise errors.LocalRequiresBoundBranch()
3468 master_branch = None3468 master_branch = None
3469 source_is_master = (self.source.user_url == bound_location)3469 source_is_master = False
3470 if bound_location:
3471 # bound_location comes from a config file, some care has to be
3472 # taken to relate it to source.user_url
3473 normalized = urlutils.normalize_url(bound_location)
3474 try:
3475 relpath = self.source.user_transport.relpath(normalized)
3476 source_is_master = (relpath == '')
3477 except (errors.PathNotChild, errors.InvalidURL):
3478 source_is_master = False
3470 if not local and bound_location and not source_is_master:3479 if not local and bound_location and not source_is_master:
3471 # not pulling from master, so we need to update master.3480 # not pulling from master, so we need to update master.
3472 master_branch = self.target.get_master_branch(possible_transports)3481 master_branch = self.target.get_master_branch(possible_transports)
34733482
=== 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 @@
3216 self.hpss_calls = []3216 self.hpss_calls = []
3217 remote_branch.copy_content_into(local)3217 remote_branch.copy_content_into(local)
3218 self.assertFalse('Branch.revision_history' in self.hpss_calls)3218 self.assertFalse('Branch.revision_history' in self.hpss_calls)
3219
3220
3221class TestUpdateBoundBranch(tests.TestCaseWithTransport):
3222
3223 def test_bug_786980(self):
3224 self.transport_server = test_server.SmartTCPServer_for_testing
3225 wt = self.make_branch_and_tree('master')
3226 checkout = wt.branch.create_checkout('checkout')
3227 wt.commit('add stuff')
3228 last_revid = wt.commit('even more stuff')
3229 bound_location = checkout.branch.get_bound_location()
3230 # For unclear reasons some users have a bound_location without a final
3231 # '/', simulate that by forcing such a value
3232 self.assertEndsWith(bound_location, '/')
3233 new_location = bound_location.rstrip('/')
3234 checkout.branch.set_bound_location(new_location)
3235 # bug 786980 was raising ReadOnlyError: A write attempt was made in a
3236 # read only transaction during the update()
3237 checkout.update()
3238 self.assertEquals(last_revid, checkout.last_revision())
32193239
=== 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 @@
33.. Fixes for situations where bzr would previously crash or give incorrect33.. Fixes for situations where bzr would previously crash or give incorrect
34 or undesirable results.34 or undesirable results.
3535
36* Accept some differences for ``bound_location`` from the config files that
37 were leading to a 'ReadOnlyError: A write attempt was made in a read only
38 transaction' error. (Vincent Ladeuil, #786980)
39
36* Don't fail with traceback if `bzr serve` is running as a service on Windows,40* Don't fail with traceback if `bzr serve` is running as a service on Windows,
37 and there is no USERNAME, nor BZR_EMAIL or other whoami-related environment41 and there is no USERNAME, nor BZR_EMAIL or other whoami-related environment
38 variables set. (Alexander Belchenko, Bug #660174)42 variables set. (Alexander Belchenko, Bug #660174)

Subscribers

People subscribed via source and target branches