{"thread":{"id":"43095","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","startedAt":"2006-11-22T22:26:57Z","lastAt":"2006-11-27T16:27:01Z","messageCount":19,"participants":["Jeff King","Ignacio Arenaza","Junio C Hamano","Martin Langhoff","Martin Langhoff (CatalystIT)","Inaki Arenaza","Christian Couder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"295423","messageId":"11642344172790-git-send-email-iarenuno@eteo.mondragon.edu","threadId":"43095","inReplyTo":null,"subject":"[PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Inaki Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-22T22:26:57Z","receivedAt":"2006-11-22T22:26:57Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"From: =?utf-8?q?I=F1aki_Arenaza?= \n\nQuoting from the CVS info manual:\n\n......................................................................\n The `gserver' and `pserver' connection methods all accept optional\n method options, specified as part of the METHOD string, like so:\n\n :METHOD[;OPTION=ARG...]:\n\n Currently, the only two valid connection options are `proxy', which\n takes a hostname as an argument, and `proxyport', which takes a port\n number as an argument. These options can be used to connect via an HTTP\n tunnel style web proxy. For example, to connect pserver via a web proxy\n at www.myproxy.net and port 8000, you would use a method of:\n\n :pserver;proxy=www.myproxy.net;proxyport=8000:\n\n *NOTE: The rest of the connection string is required to connect to\n the server as noted in the upcoming sections on password authentication,\n gserver and kserver. The example above would only modify the METHOD\n portion of the repository name.*\n\n PROXY must be supplied to connect to a CVS server via a proxy\n server, but PROXYPORT will default to port 8080 if not supplied.\n PROXYPORT may also be set via the CVS_PROXY_PORT environment variable.\n......................................................................\n\nThis patch adds support for 'proxy' and 'proxyport' connection options\nwhen using the pserver method for the CVS Root.\n\nIt has been tested with a Squid 2.5.x proxy server.\n\nPlease, CC me as I am not in the list.\n\nSigned-off-by: Iñaki Arenaza \n---\n git-cvsimport.perl | 49 ++++++++++++++++++++++++++++++++++++++++++++-----\n 1 files changed, 44 insertions(+), 5 deletions(-)\n\ndiff --git a/git-cvsimport.perl b/git-cvsimport.perl\nindex b54a948..394f3c3 100755\n--- a/git-cvsimport.perl\n+++ b/git-cvsimport.perl\n@@ -161,8 +161,22 @@ sub new {\n sub conn {\n \tmy $self = shift;\n \tmy $repo = $self->{'fullrep'};\n-\tif($repo =~ s/^:pserver:(?:(.*?)(?::(.*?))?@)?([^:\\/]*)(?::(\\d*))?//) {\n-\t\tmy($user,$pass,$serv,$port) = ($1,$2,$3,$4);\n+\tif($repo =~ s/^:pserver(?:([^:]*)):(?:(.*?)(?::(.*?))?@)?([^:\\/]*)(?::(\\d*))?//) {\n+\t\tmy($param,$user,$pass,$serv,$port) = ($1,$2,$3,$4,$5);\n+\n+\t\tmy($proxyhost,$proxyport);\n+\t\tif($param && ($param =~ m/proxy=([^;]+)/)) {\n+\t\t\t$proxyhost = $1;\n+\t\t\t# Default proxyport, if not specified, is 8080.\n+\t\t\t$proxyport = 8080;\n+\t\t\tif($ENV{\"CVS_PROXY_PORT\"}) {\n+\t\t\t\t$proxyport = $ENV{\"CVS_PROXY_PORT\"};\n+\t\t\t}\n+\t\t\tif($param =~ m/proxyport=([^;]+)/){\n+\t\t\t\t$proxyport = $1;\n+\t\t\t}\n+\t\t}\n+\n \t\t$user=\"anonymous\" unless defined $user;\n \t\tmy $rr2 = \"-\";\n \t\tunless($port) {\n@@ -187,13 +201,38 @@ sub conn {\n \t\t}\n \t\t$pass=\"A\" unless $pass;\n \n-\t\tmy $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n-\t\tdie \"Socket to $serv: $!\\n\" unless defined $s;\n+\t\tmy ($s, $rep);\n+\t\tif($proxyhost) {\n+\n+\t\t\t# Use a HTTP Proxy. Only works for HTTP proxies that\n+\t\t\t# don't require user authentication\n+\t\t\t#\n+\t\t\t# See: http://www.ietf.org/rfc/rfc2817.txt\n+\n+\t\t\t$s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort => $proxyport);\n+\t\t\tdie \"Socket to $proxyhost: $!\\n\" unless defined $s;\n+\t\t\t$s->write(\"CONNECT $serv:$port HTTP/1.1\\r\\nHost: $serv:$port\\r\\n\\r\\n\")\n+\t or die \"Write to $proxyhost: $!\\n\";\n+\t $s->flush();\n+\n+\t\t\t$rep = <$s>;\n+\n+\t\t\t# The answer should loook like 'HTTP/1.x 2yy ....'\n+\t\t\tif(!($rep =~ m#^HTTP/1\\.. 2[0-9][0-9]#)) {\n+\t\t\t\tdie \"Proxy connect: $rep\\n\";\n+\t\t\t}\n+\t\t\t# Skip the empty line of the proxy server output\n+\t\t\t<$s>;\n+\t\t} else {\n+\t\t\tmy $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n+\t\t\tdie \"Socket to $serv: $!\\n\" unless defined $s;\n+\t\t}\n+\n \t\t$s->write(\"BEGIN AUTH REQUEST\\n$repo\\n$user\\n$pass\\nEND AUTH REQUEST\\n\")\n \t\t\tor die \"Write to $serv: $!\\n\";\n \t\t$s->flush();\n \n-\t\tmy $rep = <$s>;\n+\t\t$rep = <$s>;\n \n \t\tif($rep ne \"I LOVE YOU\\n\") {\n \t\t\t$rep=\"\" unless $rep;\n-- \n1.4.4\n\n"},{"id":"295688","messageId":"7v64d5keke.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"11642344172790-git-send-email-iarenuno@eteo.mondragon.edu","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-23T22:01:37Z","receivedAt":"2006-11-23T22:01:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks. The patch looks very sane, isolated to be safe enough,\nand useful.\n\nExcept that this statement made me go \"huh?\" wondering what it\nwould do to the $filehandle to evaluate <$filehandle> in a void\ncontext:\n\n+\t\t\t# Skip the empty line of the proxy server output\n+\t\t\t<$s>;\n\nand I ended up looking in perlop.pod and came up empty.\n\nThe \"I/O Operators\" section talks about evaluating <$s> in a\nscalar context (i.e. \"$rep = <$s>\"), which we all know would\nreturn a single line, and in list context, which swallows\neverything up to EOF, an obvious disaster for this particular\nuse. I couldn't find how it is defined to behave in a void\ncontext. By experiments I know this returns only one line, but\nit leaves me feeling somewhat uneasy.\n\nAlso it has a style inconsistency between \"if(expression) {\" and\n\"if(expression){\", and I do not like either of them, but fixing\nthat should be left to a separate patch.\n\nI'll apply this unless Martin or other people on the list who\nhave stake in cvsimport objects.\n"},{"id":"294998","messageId":"45661C36.9010101@catalyst.net.nz","threadId":"43095","inReplyTo":"7v64d5keke.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Martin Langhoff (CatalystIT)","fromEmail":"martin@catalyst.net.nz","sentAt":"2006-11-23T22:09:58Z","receivedAt":"2006-11-23T22:09:58Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n> Except that this statement made me go \"huh?\" wondering what it\n> would do to the $filehandle to evaluate <$filehandle> in a void\n> context:\n> \n> +\t\t\t# Skip the empty line of the proxy server output\n> +\t\t\t<$s>;\n\nIt's a perl idiom that will discard one line of the $filehandle. If we \nare 200% certain that it is empty, then it's fine. OTOH, it may well be \na bug in the particular proxy implementation Iñaki is using -- I don't \nknow enough about CVS proxying to tell.\n\n> The \"I/O Operators\" section talks about evaluating <$s> in a\n> scalar context (i.e. \"$rep = <$s>\"), which we all know would\n> return a single line, and in list context, which swallows\n\nThis is in scalar context, and that's safe to rely on. Whether it is \nclear enough in this non-Perl-native project... is a good flamewar \nwaiting to happen :-)\n\ncheers,\n\n\nmartin\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington\nWEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St\nOFFICE: +64(4)916-7224 MOB: +64(21)364-017\n Make things as simple as possible, but no simpler - Einstein\n"},{"id":"297903","messageId":"7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"45661C36.9010101@catalyst.net.nz","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-23T23:02:00Z","receivedAt":"2006-11-23T23:02:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Martin Langhoff (CatalystIT)\" writes:\n\n> Junio C Hamano wrote:\n>> Except that this statement made me go \"huh?\" wondering what it\n>> would do to the $filehandle to evaluate <$filehandle> in a void\n>> context:\n>>\n>> +\t\t\t# Skip the empty line of the proxy server output\n>> +\t\t\t<$s>;\n>\n> It's a perl idiom that will discard one line of the $filehandle. If we\n> are 200% certain that it is empty, then it's fine. OTOH, it may well\n> be a bug in the particular proxy implementation Iñaki is using -- I\n> don't know enough about CVS proxying to tell.\n\nIt is more about HTTP proxying and it is my understanding that\nresponse to CONNECT method request has that empty line after the\nsuccessful (2xx) response line and zero or more response\nheaders. The code is still wrong; it does not have a loop to\ndiscard the potential response headers that come before the\nempty line the code we are discussing discards.\n\nBy the way does anybody know where this behaviour is officially\nspecified? Luotonen's draft-luotonen-web-proxy-tunneling-01.txt\nis pretty clear about the empty line that comes after the\nresponse headers, but that document being an internet-draft has\nexpired long time ago, but still seem to be quoted by others.\n\n>> The \"I/O Operators\" section talks about evaluating <$s> in a\n>> scalar context (i.e. \"$rep = <$s>\"), which we all know would\n>> return a single line, and in list context, which swallows\n>\n> This is in scalar context, and that's safe to rely on.\n\nIf it were in scalar context I would agree fully. That\nbehaviour is documented. But my point is that it is in void\ncontext, and I did not find document specifying what should\nhappen.\n"},{"id":"298600","messageId":"45662CB9.8030902@catalyst.net.nz","threadId":"43095","inReplyTo":"7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Martin Langhoff (CatalystIT)","fromEmail":"martin@catalyst.net.nz","sentAt":"2006-11-23T23:20:25Z","receivedAt":"2006-11-23T23:20:25Z","isPatch":true,"sender":{"key":"martin@laptop.org","avatar":null},"body":"Junio C Hamano wrote:\n\n> \"Martin Langhoff (CatalystIT)\" writes:\n>\n>>This is in scalar context, and that's safe to rely on.\n> \n> \n> If it were in scalar context I would agree fully. That\n> behaviour is documented. But my point is that it is in void\n> context, and I did not find document specifying what should\n> happen.\n\nSorry! What I meant to say is: void context is always equivalent to \nscalar context.\n\ncheers,\n\n\nmartin\n-- \n-----------------------------------------------------------------------\nMartin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington\nWEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St\nOFFICE: +64(4)916-7224 MOB: +64(21)364-017\n Make things as simple as possible, but no simpler - Einstein\n"},{"id":"294903","messageId":"46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com","threadId":"43095","inReplyTo":"7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Martin Langhoff","fromEmail":"martin.langhoff@gmail.com","sentAt":"2006-11-23T23:52:13Z","receivedAt":"2006-11-23T23:52:13Z","isPatch":true,"sender":{"key":"martin.langhoff@gmail.com","avatar":"https://gravatar.com/avatar/1e3f311b6c4c15836501901ca58f8c0b0667246488084ba524d8bc9867e22fd9?d=mp&s=160"},"body":"On 11/24/06, Junio C Hamano wrote:\n> It is more about HTTP proxying and it is my understanding that\n> response to CONNECT method request has that empty line after the\n> successful (2xx) response line and zero or more response\n> headers. The code is still wrong; it does not have a loop to\n> discard the potential response headers that come before the\n> empty line the code we are discussing discards.\n\nYou are right. It should be something along the lines of\n\n # discard headers until first blank line\n while (<$s> ne '') {\n # nothing\n }\n\nthat is, assuming we can just ignore headers happily.\n\ncheers,\n\n\n\n"},{"id":"294296","messageId":"7v7ixlhesv.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-24T00:24:48Z","receivedAt":"2006-11-24T00:24:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Martin Langhoff\" writes:\n\n> On 11/24/06, Junio C Hamano wrote:\n>> It is more about HTTP proxying and it is my understanding that\n>> response to CONNECT method request has that empty line after the\n>> successful (2xx) response line and zero or more response\n>> headers. The code is still wrong; it does not have a loop to\n>> discard the potential response headers that come before the\n>> empty line the code we are discussing discards.\n>\n> You are right. It should be something along the lines of\n>\n> # discard headers until first blank line\n> while (<$s> ne '') {\n> # nothing\n> }\n>\n> that is, assuming we can just ignore headers happily.\n\nYes, or \"1 while (<$s> ne '')\" which is listed as an example for\na kosher way to use a constant to express no-op in void context ;-).\n\n =head2 No-ops\n X X\n\n Perl doesn't officially have a no-op operator, but the bare constants\n C<0> and C<1> are special-cased to not produce a warning in a void\n context, so you can for example safely do\n\n 1 while foo();\n\n"},{"id":"293838","messageId":"20061124014205.GA19515@sigio.intra.peff.net","threadId":"43095","inReplyTo":"46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-11-24T01:42:05Z","receivedAt":"2006-11-24T01:42:05Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 24, 2006 at 12:52:13PM +1300, Martin Langhoff wrote:\n\n> You are right. It should be something along the lines of\n> \n> # discard headers until first blank line\n> while (<$s> ne '') {\n> # nothing\n> }\n> \n> that is, assuming we can just ignore headers happily.\n\nThat code won't work; the value of a blank line will actually be \"\\n\"\n(or \"\\r\\n\"). So you probably want:\n\nwhile (<$s>) {\n chomp; chomp;\n last unless $_;\n}\n\nor perhaps more readably:\n\nwhile (<$s>) {\n last if $_ eq \"\\n\" || $_ eq \"\\r\\n\";\n}\n\n"},{"id":"295435","messageId":"7vvel5ftav.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"20061124014205.GA19515@sigio.intra.peff.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-24T02:54:32Z","receivedAt":"2006-11-24T02:54:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, Nov 24, 2006 at 12:52:13PM +1300, Martin Langhoff wrote:\n>\n>> You are right. It should be something along the lines of\n>> \n>> # discard headers until first blank line\n>> while (<$s> ne '') {\n>> # nothing\n>> }\n>> \n>> that is, assuming we can just ignore headers happily.\n>\n> That code won't work; the value of a blank line will actually be \"\\n\"\n> (or \"\\r\\n\").\n\nGood point. For an HTTP proxy the kosher line termination would\nindeed be \"\\r\\n\".\n"},{"id":"296530","messageId":"200611240548.00840.chriscool@tuxfamily.org","threadId":"43095","inReplyTo":"11642344172790-git-send-email-iarenuno@eteo.mondragon.edu","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Christian Couder","fromEmail":"chriscool@tuxfamily.org","sentAt":"2006-11-24T04:48:00Z","receivedAt":"2006-11-24T04:48:00Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"Inaki Arenaza wrote:\n\n[...]\n\n>\n> +\t\tmy ($s, $rep);\n> +\t\tif($proxyhost) {\n> +\n> +\t\t\t# Use a HTTP Proxy. Only works for HTTP proxies that\n> +\t\t\t# don't require user authentication\n> +\t\t\t#\n> +\t\t\t# See: http://www.ietf.org/rfc/rfc2817.txt\n> +\n> +\t\t\t$s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort =>\n> $proxyport); +\t\t\tdie \"Socket to $proxyhost: $!\\n\" unless defined $s;\n> +\t\t\t$s->write(\"CONNECT $serv:$port HTTP/1.1\\r\\nHost:\n> $serv:$port\\r\\n\\r\\n\") +\t or die \"Write to\n> $proxyhost: $!\\n\";\n> +\t $s->flush();\n> +\n> +\t\t\t$rep = <$s>;\n> +\n> +\t\t\t# The answer should loook like 'HTTP/1.x 2yy ....'\n> +\t\t\tif(!($rep =~ m#^HTTP/1\\.. 2[0-9][0-9]#)) {\n> +\t\t\t\tdie \"Proxy connect: $rep\\n\";\n> +\t\t\t}\n> +\t\t\t# Skip the empty line of the proxy server output\n> +\t\t\t<$s>;\n> +\t\t} else {\n> +\t\t\tmy $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n\nIt seems that \"my \" should not be in the above line.\n\n> +\t\t\tdie \"Socket to $serv: $!\\n\" unless defined $s;\n> +\t\t}\n> +\n> \t\t$s->write(\"BEGIN AUTH REQUEST\\n$repo\\n$user\\n$pass\\nEND AUTH\n> REQUEST\\n\") or die \"Write to $serv: $!\\n\";\n> \t\t$s->flush();\n\nRegards,\n"},{"id":"297698","messageId":"67vel56xtm.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"200611240548.00840.chriscool@tuxfamily.org","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-24T08:41:41Z","receivedAt":"2006-11-24T08:41:41Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Christian Couder writes:\n\n>> +\t\t\tmy $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n>\n> It seems that \"my \" should not be in the above line.\n\nYup! Your right. Will send a new patch with the fixes in a few hours.\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"},{"id":"296534","messageId":"67r6vt6xq2.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"7v64d5keke.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-24T08:43:49Z","receivedAt":"2006-11-24T08:43:49Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> The \"I/O Operators\" section talks about evaluating <$s> in a\n> scalar context (i.e. \"$rep = <$s>\"), which we all know would\n> return a single line, and in list context, which swallows\n> everything up to EOF, an obvious disaster for this particular\n> use. I couldn't find how it is defined to behave in a void\n> context. By experiments I know this returns only one line, but\n> it leaves me feeling somewhat uneasy.\n\nThis is scalar context, as we are using the implicit $_ variable as\nthe destination of the asignment. It seems it's not so obvious for\nnon-Perl speakers, so I'll use a clearer idiom :-)\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"},{"id":"294035","messageId":"67mz6h6xmb.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"7v64d5keke.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-24T08:46:04Z","receivedAt":"2006-11-24T08:46:04Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Also it has a style inconsistency between \"if(expression) {\" and\n> \"if(expression){\", and I do not like either of them, but fixing\n> that should be left to a separate patch.\n\nI just tried to use the formating of the existing code, but\nit seems I missed some lines. Will send a new patch with all the\ncomments made so far in a few hours.\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"},{"id":"295022","messageId":"7vfyc9cjci.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"67mz6h6xmb.fsf@poseidon.eteo.mondragon.edu","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-24T08:57:49Z","receivedAt":"2006-11-24T08:57:49Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Arenaza writes:\n\n> ... Will send a new patch with all the\n> comments made so far in a few hours.\n\nLet's save a bit of trouble from you. Here is what I've queued\nfor 'master', with fixes from the discussion so far.\n\n---\ndiff --git a/git-cvsimport.perl b/git-cvsimport.perl\nindex b54a948..4310dea 100755\n--- a/git-cvsimport.perl\n+++ b/git-cvsimport.perl\n@@ -161,8 +161,22 @@ sub new {\n sub conn {\n \tmy $self = shift;\n \tmy $repo = $self->{'fullrep'};\n-\tif($repo =~ s/^:pserver:(?:(.*?)(?::(.*?))?@)?([^:\\/]*)(?::(\\d*))?//) {\n-\t\tmy($user,$pass,$serv,$port) = ($1,$2,$3,$4);\n+\tif($repo =~ s/^:pserver(?:([^:]*)):(?:(.*?)(?::(.*?))?@)?([^:\\/]*)(?::(\\d*))?//) {\n+\t\tmy($param,$user,$pass,$serv,$port) = ($1,$2,$3,$4,$5);\n+\n+\t\tmy($proxyhost,$proxyport);\n+\t\tif($param && ($param =~ m/proxy=([^;]+)/)) {\n+\t\t\t$proxyhost = $1;\n+\t\t\t# Default proxyport, if not specified, is 8080.\n+\t\t\t$proxyport = 8080;\n+\t\t\tif($ENV{\"CVS_PROXY_PORT\"}) {\n+\t\t\t\t$proxyport = $ENV{\"CVS_PROXY_PORT\"};\n+\t\t\t}\n+\t\t\tif($param =~ m/proxyport=([^;]+)/){\n+\t\t\t\t$proxyport = $1;\n+\t\t\t}\n+\t\t}\n+\n \t\t$user=\"anonymous\" unless defined $user;\n \t\tmy $rr2 = \"-\";\n \t\tunless($port) {\n@@ -187,13 +201,43 @@ sub conn {\n \t\t}\n \t\t$pass=\"A\" unless $pass;\n \n-\t\tmy $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n-\t\tdie \"Socket to $serv: $!\\n\" unless defined $s;\n+\t\tmy ($s, $rep);\n+\t\tif($proxyhost) {\n+\n+\t\t\t# Use a HTTP Proxy. Only works for HTTP proxies that\n+\t\t\t# don't require user authentication\n+\t\t\t#\n+\t\t\t# See: http://www.ietf.org/rfc/rfc2817.txt\n+\n+\t\t\t$s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort => $proxyport);\n+\t\t\tdie \"Socket to $proxyhost: $!\\n\" unless defined $s;\n+\t\t\t$s->write(\"CONNECT $serv:$port HTTP/1.1\\r\\nHost: $serv:$port\\r\\n\\r\\n\")\n+\t or die \"Write to $proxyhost: $!\\n\";\n+\t $s->flush();\n+\n+\t\t\t$rep = <$s>;\n+\n+\t\t\t# The answer should look like 'HTTP/1.x 2yy ....'\n+\t\t\tif(!($rep =~ m#^HTTP/1\\.. 2[0-9][0-9]#)) {\n+\t\t\t\tdie \"Proxy connect: $rep\\n\";\n+\t\t\t}\n+\t\t\t# Skip up to the empty line of the proxy server output\n+\t\t\t# including the response headers.\n+\t\t\twhile ($rep = <$s>) {\n+\t\t\t\tlast if (!defined $rep ||\n+\t\t\t\t\t $rep eq \"\\n\" ||\n+\t\t\t\t\t $rep eq \"\\r\\n\");\n+\t\t\t}\n+\t\t} else {\n+\t\t\t$s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port);\n+\t\t\tdie \"Socket to $serv: $!\\n\" unless defined $s;\n+\t\t}\n+\n \t\t$s->write(\"BEGIN AUTH REQUEST\\n$repo\\n$user\\n$pass\\nEND AUTH REQUEST\\n\")\n \t\t\tor die \"Write to $serv: $!\\n\";\n \t\t$s->flush();\n \n-\t\tmy $rep = <$s>;\n+\t\t$rep = <$s>;\n \n \t\tif($rep ne \"I LOVE YOU\\n\") {\n \t\t\t$rep=\"\" unless $rep;\n"},{"id":"296520","messageId":"67hcwp6wpp.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"7vfyc9cjci.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-24T09:05:38Z","receivedAt":"2006-11-24T09:05:38Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> Let's save a bit of trouble from you. Here is what I've queued\n> for 'master', with fixes from the discussion so far.\n\nLooks good to me, but it seems you missed the 'if(...) {' and\n'if(...){' formating issues you raised before.\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"},{"id":"295823","messageId":"7vhcwpawvg.fsf@assigned-by-dhcp.cox.net","threadId":"43095","inReplyTo":"67hcwp6wpp.fsf@poseidon.eteo.mondragon.edu","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-11-24T11:48:35Z","receivedAt":"2006-11-24T11:48:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ignacio Arenaza writes:\n\n> Junio C Hamano writes:\n>\n>> Let's save a bit of trouble from you. Here is what I've queued\n>> for 'master', with fixes from the discussion so far.\n>\n> Looks good to me, but it seems you missed the 'if(...) {' and\n> 'if(...){' formating issues you raised before.\n\nI deliberately left out the style changes to avoid cluttering\nthe patch; as I said in my first response, that should come as a\nseparate patch that does not change anything else.\n"},{"id":"297280","messageId":"20061124145700.GA13947@coredump.intra.peff.net","threadId":"43095","inReplyTo":"7vfyc9cjci.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-11-24T14:57:01Z","receivedAt":"2006-11-24T14:57:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 24, 2006 at 12:57:49AM -0800, Junio C Hamano wrote:\n\n> +\t\t\t# Skip up to the empty line of the proxy server output\n> +\t\t\t# including the response headers.\n> +\t\t\twhile ($rep = <$s>) {\n> +\t\t\t\tlast if (!defined $rep ||\n> +\t\t\t\t\t $rep eq \"\\n\" ||\n> +\t\t\t\t\t $rep eq \"\\r\\n\");\n> +\t\t\t}\n\nNit: checking defined($rep) inside the loop is redundant.\n\n"},{"id":"296315","messageId":"67d57b7l3s.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"7v7ixlhesv.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-25T12:43:19Z","receivedAt":"2006-11-25T12:43:19Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n>>> It is more about HTTP proxying and it is my understanding that\n>>> response to CONNECT method request has that empty line after the\n>>> successful (2xx) response line and zero or more response\n>>> headers. The code is still wrong; it does not have a loop to\n>>> discard the potential response headers that come before the\n>>> empty line the code we are discussing discards.\n\nI don't have the original message from Junio where he asks where the\nproxy behaviour is officially specified (as the draft has expired long\nago), so I'll answer it here.\n\nThere is a comment in the code that says the relevant RFC is 2817,\nsections 5.2 and 5.3.\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"},{"id":"296550","messageId":"67bqmsn9d6.fsf@poseidon.eteo.mondragon.edu","threadId":"43095","inReplyTo":"7vhcwpawvg.fsf@assigned-by-dhcp.cox.net","subject":"Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying","fromName":"Ignacio Arenaza","fromEmail":"iarenuno@eteo.mondragon.edu","sentAt":"2006-11-27T16:27:01Z","receivedAt":"2006-11-27T16:27:01Z","isPatch":true,"sender":{"key":"iarenuno@eteo.mondragon.edu","avatar":"https://gravatar.com/avatar/5f0b3f87344e3d8017a6f4c05cc4c5d449ad50431a6b1c719090f889cf56f197?d=mp&s=160"},"body":"Junio C Hamano writes:\n\n> I deliberately left out the style changes to avoid cluttering\n> the patch; as I said in my first response, that should come as a\n> separate patch that does not change anything else.\n\nOk, I've had a look at git-cvsimport.perl and I can't come up with a\nconsistent style for the 'if () {' sentence. According to grep there\nare 50 instances of 'if(...)' and 39 instances of 'if (...)', so I\ndon't know which is the preferred one. And I can't find a CodingStyle\ndocument either.\n\nWhich way should I fix them? And should I fix the whole file, or just\nthe ones in my original patch?\n\nSaludos. Iñaki.\n\n-- \nSchool of Management\nMondragon University\n20560 Oñati - Spain\n+34 943 718009 (ext. 225)\n\n"}]}