threads / patch / 24741

patchgit-svn: fix fetch with deleted tag

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

## tl;dr

5 messages between Aug 14, 2010 and Aug 15, 2010. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

David D. Kilzer· Aug 14, 2010, 14:07 UTC · lore

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
Show changes to 3 files +101 −3

git-svn.perl, t/t9155-git-svn-fetch-deleted-tag.sh, 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
Ævar Arnfjörð Bjarmason· Aug 14, 2010, 14:52 UTC · re: David D. Kilzer · lore

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

On Sat, Aug 14, 2010 at 14:07, David D. Kilzer <ddkilzer@kilzer.net> 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.
Show 5 quoted lines
> +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 &&
Junio usually prefers the ">foo" style to "> foo".
Show 10 quoted lines
> +       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 .. &&
If you use a subshell here it'll cd back for you.
Show 5 quoted lines
> +++ 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?

</nitpicks>
David D. Kilzer· Aug 14, 2010, 18:49 UTC · re: Ævar Arnfjörð Bjarmason · lore

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

On Sat, August 14, 2010 at 7:52:48 AM, Ævar Arnfjörð Bjarmason wrote:
Show 9 quoted lines
> On Sat, Aug 14, 2010 at 14:07, David D. Kilzer <ddkilzer@kilzer.net> wrote:
> >  +++ 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
Ævar Arnfjörð Bjarmason· Aug 14, 2010, 19:08 UTC · re: David D. Kilzer · lore

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

On Sat, Aug 14, 2010 at 18:49, David D. Kilzer <ddkilzer@kilzer.net> wrote:
Show 14 quoted lines
> 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 <ddkilzer@kilzer.net> wrote:
>> >  +++ 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.
Eric Wong· Aug 15, 2010, 06:55 UTC · re: David D. Kilzer · lore

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

"David D. Kilzer" <ddkilzer@kilzer.net> 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

← back to recent threads