Merge lp:~jameinel/bzr/2.0.3-433779-sanitize-commit-m into lp:bzr

[John A Meinel]

[John A Meinel (jameinel)]

This is a basic bugfix for bug #433779. The change is that if you supply a commit message (via -m ...) it sanitizes '\r\n' => '\n' and '\r' => '\n'.

The reason we don't allow '\r' is because our XML parsers silently translates it anyway, so we want to make sure that users are getting what they entered.

*Personally* I would have preferred Emac's bzr integration to pass '\n' appropriately rather than '\r\n', but this is a reasonable compromise.

Note that this code translates '\r\n' always, rather than only on Windows, etc... It also doesn't try to translate the message from '--file', because we open that file in 'text' mode anyway.

The test is pretty straightforward. Without the patch, the test fails with "ValueError" because we don't allow '\r'.

I did the patch on top on top of 2.0 in case we want to have it there. It was 'just' at the threshold of what I would be comfortable with, which is why I'm proposing it for bzr.dev/2.1 first.

[Vincent Ladeuil (vila)]

Thanks for that.

I fail to see what can possibly go wrong, why is why I agree we should land it in 2.1 first ;)

For the sake of example, you could have used the shell-like tests here, up to the run_bzr part.

review: Approve

[John A Meinel (jameinel)]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Vincent Ladeuil wrote:
> Review: Approve
> Thanks for that.
>
> I fail to see what can possibly go wrong, why is why I agree we should land it in 2.1 first ;)
>
> For the sake of example, you could have used the shell-like tests here, up to the run_bzr part.
>
>

Except I feel that setting up the test should be done in bzrlib code and
not in 'shell' code. If I wanted to do that, I would have used 'run_bzr'
everywhere.

But thanks for the review.
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkrwkAwACgkQJdeBCYSNAAN6lwCfS5vCl72+7xNIFd9hI7KxGy6t
VmQAn3HTairobpUxkkvKBgKZyW46zCvm
=/SJL
-----END PGP SIGNATURE-----

=== modified file 'NEWS'
--- NEWS	2009-11-03 03:58:26 +0000
+++ NEWS	2009-11-03 18:41:12 +0000
@@ -5,6 +5,7 @@
 .. contents:: List of Releases
    :depth: 1
 
+<<<<<<< TREE
 
 bzr 2.1.0b3 (not released yet)
 ##############################
@@ -192,6 +193,43 @@
   peak memory for initial copies (~200MB). (John Arbash Meinel)
 
 
+=======
+bzr 2.0.3 (not released yet)
+############################
+
+:Codename: template
+:2.0.3: ???
+
+Compatibility Breaks
+********************
+
+New Features
+************
+
+Bug Fixes
+*********
+* Sanitize commit messages that come in from the '-m' flag. We translate
+  '\r\n' => '\n' and a plain '\r' => '\n'. The storage layer doesn't
+  allow those because XML store silently translate it anyway. (The parser
+  auto-translates \r\n => \n in ways that are hard for us to catch.)
+
+Improvements
+************
+
+Documentation
+*************
+
+API Changes
+***********
+
+Internals
+*********
+
+Testing
+*******
+
+
+>>>>>>> MERGE-SOURCE
 bzr 2.0.2
 #########
 

=== modified file 'bzr'
--- bzr	2009-10-30 16:13:05 +0000
+++ bzr	2009-11-03 18:41:12 +0000
@@ -23,7 +23,11 @@
 import warnings
 
 # update this on each release
+<<<<<<< TREE
 _script_version = (2, 1, 0)
+=======
+_script_version = (2, 0, 3)
+>>>>>>> MERGE-SOURCE
 
 if __doc__ is None:
     print "bzr does not support python -OO."

=== modified file 'bzrlib/__init__.py'
--- bzrlib/__init__.py	2009-10-30 16:13:05 +0000
+++ bzrlib/__init__.py	2009-11-03 18:41:13 +0000
@@ -44,7 +44,11 @@
 # Python version 2.0 is (2, 0, 0, 'final', 0)."  Additionally we use a
 # releaselevel of 'dev' for unreleased under-development code.
 
+<<<<<<< TREE
 version_info = (2, 1, 0, 'dev', 3)
+=======
+version_info = (2, 0, 3, 'dev', 0)
+>>>>>>> MERGE-SOURCE
 
 # API compatibility version: bzrlib is currently API compatible with 1.15.
 api_minimum_version = (2, 1, 0)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2009-10-29 21:13:16 +0000
+++ bzrlib/builtins.py	2009-11-03 18:41:12 +0000
@@ -3027,6 +3027,9 @@
         def get_message(commit_obj):
             """Callback to get commit message"""
             my_message = message
+            if my_message is not None and '\r' in my_message:
+                my_message = my_message.replace('\r\n', '\n')
+                my_message = my_message.replace('\r', '\n')
             if my_message is None and not file:
                 t = make_commit_message_template_encoded(tree,
                         selected_list, diff=show_diff,

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2009-08-28 05:00:33 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2009-11-03 18:41:12 +0000
@@ -170,6 +170,24 @@
         self.assertEqual(err, 'Committing to: %s\n'
                          'Committed revision 2.\n' % expected)
 
+    def test_commit_sanitizes_CR_in_message(self):
+        # See bug #433779, basically Emacs likes to pass '\r\n' style line
+        # endings to 'bzr commit -m ""' which breaks because we don't allow
+        # '\r' in commit messages. (Mostly because of issues where XML style
+        # formats arbitrarily strip it out of the data while parsing.)
+        # To make life easier for users, we just always translate '\r\n' =>
+        # '\n'. And '\r' => '\n'.
+        a_tree = self.make_branch_and_tree('a')
+        self.build_tree(['a/b'])
+        a_tree.add('b')
+        self.run_bzr(['commit',
+                      '-m', 'a string\r\n\r\nwith mixed\r\rendings\n'],
+                     working_dir='a')
+        rev_id = a_tree.branch.last_revision()
+        rev = a_tree.branch.repository.get_revision(rev_id)
+        self.assertEqualDiff('a string\n\nwith mixed\n\nendings\n',
+                             rev.message)
+
     def test_commit_merge_reports_all_modified_files(self):
         # the commit command should show all the files that are shown by
         # bzr diff or bzr status when committing, even when they were not

=== modified file 'setup.py'