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

[Gary Poster]

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

[John A Meinel (jameinel)]

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

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

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

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

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

[John A Meinel (jameinel)]

I submitted a slightly tweaked version to pqm.

=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py	2011-02-28 23:07:53 +0000
+++ bzrlib/remote.py	2011-08-29 15:54:22 +0000
@@ -1250,12 +1250,13 @@
         # We need to accumulate additional repositories here, to pass them in
         # on various RPC's.
         #
+        # Make the check before we lock: this raises an exception.
+        self._check_fallback_repository(repository)
         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)
         # If self._real_repository was parameterised already (e.g. because a
         # _real_branch had its get_stacked_on_url method called), then the

=== modified file 'bzrlib/repository.py'
--- bzrlib/repository.py	2011-01-13 00:41:19 +0000
+++ bzrlib/repository.py	2011-08-29 15:54:22 +0000
@@ -1046,11 +1046,12 @@
         """
         if not self._format.supports_external_lookups:
             raise errors.UnstackableRepositoryFormat(self._format, self.base)
+        # Make the check before we lock: this raises an exception.
+        self._check_fallback_repository(repository)
         if self.is_locked():
             # This repository will call fallback.unlock() when we transition to
             # the unlocked state, so we make sure to increment the lock count
             repository.lock_read()
-        self._check_fallback_repository(repository)
         self._fallback_repositories.append(repository)
         self.texts.add_fallback_versioned_files(repository.texts)
         self.inventories.add_fallback_versioned_files(repository.inventories)

=== modified file 'bzrlib/tests/per_repository_reference/__init__.py'
--- bzrlib/tests/per_repository_reference/__init__.py	2011-02-09 06:35:00 +0000
+++ bzrlib/tests/per_repository_reference/__init__.py	2011-08-29 15:54:22 +0000
@@ -67,17 +67,25 @@
 class TestIncompatibleStacking(TestCaseWithRepository):
 
     def test_add_fallback_repository_rejects_incompatible(self):
-        # Repository.add_fallback_repository raises IncompatibleRepositories if
-        # you take two repositories in different serializations and try to
-        # stack them.
+        # Repository.add_fallback_repository raises IncompatibleRepositories
+        # if you take two repositories in different serializations and try to
+        # stack them.  If the referring repo is locked, the repo on which
+        # it is stacked should not be after the check fails.
         if self.make_repository('test')._format.supports_chks:
             different_fmt = '1.9'
         else:
             different_fmt = '2a'
+        referring = self.make_repository('referring')
+        referring.lock_read()
+        self.addCleanup(referring.unlock)
         repo = self.make_repository('repo', format=different_fmt)
-        referring = self.make_repository('referring')
+        # Assert precondition.
+        self.assertFalse(repo.is_locked())
+        # Assert action.
         self.assertRaises(errors.IncompatibleRepositories,
                 referring.add_fallback_repository, repo)
+        # Assert postcondition.
+        self.assertFalse(repo.is_locked())
 
 
 def external_reference_test_scenarios():