Merge lp:~mbp/bzr/715000-stacking into lp:bzr

Proposed by Martin Pool on 2011-02-08
Status: Merged
Merged at revision: 5657
Proposed branch: lp:~mbp/bzr/715000-stacking
Merge into: lp:bzr
Diff against target: 243 lines (+146/-3) (has conflicts)
8 files modified
bzr (+4/-0)
bzrlib/__init__.py (+4/-0)
bzrlib/groupcompress.py (+2/-2)
bzrlib/knit.py (+1/-1)
bzrlib/tests/per_repository_reference/__init__.py (+1/-0)
bzrlib/tests/per_repository_reference/test_graph.py (+45/-0)
bzrlib/versionedfile.py (+13/-0)
doc/en/release-notes/bzr-2.3.txt (+76/-0)
Text conflict in bzr
Text conflict in bzrlib/__init__.py
Text conflict in doc/en/release-notes/bzr-2.3.txt
To merge this branch: bzr merge lp:~mbp/bzr/715000-stacking
Reviewer Review Type Date Requested Status
Robert Collins (community) 2011-02-08 Approve on 2011-02-08
Review via email: mp+48886@code.launchpad.net

Description of the change

This tries to fix bug 715000 by making sure we get the graph from all fallback repositories.

This is done off lp:bzr/2.2 to keep our options open, but perhaps we should just merge it to 2.3 or 2.4 and deploy that to Launchpad.

To post a comment you must log in.
Andrew Bennetts (spiv) wrote :

Some quick comments (rather than a comprehensive review):

1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)

2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVersionedFiles. Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_graph_ancestry was broken is subtle/unclear. Do you know which? I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.

Vincent Ladeuil (vila) wrote :

Fwiw, merging up in 2.3 and trunk pass the whole test suite.

As mentioned by poolie in https://bugs.launchpad.net/bzr/+bug/715000/comments/6 we should investigate for sibling bugs.

Note to reviewers: you can safely ignore the conflicts which are caused by the mp being based on 2.2 while targeting trunk.

John A Meinel (jameinel) wrote :

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

On 2/8/2011 3:26 AM, Andrew Bennetts wrote:
> Some quick comments (rather than a comprehensive review):
>
> 1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)
>
> 2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVersionedFiles. Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_graph_ancestry was broken is subtle/unclear. Do you know which? I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.

I agree, I think he meant:

for br in [branch_a, branch_b, branch_c]:

Though I'm fine with a test of just branch_c, if we are just wanting to
test the transitivity of operations.

I wouldn't be surprised if there are some bugs in other places, but I
think things like "get_parent_map" already recurse via the public api,
rather than recursing to find the transitive fallbacks, and then
iterating. (so branch_c.repo.get_parent_map() calls
branch_b.repo.get_parent_map() which calls
branch_a.repo.get_parent_map() rather than
branch_c.repo.get_parent_map() going through all fallbacks directly.)

I sort of like the public api recursion approach more than the
_transitive_fallbacks approach. If only because it means that
RepoA.do_something() isn't poking at RepoB._fallback_repositories. It
only looks at its own private attributes, and a fallbacks public attributes.

transitive_fallbacks does ok with this, if you consider that the
function is actually a public Repository api. (Which I think it should
be, and should be tested as such, with a per_repository implementation
test.)

Anyway, I'm fine with this as a fix for what we are currently
encountering. I think we can do an audit of the codebase for everywhere
that _fallback_vfs or _fallback_repositories is being encountered, but
that can be separate to Martin's patch.

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

iEYEARECAAYFAk1RrJkACgkQJdeBCYSNAAMBMACcDC92UyAzUi1q7Zekhf9qmBUD
edEAniQplUIN6tovAJ+R+VgAr3R9N+ry
=HM+5
-----END PGP SIGNATURE-----

Martin Pool (mbp) wrote :

On 8 February 2011 20:26, Andrew Bennetts <email address hidden> wrote:
> Some quick comments (rather than a comprehensive review):
>
> 1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)

Ah, for the sake of easier testing I had cut out the others. It
should test all of them.

> 2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVersionedFiles.  Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_graph_ancestry was broken is subtle/unclear.  Do you know which?  I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.

I was hoping someone would tell me :-)

More seriously, I do think all the others need to be checked too, and
also by searching the bug tracker we might find reports where similar
things are failing.

Martin Pool (mbp) wrote :

On 9 February 2011 07:50, John Arbash Meinel <email address hidden> wrote:

> transitive_fallbacks does ok with this, if you consider that the
> function is actually a public Repository api. (Which I think it should
> be, and should be tested as such, with a per_repository implementation
> test.)

I was going to recurse through the public API, and originally took
that approach, but the problem was that in this case the public API
returns a KnownGraph, and I couldn't see how to combine several of
those objects. Perhaps there is a way?

John A Meinel (jameinel) wrote :

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

On 2/8/2011 4:49 PM, Martin Pool wrote:
> On 9 February 2011 07:50, John Arbash Meinel <email address hidden> wrote:
>
>> transitive_fallbacks does ok with this, if you consider that the
>> function is actually a public Repository api. (Which I think it should
>> be, and should be tested as such, with a per_repository implementation
>> test.)
>
> I was going to recurse through the public API, and originally took
> that approach, but the problem was that in this case the public API
> returns a KnownGraph, and I couldn't see how to combine several of
> those objects. Perhaps there is a way?
>

Not trivially. We have code to add new entries one-by-one, but that
isn't great.

If we wanted to tease this apart more, something that returns a dict,
and then wrapping that in a KnownGraph would be enough. Or expose more
of _find_ancestor (I don't remember the exact 'take one step' function
name).

John
=:->

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

iEYEARECAAYFAk1RzqsACgkQJdeBCYSNAAMXPACeIOmDqkT8KfkedqNEGn6Dv89u
9moAoLpnFYmQ0Nnv2rmhGrUJN+x60pkM
=YQNy
-----END PGP SIGNATURE-----

John A Meinel (jameinel) wrote :

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

On 2/8/2011 4:46 PM, Martin Pool wrote:
> On 8 February 2011 20:26, Andrew Bennetts <email address hidden> wrote:
>> Some quick comments (rather than a comprehensive review):
>>
>> 1. "for br in [branch_c]:" is a weird expression (why have a loop when there's exactly 1 item?)
>
> Ah, for the sake of easier testing I had cut out the others. It
> should test all of them.
>
>> 2. this fix appears reasonable, but you only fix one method when there are many other uses of self._fallback_vfs in e.g. GroupCompressVersionedFiles. Either there are more bugs waiting to be fixed here (hopefully easy ones to reveal with tests similar to the one you add in this patch), or the reason why only get_known_graph_ancestry was broken is subtle/unclear. Do you know which? I'm a bit worried that this may correct the immediate traceback without actually making the Launchpad scanner functional because of further issues.
>
> I was hoping someone would tell me :-)
>
> More seriously, I do think all the others need to be checked too, and
> also by searching the bug tracker we might find reports where similar
> things are failing.
>

I honestly haven't seen much failure of this. Mostly because stacking is
rare outside of launchpad, and stacking on stacked is a whole next-level
of unlikely. Since not even launchpad creates branches like this easily.

I wouldn't be surprised to have internal bugs that are latent. Doing a
quick grep doesn't show anything obvious.

_fallback_vfs is only mentioned in bzrlib/groupcompress.py and knit.py
(since those are the only VersionedFiles that actually support it, it
should make sense.)

Specific references in groupcompress:

get_known_graph_ancestry(), the code in question
_get_parent_map_with_sources(), uses the public api
_find_from_fallback(), public_api
keys(), public_api

in knit.py:

_logical_check(), public_api
get_known_graph_ancestry(), current code
_get_parent_map_with_sources(), public_api (copy & paste because of
                                            inheritance issues)
_get_remaining_record_stream, public_api
get_sha1s(), public_api
insert_record_stream(), just used to tell if we have a stacking
  boundary, and need to do extra sanity checks on the stream
iter_lines_added_or_present_in_keys(), public_api, probably should be
  deprecated anyway
keys(), public_api

_ContentMapGenerator._work, public_api
_KnitAnnotator._get_needed_texts, only used to signal fallback to
  non-optimized code

In the end, what we could do is promote "self._index.find_ancestry()" to
be a public attribute of VersionedFiles rather than hidden on the _index
object, and then it could be done via public api, rather than chaining
the transitive closure of fallback_vfs. That would allow us to
consistently go via public api. Albeit by using a slightly different api
that can chain.

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

iEYEARECAAYFAk1R0SQACgkQJdeBCYSNAAMZIwCeNQtpjB/0NPaQjNb98wW0Vxg2
T9IAnAl/pJhAoqWGbDgXq8ov5R3oyxkW
=i3cD
-----END PGP SIGNATURE-----

Robert Collins (lifeless) wrote :

This looks fine to me; I agree that renaming _fallback_vfs to _immediate_fallbacks or something would be an improvement.

review: Approve
Martin Pool (mbp) wrote :

Since lp is running 2.2 and wants to deploy this on top of 2.2, and since people seem to agree this is safe (and the change is smaller than I suspected it might be), I'll merge it there.

lp:~mbp/bzr/715000-stacking updated on 2011-02-09
5127. By Martin Pool on 2011-02-09

News for bug 715000

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzr'
--- bzr 2011-02-08 13:56:49 +0000
+++ bzr 2011-02-09 04:20:36 +0000
@@ -23,7 +23,11 @@
23import warnings23import warnings
2424
25# update this on each release25# update this on each release
26<<<<<<< TREE
26_script_version = (2, 4, 0)27_script_version = (2, 4, 0)
28=======
29_script_version = (2, 2, 5)
30>>>>>>> MERGE-SOURCE
2731
28try:32try:
29 version_info = sys.version_info33 version_info = sys.version_info
3034
=== modified file 'bzrlib/__init__.py'
--- bzrlib/__init__.py 2011-02-08 13:56:49 +0000
+++ bzrlib/__init__.py 2011-02-09 04:20:36 +0000
@@ -52,7 +52,11 @@
52# Python version 2.0 is (2, 0, 0, 'final', 0)." Additionally we use a52# Python version 2.0 is (2, 0, 0, 'final', 0)." Additionally we use a
53# releaselevel of 'dev' for unreleased under-development code.53# releaselevel of 'dev' for unreleased under-development code.
5454
55<<<<<<< TREE
55version_info = (2, 4, 0, 'dev', 1)56version_info = (2, 4, 0, 'dev', 1)
57=======
58version_info = (2, 2, 5, 'dev', 0)
59>>>>>>> MERGE-SOURCE
5660
57# API compatibility version61# API compatibility version
58api_minimum_version = (2, 4, 0)62api_minimum_version = (2, 4, 0)
5963
=== modified file 'bzrlib/groupcompress.py'
--- bzrlib/groupcompress.py 2010-09-26 20:45:28 +0000
+++ bzrlib/groupcompress.py 2011-02-09 04:20:36 +0000
@@ -1,4 +1,4 @@
1# Copyright (C) 2008, 2009, 2010 Canonical Ltd1# Copyright (C) 2008-2011 Canonical Ltd
2#2#
3# This program is free software; you can redistribute it and/or modify3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by4# it under the terms of the GNU General Public License as published by
@@ -1312,7 +1312,7 @@
1312 # KnitVersionedFiles.get_known_graph_ancestry, but they don't share1312 # KnitVersionedFiles.get_known_graph_ancestry, but they don't share
1313 # ancestry.1313 # ancestry.
1314 parent_map, missing_keys = self._index.find_ancestry(keys)1314 parent_map, missing_keys = self._index.find_ancestry(keys)
1315 for fallback in self._fallback_vfs:1315 for fallback in self._transitive_fallbacks():
1316 if not missing_keys:1316 if not missing_keys:
1317 break1317 break
1318 (f_parent_map, f_missing_keys) = fallback._index.find_ancestry(1318 (f_parent_map, f_missing_keys) = fallback._index.find_ancestry(
13191319
=== modified file 'bzrlib/knit.py'
--- bzrlib/knit.py 2011-01-11 20:42:36 +0000
+++ bzrlib/knit.py 2011-02-09 04:20:36 +0000
@@ -1196,7 +1196,7 @@
1196 def get_known_graph_ancestry(self, keys):1196 def get_known_graph_ancestry(self, keys):
1197 """Get a KnownGraph instance with the ancestry of keys."""1197 """Get a KnownGraph instance with the ancestry of keys."""
1198 parent_map, missing_keys = self._index.find_ancestry(keys)1198 parent_map, missing_keys = self._index.find_ancestry(keys)
1199 for fallback in self._fallback_vfs:1199 for fallback in self._transitive_fallbacks():
1200 if not missing_keys:1200 if not missing_keys:
1201 break1201 break
1202 (f_parent_map, f_missing_keys) = fallback._index.find_ancestry(1202 (f_parent_map, f_missing_keys) = fallback._index.find_ancestry(
12031203
=== modified file 'bzrlib/tests/per_repository_reference/__init__.py'
--- bzrlib/tests/per_repository_reference/__init__.py 2011-01-10 22:47:59 +0000
+++ bzrlib/tests/per_repository_reference/__init__.py 2011-02-09 04:20:36 +0000
@@ -105,6 +105,7 @@
105 'bzrlib.tests.per_repository_reference.test_fetch',105 'bzrlib.tests.per_repository_reference.test_fetch',
106 'bzrlib.tests.per_repository_reference.test_get_record_stream',106 'bzrlib.tests.per_repository_reference.test_get_record_stream',
107 'bzrlib.tests.per_repository_reference.test_get_rev_id_for_revno',107 'bzrlib.tests.per_repository_reference.test_get_rev_id_for_revno',
108 'bzrlib.tests.per_repository_reference.test_graph',
108 'bzrlib.tests.per_repository_reference.test_initialize',109 'bzrlib.tests.per_repository_reference.test_initialize',
109 'bzrlib.tests.per_repository_reference.test_unlock',110 'bzrlib.tests.per_repository_reference.test_unlock',
110 ]111 ]
111112
=== added file 'bzrlib/tests/per_repository_reference/test_graph.py'
--- bzrlib/tests/per_repository_reference/test_graph.py 1970-01-01 00:00:00 +0000
+++ bzrlib/tests/per_repository_reference/test_graph.py 2011-02-09 04:20:36 +0000
@@ -0,0 +1,45 @@
1# Copyright (C) 2011 Canonical Ltd
2#
3# This program is free software; you can redistribute it and/or modify
4# it under the terms of the GNU General Public License as published by
5# the Free Software Foundation; either version 2 of the License, or
6# (at your option) any later version.
7#
8# This program is distributed in the hope that it will be useful,
9# but WITHOUT ANY WARRANTY; without even the implied warranty of
10# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
11# GNU General Public License for more details.
12#
13# You should have received a copy of the GNU General Public License
14# along with this program; if not, write to the Free Software
15# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA
16
17
18"""Tests for graph operations on stacked repositories."""
19
20
21from bzrlib.tests.per_repository import TestCaseWithRepository
22
23
24class TestGraph(TestCaseWithRepository):
25
26 def test_get_known_graph_ancestry_stacked(self):
27 """get_known_graph_ancestry works correctly on stacking.
28
29 See <https://bugs.launchpad.net/bugs/715000>.
30 """
31 branch_a, branch_b, branch_c, revid_1 = self.make_double_stacked_branches()
32 for br in [branch_c]:
33 self.assertEquals(
34 [revid_1],
35 br.repository.get_known_graph_ancestry([revid_1]).topo_sort())
36
37 def make_double_stacked_branches(self):
38 wt_a = self.make_branch_and_tree('a')
39 branch_a = wt_a.branch
40 branch_b = self.make_branch('b')
41 branch_b.set_stacked_on_url('../a')
42 branch_c = self.make_branch('c')
43 branch_c.set_stacked_on_url('../b')
44 revid_1 = wt_a.commit('first commit')
45 return branch_a, branch_b, branch_c, revid_1
046
=== modified file 'bzrlib/versionedfile.py'
--- bzrlib/versionedfile.py 2010-08-28 15:34:24 +0000
+++ bzrlib/versionedfile.py 2011-02-09 04:20:36 +0000
@@ -1193,6 +1193,19 @@
1193 def _extract_blocks(self, version_id, source, target):1193 def _extract_blocks(self, version_id, source, target):
1194 return None1194 return None
11951195
1196 def _transitive_fallbacks(self):
1197 """Return the whole stack of fallback versionedfiles.
1198
1199 This VersionedFiles may have a list of fallbacks, but it doesn't
1200 necessarily know about the whole stack going down, and it can't know
1201 at open time because they may change after the objects are opened.
1202 """
1203 all_fallbacks = []
1204 for a_vfs in self._fallback_vfs:
1205 all_fallbacks.append(a_vfs)
1206 all_fallbacks.extend(a_vfs._transitive_fallbacks())
1207 return all_fallbacks
1208
11961209
1197class ThunkedVersionedFiles(VersionedFiles):1210class ThunkedVersionedFiles(VersionedFiles):
1198 """Storage for many versioned files thunked onto a 'VersionedFile' class.1211 """Storage for many versioned files thunked onto a 'VersionedFile' class.
11991212
=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt 2011-02-08 13:56:49 +0000
+++ doc/en/release-notes/bzr-2.3.txt 2011-02-09 04:20:36 +0000
@@ -2,6 +2,7 @@
2Bazaar Release Notes2Bazaar Release Notes
3####################3####################
44
5<<<<<<< TREE
5.. toctree::6.. toctree::
6 :maxdepth: 17 :maxdepth: 1
78
@@ -14,6 +15,81 @@
14*****************************15*****************************
1516
16.. These may require users to change the way they use Bazaar.17.. These may require users to change the way they use Bazaar.
18=======
19.. contents:: List of Releases
20 :depth: 1
21
22bzr 2.2.5
23#########
24
25:Codename: Suggestions welcome
26:2.2.5: NOT RELEASED YET
27
28Compatibility Breaks
29********************
30
31New Features
32************
33
34Bug Fixes
35*********
36
37* Correctly handle ``bzr log`` and `get_known_graph_ancestry` on a
38 doubly-stacked branch.
39 (James Westby, Martin Pool, #715000)
40
41Improvements
42************
43
44Documentation
45*************
46
47API Changes
48***********
49
50Internals
51*********
52
53Testing
54*******
55
56
57bzr 2.2.4
58#########
59
60:2.2.4: 2011-02-04
61
62This is a bugfix release. Only one bug has been fixed, a regression from 2.2.3
63involving only certain operations with launchpad. Upgrading is recommended for
64all users on earlier 2.2 releases.
65
66Bug Fixes
67*********
68
69* Fix communications with the Launchpad web service when using
70 launchpadlib >= 1.5.5. This was a latent bug in bzr's communication
71 with Launchpad's production instance, which only became a problem when
72 the default instance was switched from edge to production in bzr 2.2.3.
73 (Max Bowsher, #707075)
74
75
76bzr 2.2.3
77#########
78
79:2.2.3: 2011-01-20
80
81This is a bugfix release. Upgrading is recommended for all users
82on earlier 2.2 releases.
83
84Compatibility Breaks
85********************
86
87* Launchpad has announced that the ``edge.launchpad.net`` instance is
88 deprecated and may be shut down in the future
89 <http://blog.launchpad.net/general/edge-is-deprecated>. Bazaar has therefore
90 been updated in this release to talk to the main (``launchpad.net``) servers,
91 rather than the ``edge`` ones. (Vincent Ladeuil, #583667)
92>>>>>>> MERGE-SOURCE
1793
18New Features94New Features
19************95************