Merge lp:~gary/bzr/bug835035 into lp:bzr/2.3

Proposed by Gary Poster on 2011-08-27
Status: Merged
Approved by: John A Meinel on 2011-08-28
Approved revision: 5661
Merged at revision: 5661
Proposed branch: lp:~gary/bzr/bug835035
Merge into: lp:bzr/2.3
Diff against target: 70 lines (+16/-6)
3 files modified
bzrlib/remote.py (+2/-1)
bzrlib/repository.py (+2/-1)
bzrlib/tests/per_repository_reference/__init__.py (+12/-4)
To merge this branch: bzr merge lp:~gary/bzr/bug835035
Reviewer Review Type Date Requested Status
John A Meinel 2011-08-27 Needs Fixing on 2011-08-29
Review via email: mp+73124@code.launchpad.net

Description of the change

This branch fixes bug 835035 for the 2.3 line. It includes a test and a fix. It's fairly straightforward: we don't want to lock the repository that we might stack on until we are sure it will work.

The XXX comment is my biggest concern: it is being run for all formats, when I really only want it to run for local and remote repositories. I'm not sure how to address this, or if it is necessary to do so.

I also intend to provide changes for 2.4 and trunk, but I'd like to do them one at a time so I can learn what is necessary.

Thank you,

Gary

To post a comment you must log in.
John A Meinel (jameinel) wrote :

I would tend to say that we should just be more aggressive about unlocking if we're holding the lock and it fails. My big concern is that _check_fallback_repository needs to read data from the repository, and we'd like to hold the read lock so that anything we read stays cached.

However, if you can't see an easy way to do that, this patch would be ok.

review: Needs Information
Gary Poster (gary) wrote :

Thanks, John.

The reason I didn't do that is because of this comment (of yours, it seems, from bzr annotate) that you can see in the diff:

             # This repository will call fallback.unlock() when we transition to
             # the unlocked state, so we make sure to increment the lock count

That doesn't happen in the case of bug 835035, but apparently it does in some common case or other that I don't understand (I don't even know what a "fallback" is in this context).

I could simply try making a change like you describe (using a "try/finally" or a "with" within add_fallback_repository) and then run the entire test suite and see if anything breaks. If it doesn't break, I guess I could remove the comment, or supply another that you suggested. Does that sound reasonable, or would you suggest something different?

John A Meinel (jameinel) wrote :

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

On 8/28/2011 2:29 PM, Gary Poster wrote:
> Thanks, John.
>
> The reason I didn't do that is because of this comment (of yours,
> it seems, from bzr annotate) that you can see in the diff:
>
> # This repository will call fallback.unlock() when we transition
> to # the unlocked state, so we make sure to increment the lock
> count
>
> That doesn't happen in the case of bug 835035, but apparently it
> does in some common case or other that I don't understand (I don't
> even know what a "fallback" is in this context).
>
> I could simply try making a change like you describe (using a
> "try/finally" or a "with" within add_fallback_repository) and then
> run the entire test suite and see if anything breaks. If it
> doesn't break, I guess I could remove the comment, or supply
> another that you suggested. Does that sound reasonable, or would
> you suggest something different?

If you have a stacked branch, the one it is stacked *on* is the fallback.

If you look at the code, immediately after checking if it works as a
fallback, it adds it as a fallback.

The original code is:

if self.is_locked():
    # We will call fallback.unlock() when we transition to the unlocked
    # state, so always add a lock here. If a caller passes us a locked
    # repository, they are responsible for unlocking it later.
    repository.lock_read()
self._check_fallback_repository(repository)
self._fallback_repositories.append(repository)

so maybe:

dif_lock = False

if self.is_locked():
    # We will call fallback.unlock() when we transition to the unlocked
    # state, so always add a lock here. If a caller passes us a locked
    # repository, they are responsible for unlocking it later.
    repository.lock_read()
    did_lock = True
try:
    self._check_fallback_repository(repository)
except errors.IncompatibleRepositories:
    if did_lock:
      repository.unlock()
    raise
self._fallback_repositories.append(repository)

However, looking at the code, I think yours is more straigtforward to
understand, and there doesn't seem to be any data that is being cached
that matters.

  merge: approve

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

iEYEARECAAYFAk5aSdYACgkQJdeBCYSNAANtQgCbBTUmTj3T23c8OtXrpd/0obn7
GJgAoNcBe4GdzPu5rAuYs4O9v8rjjlle
=sBIE
-----END PGP SIGNATURE-----

review: Approve
Gary Poster (gary) wrote :

On Aug 28, 2011, at 9:59 AM, John Arbash Meinel wrote:

> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> On 8/28/2011 2:29 PM, Gary Poster wrote:
>> Thanks, John.
>>
>> The reason I didn't do that is because of this comment (of yours,
>> it seems, from bzr annotate) that you can see in the diff:
>>
>> # This repository will call fallback.unlock() when we transition
>> to # the unlocked state, so we make sure to increment the lock
>> count
>>
>> That doesn't happen in the case of bug 835035, but apparently it
>> does in some common case or other that I don't understand (I don't
>> even know what a "fallback" is in this context).
>>
>> I could simply try making a change like you describe (using a
>> "try/finally" or a "with" within add_fallback_repository) and then
>> run the entire test suite and see if anything breaks. If it
>> doesn't break, I guess I could remove the comment, or supply
>> another that you suggested. Does that sound reasonable, or would
>> you suggest something different?
>
> If you have a stacked branch, the one it is stacked *on* is the fallback.

Um. Duh. I should have figured that one out from context. Thanks for the explanation.

> If you look at the code, immediately after checking if it works as a
> fallback, it adds it as a fallback.
>
> The original code is:
>
> if self.is_locked():
> # We will call fallback.unlock() when we transition to the unlocked
> # state, so always add a lock here. If a caller passes us a locked
> # repository, they are responsible for unlocking it later.
> repository.lock_read()
> self._check_fallback_repository(repository)
> self._fallback_repositories.append(repository)
>
> so maybe:
>
> dif_lock = False
>
> if self.is_locked():
> # We will call fallback.unlock() when we transition to the unlocked
> # state, so always add a lock here. If a caller passes us a locked
> # repository, they are responsible for unlocking it later.
> repository.lock_read()
> did_lock = True
> try:
> self._check_fallback_repository(repository)
> except errors.IncompatibleRepositories:
> if did_lock:
> repository.unlock()
> raise
> self._fallback_repositories.append(repository)
>
>
>
> However, looking at the code, I think yours is more straigtforward to
> understand, and there doesn't seem to be any data that is being cached
> that matters.
>
> merge: approve

Cool, thank you. Should I just leave the XXX comment in the test, as is? Or maybe just leave the comment but remove the "XXX"?

John A Meinel (jameinel) wrote :

Thanks for reminding me about the test. To make it a bit better:

1) This should be a bzrlib/tests/per_repository_reference test, because repositories that support "references" are the ones that refer to stacked locations. (I'm not super happy that it isn't per_repository_fallback or whatever.)

2) Can we not hard-code the stacked-on format (format="2a"), and instead use either pack-0.92 which doesn't support stacking at all, or if you need one that supports stacking, but an incompatible serializer, then you can say something like:

if self.repository_format.? == "2a":
  fallback_format = "1.9"
else:
  fallback_format = "2a"

I think that will cover your XXX better. It will allow new implementations to also get tested, and still allow you to have full coverage, including Remote repositories, etc.

review: Needs Fixing
lp:~gary/bzr/bug835035 updated on 2011-08-29
5662. By Gary Poster on 2011-08-29

update previous commit to use existing test, which does not run the tests too many times

John A Meinel (jameinel) wrote :

I submitted a slightly tweaked version to pqm.

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
1=== modified file 'bzrlib/remote.py'
2--- bzrlib/remote.py 2011-02-28 23:07:53 +0000
3+++ bzrlib/remote.py 2011-08-29 15:54:22 +0000
4@@ -1250,12 +1250,13 @@
5 # We need to accumulate additional repositories here, to pass them in
6 # on various RPC's.
7 #
8+ # Make the check before we lock: this raises an exception.
9+ self._check_fallback_repository(repository)
10 if self.is_locked():
11 # We will call fallback.unlock() when we transition to the unlocked
12 # state, so always add a lock here. If a caller passes us a locked
13 # repository, they are responsible for unlocking it later.
14 repository.lock_read()
15- self._check_fallback_repository(repository)
16 self._fallback_repositories.append(repository)
17 # If self._real_repository was parameterised already (e.g. because a
18 # _real_branch had its get_stacked_on_url method called), then the
19
20=== modified file 'bzrlib/repository.py'
21--- bzrlib/repository.py 2011-01-13 00:41:19 +0000
22+++ bzrlib/repository.py 2011-08-29 15:54:22 +0000
23@@ -1046,11 +1046,12 @@
24 """
25 if not self._format.supports_external_lookups:
26 raise errors.UnstackableRepositoryFormat(self._format, self.base)
27+ # Make the check before we lock: this raises an exception.
28+ self._check_fallback_repository(repository)
29 if self.is_locked():
30 # This repository will call fallback.unlock() when we transition to
31 # the unlocked state, so we make sure to increment the lock count
32 repository.lock_read()
33- self._check_fallback_repository(repository)
34 self._fallback_repositories.append(repository)
35 self.texts.add_fallback_versioned_files(repository.texts)
36 self.inventories.add_fallback_versioned_files(repository.inventories)
37
38=== modified file 'bzrlib/tests/per_repository_reference/__init__.py'
39--- bzrlib/tests/per_repository_reference/__init__.py 2011-02-09 06:35:00 +0000
40+++ bzrlib/tests/per_repository_reference/__init__.py 2011-08-29 15:54:22 +0000
41@@ -67,17 +67,25 @@
42 class TestIncompatibleStacking(TestCaseWithRepository):
43
44 def test_add_fallback_repository_rejects_incompatible(self):
45- # Repository.add_fallback_repository raises IncompatibleRepositories if
46- # you take two repositories in different serializations and try to
47- # stack them.
48+ # Repository.add_fallback_repository raises IncompatibleRepositories
49+ # if you take two repositories in different serializations and try to
50+ # stack them. If the referring repo is locked, the repo on which
51+ # it is stacked should not be after the check fails.
52 if self.make_repository('test')._format.supports_chks:
53 different_fmt = '1.9'
54 else:
55 different_fmt = '2a'
56+ referring = self.make_repository('referring')
57+ referring.lock_read()
58+ self.addCleanup(referring.unlock)
59 repo = self.make_repository('repo', format=different_fmt)
60- referring = self.make_repository('referring')
61+ # Assert precondition.
62+ self.assertFalse(repo.is_locked())
63+ # Assert action.
64 self.assertRaises(errors.IncompatibleRepositories,
65 referring.add_fallback_repository, repo)
66+ # Assert postcondition.
67+ self.assertFalse(repo.is_locked())
68
69
70 def external_reference_test_scenarios():

Subscribers

People subscribed via source and target branches