Merge lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3 into lp:bzr/2.3
Status: | Merged |
---|---|
Approved by: | Andrew Bennetts |
Approved revision: | no longer in the source branch. |
Merged at revision: | 5629 |
Proposed branch: | lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3 |
Merge into: | lp:bzr/2.3 |
Diff against target: |
184 lines (+99/-7) 4 files modified
bzrlib/branch.py (+11/-7) bzrlib/tests/per_branch/test_branch.py (+67/-0) bzrlib/tests/per_branch/test_push.py (+17/-0) doc/en/release-notes/bzr-2.3.txt (+4/-0) |
To merge this branch: | bzr merge lp:~spiv/bzr/lockcontention-pushing-tag-733350-2.3 |
Related bugs: |
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Martin Pool | Approve | ||
bzr-core | Pending | ||
Review via email: mp+53947@code.launchpad.net |
Commit message
Fix LockContention error when pushing new tags to bound branch (#733350)
Description of the change
Fixes a regression in 2.3.0: LockContention errors when pushing a new tag to a bound branch, and adds a test that fails without the fix.
The problem is that GenericInterBra
This patch fixes that by adding caching of the return value of get_master_branch (as suggested by a code comment from 2006!), so that both places now lock_write the same branch. This possibly adds some small performance improvement for other cases? I've added tests that the cache is invalidated when it should be (without requiring that branch implementations necessarily cache at all).
The other way to fix this would be by passing an extra parameter to _basic_push to tell it to pass ignore_master=True to tags.merge_to, but this would break plugins like bzr-svn that implement _basic_push (despite it's apparently private name). If you look back in the revision history of this branch you'll see I implemented a hackish solution for this by essentially passing the param via setting and unsetting a private instance variable that our _basic_push implementation would check, but this seemed just too ugly, even if in some ways it's a more conservative fix (and this is targeted to a stable branch).
PEP8: docstrings ought to have just one sentence on the first line. (I guess you accidentally reflowed them.)
Thanks for fixing this and for the clear cover letter. It makes sense to me.
I'd like a docstring mention for the new ivar.
You may want to wait for a second review in case someone knows of problem consequences.
(tweak)