[PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Inaki Arenaza]

From: =?utf-8?q?I=F1aki_Arenaza?= <iarenuno@eteo.mondragon.edu>

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 <iarenuno@eteo.mondragon.edu>
---
 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="<unknown>" unless $rep;
-- 
1.4.4

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

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.

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Martin Langhoff (CatalystIT)]

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

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

"Martin Langhoff (CatalystIT)" <martin@catalyst.net.nz> 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.

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Martin Langhoff (CatalystIT)]

Junio C Hamano wrote:

> "Martin Langhoff (CatalystIT)" <martin@catalyst.net.nz> 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

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Martin Langhoff]

On 11/24/06, Junio C Hamano <junkio@cox.net> 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,

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

> On 11/24/06, Junio C Hamano <junkio@cox.net> 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<no-op> X<nop>

        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();

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Jeff King]

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";
}

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

Jeff King <peff@peff.net> 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".

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Christian Couder]

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,

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Christian Couder <chriscool@tuxfamily.org> 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)

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Junio C Hamano <junkio@cox.net> 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)

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Junio C Hamano <junkio@cox.net> 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)

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

Ignacio Arenaza <iarenuno@eteo.mondragon.edu> 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="<unknown>" unless $rep;

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Junio C Hamano <junkio@cox.net> 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)

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Junio C Hamano]

Ignacio Arenaza <iarenuno@eteo.mondragon.edu> writes:

> Junio C Hamano <junkio@cox.net> 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.

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Jeff King]

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.

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Junio C Hamano <junkio@cox.net> 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)

Re: [PATCH] git-cvsimport: add suport for CVS pserver method HTTP/1.x proxying

[Ignacio Arenaza]

Junio C Hamano <junkio@cox.net> 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)