Merge lp:~vila/bzr/186920-lp-proxy into lp:bzr

[Vincent Ladeuil]

[Vincent Ladeuil (vila)]

While making things work for lp (which use the default 443 port for https),
the previous patch wasn't correct for non-default ports.

Manual tests showed that some proxies (at least tinyproxy)
will work even, when connecting to an https server running on localhost:25019
by issuing:

    CONNECT localhost:25019:443 HTTP/1.1

and ignoring the additional :443...

This patch will, instead, correctly issue:

    CONNECT localhost:25019

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/186920-lp-proxy into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> While making things work for lp (which use the default 443 port for https),
> the previous patch wasn't correct for non-default ports.
>
> Manual tests showed that some proxies (at least tinyproxy)
> will work even, when connecting to an https server running on localhost:25019
> by issuing:
>
> CONNECT localhost:25019:443 HTTP/1.1
>
> and ignoring the additional :443...
>
> This patch will, instead, correctly issue:
>
> CONNECT localhost:25019

What, no tests? :)

So it seems that 'get_host()' already has the port number. It seems a
little strange to forcibly split it just to put it back together. What
about this:

self.proxied_host = self.get_host()
host, port = urllib.splitport(self.proxied_host)
if port is None:
  # We always need to have a :port when going through some proxies, so
  # set it to the default port here
  if self.type == 'https':
    conn_class = HTTPSConnection
  else:
    ...
  self.proxied_host = '%s:%s' % (host, conn_class.default_port)

Anyway, it doesn't really matter.

  review: approve
  merge: approve

Either way.

John
=:->

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

iEYEARECAAYFAkrwQHQACgkQJdeBCYSNAAPERwCfaPZWtWs/up8B3HA9mW6N0OrZ
0X0AoKYIlGrFl+qiH/9X6xj5VFnDYFwG
=/rOT
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'NEWS'
=== modified file 'bzrlib/transport/http/_urllib2_wrappers.py'
--- bzrlib/transport/http/_urllib2_wrappers.py	2009-10-30 21:02:37 +0000
+++ bzrlib/transport/http/_urllib2_wrappers.py	2009-11-03 14:20:31 +0000
@@ -366,13 +366,16 @@
 
     def set_proxy(self, proxy, type):
         """Set the proxy and remember the proxied host."""
-        # We need to set the default port ourselves way before it gets set
-        # in the HTTP[S]Connection object at build time.
-        if self.type == 'https':
-            conn_class = HTTPSConnection
-        else:
-            conn_class = HTTPConnection
-        self.proxied_host = '%s:%s' % (self.get_host(), conn_class.default_port)
+        host, port = urllib.splitport(self.get_host())
+        if port is None:
+            # We need to set the default port ourselves way before it gets set
+            # in the HTTP[S]Connection object at build time.
+            if self.type == 'https':
+                conn_class = HTTPSConnection
+            else:
+                conn_class = HTTPConnection
+            port = conn_class.default_port
+        self.proxied_host = '%s:%s' % (host, port)
         urllib2.Request.set_proxy(self, proxy, type)