CVS import broken?

Hi,

I track quite a few CVS repos with 'git-cvsimport -k -i', but recently it 
stopped working (of course, I was not reimporting, but incrementally 
tracking them). I think it is the introduction of one-index-per-branch 
policy:

fatal: index file mmap failed (Invalid argument)
unable to write to git-update-index:  at /.../git-cvsimport line 606, <CVS> line 277425.

It seems that git-cvsimport makes a temporary file of size 0, which cannot 
get mmap()ed, because it has size 0.

Any help appreciated,
Dscho

On 6/24/06, Johannes Schindelin [off-list ref] wrote:

It seems that git-cvsimport makes a temporary file of size 0, which cannot
get mmap()ed, because it has size 0.

This switch to tmpnam() avoids creating the tmpfile in the first place and
streamlines the code. This handling of tmpfiles is slightly safer, but there
is an inherent race condition.

---
NOTE: (a) I cannot reproduce the problem and (b) this is only lightly tested,
if trivial.

However, this switch to tmpnam() avoids creating the tmpfile in the first place
and streamlines the code. This usage of tempfiles is open to a race condition
if someone could guess the name returned by tmpnam, but even this is safer than
what we did before, which was creating a file, closing the fh and then
clobbering it from git-read-tree.

And if someone can guess the name that tmpnam() returns their magic is strong
enough that they'll go for more interesting targets.
Signed-off-by: Martin Langhoff <redacted>
---
 git-cvsimport.perl |   20 +++++---------------
 1 files changed, 5 insertions(+), 15 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index f3daa6c..d961b7b 100644
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl

@@ -17,7 +17,7 @@ use strict;
 use warnings;
 use Getopt::Std;
 use File::Spec;
-use File::Temp qw(tempfile);
+use File::Temp qw(tempfile tmpnam);
 use File::Path qw(mkpath);
 use File::Basename qw(basename dirname);
 use Time::Local;

@@ -467,12 +467,8 @@ my $orig_git_index;
 $orig_git_index = $ENV{GIT_INDEX_FILE} if exists $ENV{GIT_INDEX_FILE};
 
 my %index; # holds filenames of one index per branch
-{   # init with an index for origin
-    my ($fh, $fn) = tempfile('gitXXXXXX', SUFFIX => '.idx',
-			     DIR => File::Spec->tmpdir());
-    close ($fh);
-    $index{$opt_o} = $fn;
-}
+$index{$opt_o} = tmpnam();
+
 $ENV{GIT_INDEX_FILE} = $index{$opt_o};
 unless(-d $git_dir) {
 	system("git-init-db");

@@ -502,10 +498,7 @@ unless(-d $git_dir) {
 
 	# populate index
 	unless ($index{$last_branch}) {
-	    my ($fh, $fn) = tempfile('gitXXXXXX', SUFFIX => '.idx',
-				     DIR => File::Spec->tmpdir());
-	    close ($fh);
-	    $index{$last_branch} = $fn;
+	    $index{$last_branch} = tmpnam();
 	}
 	$ENV{GIT_INDEX_FILE} = $index{$last_branch};
 	system('git-read-tree', $last_branch);

@@ -818,10 +811,7 @@ while(<CVS>) {
 		if(($ancestor || $branch) ne $last_branch) {
 			print "Switching from $last_branch to $branch\n" if $opt_v;
 			unless ($index{$branch}) {
-			    my ($fh, $fn) = tempfile('gitXXXXXX', SUFFIX => '.idx',
-						     DIR => File::Spec->tmpdir());
-			    close ($fh);
-			    $index{$branch} = $fn;
+			    $index{$branch} = tmpnam();
 			    $ENV{GIT_INDEX_FILE} = $index{$branch};
 			    system("git-read-tree", $branch);
 			    die "read-tree failed: $?\n" if $?;

-- 
1.4.0.gcda2

Hi,

On Sat, 24 Jun 2006, Martin Langhoff wrote:

On 6/24/06, Johannes Schindelin [off-list ref] wrote:

quoted

It seems that git-cvsimport makes a temporary file of size 0, which cannot
get mmap()ed, because it has size 0.

This switch to tmpnam() avoids creating the tmpfile in the first place and
streamlines the code. This handling of tmpfiles is slightly safer, but there
is an inherent race condition.

Thank you. This fixes the error.

HOWEVER, it does not fix the main problem: when I try to git-cvsimport, 
there is no index for that branch yet, since I used to git-cvsimport with 
the old cvsimport.

Now, when cvsimport sees there is no index, it evidently assumes that the 
current state is an empty tree, which is *not* true.

The effect is: the first commit removes all files from the tree which were 
not touched by the cvs commit. Bad.

This usage of tempfiles is open to a race condition

I would not care too strongly about that. Eventually, I really would like 
this file to reside in $GIT_DIR, not /tmp, but whatever. That is not my 
biggest concern right now. That I cannot update since June 18th, however, 
is.

Ciao,
Dscho

On 6/24/06, Johannes Schindelin [off-list ref] wrote:

Thank you. This fixes the error.

Your welcome!

HOWEVER, it does not fix the main problem: when I try to git-cvsimport,
there is no index for that branch yet, since I used to git-cvsimport with
the old cvsimport.

Now, when cvsimport sees there is no index, it evidently assumes that the
current state is an empty tree, which is *not* true.

The effect is: the first commit removes all files from the tree which were
not touched by the cvs commit. Bad.

I don't quite understand. No it shouldn't be the case -- it should
create the index using git-read-tree based on the tip of the branch.
Right after the call to tmpnam() the code looks like

 $index{$branch} = tmpnam();
 $ENV{GIT_INDEX_FILE} = $index{$branch};
 system("git-read-tree", $branch);
 die "read-tree failed: $?\n" if $?;

quoted

This usage of tempfiles is open to a race condition

I would not care too strongly about that. Eventually, I really would like
this file to reside in $GIT_DIR, not /tmp, but whatever. That is not my
biggest concern right now. That I cannot update since June 18th, however,
is.

It's worrying me too. Running some tests now...




martin