Rev 3832: Before allowing commit to succeed, verify the texts will be 'safe'. in http://bzr.arbash-meinel.com/branches/bzr/1.10-dev/revision_strictness

[John Arbash Meinel]

At http://bzr.arbash-meinel.com/branches/bzr/1.10-dev/revision_strictness

------------------------------------------------------------
revno: 3832
revision-id: john at arbash-meinel.com-20081112073857-fm1qchpnttxt6vav
parent: pqm at pqm.ubuntu.com-20081112012514-6y8u99lf11pk0rdm
committer: John Arbash Meinel <john at arbash-meinel.com>
branch nick: revision_strictness
timestamp: Wed 2008-11-12 01:38:57 -0600
message:
  Before allowing commit to succeed, verify the texts will be 'safe'.
  
  This is done via CommitBuilder, so that repositories which could handle those
  texts can allow them. For now, all repos use XML serialization, and need to
  assert that we don't have characters that won't round-trip through XML.
-------------- next part --------------
=== modified file 'NEWS'
--- a/NEWS	2008-11-11 04:52:05 +0000
+++ b/NEWS	2008-11-12 07:38:57 +0000
@@ -34,6 +34,11 @@
     * Transport implementations must provide copy_tree_to_transport.  A default
       implementation is provided for Transport subclasses.
 
+    * ``CommitBuilder`` now validates the strings it will be committing,
+      to ensure that they do not have characters that will not be properly
+      round-tripped. For now, it just checks for characters that are
+      invalid in the XML form. (John Arbash Meinel)
+
   TESTING:
 
     * ``bzr selftest`` now fails if no doctests are found in a module

=== modified file 'bzrlib/repository.py'
--- a/bzrlib/repository.py	2008-11-03 04:12:11 +0000
+++ b/bzrlib/repository.py	2008-11-12 07:38:57 +0000
@@ -102,6 +102,7 @@
 
         self._revprops = {}
         if revprops is not None:
+            self._validate_revprops(revprops)
             self._revprops.update(revprops)
 
         if timestamp is None:
@@ -117,11 +118,24 @@
         self._generate_revision_if_needed()
         self.__heads = graph.HeadsCache(repository.get_graph()).heads
 
+    def _validate_unicode_text(self, text, context):
+        """Verify things like commit messages don't have bogus characters."""
+        if '\r' in text:
+            raise ValueError('Invalid value for %s: %r' % (context, text))
+
+    def _validate_revprops(self, revprops):
+        for key, value in revprops.iteritems():
+            # We know that the XML serializers do not round trip '\r'
+            # correctly, so refuse to accept them
+            self._validate_unicode_text(value,
+                                        'revision property (%s)' % (key,))
+
     def commit(self, message):
         """Make the actual commit.
 
         :return: The revision id of the recorded revision.
         """
+        self._validate_unicode_text(message, 'commit message')
         rev = _mod_revision.Revision(
                        timestamp=self._timestamp,
                        timezone=self._timezone,

=== modified file 'bzrlib/tests/per_repository/test_commit_builder.py'
--- a/bzrlib/tests/per_repository/test_commit_builder.py	2008-09-22 05:15:20 +0000
+++ b/bzrlib/tests/per_repository/test_commit_builder.py	2008-11-12 07:38:57 +0000
@@ -585,3 +585,21 @@
 
     def test_last_modified_file_link(self):
         self._check_kind_change(self.make_file, self.make_link)
+
+    def test_get_commit_builder_with_invalid_revprops(self):
+        branch = self.make_branch('.')
+        branch.repository.lock_write()
+        self.addCleanup(branch.repository.unlock)
+        self.assertRaises(ValueError, branch.repository.get_commit_builder,
+            branch, [], branch.get_config(),
+            revprops={'invalid': u'property\rwith\r\ninvalid chars'})
+
+    def test_commit_builder_commit_with_invalid_message(self):
+        branch = self.make_branch('.')
+        branch.repository.lock_write()
+        self.addCleanup(branch.repository.unlock)
+        builder = branch.repository.get_commit_builder(branch, [],
+            branch.get_config())
+        self.addCleanup(branch.repository.abort_write_group)
+        self.assertRaises(ValueError, builder.commit,
+            u'Invalid\r\ncommit message\r\n')