# Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying 19 messages from 2006-11-22 to 2006-11-27. Participants: Jeff King, Ignacio Arenaza, Junio C Hamano, Martin Langhoff, Martin Langhoff (CatalystIT), Inaki Arenaza, Christian Couder. Thread: https://gitlist.dev/t/43095 ## Inaki Arenaza, 2006-11-22 22:26 Subject: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <11642344172790-git-send-email-iarenuno@eteo.mondragon.edu> URL: https://gitlist.dev/e/11642344172790-git-send-email-iarenuno%40eteo.mondragon.edu ``` From: =?utf-8?q?I=F1aki_Arenaza?= Quoting from the CVS info manual: ...................................................................... The `gserver' and `pserver' connection methods all accept optional method options, specified as part of the METHOD string, like so: :METHOD[;OPTION=ARG...]: Currently, the only two valid connection options are `proxy', which takes a hostname as an argument, and `proxyport', which takes a port number as an argument. These options can be used to connect via an HTTP tunnel style web proxy. For example, to connect pserver via a web proxy at www.myproxy.net and port 8000, you would use a method of: :pserver;proxy=www.myproxy.net;proxyport=8000: *NOTE: The rest of the connection string is required to connect to the server as noted in the upcoming sections on password authentication, gserver and kserver. The example above would only modify the METHOD portion of the repository name.* PROXY must be supplied to connect to a CVS server via a proxy server, but PROXYPORT will default to port 8080 if not supplied. PROXYPORT may also be set via the CVS_PROXY_PORT environment variable. ...................................................................... This patch adds support for 'proxy' and 'proxyport' connection options when using the pserver method for the CVS Root. It has been tested with a Squid 2.5.x proxy server. Please, CC me as I am not in the list. Signed-off-by: Iñaki Arenaza --- git-cvsimport.perl | 49 ++++++++++++++++++++++++++++++++++++++++++++----- 1 files changed, 44 insertions(+), 5 deletions(-) diff --git a/git-cvsimport.perl b/git-cvsimport.perl index b54a948..394f3c3 100755 --- a/git-cvsimport.perl +++ b/git-cvsimport.perl @@ -161,8 +161,22 @@ sub new { sub conn { my $self = shift; my $repo = $self->{'fullrep'}; - if($repo =~ s/^:pserver:(?:(.*?)(?::(.*?))?@)?([^:\/]*)(?::(\d*))?//) { - my($user,$pass,$serv,$port) = ($1,$2,$3,$4); + if($repo =~ s/^:pserver(?:([^:]*)):(?:(.*?)(?::(.*?))?@)?([^:\/]*)(?::(\d*))?//) { + my($param,$user,$pass,$serv,$port) = ($1,$2,$3,$4,$5); + + my($proxyhost,$proxyport); + if($param && ($param =~ m/proxy=([^;]+)/)) { + $proxyhost = $1; + # Default proxyport, if not specified, is 8080. + $proxyport = 8080; + if($ENV{"CVS_PROXY_PORT"}) { + $proxyport = $ENV{"CVS_PROXY_PORT"}; + } + if($param =~ m/proxyport=([^;]+)/){ + $proxyport = $1; + } + } + $user="anonymous" unless defined $user; my $rr2 = "-"; unless($port) { @@ -187,13 +201,38 @@ sub conn { } $pass="A" unless $pass; - my $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); - die "Socket to $serv: $!\n" unless defined $s; + my ($s, $rep); + if($proxyhost) { + + # Use a HTTP Proxy. Only works for HTTP proxies that + # don't require user authentication + # + # See: http://www.ietf.org/rfc/rfc2817.txt + + $s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort => $proxyport); + die "Socket to $proxyhost: $!\n" unless defined $s; + $s->write("CONNECT $serv:$port HTTP/1.1\r\nHost: $serv:$port\r\n\r\n") + or die "Write to $proxyhost: $!\n"; + $s->flush(); + + $rep = <$s>; + + # The answer should loook like 'HTTP/1.x 2yy ....' + if(!($rep =~ m#^HTTP/1\.. 2[0-9][0-9]#)) { + die "Proxy connect: $rep\n"; + } + # Skip the empty line of the proxy server output + <$s>; + } else { + my $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); + die "Socket to $serv: $!\n" unless defined $s; + } + $s->write("BEGIN AUTH REQUEST\n$repo\n$user\n$pass\nEND AUTH REQUEST\n") or die "Write to $serv: $!\n"; $s->flush(); - my $rep = <$s>; + $rep = <$s>; if($rep ne "I LOVE YOU\n") { $rep="" unless $rep; -- 1.4.4 ``` ## Junio C Hamano, 2006-11-23 22:01 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7v64d5keke.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v64d5keke.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <11642344172790-git-send-email-iarenuno@eteo.mondragon.edu> ``` Thanks. The patch looks very sane, isolated to be safe enough, and useful. Except that this statement made me go "huh?" wondering what it would do to the $filehandle to evaluate <$filehandle> in a void context: + # Skip the empty line of the proxy server output + <$s>; and I ended up looking in perlop.pod and came up empty. The "I/O Operators" section talks about evaluating <$s> in a scalar context (i.e. "$rep = <$s>"), which we all know would return a single line, and in list context, which swallows everything up to EOF, an obvious disaster for this particular use. I couldn't find how it is defined to behave in a void context. By experiments I know this returns only one line, but it leaves me feeling somewhat uneasy. Also it has a style inconsistency between "if(expression) {" and "if(expression){", and I do not like either of them, but fixing that should be left to a separate patch. I'll apply this unless Martin or other people on the list who have stake in cvsimport objects. ``` ## Martin Langhoff (CatalystIT), 2006-11-23 22:09 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <45661C36.9010101@catalyst.net.nz> URL: https://gitlist.dev/e/45661C36.9010101%40catalyst.net.nz In-Reply-To: <7v64d5keke.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > Except that this statement made me go "huh?" wondering what it > would do to the $filehandle to evaluate <$filehandle> in a void > context: > > + # Skip the empty line of the proxy server output > + <$s>; It's a perl idiom that will discard one line of the $filehandle. If we are 200% certain that it is empty, then it's fine. OTOH, it may well be a bug in the particular proxy implementation Iñaki is using -- I don't know enough about CVS proxying to tell. > The "I/O Operators" section talks about evaluating <$s> in a > scalar context (i.e. "$rep = <$s>"), which we all know would > return a single line, and in list context, which swallows This is in scalar context, and that's safe to rely on. Whether it is clear enough in this non-Perl-native project... is a good flamewar waiting to happen :-) cheers, martin -- ----------------------------------------------------------------------- Martin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington WEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St OFFICE: +64(4)916-7224 MOB: +64(21)364-017 Make things as simple as possible, but no simpler - Einstein ``` ## Junio C Hamano, 2006-11-23 23:02 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vlkm1ix7b.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <45661C36.9010101@catalyst.net.nz> ``` "Martin Langhoff (CatalystIT)" writes: > Junio C Hamano wrote: >> Except that this statement made me go "huh?" wondering what it >> would do to the $filehandle to evaluate <$filehandle> in a void >> context: >> >> + # Skip the empty line of the proxy server output >> + <$s>; > > It's a perl idiom that will discard one line of the $filehandle. If we > are 200% certain that it is empty, then it's fine. OTOH, it may well > be a bug in the particular proxy implementation Iñaki is using -- I > don't know enough about CVS proxying to tell. It is more about HTTP proxying and it is my understanding that response to CONNECT method request has that empty line after the successful (2xx) response line and zero or more response headers. The code is still wrong; it does not have a loop to discard the potential response headers that come before the empty line the code we are discussing discards. By the way does anybody know where this behaviour is officially specified? Luotonen's draft-luotonen-web-proxy-tunneling-01.txt is pretty clear about the empty line that comes after the response headers, but that document being an internet-draft has expired long time ago, but still seem to be quoted by others. >> The "I/O Operators" section talks about evaluating <$s> in a >> scalar context (i.e. "$rep = <$s>"), which we all know would >> return a single line, and in list context, which swallows > > This is in scalar context, and that's safe to rely on. If it were in scalar context I would agree fully. That behaviour is documented. But my point is that it is in void context, and I did not find document specifying what should happen. ``` ## Martin Langhoff (CatalystIT), 2006-11-23 23:20 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <45662CB9.8030902@catalyst.net.nz> URL: https://gitlist.dev/e/45662CB9.8030902%40catalyst.net.nz In-Reply-To: <7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano wrote: > "Martin Langhoff (CatalystIT)" writes: > >>This is in scalar context, and that's safe to rely on. > > > If it were in scalar context I would agree fully. That > behaviour is documented. But my point is that it is in void > context, and I did not find document specifying what should > happen. Sorry! What I meant to say is: void context is always equivalent to scalar context. cheers, martin -- ----------------------------------------------------------------------- Martin @ Catalyst .Net .NZ Ltd, PO Box 11-053, Manners St, Wellington WEB: http://catalyst.net.nz/ PHYS: Level 2, 150-154 Willis St OFFICE: +64(4)916-7224 MOB: +64(21)364-017 Make things as simple as possible, but no simpler - Einstein ``` ## Martin Langhoff, 2006-11-23 23:52 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com> URL: https://gitlist.dev/e/46a038f90611231552x3e33eec4w7863ce6a1da5781b%40mail.gmail.com In-Reply-To: <7vlkm1ix7b.fsf@assigned-by-dhcp.cox.net> ``` On 11/24/06, Junio C Hamano wrote: > It is more about HTTP proxying and it is my understanding that > response to CONNECT method request has that empty line after the > successful (2xx) response line and zero or more response > headers. The code is still wrong; it does not have a loop to > discard the potential response headers that come before the > empty line the code we are discussing discards. You are right. It should be something along the lines of # discard headers until first blank line while (<$s> ne '') { # nothing } that is, assuming we can just ignore headers happily. cheers, ``` ## Junio C Hamano, 2006-11-24 00:24 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7v7ixlhesv.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7v7ixlhesv.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com> ``` "Martin Langhoff" writes: > On 11/24/06, Junio C Hamano wrote: >> It is more about HTTP proxying and it is my understanding that >> response to CONNECT method request has that empty line after the >> successful (2xx) response line and zero or more response >> headers. The code is still wrong; it does not have a loop to >> discard the potential response headers that come before the >> empty line the code we are discussing discards. > > You are right. It should be something along the lines of > > # discard headers until first blank line > while (<$s> ne '') { > # nothing > } > > that is, assuming we can just ignore headers happily. Yes, or "1 while (<$s> ne '')" which is listed as an example for a kosher way to use a constant to express no-op in void context ;-). =head2 No-ops X X Perl doesn't officially have a no-op operator, but the bare constants C<0> and C<1> are special-cased to not produce a warning in a void context, so you can for example safely do 1 while foo(); ``` ## Jeff King, 2006-11-24 01:42 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <20061124014205.GA19515@sigio.intra.peff.net> URL: https://gitlist.dev/e/20061124014205.GA19515%40sigio.intra.peff.net In-Reply-To: <46a038f90611231552x3e33eec4w7863ce6a1da5781b@mail.gmail.com> ``` On Fri, Nov 24, 2006 at 12:52:13PM +1300, Martin Langhoff wrote: > You are right. It should be something along the lines of > > # discard headers until first blank line > while (<$s> ne '') { > # nothing > } > > that is, assuming we can just ignore headers happily. That code won't work; the value of a blank line will actually be "\n" (or "\r\n"). So you probably want: while (<$s>) { chomp; chomp; last unless $_; } or perhaps more readably: while (<$s>) { last if $_ eq "\n" || $_ eq "\r\n"; } ``` ## Junio C Hamano, 2006-11-24 02:54 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7vvel5ftav.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vvel5ftav.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <20061124014205.GA19515@sigio.intra.peff.net> ``` Jeff King writes: > On Fri, Nov 24, 2006 at 12:52:13PM +1300, Martin Langhoff wrote: > >> You are right. It should be something along the lines of >> >> # discard headers until first blank line >> while (<$s> ne '') { >> # nothing >> } >> >> that is, assuming we can just ignore headers happily. > > That code won't work; the value of a blank line will actually be "\n" > (or "\r\n"). Good point. For an HTTP proxy the kosher line termination would indeed be "\r\n". ``` ## Christian Couder, 2006-11-24 04:48 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <200611240548.00840.chriscool@tuxfamily.org> URL: https://gitlist.dev/e/200611240548.00840.chriscool%40tuxfamily.org In-Reply-To: <11642344172790-git-send-email-iarenuno@eteo.mondragon.edu> ``` Inaki Arenaza wrote: [...] > > + my ($s, $rep); > + if($proxyhost) { > + > + # Use a HTTP Proxy. Only works for HTTP proxies that > + # don't require user authentication > + # > + # See: http://www.ietf.org/rfc/rfc2817.txt > + > + $s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort => > $proxyport); + die "Socket to $proxyhost: $!\n" unless defined $s; > + $s->write("CONNECT $serv:$port HTTP/1.1\r\nHost: > $serv:$port\r\n\r\n") + or die "Write to > $proxyhost: $!\n"; > + $s->flush(); > + > + $rep = <$s>; > + > + # The answer should loook like 'HTTP/1.x 2yy ....' > + if(!($rep =~ m#^HTTP/1\.. 2[0-9][0-9]#)) { > + die "Proxy connect: $rep\n"; > + } > + # Skip the empty line of the proxy server output > + <$s>; > + } else { > + my $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); It seems that "my " should not be in the above line. > + die "Socket to $serv: $!\n" unless defined $s; > + } > + > $s->write("BEGIN AUTH REQUEST\n$repo\n$user\n$pass\nEND AUTH > REQUEST\n") or die "Write to $serv: $!\n"; > $s->flush(); Regards, ``` ## Ignacio Arenaza, 2006-11-24 08:41 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67vel56xtm.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67vel56xtm.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <200611240548.00840.chriscool@tuxfamily.org> ``` Christian Couder writes: >> + my $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); > > It seems that "my " should not be in the above line. Yup! Your right. Will send a new patch with the fixes in a few hours. Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ``` ## Ignacio Arenaza, 2006-11-24 08:43 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67r6vt6xq2.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67r6vt6xq2.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <7v64d5keke.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > The "I/O Operators" section talks about evaluating <$s> in a > scalar context (i.e. "$rep = <$s>"), which we all know would > return a single line, and in list context, which swallows > everything up to EOF, an obvious disaster for this particular > use. I couldn't find how it is defined to behave in a void > context. By experiments I know this returns only one line, but > it leaves me feeling somewhat uneasy. This is scalar context, as we are using the implicit $_ variable as the destination of the asignment. It seems it's not so obvious for non-Perl speakers, so I'll use a clearer idiom :-) Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ``` ## Ignacio Arenaza, 2006-11-24 08:46 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67mz6h6xmb.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67mz6h6xmb.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <7v64d5keke.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > Also it has a style inconsistency between "if(expression) {" and > "if(expression){", and I do not like either of them, but fixing > that should be left to a separate patch. I just tried to use the formating of the existing code, but it seems I missed some lines. Will send a new patch with all the comments made so far in a few hours. Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ``` ## Junio C Hamano, 2006-11-24 08:57 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7vfyc9cjci.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vfyc9cjci.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <67mz6h6xmb.fsf@poseidon.eteo.mondragon.edu> ``` Ignacio Arenaza writes: > ... Will send a new patch with all the > comments made so far in a few hours. Let's save a bit of trouble from you. Here is what I've queued for 'master', with fixes from the discussion so far. --- diff --git a/git-cvsimport.perl b/git-cvsimport.perl index b54a948..4310dea 100755 --- a/git-cvsimport.perl +++ b/git-cvsimport.perl @@ -161,8 +161,22 @@ sub new { sub conn { my $self = shift; my $repo = $self->{'fullrep'}; - if($repo =~ s/^:pserver:(?:(.*?)(?::(.*?))?@)?([^:\/]*)(?::(\d*))?//) { - my($user,$pass,$serv,$port) = ($1,$2,$3,$4); + if($repo =~ s/^:pserver(?:([^:]*)):(?:(.*?)(?::(.*?))?@)?([^:\/]*)(?::(\d*))?//) { + my($param,$user,$pass,$serv,$port) = ($1,$2,$3,$4,$5); + + my($proxyhost,$proxyport); + if($param && ($param =~ m/proxy=([^;]+)/)) { + $proxyhost = $1; + # Default proxyport, if not specified, is 8080. + $proxyport = 8080; + if($ENV{"CVS_PROXY_PORT"}) { + $proxyport = $ENV{"CVS_PROXY_PORT"}; + } + if($param =~ m/proxyport=([^;]+)/){ + $proxyport = $1; + } + } + $user="anonymous" unless defined $user; my $rr2 = "-"; unless($port) { @@ -187,13 +201,43 @@ sub conn { } $pass="A" unless $pass; - my $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); - die "Socket to $serv: $!\n" unless defined $s; + my ($s, $rep); + if($proxyhost) { + + # Use a HTTP Proxy. Only works for HTTP proxies that + # don't require user authentication + # + # See: http://www.ietf.org/rfc/rfc2817.txt + + $s = IO::Socket::INET->new(PeerHost => $proxyhost, PeerPort => $proxyport); + die "Socket to $proxyhost: $!\n" unless defined $s; + $s->write("CONNECT $serv:$port HTTP/1.1\r\nHost: $serv:$port\r\n\r\n") + or die "Write to $proxyhost: $!\n"; + $s->flush(); + + $rep = <$s>; + + # The answer should look like 'HTTP/1.x 2yy ....' + if(!($rep =~ m#^HTTP/1\.. 2[0-9][0-9]#)) { + die "Proxy connect: $rep\n"; + } + # Skip up to the empty line of the proxy server output + # including the response headers. + while ($rep = <$s>) { + last if (!defined $rep || + $rep eq "\n" || + $rep eq "\r\n"); + } + } else { + $s = IO::Socket::INET->new(PeerHost => $serv, PeerPort => $port); + die "Socket to $serv: $!\n" unless defined $s; + } + $s->write("BEGIN AUTH REQUEST\n$repo\n$user\n$pass\nEND AUTH REQUEST\n") or die "Write to $serv: $!\n"; $s->flush(); - my $rep = <$s>; + $rep = <$s>; if($rep ne "I LOVE YOU\n") { $rep="" unless $rep; ``` ## Ignacio Arenaza, 2006-11-24 09:05 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67hcwp6wpp.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67hcwp6wpp.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <7vfyc9cjci.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > Let's save a bit of trouble from you. Here is what I've queued > for 'master', with fixes from the discussion so far. Looks good to me, but it seems you missed the 'if(...) {' and 'if(...){' formating issues you raised before. Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ``` ## Junio C Hamano, 2006-11-24 11:48 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <7vhcwpawvg.fsf@assigned-by-dhcp.cox.net> URL: https://gitlist.dev/e/7vhcwpawvg.fsf%40assigned-by-dhcp.cox.net In-Reply-To: <67hcwp6wpp.fsf@poseidon.eteo.mondragon.edu> ``` Ignacio Arenaza writes: > Junio C Hamano writes: > >> Let's save a bit of trouble from you. Here is what I've queued >> for 'master', with fixes from the discussion so far. > > Looks good to me, but it seems you missed the 'if(...) {' and > 'if(...){' formating issues you raised before. I deliberately left out the style changes to avoid cluttering the patch; as I said in my first response, that should come as a separate patch that does not change anything else. ``` ## Jeff King, 2006-11-24 14:57 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <20061124145700.GA13947@coredump.intra.peff.net> URL: https://gitlist.dev/e/20061124145700.GA13947%40coredump.intra.peff.net In-Reply-To: <7vfyc9cjci.fsf@assigned-by-dhcp.cox.net> ``` On Fri, Nov 24, 2006 at 12:57:49AM -0800, Junio C Hamano wrote: > + # Skip up to the empty line of the proxy server output > + # including the response headers. > + while ($rep = <$s>) { > + last if (!defined $rep || > + $rep eq "\n" || > + $rep eq "\r\n"); > + } Nit: checking defined($rep) inside the loop is redundant. ``` ## Ignacio Arenaza, 2006-11-25 12:43 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67d57b7l3s.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67d57b7l3s.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <7v7ixlhesv.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: >>> It is more about HTTP proxying and it is my understanding that >>> response to CONNECT method request has that empty line after the >>> successful (2xx) response line and zero or more response >>> headers. The code is still wrong; it does not have a loop to >>> discard the potential response headers that come before the >>> empty line the code we are discussing discards. I don't have the original message from Junio where he asks where the proxy behaviour is officially specified (as the draft has expired long ago), so I'll answer it here. There is a comment in the code that says the relevant RFC is 2817, sections 5.2 and 5.3. Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ``` ## Ignacio Arenaza, 2006-11-27 16:27 Subject: Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying Message-ID: <67bqmsn9d6.fsf@poseidon.eteo.mondragon.edu> URL: https://gitlist.dev/e/67bqmsn9d6.fsf%40poseidon.eteo.mondragon.edu In-Reply-To: <7vhcwpawvg.fsf@assigned-by-dhcp.cox.net> ``` Junio C Hamano writes: > I deliberately left out the style changes to avoid cluttering > the patch; as I said in my first response, that should come as a > separate patch that does not change anything else. Ok, I've had a look at git-cvsimport.perl and I can't come up with a consistent style for the 'if () {' sentence. According to grep there are 50 instances of 'if(...)' and 39 instances of 'if (...)', so I don't know which is the preferred one. And I can't find a CodingStyle document either. Which way should I fix them? And should I fix the whole file, or just the ones in my original patch? Saludos. Iñaki. -- School of Management Mondragon University 20560 Oñati - Spain +34 943 718009 (ext. 225) ```