Merge lp:~spiv/bzr/push-relock into lp:bzr

[Andrew Bennetts]

Avoid relocking in cmd_push.

This fixes more relock warnings, this time in cmd_push.

This one is a bit uglier than the previous patches: most of the time cmd_push just needs a read-lock on br_from (the source branch), but occasionally it wants to set the remembered push location. And sometimes it won't know that it will want to until after reading the branch.conf -- i.e. after the read-lock has been acquired.

Unfortunately, we don't have an API to upgrade a read lock to write lock, so instead this patch opens a separate instance of the (already read-locked) branch so it can write-lock that and set the push location. That second instance is then discarded. The downside, as explained in the comment, is that the read-locked version will not see the update, but in this case I don't think that matters.

[Robert Collins (lifeless)]

This won't work with older bzr formats that use a mutex across all processes
on write locks. I don't know if that matters, but it should be at least
documented, I think. Other than that it seems that perhaps its worth doing
an api to upgrade to a write lock?

[Andrew Bennetts (spiv)]

Hmm. I see what you mean (about older formats). I suppose the thing to
do is to catch LockContention, and if it occurs emit a warning to the
user that the push location could not be set due lock contention, and
then let the rest of the code proceed as normal. I think even for older
formats that's probably an improvement in behaviour: I'd rather the
overall push worked immediately even if bzr can't remember the location
because the branch is locked.

I'd love an API to upgrade to a write lock — and I think we'll need one
if we ever decide to pursue a zero relocks as a goal. But it looks like
a moderately large chunk of work. Maybe I'm wrong?

[John A Meinel (jameinel)]

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

Andrew Bennetts wrote:
> Hmm. I see what you mean (about older formats). I suppose the thing to
> do is to catch LockContention, and if it occurs emit a warning to the
> user that the push location could not be set due lock contention, and
> then let the rest of the code proceed as normal. I think even for older
> formats that's probably an improvement in behaviour: I'd rather the
> overall push worked immediately even if bzr can't remember the location
> because the branch is locked.
>
> I'd love an API to upgrade to a write lock — and I think we'll need one
> if we ever decide to pursue a zero relocks as a goal. But it looks like
> a moderately large chunk of work. Maybe I'm wrong?

Well, we already have *some* of that in the code today. Because Dirstate
can be updated within a logical 'read' lock. (status updating the hash
cache). So it has some "try to switch to a write lock" code.

I don't know what that would imply for something like Branch, though.
For Repository, it is trivial, because a write-lock is *still* a no-op
until you 'commit_write_group'. Branch and WT need to actually do
something to write-lock, though.

And we'd also need to decide what to do when the write-lock is already
taken.

John
=:->

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

iEYEARECAAYFAkvRveYACgkQJdeBCYSNAAMvcQCgyiKttmHFv3LjucqKkXbf2IE6
MRUAnR+fgSwPNzlgx+zb3Mt/Ev7ysFpN
=wyUS
-----END PGP SIGNATURE-----

Unmerged revisions

5148. By Andrew Bennetts on 2010-04-20

Tweak NEWS entry wording for consistency.

5147. By Andrew Bennetts on 2010-04-20

Merge lp:bzr.

5146. By Andrew Bennetts on 2010-04-20

Add NEWS entry.

5145. By Andrew Bennetts on 2010-04-12

Try to avoid relocking in cmd_push using an ugly hack for the --remember case.

=== modified file 'NEWS'
--- NEWS	2010-04-20 10:30:30 +0000
+++ NEWS	2010-04-20 14:02:15 +0000
@@ -93,6 +93,9 @@
 * ``bzr pull`` locks the branches and tree only once.
   (Andrew Bennetts)
   
+* ``bzr push`` read-locks the source branch only once.
+  (Andrew Bennetts)
+
 * Index lookups in pack repositories search recently hit pack files first.  
   In repositories with many pack files this can greatly reduce the
   number of files accessed, the number of bytes read, and the number of

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-04-19 13:04:30 +0000
+++ bzrlib/builtins.py	2010-04-20 14:02:15 +0000
@@ -1127,6 +1127,8 @@
         # Get the source branch
         (tree, br_from,
          _unused) = bzrdir.BzrDir.open_containing_tree_or_branch(directory)
+        br_from.lock_read()
+        self.add_cleanup(br_from.unlock)
         # Get the tip's revision_id
         revision = _get_one_revision('push', revision)
         if revision is not None:

=== modified file 'bzrlib/bzrdir.py'
--- bzrlib/bzrdir.py	2010-04-15 15:03:15 +0000
+++ bzrlib/bzrdir.py	2010-04-20 14:02:15 +0000
@@ -61,6 +61,7 @@
     sha_string,
     )
 from bzrlib.push import (
+    _set_push_location,
     PushResult,
     )
 from bzrlib.repofmt import pack_repo
@@ -1283,7 +1284,7 @@
             repository_to.fetch(source.repository, revision_id=revision_id)
             br_to = source.clone(self, revision_id=revision_id)
             if source.get_push_location() is None or remember:
-                source.set_push_location(br_to.base)
+                _set_push_location(source, br_to.base)
             push_result.stacked_on = None
             push_result.branch_push_result = None
             push_result.old_revno = None
@@ -1294,7 +1295,7 @@
         else:
             # We have successfully opened the branch, remember if necessary:
             if source.get_push_location() is None or remember:
-                source.set_push_location(br_to.base)
+                _set_push_location(source, br_to.base)
             try:
                 tree_to = self.open_workingtree()
             except errors.NotLocalUrl:

=== modified file 'bzrlib/push.py'
--- bzrlib/push.py	2010-02-16 16:08:40 +0000
+++ bzrlib/push.py	2010-04-20 14:02:15 +0000
@@ -20,6 +20,7 @@
     builtins,
     branch,
     bzrdir,
+    cleanup,
     errors,
     revision as _mod_revision,
     transport,
@@ -57,6 +58,24 @@
             self.branch_push_result.report(to_file)
 
 
+def _set_push_location(br, location):
+    """Helper for br.set_push_location(location) that tries to workaround br
+    being read-locked.
+    """
+    try:
+        br.set_push_location(location)
+    except errors.ReadOnlyError:
+        # XXX: Hack to work-around lack of a way to upgrade a read-lock to a
+        # write-lock.  This has the down-side that the config cached in 'br'
+        # won't be aware of the changed push location, but the only code that
+        # could notice would be a plugin that overrides cmd_push.
+        br_unlocked = br.bzrdir.open_branch()
+        op = cleanup.OperationWithCleanups(br_unlocked.set_push_location)
+        br_unlocked.lock_write()
+        op.add_cleanup(br_unlocked.unlock)
+        op.run_simple(location)
+
+
 def _show_push_branch(br_from, revision_id, location, to_file, verbose=False,
     overwrite=False, remember=False, stacked_on=None, create_prefix=False,
     use_existing_dir=False):
@@ -131,7 +150,7 @@
         push_result.old_revid = _mod_revision.NULL_REVISION
         push_result.old_revno = 0
         if br_from.get_push_location() is None or remember:
-            br_from.set_push_location(br_to.base)
+            _set_push_location(br_from, br_to.base)
     else:
         if stacked_on is not None:
             warning("Ignoring request for a stacked branch as repository "