git-cvs-import retries

git-cvs-import retries

[Martin Mares]

Hello!

I am trying git-cvsimport on a rather huge repository and the CVS server
sometimes drops the connection and the whole importing aborts, although
it contains some retrying logic. I've noticed that in the connection closes
I experience, $res ends up being empty instead of undefined. This is tested
by the `server went again' check, but not by the retry check a couple of
lines before.

This patch extends the retry check and makes the symptoms go away.
However, take it with a grain of salt as I don't understand yet why the
connection is aborted.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
A jury consists of 12 persons chosen to decide who has the better lawyer.


Signed-Off-By: Martin Mares <mj@ucw.cz>

--- old/git-cvsimport	2006-02-17 13:02:24.000000000 +0100
+++ new/git-cvsimport	2006-02-17 18:13:06.000000000 +0100
@@ -371,7 +371,7 @@
 
 	$self->_file($fn,$rev) and $res = $self->_line($fh);
 
-	if (!defined $res) {
+	if (!defined $res || $res eq '') {
 	    # retry
 	    $self->conn();
 	    $self->_file($fn,$rev)

Re: git-cvs-import retries

[Junio C Hamano]

Martin Mares <mj@ucw.cz> writes:

> Hello!
>...
> This patch extends the retry check and makes the symptoms go away.
> However, take it with a grain of salt as I don't understand yet why the
> connection is aborted.
>
> 				Have a nice fortnight
> -- 
> Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
> Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
> A jury consists of 12 persons chosen to decide who has the better lawyer.
>
>
> Signed-Off-By: Martin Mares <mj@ucw.cz>
>
> --- old/git-cvsimport	2006-02-17 13:02:24.000000000 +0100

First, one technicality.  You can see what's wrong with the
above, right?  Remember, the top part of your message goes into
the commit log, so we do not want "Hello!" nor signature.

> +++ new/git-cvsimport	2006-02-17 18:13:06.000000000 +0100
> @@ -371,7 +371,7 @@
>  
>  	$self->_file($fn,$rev) and $res = $self->_line($fh);
>  
> -	if (!defined $res) {
> +	if (!defined $res || $res eq '') {
>  	    # retry
>  	    $self->conn();
>  	    $self->_file($fn,$rev)

I read _line() three times but its return value is the lexical
variable $res which is initialized to 0 and then either reset to
0 by assignment or updated with $res += somethingelse.  So I do
not see how you can get a defined but empty string in there.
Even when _file() returns false, the $res variable in file()
(the function you are modifying) is not initialized, so it would
stay undefined.

Maybe I am missing something very obvious, but I cannot see how
this can make any difference.  Please enlighten.

Re: git-cvs-import retries

[Martin Mares]

Hi Junio!

> First, one technicality.  You can see what's wrong with the
> above, right?  Remember, the top part of your message goes into
> the commit log, so we do not want "Hello!" nor signature.

Sorry about that, the patch was intended more for discussion than
for applying.

> I read _line() three times but its return value is the lexical
> variable $res which is initialized to 0 and then either reset to
> 0 by assignment or updated with $res += somethingelse.  So I do
> not see how you can get a defined but empty string in there.

You almost convinced me that my fix couldn't have changed anything :-)

But it did and I finally understand why: _line() can exit not only
by return, but also by falling over when readline() returns undef.
In this case, something weird is returned (the most recent expression
evaluated) and it's *sometimes* the empty string.

I will send a new patch.

				Have a nice fortnight
-- 
Martin `MJ' Mares   <mj@ucw.cz>   http://atrey.karlin.mff.cuni.cz/~mj/
Faculty of Math and Physics, Charles University, Prague, Czech Rep., Earth
The first myth of management is that it exists.

Re: git-cvs-import retries

[Junio C Hamano]

Martin Mares <mj@ucw.cz> writes:

> But it did and I finally understand why: _line() can exit not only
> by return, but also by falling over when readline() returns undef.

Ah, you are right and I feel stupid.  It's been a while since I
wrote real Perl code the last time, and forgot that "the last
evaluation" rule when I was reading the code.

It did not help that one of the languages I use in my day-job
(which I am too ashamed to even name) uses "if fell off at the
end return undef" rule X-<.

[PATCH] Fix retries in git-cvsimport

[Martin Mares]

Fixed a couple of bugs in recovering from broken connections:

The _line() method now returns undef correctly when the connection
is broken instead of falling off the function and returning garbage.

Retries are now reported to stderr and the eventual partially
downloaded file is discarded instead of being appended to.

The "Server gone away" test has been removed, because it was
reachable only if the garbage return bug bit.

Signed-Off-By: Martin Mares <mj@ucw.cz>

--- old/git-cvsimport	2006-02-17 13:02:24.000000000 +0100
+++ new/git-cvsimport	2006-02-18 14:16:33.000000000 +0100
@@ -361,6 +361,7 @@
 			}
 		}
 	}
+	return undef;
 }
 sub file {
 	my($self,$fn,$rev) = @_;
@@ -372,19 +373,15 @@
 	$self->_file($fn,$rev) and $res = $self->_line($fh);
 
 	if (!defined $res) {
-	    # retry
+	    print STDERR "Server has gone away while fetching $fn $rev, retrying...\n";
+	    truncate $fh, 0;
 	    $self->conn();
-	    $self->_file($fn,$rev)
-		    or die "No file command send\n";
+	    $self->_file($fn,$rev) or die "No file command send";
 	    $res = $self->_line($fh);
-	    die "No input: $fn $rev\n" unless defined $res;
+	    die "Retry failed" unless defined $res;
 	}
 	close ($fh);
 
-	if ($res eq '') {
-	    die "Looks like the server has gone away while fetching $fn $rev -- exiting!";
-	}
-
 	return ($name, $res);
 }