threads / patch / 43095

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

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

## tl;dr

19 messages between Nov 22, 2006 and Nov 27, 2006. Diffs are folded; open one to read it.

replies: 18people: 6as markdown or json

Inaki Arenaza· Nov 22, 2006, 22:26 UTC · lore

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

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(-)
Show changes to git-cvsimport.perl +44 −5
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
Junio C Hamano· Nov 23, 2006, 22:01 UTC · re: Inaki Arenaza · lore

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)· Nov 23, 2006, 22:09 UTC · re: Junio C Hamano · lore
Junio C Hamano wrote:
Show 6 quoted lines
> 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· Nov 23, 2006, 23:02 UTC · re: Martin Langhoff (CatalystIT) · lore
"Martin Langhoff (CatalystIT)" <martin@catalyst.net.nz> writes:
Show 12 quoted lines
> 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.

Show 5 quoted lines
>> 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)· Nov 23, 2006, 23:20 UTC · re: Junio C Hamano · lore
Junio C Hamano wrote:
Show 9 quoted lines
> "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
Martin Langhoff· Nov 23, 2006, 23:52 UTC · re: Junio C Hamano · lore
On 11/24/06, Junio C Hamano <junkio@cox.net> wrote:
Show 6 quoted lines
> 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· Nov 24, 2006, 00:24 UTC · re: Martin Langhoff · lore
"Martin Langhoff" <martin.langhoff@gmail.com> writes:
Show 16 quoted lines
> 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();
Ignacio Arenaza· Nov 25, 2006, 12:43 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> writes:
Show 6 quoted lines
>>> 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)
Jeff King· Nov 24, 2006, 01:42 UTC · re: Martin Langhoff · lore
On Fri, Nov 24, 2006 at 12:52:13PM +1300, Martin Langhoff wrote:
Show 8 quoted lines
> 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· Nov 24, 2006, 02:54 UTC · re: Jeff King · lore
Jeff King <peff@peff.net> writes:
Show 13 quoted lines
> 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".

Ignacio Arenaza· Nov 24, 2006, 08:43 UTC · re: Junio C Hamano · lore
Junio C Hamano <junkio@cox.net> writes:
Show 7 quoted lines
> 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· Nov 24, 2006, 08:46 UTC · re: Junio C Hamano · lore
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)
Junio C Hamano· Nov 24, 2006, 08:57 UTC · re: Ignacio Arenaza · lore
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.

---
Show changes to git-cvsimport.perl +49 −5
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;
Ignacio Arenaza· Nov 24, 2006, 09:05 UTC · re: Junio C Hamano · lore
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)
Junio C Hamano· Nov 24, 2006, 11:48 UTC · re: Ignacio Arenaza · lore
Ignacio Arenaza <iarenuno@eteo.mondragon.edu> writes:
Show 7 quoted lines
> 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.

Ignacio Arenaza· Nov 27, 2006, 16:27 UTC · re: Junio C Hamano · lore
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)
Jeff King· Nov 24, 2006, 14:57 UTC · re: Junio C Hamano · lore
On Fri, Nov 24, 2006 at 12:57:49AM -0800, Junio C Hamano wrote:
Show 7 quoted lines
> +			# 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.
Christian Couder· Nov 24, 2006, 04:48 UTC · re: Inaki Arenaza · lore
Inaki Arenaza wrote:
[...]
Show 26 quoted lines
>
> +		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.
Show 6 quoted lines
> +			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· Nov 24, 2006, 08:41 UTC · re: Christian Couder · lore
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)

← back to recent threads