Merge lp:~a-s-usov/bzr-fastimport/renaming-tags into lp:bzr-fastimport

[Alex Usov]

As we discussed in the ticket -- implements new command-line flag for fast-export:
--rewrite-tag-names

The flag is is switched off by default.

If enabled tags which have incorrect name will be rewritten by replacing all invalid symbols (or substrings like .lock suffix) with underscore.
Some extra care is taken to ensure rewritten names are unique.

[Jelmer Vernooij (jelmer)]

Thanks for the patch. This mostly looks ok; some minor issues:

* two empty lines are necessary before "+class CheckRefnameRewriting(tests.TestCase):" and "+def sanitize_ref_name_for_git(name_dict, refname):", consistent with PEP8.
* The --rewrite-git-tags option needs a test
* Please use underscores rather than camelcasing
* rewrite_dict could be a local variable rather than a class variable

I'm not sure if it's all that useful to try to prevent duplicate tag names, as this is pretty much impossible. Even with your changes there are risks of duplicate tags (e.g. with incremental imports). Likewise, your change may cause tag names to change if another tag is removed.

[Jelmer Vernooij (jelmer)]

review: Needs Fixing

[lp:~a-s-usov/bzr-fastimport/renaming-tags]

337. By Jelmer Vernooij on 2011-10-17

Merge support for --rewrite-tag-names.

Unmerged revisions

=== modified file 'cmds.py'
--- cmds.py	2011-10-06 00:11:52 +0000
+++ cmds.py	2011-10-17 10:30:29 +0000
@@ -578,6 +578,10 @@
      future once the feature names and definitions are formally agreed
      to by the broader fast-import developer community.
 
+     Git has much stricter naming rules for tags and fast-export --plain
+     will skip tags which can't be imported into git. If you want to rename
+     these tags use --rewrite-tag-names.
+     
     :Examples:
 
      To produce data destined for import into Bazaar::
@@ -624,12 +628,15 @@
                     Option('plain',
                         help="Exclude metadata to maximise interoperability."
                         ),
+                    Option('rewrite-tag-names',
+                        help="Rewrite invalid tag names in plain mode."
+                        ),
                      ]
     encoding_type = 'exact'
     def run(self, source, destination=None, verbose=False,
         git_branch="master", checkpoint=10000, marks=None,
         import_marks=None, export_marks=None, revision=None,
-        plain=True):
+        plain=True, rewrite_tag_names=False):
         load_fastimport()
         from bzrlib.plugins.fastimport import exporter
 
@@ -639,7 +646,8 @@
             destination=destination,
             git_branch=git_branch, checkpoint=checkpoint,
             import_marks_file=import_marks, export_marks_file=export_marks,
-            revision=revision, verbose=verbose, plain_format=plain)
+            revision=revision, verbose=verbose, plain_format=plain,
+            rewrite_tags=rewrite_tag_names)
         return exporter.run()
 
 

=== modified file 'exporter.py'
--- exporter.py	2011-10-06 00:11:52 +0000
+++ exporter.py	2011-10-17 10:30:29 +0000
@@ -46,7 +46,7 @@
 # set new_git_branch to the previously used name)
 
 from email.Utils import parseaddr
-import sys, time
+import sys, time, re
 
 import bzrlib.branch
 import bzrlib.revision
@@ -112,18 +112,52 @@
     return True
 
 
+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.
+
+    By rewriting the refname we are breaking uniqueness guarantees provided by bzr
+    so we have to manually
+    verify that resulting ref names are unique.
+
+    :param refname: refname to rewrite
+    :return: new refname
+    """
+    new_refname = re.sub(
+        # '/.' in refname or startswith '.'
+        r"/\.|^\."
+        # '..' in refname
+        r"|\.\."
+        # ord(c) < 040
+        r"|[" + "".join([chr(x) for x in range(040)]) + r"]"
+        # c in '\177 ~^:?*['
+        r"|[\177 ~^:?*[]"
+        # last char in "/."
+        r"|[/.]$"
+        # endswith '.lock'
+        r"|.lock$"
+        # "@{" in refname
+        r"|@{"
+        # "\\" in refname
+        r"|\\",
+        "_", refname)
+    return new_refname
 
 class BzrFastExporter(object):
 
     def __init__(self, source, destination, git_branch=None, checkpoint=-1,
         import_marks_file=None, export_marks_file=None, revision=None,
-        verbose=False, plain_format=False):
+        verbose=False, plain_format=False, rewrite_tags=False):
         """Export branch data in fast import format.
 
         :param plain_format: if True, 'classic' fast-import format is
           used without any extended features; if False, the generated
           data is richer and includes information like multiple
           authors, revision properties, etc.
+
+        :param rewrite_tags: if True tag names will be rewritten to be
+          git-compatible. Otherwise tags which aren't valid for git will
+          be skiped.          
         """
         self.source = source
         self.outf = _get_output_stream(destination)
@@ -134,6 +168,7 @@
         self.revision = revision
         self.excluded_revisions = set()
         self.plain_format = plain_format
+        self.rewrite_tags = rewrite_tags
         self._multi_author_api_available = hasattr(bzrlib.revision.Revision,
             'get_apparent_authors')
         self.properties_to_exclude = ['authors', 'author']
@@ -559,9 +594,15 @@
             else:
                 git_ref = 'refs/tags/%s' % tag.encode("utf-8")
                 if self.plain_format and not check_ref_format(git_ref):
-                    self.warning('not creating tag %r as its name would not be '
-                                 'valid in git.', git_ref)
-                    continue
+                    if self.rewrite_tags:
+                        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:
+                        self.warning('not creating tag %r as its name would not be '
+                                     'valid in git.', git_ref)
+                        continue
                 self.print_cmd(commands.ResetCommand(git_ref, ":" + str(mark)))
 
     def _next_tmp_branch_name(self):

=== modified file 'tests/test_commands.py'
--- tests/test_commands.py	2011-10-06 00:11:52 +0000
+++ tests/test_commands.py	2011-10-17 10:30:29 +0000
@@ -80,6 +80,25 @@
         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

=== modified file 'tests/test_exporter.py'
--- tests/test_exporter.py	2011-10-06 00:11:52 +0000
+++ tests/test_exporter.py	2011-10-17 10:30:29 +0000
@@ -24,6 +24,7 @@
 from bzrlib.plugins.fastimport.exporter import (
     _get_output_stream,
     check_ref_format,
+    sanitize_ref_name_for_git
     )
 
 from bzrlib.plugins.fastimport.tests import (
@@ -79,11 +80,45 @@
 
     def test_invalid(self):
         self.assertFalse(check_ref_format('foo'))
+        self.assertFalse(check_ref_format('foo/.bar'))
         self.assertFalse(check_ref_format('heads/foo/'))
+        self.assertFalse(check_ref_format('heads/foo.'))
         self.assertFalse(check_ref_format('./foo'))
         self.assertFalse(check_ref_format('.refs/foo'))
         self.assertFalse(check_ref_format('heads/foo..bar'))
         self.assertFalse(check_ref_format('heads/foo?bar'))
         self.assertFalse(check_ref_format('heads/foo.lock'))
         self.assertFalse(check_ref_format('heads/v@{ation'))
+        self.assertFalse(check_ref_format('heads/foo\\bar'))
         self.assertFalse(check_ref_format('heads/foo\bar'))
+        self.assertFalse(check_ref_format('heads/foo bar'))
+        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('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('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')))