[PATCH] git-svn: fix fetch with deleted tag

Currently git-svn assumes that two tags created from the same
revision will have the same repo url, so it uses a ref to the
tag without checking that its url matches the current url.

This causes issues when fetching an svn repo where a tag was
created, deleted, and then recreated under the following
circumstances:

- Both tags were copied from the same revision.
- Both tags had the same name.
- Both tags had different repository paths.
- [Optional] Both tags have a file with the same name but
  different content.

When all four conditions are met, a checksum mismatch error
occurs because the content of two files with the same path
differ (see t/t9155--git-svn-fetch-deleted-tag.sh):

    Checksum mismatch: ChangeLog 065854....
    expected: ce771b....
         got: 9563fd....

When only the first three conditions are met, no error occurs
but the tag in git matches the first (deleted) tag instead of
the last (most recent) tag (see
t/t9156-git-svn-fetch-deleted-tag-2.sh).

The fix is to verify that the repo url for the ref matches the
current url.  If the urls do not match, then a "tail" is grown
on the tag name by appending a dash and rechecking the new ref's
repo url until either a matching repo url is found or a new tag
is created.

Also fix a regular expression used to remove the revision from
the end of a tag or branch name.  The regex did not account for
any "tail" (dashes) that may have been added to the end of the
tag name (which first appeared in a00439ac).  If not fixed, tags
with names like "tags/mytag@5--@2" may be created.
---
Originally reported in: [BUG/TEST] git-svn: fetch fails with deleted tag
<http://marc.info/?t=128115948900001&r=1&w=2>
<http://thread.gmane.org/gmane.comp.version-control.git/152844>
<message://%3c1281159415-60900-1-git-send-email-ddkilzer@kilzer.net%3e>

 git-svn.perl                           |   16 +++++++++--
 t/t9155-git-svn-fetch-deleted-tag.sh   |   43 ++++++++++++++++++++++++++++++
 t/t9156-git-svn-fetch-deleted-tag-2.sh |   45 ++++++++++++++++++++++++++++++++
 3 files changed, 101 insertions(+), 3 deletions(-)
 create mode 100755 t/t9155-git-svn-fetch-deleted-tag.sh
 create mode 100755 t/t9156-git-svn-fetch-deleted-tag-2.sh

diff --git a/git-svn.perl b/git-svn.perl
index 8d2ef3d..c06d9d0 100755
--- a/git-svn.perl
+++ b/git-svn.perl

@@ -2957,18 +2957,28 @@ sub other_gs {
 	my $gs = Git::SVN->find_by_url($new_url, $url, $branch_from);
 	unless ($gs) {
 		my $ref_id = $old_ref_id;
-		$ref_id =~ s/\@\d+$//;
+		$ref_id =~ s/\@\d+-*$//;
 		$ref_id .= "\@$r";
 		# just grow a tail if we're not unique enough :x
 		$ref_id .= '-' while find_ref($ref_id);
-		print STDERR "Initializing parent: $ref_id\n" unless $::_q > 1;
 		my ($u, $p, $repo_id) = ($new_url, '', $ref_id);
 		if ($u =~ s#^\Q$url\E(/|$)##) {
 			$p = $u;
 			$u = $url;
 			$repo_id = $self->{repo_id};
 		}
-		$gs = Git::SVN->init($u, $p, $repo_id, $ref_id, 1);
+		while (1) {
+			# It is possible to tag two different subdirectories
+			# at the same revision.  If the url for an existing
+			# ref does not match, we must create a new ref.
+			$gs = Git::SVN->init($u, $p, $repo_id, $ref_id, 1);
+			my (undef, $max_commit) = $gs->rev_map_max(1);
+			last if (!$max_commit);
+			my ($url, undef, undef) = ::cmt_metadata($max_commit);
+			last if ($url eq $gs->full_url);
+			$ref_id .= '-';
+		}
+		print STDERR "Initializing parent: $ref_id\n" unless $::_q > 1;
 	}
 	$gs
 }

diff --git a/t/t9155-git-svn-fetch-deleted-tag.sh b/t/t9155-git-svn-fetch-deleted-tag.sh
new file mode 100755
index 0000000..4b50d7f
--- /dev/null
+++ b/t/t9155-git-svn-fetch-deleted-tag.sh

@@ -0,0 +1,43 @@
+#!/bin/sh
+
+test_description='git svn fetch deleted tag'
+
+. ./lib-git-svn.sh
+
+test_expect_success 'setup svn repo' '
+	mkdir -p import/trunk/subdir &&
+	mkdir -p import/branches &&
+	mkdir -p import/tags &&
+	echo "base" > import/trunk/subdir/file &&
+	svn_cmd import -m "import for git svn" import "$svnrepo" &&
+	rm -rf import &&
+
+	svn_cmd mkdir --parents -m "create mybranch directory" "$svnrepo/branches/mybranch" &&
+	svn_cmd cp -m "create branch mybranch" "$svnrepo/trunk" "$svnrepo/branches/mybranch/trunk" &&
+
+	svn_cmd co "$svnrepo/trunk" svn_project &&
+	cd svn_project &&
+
+	echo "trunk change" >> subdir/file &&
+	svn_cmd ci -m "trunk change" subdir/file &&
+
+	svn_cmd switch "$svnrepo/branches/mybranch/trunk" &&
+	echo "branch change" >> subdir/file &&
+	svn_cmd ci -m "branch change" subdir/file &&
+
+	cd .. &&
+	svn_cmd cp -m "create mytag attempt 1" -r5 "$svnrepo/trunk/subdir" "$svnrepo/tags/mytag" &&
+	svn_cmd rm -m "delete mytag attempt 1" "$svnrepo/tags/mytag" &&
+	svn_cmd cp -m "create mytag attempt 2" -r5 "$svnrepo/branches/mybranch/trunk/subdir" "$svnrepo/tags/mytag"
+'
+
+test_expect_success 'fetch deleted tags from same revision with checksum error' '
+	git svn init --stdlayout "$svnrepo" git_project &&
+	cd git_project &&
+	git svn fetch &&
+
+	git diff --exit-code mybranch:trunk/subdir/file tags/mytag:file &&
+	git diff --exit-code master:subdir/file tags/mytag^:file
+'
+
+test_done

diff --git a/t/t9156-git-svn-fetch-deleted-tag-2.sh b/t/t9156-git-svn-fetch-deleted-tag-2.sh
new file mode 100755
index 0000000..ad8589c
--- /dev/null
+++ b/t/t9156-git-svn-fetch-deleted-tag-2.sh

@@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git svn fetch deleted tag 2'
+
+. ./lib-git-svn.sh
+
+test_expect_success 'setup svn repo' '
+	mkdir -p import/branches &&
+	mkdir -p import/tags &&
+	mkdir -p import/trunk/subdir1 &&
+	mkdir -p import/trunk/subdir2 &&
+	mkdir -p import/trunk/subdir3 &&
+	echo "file1" > import/trunk/subdir1/file &&
+	echo "file2" > import/trunk/subdir2/file &&
+	echo "file3" > import/trunk/subdir3/file &&
+	svn_cmd import -m "import for git svn" import "$svnrepo" &&
+	rm -rf import &&
+
+	svn_cmd co "$svnrepo/trunk" svn_project &&
+	cd svn_project &&
+
+	echo "change1" >> subdir1/file &&
+	echo "change2" >> subdir2/file &&
+	echo "change3" >> subdir3/file &&
+	svn_cmd ci -m "change" . &&
+
+	cd .. &&
+	svn_cmd cp -m "create mytag 1" -r2 "$svnrepo/trunk/subdir1" "$svnrepo/tags/mytag" &&
+	svn_cmd rm -m "delete mytag 1" "$svnrepo/tags/mytag" &&
+	svn_cmd cp -m "create mytag 2" -r2 "$svnrepo/trunk/subdir2" "$svnrepo/tags/mytag" &&
+	svn_cmd rm -m "delete mytag 2" "$svnrepo/tags/mytag" &&
+	svn_cmd cp -m "create mytag 3" -r2 "$svnrepo/trunk/subdir3" "$svnrepo/tags/mytag"
+'
+
+test_expect_success 'fetch deleted tags from same revision with no checksum error' '
+	git svn init --stdlayout "$svnrepo" git_project &&
+	cd git_project &&
+	git svn fetch &&
+
+	git diff --exit-code master:subdir3/file tags/mytag:file &&
+	git diff --exit-code master:subdir2/file tags/mytag^:file &&
+	git diff --exit-code master:subdir1/file tags/mytag^^:file
+'
+
+test_done

-- 
1.7.2.1.49.g98551

On Sat, Aug 14, 2010 at 14:07, David D. Kilzer [off-list ref] wrote:

+                       my ($url, undef, undef) = ::cmt_metadata($max_commit);

This can just be:

    my ($url) = ::cmt_metadata($max_commit);

Perl will throw the extra arguments away for you.

Junio usually prefers the ">foo" style to "> foo".

If you use a subshell here it'll cd back for you.

Any reason not to include both of these in the same file? Just to
avoid having to manually reset the repository?

</nitpicks>

On Sat, August 14, 2010 at 7:52:48 AM, Ævar Arnfjörð Bjarmason wrote:

On Sat, Aug 14, 2010 at 14:07, David D. Kilzer [off-list ref] wrote:

quoted

 +++ b/t/t9156-git-svn-fetch-deleted-tag-2.sh
 @@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git  svn fetch deleted tag 2'

Any reason not to include both of these in the  same file? Just to
avoid having to manually reset the  repository?

It was easier to run the tests individually when working on them, and I was 
hesitant to combine the setup steps from each test since it wouldn't be as clear 
which steps were for which test in the future.  I realize this may be slower 
when running the tests, but it makes them easier to maintain, especially when 
one hasn't looked at the tests in a while.

Is there a nice way to reset the repository between steps?

Thanks for the feedback!  I've already applied your other suggestions.

Dave

On Sat, Aug 14, 2010 at 18:49, David D. Kilzer [off-list ref] wrote:

On Sat, August 14, 2010 at 7:52:48 AM, Ævar Arnfjörð Bjarmason wrote:

quoted

On Sat, Aug 14, 2010 at 14:07, David D. Kilzer [off-list ref] wrote:

quoted

 +++ b/t/t9156-git-svn-fetch-deleted-tag-2.sh
 @@ -0,0 +1,45 @@
+#!/bin/sh
+
+test_description='git  svn fetch deleted tag 2'

Any reason not to include both of these in the  same file? Just to
avoid having to manually reset the  repository?

It was easier to run the tests individually when working on them, and I was
hesitant to combine the setup steps from each test since it wouldn't be as clear
which steps were for which test in the future.

If you think it's easier to maintain like this then by all means keep
it as it is. I was just wondering why it was like this, that's all.

I realize this may be slower when running the tests, but it makes
them easier to maintain, especially when one hasn't looked at the
tests in a while.

The SLOOOOW part of running the git svn tests is definitely *not* the
tiny bit of shellscript required to execute each *.sh file :)

Is there a nice way to reset the repository between steps?

I don't know if this applies in this case but if you need fresh repos
for each tests you can usually do:

    test_expect_success 'test #1' '
        (test_create_repo one &&
        cd one &&
    	...)
    '

    test_expect_success 'test #2' '
        (test_create_repo two &&
        cd two &&
    	...)
    '

But I haven't looked at the svn_* functions you're using, so perhaps
that's not possible here.

Thanks for the feedback!  I've already applied your other suggestions.

Cool, good to know that it was helpful.

"David D. Kilzer" [off-list ref] wrote:

Thanks for the feedback!  I've already applied your other suggestions.

Thanks David and Ævar!

David: Everything looks alright to me with Ævar's suggestions, so I'll
ack whenever you have the final patch ready.


On a related note, if anybody has the time/patience to do some grunt
work: converting all existing and fragile "cd" usage to use subshells
would be much appreciated and help set better examples for introducing
new code.

-- 
Eric Wong