[MERGE][#230550] Fix spurious "No repository present" errors when accessing branches in shared repos via bzr+http.

[John Arbash Meinel]

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

Andrew Bennetts wrote:
| This patch fixes <https://bugs.launchpad.net/bzr/+bug/230550>, which has been
| marked Critical as it appears to be a regression since 1.3.1.
|
| The problem was that although a smart medium can be intrinsically connected to a
| particular location, that location was being tracked in the _SmartClient object
| rather than the SmartClientMedium.  This made it possible for the 'base'
| attribute of the _SmartClient to be inconsistent with actual location the
| 'medium' attribute was connected to, and sure enough the slightly confused logic
| in RemoteTransport/RemoteHTTPTransport caused this to happen.  The result is
| false "No repository present" errors when accessing a shared repository via
| bzr+http, as described in the bug report.
|
| So I've removed the 'base' parameter (and instance attribute) from _SmartClient
| and put it on SmartClientMedium where it belongs.  This removes the room for
| error in tracking the right base to use when sharing a medium between
| RemoteTransport.
|
| This change had some cosmetic knock-on effects in a bunch of
| test_smart_transport tests which has inflated the diff size a bit.  I also took
| the opportunity to make the RemoteTransport.__init__ docstring match reality,
| and to reduce the nested ifs in that method a little.
|
| I haven't added any new tests, but I did add some new assertions to
| RemoteHTTPTransportTestCase that fail without these changes.
|
| -Andrew.
|
|

A lot of these seem like incompatible API changes. Are they all on private
classes? I'm looking at stuff like:

~ class SmartTCPClientMedium(SmartClientStreamMedium):
~     """A client medium using TCP."""

- -    def __init__(self, host, port):
+    def __init__(self, host, port, base):

^- Which now has an extra required parameter. I realize this isn't really a
candidate for subclassing like Repository is. But AFAICT it *is* an API break,
and should thus be documented in NEWS.


SmartClient.remote_path_from_transport

sort of makes me want to cry. Why does it have to know so much about the URLs it
supports. Why does it treat "bzr://" so different from "bzr+http://"? It just
feels dirty, with the knowledge in the wrong location. I won't say it isn't
necessary, and certainly the function already exists and is being used.


Is there any way we could have a couple more smoke tests for round-tripping
'bzr+http' ? I feel like it keeps breaking, and the test suite should be
catching this for us.

Maybe we need implementation tests for "SmartMediums" or something like that.
(Since that would have caught the _remote_is_at_least_1_2, that broke HTTPUrllib
and HTTPPycurl). I certainly feel like our test infrastructure is heavily
lacking if every release keeps breaking bzr+http.


BB:approve

For this patch, but I would really like us to find an answer for these issues.

John
=:->

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

iEYEARECAAYFAkgxptIACgkQJdeBCYSNAAOdTgCgt9Y9kckh8LV+OoSdaCqAECcX
JPcAnjE9/ugGTnl+GX2In6kRvW9T3s4/
=rhWc
-----END PGP SIGNATURE-----