git-cvsimport bug

10 messages, 3 authors, 2016-06-15 · open the first message on its own page

git-cvsimport bug

From: Emanuele Giaquinta <hidden>
Date: 2016-06-15 22:43:54

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

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

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

[PATCH 1/3] Add basic cvsimport tests

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

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 <redacted>
---
 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: git-cvsimport bug

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

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 2/3] cvsimport: use show-ref to support packed refs

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

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

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

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

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 <redacted>
---
 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 2/3] cvsimport: use show-ref to support packed refs

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:54

Hi,

On Wed, 28 Nov 2007, Jeff King wrote:
quoted hunk
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

From: Jeff King <hidden>
Date: 2016-06-15 22:43:54

On Wed, Nov 28, 2007 at 07:16:36PM +0000, Johannes Schindelin wrote:
quoted
-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

From: Johannes Schindelin <hidden>
Date: 2016-06-15 22:43:54

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: git-cvsimport bug

From: Emanuele Giaquinta <hidden>
Date: 2016-06-15 22:43:55

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
Keyboard shortcuts
hback out one level
jnext message in thread
kprevious message in thread
ldrill in
Escclose help / fold thread tree
?toggle this help