git-cvsimport bug

git-cvsimport bug

[Emanuele Giaquinta]

Hi,

a call to git-cvsimport after a 'git-pack-refs --all' seems to mess up
the origin branch, because the first new change is committed with no
parent (git-cvsimport reports 'Parent ID (empty)').
It does not happen if I disable loose refs pruning with --no-prune. A
tiny test repo you can use to reproduce the problem is
mextli.tomaw.net/~exg/lzf.tar. Is it expected?

Thanks,

Emanuele Giaquinta

Re: git-cvsimport bug

[Jeff King]

On Tue, Nov 27, 2007 at 04:01:36PM +0100, Emanuele Giaquinta wrote:

> a call to git-cvsimport after a 'git-pack-refs --all' seems to mess up
> the origin branch, because the first new change is committed with no
> parent (git-cvsimport reports 'Parent ID (empty)').
> It does not happen if I disable loose refs pruning with --no-prune. A
> tiny test repo you can use to reproduce the problem is
> mextli.tomaw.net/~exg/lzf.tar. Is it expected?

Some of git-cvsimport is quite old, and it accesses the ref files
directly. It should be fairly easy to fix; I will post a patch in a few
minutes.

-Peff

Re: git-cvsimport bug

[Jeff King]

On Wed, Nov 28, 2007 at 11:57:46AM -0500, Jeff King wrote:

> Some of git-cvsimport is quite old, and it accesses the ref files
> directly. It should be fairly easy to fix; I will post a patch in a few
> minutes.

The patch series is:

  1/3: Add basic cvsimport tests

       We had no tests before, so this at least gives a sanity check. I
       added a t9600 series, though perhaps the cvs-related tests
       (cvsserver and exportcommit) should collapse to a single t9[0-9]*
       series.

  2/3: cvsimport: use show-ref to support packed refs

       This fix is hopefully obvious, and the included test fails
       without it (and this should probably fix Emanuele's problem).

  3/3: cvsimport: miscellaneous packed-ref fixes

       This fixes all of the packed-ref problem spots I could find.
       However, I have no tests that show the problems or that verify
       that the fixes are sane. So apply with caution.

       Probably cvsimport would benefit greatly from a conversion to
       Git.pm, but that is likely to involve a lot of rewriting, and I
       have neither the time nor the inclination for that right now.

-Peff

[PATCH 1/3] Add basic cvsimport tests

[Jeff King]

We weren't even testing basic things before, so let's at
least try importing and updating a trivial repository, which
will catch total breakage.

Signed-off-by: Jeff King <peff@peff.net>
---
 t/t9600-cvsimport.sh |   99 ++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 99 insertions(+), 0 deletions(-)
 create mode 100755 t/t9600-cvsimport.sh

diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
new file mode 100755
index 0000000..1ee06bb
--- /dev/null
+++ b/t/t9600-cvsimport.sh
@@ -0,0 +1,99 @@
+#!/bin/sh
+
+test_description='git-cvsimport basic tests'
+. ./test-lib.sh
+
+if ! ( type cvs && type cvsps ) >/dev/null 2>&1
+then
+	test_expect_success 'skipping cvsimport tests, cvs/cvsps not found' ''
+	test_done
+	exit
+fi
+
+CVSROOT=$(pwd)/cvsroot
+export CVSROOT
+# for clean cvsps cache
+HOME=$(pwd)
+export HOME
+
+test_expect_success 'setup cvsroot' 'cvs init'
+
+test_expect_success 'setup a cvs module' '
+
+	mkdir $CVSROOT/module &&
+	cvs co -d module-cvs module &&
+	cd module-cvs &&
+	cat <<EOF >o_fortuna &&
+O Fortuna
+velut luna
+statu variabilis,
+
+semper crescis
+aut decrescis;
+vita detestabilis
+
+nunc obdurat
+et tunc curat
+ludo mentis aciem,
+
+egestatem,
+potestatem
+dissolvit ut glaciem.
+EOF
+	cvs add o_fortuna &&
+	cat <<EOF >message &&
+add "O Fortuna" lyrics
+
+These public domain lyrics make an excellent sample text.
+EOF
+	cvs commit -F message &&
+	cd ..
+'
+
+test_expect_success 'import a trivial module' '
+
+	git cvsimport -a -z 0 -C module-git module &&
+	git diff module-cvs/o_fortuna module-git/o_fortuna
+
+'
+
+test_expect_success 'update cvs module' '
+
+	cd module-cvs &&
+	cat <<EOF >o_fortuna &&
+O Fortune,
+like the moon
+you are changeable,
+
+ever waxing
+and waning;
+hateful life
+
+first oppresses
+and then soothes
+as fancy takes it;
+
+poverty
+and power
+it melts them like ice.
+EOF
+	cat <<EOF >message &&
+translate to English
+
+My Latin is terrible.
+EOF
+	cvs commit -F message &&
+	cd ..
+'
+
+test_expect_success 'update git module' '
+
+	cd module-git &&
+	git cvsimport -a -z 0 module &&
+	git merge origin &&
+	cd .. &&
+	git diff module-cvs/o_fortuna module-git/o_fortuna
+
+'
+
+test_done
-- 
1.5.3.6.2039.g0495

Re: [PATCH 1/3] Add basic cvsimport tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +	cat <<EOF >o_fortuna &&
> +O Fortuna
> + ...

Copyrights?

> +add "O Fortuna" lyrics
> +
> +These public domain lyrics make an excellent sample text.
> +EOF

Ah, really?  Ok... 

Re: [PATCH 1/3] Add basic cvsimport tests

[David Kastrup]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> +	cat <<EOF >o_fortuna &&
>> +O Fortuna
>> + ...
>
> Copyrights?

Benediktbeuern, thirteenth century.

>> +add "O Fortuna" lyrics
>> +
>> +These public domain lyrics make an excellent sample text.
>> +EOF

As long as nobody adds Orff's musical arrangements...

-- 
David Kastrup, Kriemhildstr. 15, 44793 Bochum

Re: [PATCH 1/3] Add basic cvsimport tests

[Andreas Ericsson]

Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
> 
>> +	cat <<EOF >o_fortuna &&
>> +O Fortuna
>> + ...
> 
> Copyrights?
> 
>> +add "O Fortuna" lyrics
>> +
>> +These public domain lyrics make an excellent sample text.
>> +EOF
> 
> Ah, really?  Ok... 


The poet, whoever he/she was (no-one knows today), has been dead
nearly 800 years, so any copyrights that might once have been in
place have expired.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

[PATCH 2/3] cvsimport: use show-ref to support packed refs

[Jeff King]

Previously, if refs were packed, git-cvsimport would assume
that particular refs did not exist. This could lead to, for
example, overwriting previous 'origin' commits that were
packed.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-cvsimport.perl   |   24 +++++++++---------------
 t/t9600-cvsimport.sh |    2 ++
 2 files changed, 11 insertions(+), 15 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index efa6a0c..b852f2f 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -527,18 +527,12 @@ sub is_sha1 {
 	return $s =~ /^[a-f0-9]{40}$/;
 }
 
-sub get_headref ($$) {
-    my $name    = shift;
-    my $git_dir = shift;
-
-    my $f = "$git_dir/$remote/$name";
-    if (open(my $fh, $f)) {
-	    chomp(my $r = <$fh>);
-	    is_sha1($r) or die "Cannot get head id for $name ($r): $!";
-	    return $r;
-    }
-    die "unable to open $f: $!" unless $! == POSIX::ENOENT;
-    return undef;
+sub get_headref ($) {
+	my $name = shift;
+	my $r = `git show-ref -s '$name'`;
+	return undef unless $? == 0;
+	chomp $r;
+	return $r;
 }
 
 -d $git_tree
@@ -698,7 +692,7 @@ my (@old,@new,@skipped,%ignorebranch);
 $ignorebranch{'#CVSPS_NO_BRANCH'} = 1;
 
 sub commit {
-	if ($branch eq $opt_o && !$index{branch} && !get_headref($branch, $git_dir)) {
+	if ($branch eq $opt_o && !$index{branch} && !get_headref($branch)) {
 	    # looks like an initial commit
 	    # use the index primed by git-init
 	    $ENV{GIT_INDEX_FILE} = "$git_dir/index";
@@ -722,7 +716,7 @@ sub commit {
 	update_index(@old, @new);
 	@old = @new = ();
 	my $tree = write_tree();
-	my $parent = get_headref($last_branch, $git_dir);
+	my $parent = get_headref($last_branch);
 	print "Parent ID " . ($parent ? $parent : "(empty)") . "\n" if $opt_v;
 
 	my @commit_args;
@@ -733,7 +727,7 @@ sub commit {
 	foreach my $rx (@mergerx) {
 		next unless $logmsg =~ $rx && $1;
 		my $mparent = $1 eq 'HEAD' ? $opt_o : $1;
-		if (my $sha1 = get_headref($mparent, $git_dir)) {
+		if (my $sha1 = get_headref($mparent)) {
 			push @commit_args, '-p', $mparent;
 			print "Merge parent branch: $mparent\n" if $opt_v;
 		}
diff --git a/t/t9600-cvsimport.sh b/t/t9600-cvsimport.sh
index 1ee06bb..3338d44 100755
--- a/t/t9600-cvsimport.sh
+++ b/t/t9600-cvsimport.sh
@@ -57,6 +57,8 @@ test_expect_success 'import a trivial module' '
 
 '
 
+test_expect_success 'pack refs' 'cd module-git && git gc && cd ..'
+
 test_expect_success 'update cvs module' '
 
 	cd module-cvs &&
-- 
1.5.3.6.2039.g0495

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Jeff King wrote:

> diff --git a/git-cvsimport.perl b/git-cvsimport.perl
> index efa6a0c..b852f2f 100755
> --- a/git-cvsimport.perl
> +++ b/git-cvsimport.perl
> @@ -527,18 +527,12 @@ sub is_sha1 {
>  	return $s =~ /^[a-f0-9]{40}$/;
>  }
>  
> -sub get_headref ($$) {
> -    my $name    = shift;
> -    my $git_dir = shift;
> -
> -    my $f = "$git_dir/$remote/$name";
> -    if (open(my $fh, $f)) {
> -	    chomp(my $r = <$fh>);
> -	    is_sha1($r) or die "Cannot get head id for $name ($r): $!";
> -	    return $r;
> -    }
> -    die "unable to open $f: $!" unless $! == POSIX::ENOENT;
> -    return undef;
> +sub get_headref ($) {
> +	my $name = shift;
> +	my $r = `git show-ref -s '$name'`;
> +	return undef unless $? == 0;
> +	chomp $r;
> +	return $r;
>  }

Where has $remote gone?

Ciao,
Dscho

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Jeff King]

On Wed, Nov 28, 2007 at 07:16:36PM +0000, Johannes Schindelin wrote:

> > -sub get_headref ($$) {
> > -    my $name    = shift;
> > -    my $git_dir = shift;
> > -
> > -    my $f = "$git_dir/$remote/$name";
> > -    if (open(my $fh, $f)) {
> > -	    chomp(my $r = <$fh>);
> > -	    is_sha1($r) or die "Cannot get head id for $name ($r): $!";
> > -	    return $r;
> > -    }
> > -    die "unable to open $f: $!" unless $! == POSIX::ENOENT;
> > -    return undef;
> > +sub get_headref ($) {
> > +	my $name = shift;
> > +	my $r = `git show-ref -s '$name'`;
> > +	return undef unless $? == 0;
> > +	chomp $r;
> > +	return $r;
> >  }
> 
> Where has $remote gone?

Gah, thank you. Obviously I deleted it without looking when I removed
the now-unnecessary $git_dir. However, let me put a curse on whoever is
responsible for randomly using "$remote" as a global in this function,
when all of the other parameters (including the
obvious-candidate-for-a-global $git_dir) are passed in.

Since get_headref is useful in contexts where "$remote" is not always
prepended (see patch 3/3), I think the best solution is:

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index 1b5f187..bbf9799 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -692,7 +692,8 @@ my (@old,@new,@skipped,%ignorebranch);
 $ignorebranch{'#CVSPS_NO_BRANCH'} = 1;
 
 sub commit {
-	if ($branch eq $opt_o && !$index{branch} && !get_headref($branch)) {
+	if ($branch eq $opt_o && !$index{branch} &&
+		!get_headref("$remote/$branch")) {
 	    # looks like an initial commit
 	    # use the index primed by git-init
 	    $ENV{GIT_INDEX_FILE} = "$git_dir/index";
@@ -716,7 +717,7 @@ sub commit {
 	update_index(@old, @new);
 	@old = @new = ();
 	my $tree = write_tree();
-	my $parent = get_headref($last_branch);
+	my $parent = get_headref("$remote/$last_branch");
 	print "Parent ID " . ($parent ? $parent : "(empty)") . "\n" if $opt_v;
 
 	my @commit_args;
@@ -727,7 +728,7 @@ sub commit {
 	foreach my $rx (@mergerx) {
 		next unless $logmsg =~ $rx && $1;
 		my $mparent = $1 eq 'HEAD' ? $opt_o : $1;
-		if (my $sha1 = get_headref($mparent)) {
+		if (my $sha1 = get_headref("$remote/$mparent")) {
 			push @commit_args, '-p', $mparent;
 			print "Merge parent branch: $mparent\n" if $opt_v;
 		}

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Johannes Schindelin]

Hi,

On Wed, 28 Nov 2007, Jeff King wrote:

> Since get_headref is useful in contexts where "$remote" is not always
> prepended (see patch 3/3), I think the best solution is:
>
> [PATCH prefixing the argument to get_headref() with "$remote/"]

Yes, I think so, too.

Thanks,
Dscho

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Since get_headref is useful in contexts where "$remote" is not always
> prepended (see patch 3/3), I think the best solution is:

...

> +	if ($branch eq $opt_o && !$index{branch} &&
> +		!get_headref("$remote/$branch")) {
> +	my $parent = get_headref("$remote/$last_branch");
> +		if (my $sha1 = get_headref("$remote/$mparent")) {

So the definition of get_headref() is to always take everything under
but not including "refs/"?  IOW, the tip of the master branch is asked
with get_headref("heads/master")?

I think show-ref can easily be confused by its tail matching behaviour,
and is a bad command to use in a context like this.  To be safe, I think
get_headref() should be:

	sub get_headref {
        	my ($it) = (@_);
                my $r = `git-rev-parse --verify "refs/$it"`;
		return undef unless $? == 0;
		chomp $r;
		return $r;
	}

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Jeff King]

On Wed, Nov 28, 2007 at 02:23:13PM -0800, Junio C Hamano wrote:

> > +	if ($branch eq $opt_o && !$index{branch} &&
> > +		!get_headref("$remote/$branch")) {
> > +	my $parent = get_headref("$remote/$last_branch");
> > +		if (my $sha1 = get_headref("$remote/$mparent")) {
> 
> So the definition of get_headref() is to always take everything under
> but not including "refs/"?  IOW, the tip of the master branch is asked
> with get_headref("heads/master")?

No, the $remote variable is "refs/remotes/origin" or "refs/heads". See
the old version of get_headref, which actually looked up
"$git_dir/$remote/$branch".

> I think show-ref can easily be confused by its tail matching behaviour,
> and is a bad command to use in a context like this.  To be safe, I think
> get_headref() should be:
> 
> 	sub get_headref {
>         	my ($it) = (@_);
>                 my $r = `git-rev-parse --verify "refs/$it"`;
> 		return undef unless $? == 0;
> 		chomp $r;
> 		return $r;
> 	}

I have no comment on which is the best command to use (you would know
much better than I), but adding "refs/" is wrong.

-Peff

Re: [PATCH 2/3] cvsimport: use show-ref to support packed refs

[Jeff King]

On Wed, Nov 28, 2007 at 05:43:13PM -0500, Jeff King wrote:

> > 	sub get_headref {
> >         	my ($it) = (@_);
> >                 my $r = `git-rev-parse --verify "refs/$it"`;
> > 		return undef unless $? == 0;
> > 		chomp $r;
> > 		return $r;
> > 	}
> 
> I have no comment on which is the best command to use (you would know
> much better than I), but adding "refs/" is wrong.

BTW, one bad thing about the new get_headref compared to the original is
that it does not distinguish very well between "ref does not exist" and
"an error occurred." Is there a lookup command that makes such a
distinction possible?

-Peff

[PATCH 3/3] cvsimport: miscellaneous packed-ref fixes

[Jeff King]

These were found with a grep for '$git_dir'; they all
replace a direct access of "$git_dir/refs/..." with a call
to git-show-ref or git-update-ref.

Signed-off-by: Jeff King <peff@peff.net>
---
 git-cvsimport.perl |   20 +++++++++-----------
 1 files changed, 9 insertions(+), 11 deletions(-)

diff --git a/git-cvsimport.perl b/git-cvsimport.perl
index b852f2f..1b5f187 100755
--- a/git-cvsimport.perl
+++ b/git-cvsimport.perl
@@ -864,29 +864,27 @@ while (<CVS>) {
 				print STDERR "Branch $branch erroneously stems from itself -- changed ancestor to $opt_o\n";
 				$ancestor = $opt_o;
 			}
-			if (-f "$git_dir/$remote/$branch") {
+			if (defined get_headref("$remote/$branch")) {
 				print STDERR "Branch $branch already exists!\n";
 				$state=11;
 				next;
 			}
-			unless (open(H,"$git_dir/$remote/$ancestor")) {
+			my $id = get_headref("$remote/$ancestor");
+			if (!$id) {
 				print STDERR "Branch $ancestor does not exist!\n";
 				$ignorebranch{$branch} = 1;
 				$state=11;
 				next;
 			}
-			chomp(my $id = <H>);
-			close(H);
-			unless (open(H,"> $git_dir/$remote/$branch")) {
-				print STDERR "Could not create branch $branch: $!\n";
+
+			system(qw(git update-ref -m cvsimport),
+				"$remote/$branch", $id);
+			if($? != 0) {
+				print STDERR "Could not create branch $branch\n";
 				$ignorebranch{$branch} = 1;
 				$state=11;
 				next;
 			}
-			print H "$id\n"
-				or die "Could not write branch $branch: $!";
-			close(H)
-				or die "Could not write branch $branch: $!";
 		}
 		$last_branch = $branch if $branch ne $last_branch;
 		$state = 9;
@@ -998,7 +996,7 @@ if ($orig_branch) {
 	$orig_branch = "master";
 	print "DONE; creating $orig_branch branch\n" if $opt_v;
 	system("git-update-ref", "refs/heads/master", "$remote/$opt_o")
-		unless -f "$git_dir/refs/heads/master";
+		defined get_headref('refs/heads/master');
 	system("git-symbolic-ref", "$remote/HEAD", "$remote/$opt_o")
 		if ($opt_r && $opt_o ne 'HEAD');
 	system('git-update-ref', 'HEAD', "$orig_branch");
-- 
1.5.3.6.2039.g0495

Re: [PATCH 3/3] cvsimport: miscellaneous packed-ref fixes

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> @@ -998,7 +996,7 @@ if ($orig_branch) {
>  	$orig_branch = "master";
>  	print "DONE; creating $orig_branch branch\n" if $opt_v;
>  	system("git-update-ref", "refs/heads/master", "$remote/$opt_o")
> -		unless -f "$git_dir/refs/heads/master";
> +		defined get_headref('refs/heads/master');

Where did the unless go ;-)?

Thanks, queued.

Re: [PATCH 3/3] cvsimport: miscellaneous packed-ref fixes

[Jeff King]

On Wed, Nov 28, 2007 at 04:52:28PM -0800, Junio C Hamano wrote:

> > @@ -998,7 +996,7 @@ if ($orig_branch) {
> >  	$orig_branch = "master";
> >  	print "DONE; creating $orig_branch branch\n" if $opt_v;
> >  	system("git-update-ref", "refs/heads/master", "$remote/$opt_o")
> > -		unless -f "$git_dir/refs/heads/master";
> > +		defined get_headref('refs/heads/master');
> 
> Where did the unless go ;-)?

Bah, I even ran this through the test suite, but obviously forgot a
'make' in the middle.

> Thanks, queued.

Thanks. Sorry for all the mistakes; I think I made more work in catching
my errors than went into the original patch series. ;)

-Peff

Re: git-cvsimport bug

[Emanuele Giaquinta]

On Wed, Nov 28, 2007 at 01:55:04PM -0500, Jeff King wrote:

>   2/3: cvsimport: use show-ref to support packed refs
> 
>        This fix is hopefully obvious, and the included test fails
>        without it (and this should probably fix Emanuele's problem).

It does, thanks!

Emanuele