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

[Martin Pool]

[Martin Pool (mbp)]

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.

[Martin Pool (mbp)]

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

[John A Meinel (jameinel)]

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

[Martin Pool (mbp)]

> 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.

[John A Meinel (jameinel)]

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

review: Approve

[Martin Pool (mbp)]

Setting this back to inprogress because it needs nontrivial fixes.

Unmerged revisions

5012. By Martin Pool <email address hidden> on 2010-02-06

Remove RemoteBranch._control_files member too

5011. By Martin Pool <email address hidden> on 2010-02-06

Remove unneeded RemoteBranchFormat.control_files

=== 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 @@
     NoSuchRevision,
     SmartProtocolError,
     )
-from bzrlib.lockable_files import LockableFiles
 from bzrlib.smart import client, vfs, repository as smart_repo
 from bzrlib.revision import ensure_null, NULL_REVISION
 from bzrlib.trace import mutter, note, warning
@@ -2002,26 +2001,6 @@
             yield content
 
 
-class RemoteBranchLockableFiles(LockableFiles):
-    """A 'LockableFiles' implementation that talks to a smart server.
-
-    This is not a public interface class.
-    """
-
-    def __init__(self, bzrdir, _client):
-        self.bzrdir = bzrdir
-        self._client = _client
-        self._need_find_modes = True
-        LockableFiles.__init__(
-            self, bzrdir.get_branch_transport(None),
-            'lock', lockdir.LockDir)
-
-    def _find_modes(self):
-        # RemoteBranches don't let the client set the mode of control files.
-        self._dir_mode = None
-        self._file_mode = None
-
-
 class RemoteBranchFormat(branch.BranchFormat):
 
     def __init__(self, network_name=None):
@@ -2182,8 +2161,11 @@
         # Fill out expected attributes of branch for bzrlib API users.
         self._clear_cached_state()
         self.base = self.bzrdir.root_transport.base
+<<<<<<< TREE
         self._name = name
         self._control_files = None
+=======
+>>>>>>> MERGE-SOURCE
         self._lock_mode = None
         self._lock_token = None
         self._repo_lock_token = None
@@ -2291,12 +2273,7 @@
 
     @property
     def control_files(self):
-        # Defer actually creating RemoteBranchLockableFiles until its needed,
-        # because it triggers an _ensure_real that we otherwise might not need.
-        if self._control_files is None:
-            self._control_files = RemoteBranchLockableFiles(
-                self.bzrdir, self._client)
-        return self._control_files
+        raise NotImplementedError("RemoteBranch.control_files is no longer supported")
 
     def _get_checkout_format(self):
         self._ensure_real()