Implement comments from patch review:

- style fixes
- add integration test for --rewrite-tag-names
- removed rewrite_dict as we can't really guarantee uniqness of tag names.

3 files changed, 47 insertions, 44 deletions

diff --git a/exporter.py b/exporter.py
index fd1de00..cad3241 100644
--- a/exporter.py
+++ b/exporter.py
@@ -111,7 +111,8 @@ def check_ref_format(refname):
         return False
     return True
 
-def sanitize_ref_name_for_git(name_dict, refname):
+
+def sanitize_ref_name_for_git(refname):
     """Rewrite refname so that it will be accepted by git-fast-import.
     For the detailed rules see check_ref_format.
 
@@ -119,11 +120,10 @@ def sanitize_ref_name_for_git(name_dict, refname):
     so we have to manually
     verify that resulting ref names are unique.
 
-    :param name_dict: additional dictionary used to enforce uniqueness of resulting refname's
     :param refname: refname to rewrite
     :return: new refname
     """
-    newRefname = re.sub(
+    new_refname = re.sub(
         # '/.' in refname or startswith '.'
         r"/\.|^\."
         # '..' in refname
@@ -141,12 +141,7 @@ def sanitize_ref_name_for_git(name_dict, refname):
         # "\\" in refname
         r"|\\",
         "_", refname)
-    idx = name_dict.get(newRefname, 1)
-    name_dict[newRefname] = idx + 1
-    if idx != 1:
-        # append index to the resulting refname if it's not unique
-        newRefname += "_" + str(idx)
-    return newRefname
+    return new_refname
 
 class BzrFastExporter(object):
 
@@ -174,7 +169,6 @@ class BzrFastExporter(object):
         self.excluded_revisions = set()
         self.plain_format = plain_format
         self.rewrite_tags = rewrite_tags
-        self.rewrite_dict = dict()
         self._multi_author_api_available = hasattr(bzrlib.revision.Revision,
             'get_apparent_authors')
         self.properties_to_exclude = ['authors', 'author']
@@ -601,11 +595,11 @@ class BzrFastExporter(object):
                 git_ref = 'refs/tags/%s' % tag.encode("utf-8")
                 if self.plain_format and not check_ref_format(git_ref):
                     if self.rewrite_tags:
-                        new_ref = sanitize_ref_name_for_git(self.rewrite_dict, git_ref)
+                        new_ref = sanitize_ref_name_for_git(git_ref)
                         self.warning('tag %r is exported as %r to be valid in git.',
                                      git_ref, new_ref)
                         git_ref = new_ref
-                    else:                       
+                    else:
                         self.warning('not creating tag %r as its name would not be '
                                      'valid in git.', git_ref)
                         continue
diff --git a/tests/test_commands.py b/tests/test_commands.py
index 282cfc3..5729660 100644
--- a/tests/test_commands.py
+++ b/tests/test_commands.py
@@ -80,6 +80,25 @@ class TestFastExport(ExternalBase):
         except AttributeError: # bzr < 2.4
             self.failUnlessExists("br.fi")
 
+    def test_tag_rewriting(self):
+        tree = self.make_branch_and_tree("br")
+        tree.commit("pointless")
+        self.assertTrue(tree.branch.supports_tags())
+        rev_id = tree.branch.dotted_revno_to_revision_id((1,))
+        tree.branch.tags.set_tag("goodTag", rev_id)
+        tree.branch.tags.set_tag("bad Tag", rev_id)
+        
+        # first check --no-rewrite-tag-names
+        data = self.run_bzr("fast-export --plain --no-rewrite-tag-names br")[0]
+        self.assertNotEqual(-1, data.find("reset refs/tags/goodTag"))
+        self.assertEqual(data.find("reset refs/tags/"), data.rfind("reset refs/tags/"))
+        
+        # and now with --rewrite-tag-names
+        data = self.run_bzr("fast-export --plain --rewrite-tag-names br")[0]
+        self.assertNotEqual(-1, data.find("reset refs/tags/goodTag"))
+        # "bad Tag" should be exported as bad_Tag
+        self.assertNotEqual(-1, data.find("reset refs/tags/bad_Tag"))
+
 
 simple_fast_import_stream = """commit refs/heads/master
 mark :1
diff --git a/tests/test_exporter.py b/tests/test_exporter.py
index 957945f..f1c9530 100644
--- a/tests/test_exporter.py
+++ b/tests/test_exporter.py
@@ -95,40 +95,30 @@ class CheckRefFormatTests(tests.TestCase):
         self.assertFalse(check_ref_format('heads/foo\020bar'))
         self.assertFalse(check_ref_format('heads/foo\177bar'))
 
+
 class CheckRefnameRewriting(tests.TestCase):
     """Tests for sanitize_ref_name_for_git function"""
 
     def test_passthrough_valid(self):
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'heads/foo'), 'heads/foo')
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'foo/bar/baz'), 'foo/bar/baz')
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'refs///heads/foo'), 'refs///heads/foo')
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'foo./bar'), 'foo./bar')
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'heads/foo@bar'), 'heads/foo@bar')
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'heads/fix.lock.error'), 'heads/fix.lock.error')
-
-    def test_rewrite_to_unique_names(self):
-        self.assertEqual(sanitize_ref_name_for_git(dict(), 'heads/foo/'), 'heads/foo_')
-        # check that with persistent dictionary we generate unique names on each invocation
-        q = dict()
-        self.assertNotEqual(
-            sanitize_ref_name_for_git(q, 'heads/foo/'),
-            sanitize_ref_name_for_git(q, 'heads/foo/'))
-        self.assertNotEqual(
-            sanitize_ref_name_for_git(q, 'heads/foo/'),
-            sanitize_ref_name_for_git(q, 'heads/foo/'))
-        
+        self.assertEqual(sanitize_ref_name_for_git('heads/foo'), 'heads/foo')
+        self.assertEqual(sanitize_ref_name_for_git('foo/bar/baz'), 'foo/bar/baz')
+        self.assertEqual(sanitize_ref_name_for_git('refs///heads/foo'), 'refs///heads/foo')
+        self.assertEqual(sanitize_ref_name_for_git('foo./bar'), 'foo./bar')
+        self.assertEqual(sanitize_ref_name_for_git('heads/foo@bar'), 'heads/foo@bar')
+        self.assertEqual(sanitize_ref_name_for_git('heads/fix.lock.error'), 'heads/fix.lock.error')
+
     def test_rewrite_invalid(self):
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'foo./bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo/')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo.')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), './foo')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), '.refs/foo')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo..bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo?bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo.lock')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/v@{ation')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo\bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo\\bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo\020bar')))
-        self.assertTrue(check_ref_format(sanitize_ref_name_for_git(dict(), 'heads/foo\177bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('foo./bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo/')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo.')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('./foo')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('.refs/foo')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo..bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo?bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo.lock')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/v@{ation')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo\bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo\\bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo\020bar')))
+        self.assertTrue(check_ref_format(sanitize_ref_name_for_git('heads/foo\177bar')))