[merge] avoid RemoteLockableFiles special handling of branch.conf (and query: purpose of Branch.get_config_file rpc)

[John Arbash Meinel]

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

Martin Pool wrote:
| After setting out to ask a question about this code I have convinced
| myself that it's dead and should be removed.  Here's the question
| first:
|
| RemoteBranchLockableFiles has a special case that when retrieving the
| branch.conf file, it sends a special rpc rather than a regular file
| rpc.  This seems to have no net effect at present as the server just
| gets the file from its backing transport.  Is this used by something
| in Launchpad, or was it a step towards removing vfs-based access?
|
| There is a test TestBranchControlGetBranchConf that the LockableFiles
| does trap access to this file, but it's not clear to me this hook is
| effective in practice, because this file seems to normally be accessed
| through a TransportConfig, which bypasses the LockableFiles
| altogether.  It's never hit in running the test suite.
|
| So for now I'm inclined to remove the special case from
| RemoteBranchLockableFiles, while of course leaving it supported on the
| server in case old clients call it.
|
| In the future if we want to disable vfs methods we would need a way to
| get a branch's configuration.  I think that should be explicitly
| handled at the Branch level: either giving it a
| get_branch_conf_bytes() method (which locally just reads the file), or
| perhaps inverting control and giving it a method to give you back a
| Config object.
|
|


I find the special casing of "if path == 'branch.conf'" to be very ugly, and I'm
happy to see that go.

I don't understand why Branch.get_config() couldn't be updated for
RemoteBranch.get_config() so that you can use the RPC.

I guess because TreeConfig peeks and does:
~        transport = branch.control_files._transport
~        self._config = TransportConfig(transport, 'branch.conf')
~        self.branch = branch

which is buried fairly deep underneath "BranchConfig()".

I certainly would rather see RemoteBranch.get_config() returning a
RemoteBranchConfig object, or a BranchConfig object that is using a better api
than a raw transport + filename.


You patch doesn't quite match my bzr.dev, since there is no @deprecated_method()
for 'get()'.

Also, as you mentioned, we are calling directly to Transport.get() not
LockableFiles.get() so probably this function is not being called. (I assume
this was part of the earlier refactoring to stop going through LockableFiles and
go more directly to the Transport.)

So +1 on removing the RemoteLockableFiles.get() method (though a deprecation
*might* be better).

BB:tweak

John
=:->

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

iEYEARECAAYFAkggVQ8ACgkQJdeBCYSNAANGigCeNFHUXLVrbNbYXwVgyIQzIQAs
3D0AnjLtHHMCXAF9GU/kdqCa0hKSY8Tj
=zHI5
-----END PGP SIGNATURE-----