git-svn messed up import, badly

git-svn messed up import, badly

[Björn Steinbrink]

Hi,

Samuel reported on #git that a git-svn import was failing for him.  I
tried to reproduce that, and while it failed on a different svn revision
for me, it still failed pretty badly.

The repo is at:
	svn://scm.gforge.inria.fr/svn/coq/

I cloned with:
	git svn clone -s svn://scm.gforge.inria.fr/svn/coq/

And with my current git-svn import, I'm getting this error message:

Checksum mismatch: trunk/.depend 16e748c219f9f95bf3d05c6b2af5444290bc8471
expected: 05fb5edb8c8057be006c7e913ae0c764
     got: 763b9a426c5bd61e0a85252459d37cfa

I got that when it bailed out from "git svn clone" and I get it on each
"git svn fetch" run, trying to resume the import.

Looking at the history of trunk/.depend and comparing the diffs that git
gives me to those that "svn diff" gives me, I noticed that the diffs for
changes to .depend introduced in svn revision 2314 differ heavily
between svn and my git-svn import.

Tarball of the repo and the two diffs is at:
http://people.linux-vserver.org/~doener/broken-git-svn-import.tbz
[4.8MB]

$ git --version
git version 1.5.6.1.78.gde8d9

git svn --version
git-svn version 1.5.6.1.78.gde8d9 (svn 1.4.6)

Avery and Adam on Cc:, because Avery reported a somewhat similar issue
that he bisected down to the git-svn speed-up patch by Adam.

Unfortunately, I currently don't have enough time on my hands to perform
a bisection.

If you need anything else, let me know.

Thanks,
Björn

[PATCH] Revert "git-svn: Speed up fetch"

[Avery Pennarun]

This reverts commit ffe256f9bac8a40ff751a9341a5869d98f72c285, because it
was causing errors of the form:

Checksum mismatch: trunk/.depend 16e748c219f9f95bf3d05c6b2af5444290bc8471
expected: 05fb5edb8c8057be006c7e913ae0c764
     got: 763b9a426c5bd61e0a85252459d37cfa
     
Note that the exact failing file and checksum seems to vary if you clear
the repository and try again.

Conflicts:

	git-svn.perl
---
Obviously it would be better to actually fix the bug here than the revert
the patch (because the patch really *does* make fetch go a lot faster), but
I don't know where to begin, and it's a pain to debug because of the
variability.

 git-svn.perl |   42 ++++++++++++++++++++++--------------------
 1 files changed, 22 insertions(+), 20 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 4c9c59b..a02bcf4 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4,7 +4,7 @@
 use warnings;
 use strict;
 use vars qw/	$AUTHOR $VERSION
-		$sha1 $sha1_short $_revision $_repository
+		$sha1 $sha1_short $_revision
 		$_q $_authors %users/;
 $AUTHOR = 'Eric Wong <normalperson@yhbt.net>';
 $VERSION = '@@GIT_VERSION@@';
@@ -223,7 +223,6 @@ unless ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
 		}
 		$ENV{GIT_DIR} = $git_dir;
 	}
-	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }
 
 my %opts = %{$cmd{$cmd}->[2]} if (defined $cmd);
@@ -305,7 +304,6 @@ sub do_git_init_db {
 			}
 		}
 		command_noisy(@init_db);
-		$_repository = Git->repository(Repository => ".git");
 	}
 	my $set;
 	my $pfx = "svn-remote.$Git::SVN::default_repo_id";
@@ -322,7 +320,6 @@ sub init_subdir {
 	mkpath([$repo_path]) unless -d $repo_path;
 	chdir $repo_path or die "Couldn't chdir to $repo_path: $!\n";
 	$ENV{GIT_DIR} = '.git';
-	$_repository = Git->repository(Repository => $ENV{GIT_DIR});
 }
 
 sub cmd_clone {
@@ -3040,7 +3037,6 @@ use vars qw/@ISA/;
 use strict;
 use warnings;
 use Carp qw/croak/;
-use File::Temp qw/tempfile/;
 use IO::File qw//;
 
 # file baton members: path, mode_a, mode_b, pool, fh, blob, base
@@ -3196,9 +3192,14 @@ sub apply_textdelta {
 	my $base = IO::File->new_tmpfile;
 	$base->autoflush(1);
 	if ($fb->{blob}) {
-		print $base 'link ' if ($fb->{mode_a} == 120000);
-		my $size = $::_repository->cat_blob($fb->{blob}, $base);
-		die "Failed to read object $fb->{blob}" if ($size < 0);
+		defined (my $pid = fork) or croak $!;
+		if (!$pid) {
+			open STDOUT, '>&', $base or croak $!;
+			print STDOUT 'link ' if ($fb->{mode_a} == 120000);
+			exec qw/git-cat-file blob/, $fb->{blob} or croak $!;
+		}
+		waitpid $pid, 0;
+		croak $? if $?;
 
 		if (defined $exp) {
 			seek $base, 0, 0 or croak $!;
@@ -3239,18 +3240,14 @@ sub close_file {
 				sysseek($fh, 0, 0) or croak $!;
 			}
 		}
-
-		my ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);
-		my $result;
-		while ($result = sysread($fh, my $string, 1024)) {
-			syswrite($tmp_fh, $string, $result);
+		defined(my $pid = open my $out,'-|') or die "Can't fork: $!\n";
+		if (!$pid) {
+			open STDIN, '<&', $fh or croak $!;
+			exec qw/git-hash-object -w --stdin/ or croak $!;
 		}
-		defined $result or croak $!;
-		close $tmp_fh or croak $!;
-
+		chomp($hash = do { local $/; <$out> });
+		close $out or croak $!;
 		close $fh or croak $!;
-
-		$hash = $::_repository->hash_and_insert_object($tmp_filename);
 		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
 		close $fb->{base} or croak $!;
 	} else {
@@ -3576,8 +3573,13 @@ sub chg_file {
 	} elsif ($m->{mode_a} =~ /^120/ && $m->{mode_b} !~ /^120/) {
 		$self->change_file_prop($fbat,'svn:special',undef);
 	}
-	my $size = $::_repository->cat_blob($m->{sha1_b}, $fh);
-	croak "Failed to read object $m->{sha1_b}" if ($size < 0);
+	defined(my $pid = fork) or croak $!;
+	if (!$pid) {
+		open STDOUT, '>&', $fh or croak $!;
+		exec qw/git-cat-file blob/, $m->{sha1_b} or croak $!;
+	}
+	waitpid $pid, 0;
+	croak $? if $?;
 	$fh->flush == 0 or croak $!;
 	seek $fh, 0, 0 or croak $!;
 
-- 
1.5.4.3

[PATCH v2] git-svn: avoid filling up the disk with temp files.

[Avery Pennarun]

Commit ffe256f9bac8a40ff751a9341a5869d98f72c285 ("git-svn: Speed up fetch")
introduced changes that create a temporary file for each object fetched by
svn.  These files should be deleted automatically, but perl apparently
doesn't do this until the process exits (or perhaps when its garbage
collector runs).

This means that on a large fetch, especially with lots of branches, we
sometimes fill up /tmp completely, which prevents the next temp file from
being written completely.  This is aggravated by the fact that a new temp
file is created for each updated file, even if that update produces a file
identical to one already in git.  Thus, it can happen even if there's lots
of disk space to store the finished repository.

We weren't adequately checking for write errors, so this would result in an
invalid file getting committed, which caused git-svn to fail later with an
invalid checksum.

This patch adds a check to syswrite() so similar problems don't lead to
corruption in the future.  It also unlink()'s each temp file explicitly
when we're done with it, so the disk doesn't need to fill up.

Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
---

Please use this in favour of the "Revert "git-svn: Speed up fetch" I sent
earlier.  I ended up having a surprise inspiration that led to a real fix :)

 git-svn.perl |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/git-svn.perl b/git-svn.perl
index 263d66c..0011387 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -3243,7 +3243,9 @@ sub close_file {
 		my ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);
 		my $result;
 		while ($result = sysread($fh, my $string, 1024)) {
-			syswrite($tmp_fh, $string, $result);
+			my $wrote = syswrite($tmp_fh, $string, $result);
+			defined($wrote) && $wrote == $result
+				or croak("write $tmp_filename: $!\n");
 		}
 		defined $result or croak $!;
 		close $tmp_fh or croak $!;
@@ -3251,6 +3253,7 @@ sub close_file {
 		close $fh or croak $!;
 
 		$hash = $::_repository->hash_and_insert_object($tmp_filename);
+		unlink($tmp_filename);
 		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
 		close $fb->{base} or croak $!;
 	} else {
-- 
1.5.4.3

Re: [PATCH] Revert "git-svn: Speed up fetch"

[Mikael Magnusson]

2008/6/28 Avery Pennarun <apenwarr@gmail.com>:
> This reverts commit ffe256f9bac8a40ff751a9341a5869d98f72c285, because it
> was causing errors of the form:
>
> Checksum mismatch: trunk/.depend 16e748c219f9f95bf3d05c6b2af5444290bc8471
> expected: 05fb5edb8c8057be006c7e913ae0c764
>     got: 763b9a426c5bd61e0a85252459d37cfa
>
> Note that the exact failing file and checksum seems to vary if you clear
> the repository and try again.
>
> Conflicts:
>
>        git-svn.perl

Wasn't this the problem that was fixed by
d683a0e00cd4734b4fab704baef1ee76205722be[1]?

-- 
Mikael Magnusson

[1]

commit d683a0e00cd4734b4fab704baef1ee76205722be
Author: Junio C Hamano <gitster@pobox.com>
Date:   Tue May 27 23:33:22 2008 -0700

    Git::cat_blob: allow using an empty blob to fix git-svn breakage

    Recent "git-svn optimization" series introduced Git::cat_blob() subroutine
    whose interface was broken in that it returned the size of the blob but
    signalled an error by returning 0.  You can never use an empty blob with
    such an interface.

    This fixes the interface to return a negative value to signal an error.

    Reported by Björn Steinbrink.

    Signed-off-by: Junio C Hamano <gitster@pobox.com>

Re: [PATCH] Revert "git-svn: Speed up fetch"

[Avery Pennarun]

On 6/28/08, Mikael Magnusson <mikachu@gmail.com> wrote:
> Wasn't this the problem that was fixed by
>  d683a0e00cd4734b4fab704baef1ee76205722be?
>
>  commit d683a0e00cd4734b4fab704baef1ee76205722be
>  Author: Junio C Hamano <gitster@pobox.com>
>  Date:   Tue May 27 23:33:22 2008 -0700
>
>     Git::cat_blob: allow using an empty blob to fix git-svn breakage
> [...]

No, I have that patch and the problem still occurred.

Avery

Re: [PATCH v2] git-svn: avoid filling up the disk with temp files.

[Björn Steinbrink]

On 2008.06.28 19:33:56 -0400, Avery Pennarun wrote:
> Commit ffe256f9bac8a40ff751a9341a5869d98f72c285 ("git-svn: Speed up fetch")
> introduced changes that create a temporary file for each object fetched by
> svn.  These files should be deleted automatically, but perl apparently
> doesn't do this until the process exits (or perhaps when its garbage
> collector runs).
> 
> This means that on a large fetch, especially with lots of branches, we
> sometimes fill up /tmp completely, which prevents the next temp file from
> being written completely.  This is aggravated by the fact that a new temp
> file is created for each updated file, even if that update produces a file
> identical to one already in git.  Thus, it can happen even if there's lots
> of disk space to store the finished repository.
> 
> We weren't adequately checking for write errors, so this would result in an
> invalid file getting committed, which caused git-svn to fail later with an
> invalid checksum.
> 
> This patch adds a check to syswrite() so similar problems don't lead to
> corruption in the future.  It also unlink()'s each temp file explicitly
> when we're done with it, so the disk doesn't need to fill up.

Oh sweet! That also means that I don't have to fear about the repos I
already track with git-svn being corrupted (at least I hope so). :-)

And yeah, looking at /tmp, there are still 58K temp files belonging to
the failed import. Temporarily mounting a 8k tmpfs there triggers the
bug immediately, while with this patch I get a error message.

I'm running another import of the coq repo that failed earlier, just to
make sure, but I also just noticed that cat-file --batch is leaking
memory. For example this one shoots up to about 700M RSS usage with
git.git:

git rev-list --objects origin/master | \
	sed 's/ .*//' | \
	git cat-file --batch > /dev/null

I'll follow-up with a patch that at least fixes the worst part of that,
getting the RSS usage for the above test down to about 40M.

Thanks!
Björn

[PATCH] git cat-file: Fix memory leak in batch mode

[Björn Steinbrink]

When run in batch mode, git cat-file never frees the memory for the blob
contents it is printing. This quickly adds up and causes git-svn to be
hardly usable for imports of large svn repos, because it uses cat-file in
batch mode and cat-file's memory usage easily reaches several hundred MB
without any good reason.

Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
---
 builtin-cat-file.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/builtin-cat-file.c b/builtin-cat-file.c
index bd343ef..f966dcb 100644
--- a/builtin-cat-file.c
+++ b/builtin-cat-file.c
@@ -183,6 +183,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
 		fflush(stdout);
 	}
 
+	free(contents);
 	return 0;
 }
 
-- 
1.5.6.1.94.gd3899.dirty

Re: [PATCH v2] git-svn: avoid filling up the disk with temp files.

[Björn Steinbrink]

On 2008.06.29 02:58:58 +0200, Björn Steinbrink wrote:
> On 2008.06.28 19:33:56 -0400, Avery Pennarun wrote:
> > Commit ffe256f9bac8a40ff751a9341a5869d98f72c285 ("git-svn: Speed up fetch")
> > introduced changes that create a temporary file for each object fetched by
> > svn.  These files should be deleted automatically, but perl apparently
> > doesn't do this until the process exits (or perhaps when its garbage
> > collector runs).
> > 
> > This means that on a large fetch, especially with lots of branches, we
> > sometimes fill up /tmp completely, which prevents the next temp file from
> > being written completely.  This is aggravated by the fact that a new temp
> > file is created for each updated file, even if that update produces a file
> > identical to one already in git.  Thus, it can happen even if there's lots
> > of disk space to store the finished repository.
> > 
> > We weren't adequately checking for write errors, so this would result in an
> > invalid file getting committed, which caused git-svn to fail later with an
> > invalid checksum.
> > 
> > This patch adds a check to syswrite() so similar problems don't lead to
> > corruption in the future.  It also unlink()'s each temp file explicitly
> > when we're done with it, so the disk doesn't need to fill up.
> 
> I'm running another import of the coq repo that failed earlier, just to
> make sure, ...

Import completed successfully, so:

Tested-by: Björn Steinbrink <B.Steinbrink@gmx.de>

Thanks,
Björn

Re: [PATCH v2] git-svn: avoid filling up the disk with temp files.

[Eric Wong]

Avery Pennarun <apenwarr@gmail.com> wrote:
> Commit ffe256f9bac8a40ff751a9341a5869d98f72c285 ("git-svn: Speed up fetch")
> introduced changes that create a temporary file for each object fetched by
> svn.  These files should be deleted automatically, but perl apparently
> doesn't do this until the process exits (or perhaps when its garbage
> collector runs).
> 
> This means that on a large fetch, especially with lots of branches, we
> sometimes fill up /tmp completely, which prevents the next temp file from
> being written completely.  This is aggravated by the fact that a new temp
> file is created for each updated file, even if that update produces a file
> identical to one already in git.  Thus, it can happen even if there's lots
> of disk space to store the finished repository.
> 
> We weren't adequately checking for write errors, so this would result in an
> invalid file getting committed, which caused git-svn to fail later with an
> invalid checksum.
> 
> This patch adds a check to syswrite() so similar problems don't lead to
> corruption in the future.  It also unlink()'s each temp file explicitly
> when we're done with it, so the disk doesn't need to fill up.
> 
> Signed-off-by: Avery Pennarun <apenwarr@gmail.com>
> ---
 
> Please use this in favour of the "Revert "git-svn: Speed up fetch" I sent
> earlier.  I ended up having a surprise inspiration that led to a real fix :)

Ouch, I didn't noticed these unchecked syscalls :x

Very graciously
Acked-by: Eric Wong <normalperson@yhbt.net>

Apologies to all users who were bitten by this bug.

>  git-svn.perl |    5 ++++-
>  1 files changed, 4 insertions(+), 1 deletions(-)
> 
> diff --git a/git-svn.perl b/git-svn.perl
> index 263d66c..0011387 100755
> --- a/git-svn.perl
> +++ b/git-svn.perl
> @@ -3243,7 +3243,9 @@ sub close_file {
>  		my ($tmp_fh, $tmp_filename) = File::Temp::tempfile(UNLINK => 1);
>  		my $result;
>  		while ($result = sysread($fh, my $string, 1024)) {
> -			syswrite($tmp_fh, $string, $result);
> +			my $wrote = syswrite($tmp_fh, $string, $result);
> +			defined($wrote) && $wrote == $result
> +				or croak("write $tmp_filename: $!\n");
>  		}
>  		defined $result or croak $!;
>  		close $tmp_fh or croak $!;
> @@ -3251,6 +3253,7 @@ sub close_file {
>  		close $fh or croak $!;
>  
>  		$hash = $::_repository->hash_and_insert_object($tmp_filename);
> +		unlink($tmp_filename);
>  		$hash =~ /^[a-f\d]{40}$/ or die "not a sha1: $hash\n";
>  		close $fb->{base} or croak $!;
>  	} else {
> -- 
> 1.5.4.3

Re: [PATCH] git cat-file: Fix memory leak in batch mode

[Junio C Hamano]

Björn Steinbrink <B.Steinbrink@gmx.de> writes:

> When run in batch mode, git cat-file never frees the memory for the blob
> contents it is printing. This quickly adds up and causes git-svn to be
> hardly usable for imports of large svn repos, because it uses cat-file in
> batch mode and cat-file's memory usage easily reaches several hundred MB
> without any good reason.
>
> Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> ---
>  builtin-cat-file.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/builtin-cat-file.c b/builtin-cat-file.c
> index bd343ef..f966dcb 100644
> --- a/builtin-cat-file.c
> +++ b/builtin-cat-file.c
> @@ -183,6 +183,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
>  		fflush(stdout);
>  	}
>  
> +	free(contents);
>  	return 0;
>  }

Thanks, except that it should go inside the "if (print_contents == BATCH)"
block to avoid freeing an uninitialized pointer.

Re: [PATCH] git cat-file: Fix memory leak in batch mode

[Björn Steinbrink]

On 2008.06.28 20:36:46 -0700, Junio C Hamano wrote:
> Björn Steinbrink <B.Steinbrink@gmx.de> writes:
> 
> > When run in batch mode, git cat-file never frees the memory for the blob
> > contents it is printing. This quickly adds up and causes git-svn to be
> > hardly usable for imports of large svn repos, because it uses cat-file in
> > batch mode and cat-file's memory usage easily reaches several hundred MB
> > without any good reason.
> >
> > Signed-off-by: Björn Steinbrink <B.Steinbrink@gmx.de>
> > ---
> >  builtin-cat-file.c |    1 +
> >  1 files changed, 1 insertions(+), 0 deletions(-)
> >
> > diff --git a/builtin-cat-file.c b/builtin-cat-file.c
> > index bd343ef..f966dcb 100644
> > --- a/builtin-cat-file.c
> > +++ b/builtin-cat-file.c
> > @@ -183,6 +183,7 @@ static int batch_one_object(const char *obj_name, int print_contents)
> >  		fflush(stdout);
> >  	}
> >  
> > +	free(contents);
> >  	return 0;
> >  }
> 
> Thanks, except that it should go inside the "if (print_contents == BATCH)"
> block to avoid freeing an uninitialized pointer.

Ah crap, I even wondered about the kill-a-warning initialization of
"contents", but my brain was already asleep.

Thanks,
Björn