Submitter | Yuya Nishihara |
---|---|
Date | May 22, 2014, 1:55 p.m. |
Message ID | <b8bca4e9640673f43e90.1400766917@mimosa> |
Download | mbox | patch |
Permalink | /patch/4848/ |
State | Accepted |
Headers | show |
Comments
On 05/22/2014 03:55 PM, Yuya Nishihara wrote: > # HG changeset patch > # User Yuya Nishihara <yuya@tcha.org> > # Date 1400763926 -32400 > # Thu May 22 22:05:26 2014 +0900 > # Branch stable > # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 > # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 > proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 Which Python versions has this been tested / reviewed with? Do the test suite have test coverage for this? /Mads > With Python 2.7.7rc1, "hg pull" through HTTP CONNECT tunnel fails due to the > removal of _set_hostport [1]. > > ... > File "mercurial/url.py", line 372, in https_open > return self.do_open(self._makeconnection, req) > ... > File "mercurial/url.py", line 342, in connect > _generic_proxytunnel(self) > File "mercurial/url.py", line 228, in _generic_proxytunnel > self._set_hostport(self.host, self.port) > AttributeError: httpsconnection instance has no attribute '_set_hostport' > > self._set_hostport(self.host, self.port) should be noop and can be removed > because: > > - _set_hostport() [2] was the function to parse "host:port" string and > set them to self.host and self.port, > - and (self.host, self.port) pair should be valid since connect() is called > prior to _generic_proxytunnel(). > > [1]: http://hg.python.org/cpython/rev/568041fd8090 > [2]: http://hg.python.org/cpython/file/3a1db0d2747e/Lib/httplib.py#l721 > > diff --git a/mercurial/url.py b/mercurial/url.py > --- a/mercurial/url.py > +++ b/mercurial/url.py > @@ -225,7 +225,6 @@ def _generic_proxytunnel(self): > proxyheaders = dict( > [(x, self.headers[x]) for x in self.headers > if x.lower().startswith('proxy-')]) > - self._set_hostport(self.host, self.port) > self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport) > for header in proxyheaders.iteritems(): > self.send('%s: %s\r\n' % header) > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote: > On 05/22/2014 03:55 PM, Yuya Nishihara wrote: > > # HG changeset patch > > # User Yuya Nishihara <yuya@tcha.org> > > # Date 1400763926 -32400 > > # Thu May 22 22:05:26 2014 +0900 > > # Branch stable > > # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 > > # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 > > proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 > > Which Python versions has this been tested / reviewed with? Tested with - Python 2.7.6 (Debian python2.7 2.7.6-8) - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1) and checked _set_hostport implementation at v2.4, v2.5 and v2.6. > Do the test suite have test coverage for this? test-https.t should fail with Python 2.7.7rc1. Regards,
On 05/22/2014 04:34 PM, Yuya Nishihara wrote: > On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote: >> On 05/22/2014 03:55 PM, Yuya Nishihara wrote: >>> # HG changeset patch >>> # User Yuya Nishihara <yuya@tcha.org> >>> # Date 1400763926 -32400 >>> # Thu May 22 22:05:26 2014 +0900 >>> # Branch stable >>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 >>> # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 >>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 >> Which Python versions has this been tested / reviewed with? > Tested with > > - Python 2.7.6 (Debian python2.7 2.7.6-8) > - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1) > > and checked _set_hostport implementation at v2.4, v2.5 and v2.6. FWIW, it is relatively simple and fast to test different Python versions using contrib/Makefile.python . >> Do the test suite have test coverage for this? > test-https.t should fail with Python 2.7.7rc1. Will it also verify that the effect of _set_hostport still is achieved? /Mads
On Thu, 22 May 2014 17:16:42 +0200, Mads Kiilerich wrote: > On 05/22/2014 04:34 PM, Yuya Nishihara wrote: > > On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote: > >> On 05/22/2014 03:55 PM, Yuya Nishihara wrote: > >>> # HG changeset patch > >>> # User Yuya Nishihara <yuya@tcha.org> > >>> # Date 1400763926 -32400 > >>> # Thu May 22 22:05:26 2014 +0900 > >>> # Branch stable > >>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 > >>> # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 > >>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 > >> Which Python versions has this been tested / reviewed with? > > Tested with > > > > - Python 2.7.6 (Debian python2.7 2.7.6-8) > > - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1) > > > > and checked _set_hostport implementation at v2.4, v2.5 and v2.6. > > FWIW, it is relatively simple and fast to test different Python versions > using contrib/Makefile.python . Thanks. "hg clone" through tinyproxy.py worked on Python 2.4.6. But I couldn't figure out a good way to write test of HTTP CONNECT tunnel for Python 2.4 or 2.5, without ssl module. > >> Do the test suite have test coverage for this? > > test-https.t should fail with Python 2.7.7rc1. > > Will it also verify that the effect of _set_hostport still is achieved? It isn't easy to answer. AFAIK, "_set_hostport(self.host, self.port)" is "self.host, self.port = self.host, self.port", and it seems the result of _set_hostport isn't used because the connection is already established, but it might be wrong in a certain case. Regards,
On 05/23/2014 04:57 PM, Yuya Nishihara wrote: > On Thu, 22 May 2014 17:16:42 +0200, Mads Kiilerich wrote: >> On 05/22/2014 04:34 PM, Yuya Nishihara wrote: >>> On Thu, 22 May 2014 16:03:27 +0200, Mads Kiilerich wrote: >>>> On 05/22/2014 03:55 PM, Yuya Nishihara wrote: >>>>> # HG changeset patch >>>>> # User Yuya Nishihara <yuya@tcha.org> >>>>> # Date 1400763926 -32400 >>>>> # Thu May 22 22:05:26 2014 +0900 >>>>> # Branch stable >>>>> # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 >>>>> # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 >>>>> proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 >>>> Which Python versions has this been tested / reviewed with? >>> Tested with >>> >>> - Python 2.7.6 (Debian python2.7 2.7.6-8) >>> - Python 2.7.7rc1 (Debian python2.7 2.7.7~rc1-1) >>> >>> and checked _set_hostport implementation at v2.4, v2.5 and v2.6. >> FWIW, it is relatively simple and fast to test different Python versions >> using contrib/Makefile.python . > Thanks. > > "hg clone" through tinyproxy.py worked on Python 2.4.6. But I couldn't > figure out a good way to write test of HTTP CONNECT tunnel for Python 2.4 or > 2.5, without ssl module. It is probably not possible. The Python http implementation has changed a lot over time. Proxys did probably not really work correctly for our use before 2.6.3 or something like that. It seems like the change is fine, then. Assuming Python really want to change the implementation in such a way that it breaks other tools. (They could argue that we are messing with private methods. But. But. But that was necessary ... :-( ) /Mads
On Thu, May 22, 2014 at 10:55:17PM +0900, Yuya Nishihara wrote: > # HG changeset patch > # User Yuya Nishihara <yuya@tcha.org> > # Date 1400763926 -32400 > # Thu May 22 22:05:26 2014 +0900 > # Branch stable > # Node ID b8bca4e9640673f43e9027c199c25ff828531fe9 > # Parent 54d7657d7d1e6a62315eea53f4498657e766bb60 > proxy: remove unneeded _set_hostport for compatibility with Python 2.7.7rc1 queued for stable per Mads' review, and as it seems sensible and well-researched. > > With Python 2.7.7rc1, "hg pull" through HTTP CONNECT tunnel fails due to the > removal of _set_hostport [1]. > > ... > File "mercurial/url.py", line 372, in https_open > return self.do_open(self._makeconnection, req) > ... > File "mercurial/url.py", line 342, in connect > _generic_proxytunnel(self) > File "mercurial/url.py", line 228, in _generic_proxytunnel > self._set_hostport(self.host, self.port) > AttributeError: httpsconnection instance has no attribute '_set_hostport' > > self._set_hostport(self.host, self.port) should be noop and can be removed > because: > > - _set_hostport() [2] was the function to parse "host:port" string and > set them to self.host and self.port, > - and (self.host, self.port) pair should be valid since connect() is called > prior to _generic_proxytunnel(). > > [1]: http://hg.python.org/cpython/rev/568041fd8090 > [2]: http://hg.python.org/cpython/file/3a1db0d2747e/Lib/httplib.py#l721 > > diff --git a/mercurial/url.py b/mercurial/url.py > --- a/mercurial/url.py > +++ b/mercurial/url.py > @@ -225,7 +225,6 @@ def _generic_proxytunnel(self): > proxyheaders = dict( > [(x, self.headers[x]) for x in self.headers > if x.lower().startswith('proxy-')]) > - self._set_hostport(self.host, self.port) > self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport) > for header in proxyheaders.iteritems(): > self.send('%s: %s\r\n' % header) > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
Patch
diff --git a/mercurial/url.py b/mercurial/url.py --- a/mercurial/url.py +++ b/mercurial/url.py @@ -225,7 +225,6 @@ def _generic_proxytunnel(self): proxyheaders = dict( [(x, self.headers[x]) for x in self.headers if x.lower().startswith('proxy-')]) - self._set_hostport(self.host, self.port) self.send('CONNECT %s HTTP/1.0\r\n' % self.realhostport) for header in proxyheaders.iteritems(): self.send('%s: %s\r\n' % header)