threads / discuss / 3373

git-cvs-import retries

Subject: git-cvs-import retries

## tl;dr

5 messages between Feb 17, 2006 and Feb 18, 2006.

replies: 4people: 2as markdown or json

Martin Mares· Feb 17, 2006, 19:38 UTC · lore

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)

Junio C Hamano· Feb 18, 2006, 07:27 UTC · re: Martin Mares · lore

Re: git-cvs-import retries

Martin Mares <mj@ucw.cz> writes:

Show 16 quoted lines

> 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.

Show 10 quoted lines

> +++ 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.

Martin Mares· Feb 18, 2006, 13:14 UTC · re: Junio C Hamano · lore

Re: git-cvs-import retries

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.

Junio C Hamano· Feb 18, 2006, 18:42 UTC · re: Martin Mares · lore

Re: git-cvs-import retries

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-<.

Martin Mares· Feb 18, 2006, 20:44 UTC · re: Junio C Hamano · lore

[PATCH] Fix retries in git-cvsimport

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