Bazaar

Merge lp:~mbp/bzr/remove-controlfiles into lp:bzr

  1. remove-controlfiles
  2. Merge into bzr.dev
Proposed by Martin Pool
Status: Work in progress
Proposed branch: lp:~mbp/bzr/remove-controlfiles
Merge into: lp:bzr
Diff against target: 64 lines (+4/-27) (has conflicts)
1 file modified
bzrlib/remote.py (+4/-27)
Text conflict in bzrlib/remote.py
To merge this branch: bzr merge lp:~mbp/bzr/remove-controlfiles
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+18752@code.launchpad.net
To post a comment you must log in.
Revision history for this message
Martin Pool (mbp) wrote :

RemoteBranch.control_files shouldn't need to be exposed; all operations on the branch itself are done at a more abstract level.

I'm not 100% sure this is tested enough to catch callers that are depending on it.

Reply
Revision history for this message
Martin Pool (mbp) wrote :

this causes at least one failure in

  File "/usr/lib/python2.6/dist-packages/testtools/runtest.py", line 128, in _run_user
    return fn(*args)
  File "/usr/lib/python2.6/dist-packages/testtools/testcase.py", line 368, in _run_test_method
    testMethod()
  File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/per_branch/test_locking.py", line 64, in test_01_lock_read
    b = self.get_instrumented_branch()
  File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/per_branch/test_locking.py", line 42, in get_instrumented_branch
    bcf = b.control_files
  File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/lock_helpers.py", line 55, in __getattr__
    return getattr(self._other, attr)
  File "/home/mbp/bzr/remove-controlfiles/bzrlib/remote.py", line 2242, in control_files
    raise NotImplementedError("RemoteBranch.control_files is no longer supported")
NotImplementedError: RemoteBranch.control_files is no longer supported

but I think those tests sholud be locking the branch, not its control_files

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

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

Martin Pool wrote:
> this causes at least one failure in
>
> File "/usr/lib/python2.6/dist-packages/testtools/runtest.py", line 128, in _run_user
> return fn(*args)
> File "/usr/lib/python2.6/dist-packages/testtools/testcase.py", line 368, in _run_test_method
> testMethod()
> File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/per_branch/test_locking.py", line 64, in test_01_lock_read
> b = self.get_instrumented_branch()
> File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/per_branch/test_locking.py", line 42, in get_instrumented_branch
> bcf = b.control_files
> File "/home/mbp/bzr/remove-controlfiles/bzrlib/tests/lock_helpers.py", line 55, in __getattr__
> return getattr(self._other, attr)
> File "/home/mbp/bzr/remove-controlfiles/bzrlib/remote.py", line 2242, in control_files
> raise NotImplementedError("RemoteBranch.control_files is no longer supported")
> NotImplementedError: RemoteBranch.control_files is no longer supported
>
>
> but I think those tests sholud be locking the branch, not its control_files

It is a 'per_branch' 'test_locking' test, which certainly looks like it
should be testing at the Branch level.

The only question in whether '.control_files' is part of the Branch api,
and thus needs to be deprecated rather than just removed.

John
=:->

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

iEYEARECAAYFAktt0scACgkQJdeBCYSNAAPAqwCfcTiV0x+/RIfRHy+oYJx0RtwI
n7gAoJecp6c0PAvbPd5gW8NR4K4gazup
=tCjB
-----END PGP SIGNATURE-----

Reply
Revision history for this message
Martin Pool (mbp) wrote :

> The only question in whether '.control_files' is part of the Branch api, and thus needs to be deprecated rather than just removed.

It probably does count of part of the api. I think it probably shouldn't be, because it's not any particularly distinct part of it, just a historic implementation detail. It probably should be specifically removed.

However our machinery for doing deprecations on things like this that can be called publicly, overridden, and also up-called is a bit weak.

Reply
Revision history for this message
John A Meinel (jameinel) wrote :

I would say you should feel free to update the tests rather than require .control_files to be valid.

review: Approve
Reply
Revision history for this message
Martin Pool (mbp) wrote :

Setting this back to inprogress because it needs nontrivial fixes.

Reply

Unmerged revisions

5012. By Martin Pool <email address hidden>

Remove RemoteBranch._control_files member too

5011. By Martin Pool <email address hidden>

Remove unneeded RemoteBranchFormat.control_files

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/remote.py'
--- bzrlib/remote.py 2010-04-20 04:16:50 +0000
+++ bzrlib/remote.py 2010-04-22 01:41:30 +0000
@@ -40,7 +40,6 @@
40 NoSuchRevision,40 NoSuchRevision,
41 SmartProtocolError,41 SmartProtocolError,
42 )42 )
43from bzrlib.lockable_files import LockableFiles
44from bzrlib.smart import client, vfs, repository as smart_repo43from bzrlib.smart import client, vfs, repository as smart_repo
45from bzrlib.revision import ensure_null, NULL_REVISION44from bzrlib.revision import ensure_null, NULL_REVISION
46from bzrlib.trace import mutter, note, warning45from bzrlib.trace import mutter, note, warning
@@ -2002,26 +2001,6 @@
2002 yield content2001 yield content
20032002
20042003
2005class RemoteBranchLockableFiles(LockableFiles):
2006 """A 'LockableFiles' implementation that talks to a smart server.
2007
2008 This is not a public interface class.
2009 """
2010
2011 def __init__(self, bzrdir, _client):
2012 self.bzrdir = bzrdir
2013 self._client = _client
2014 self._need_find_modes = True
2015 LockableFiles.__init__(
2016 self, bzrdir.get_branch_transport(None),
2017 'lock', lockdir.LockDir)
2018
2019 def _find_modes(self):
2020 # RemoteBranches don't let the client set the mode of control files.
2021 self._dir_mode = None
2022 self._file_mode = None
2023
2024
2025class RemoteBranchFormat(branch.BranchFormat):2004class RemoteBranchFormat(branch.BranchFormat):
20262005
2027 def __init__(self, network_name=None):2006 def __init__(self, network_name=None):
@@ -2182,8 +2161,11 @@
2182 # Fill out expected attributes of branch for bzrlib API users.2161 # Fill out expected attributes of branch for bzrlib API users.
2183 self._clear_cached_state()2162 self._clear_cached_state()
2184 self.base = self.bzrdir.root_transport.base2163 self.base = self.bzrdir.root_transport.base
2164<<<<<<< TREE
2185 self._name = name2165 self._name = name
2186 self._control_files = None2166 self._control_files = None
2167=======
2168>>>>>>> MERGE-SOURCE
2187 self._lock_mode = None2169 self._lock_mode = None
2188 self._lock_token = None2170 self._lock_token = None
2189 self._repo_lock_token = None2171 self._repo_lock_token = None
@@ -2291,12 +2273,7 @@
22912273
2292 @property2274 @property
2293 def control_files(self):2275 def control_files(self):
2294 # Defer actually creating RemoteBranchLockableFiles until its needed,2276 raise NotImplementedError("RemoteBranch.control_files is no longer supported")
2295 # because it triggers an _ensure_real that we otherwise might not need.
2296 if self._control_files is None:
2297 self._control_files = RemoteBranchLockableFiles(
2298 self.bzrdir, self._client)
2299 return self._control_files
23002277
2301 def _get_checkout_format(self):2278 def _get_checkout_format(self):
2302 self._ensure_real()2279 self._ensure_real()