Merge lp:~vila/bzr-gtk/1132719-dont-expand-commit-message into lp:bzr-gtk

[Vincent Ladeuil]

This fixes bug #1132719 by not trying to expand config options in commit
messages (global or per-file).

The fix is trivial, the tests are not prettier after the fix but provided
enough support and coverage to fix the bug TDD style ;)

Since the expansion was attempted on the values retrieved in branch.conf,
this should work even for users with existing commit messages.

[Jelmer Vernooij (jelmer)]

Looks good, thanks!

review: Approve

[Richard Wilbur (richard-wilbur)]

Good fix for an annoying bug! I'm glad it could be so trivial.

review: Approve

[John A Meinel (jameinel)]

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

On 2013-02-26 14:42, Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging
> lp:~vila/bzr-gtk/1132719-dont-expand-commit-message into
> lp:bzr-gtk.
>
> Requested reviews: Bazaar GTK maintainers (bzr-gtk) Related bugs:
> Bug #1132719 in Bazaar GTK+ Frontends: "bzr-gtk should not try to
> expand config options where none are expected"
> https://bugs.launchpad.net/bzr-gtk/+bug/1132719
>
> For more details, see:
> https://code.launchpad.net/~vila/bzr-gtk/1132719-dont-expand-commit-message/+merge/150521
>
> This fixes bug #1132719 by not trying to expand config options in
> commit messages (global or per-file).
>
> The fix is trivial, the tests are not prettier after the fix but
> provided enough support and coverage to fix the bug TDD style ;)
>
> Since the expansion was attempted on the values retrieved in
> branch.conf, this should work even for users with existing commit
> messages.
>

 status: approve

John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.13 (Cygwin)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iEYEARECAAYFAlEvQd4ACgkQJdeBCYSNAAO8rwCeL9T7AoBKxcAWLpJcyRkjn99H
V3IAnRgUMqAST/54JzKvBv7nCOoZqGIC
=Cmuk
-----END PGP SIGNATURE-----

review: Approve

=== modified file 'NEWS'
--- NEWS	2012-07-09 15:23:26 +0000
+++ NEWS	2013-02-26 10:41:21 +0000
@@ -13,6 +13,10 @@
 
   * Fix searching in tree views. (Jelmer Vernooij, #928301)
 
+  * Don't try to expand options in commit messages. They've been saved from
+    previous commits and won't ever contain option references.
+    (Vincent Ladeuil, #1132719)
+
  CHANGES
 
   * bzr-notify has been removed, as it was causing regressions

=== modified file 'commitmsgs.py'
--- commitmsgs.py	2011-12-20 16:47:38 +0000
+++ commitmsgs.py	2013-02-26 10:41:21 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2006 by Szilveszter Farkas (Phanatic) <[e-mail]>
+# Copyright (C) 2011, 2013 by Szilveszter Farkas (Phanatic) <[e-mail]>
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -35,10 +35,11 @@
         else:
             config = branch.get_config()
             self.global_message = config.get_user_option(
-                'gtk_global_commit_message')
+                'gtk_global_commit_message', expand=False)
             if self.global_message is None:
                 self.global_message = u''
-            file_messages = config.get_user_option('gtk_file_commit_messages')
+            file_messages = config.get_user_option(
+                'gtk_file_commit_messages' , expand=False)
             if file_messages: # unicode and B-encoded:
                 self.file_messages = bencode.bdecode(
                     file_messages.encode('UTF-8'))

=== modified file 'tests/test_commit.py'
--- tests/test_commit.py	2012-03-22 17:14:22 +0000
+++ tests/test_commit.py	2013-02-26 10:41:21 +0000
@@ -1,4 +1,4 @@
-# Copyright (C) 2007, 2008 John Arbash Meinel <[e-mail]>
+# Copyright (C) 2007, 2008, 2009, 2011, 2012, 2013 John Arbash Meinel <[e-mail]>
 #
 # This program is free software; you can redistribute it and/or modify
 # it under the terms of the GNU General Public License as published by
@@ -1185,8 +1185,10 @@
             'post_uncommit', commitmsgs.save_commit_messages, None)
 
     def _get_file_info_dict(self, rank):
-        file_info = [dict(path='a', file_id='a-id', message='a msg %d' % rank),
-                     dict(path='b', file_id='b-id', message='b msg %d' % rank)]
+        file_info = [dict(path='a', file_id='a-id',
+                          message='a {msg} %d' % rank),
+                     dict(path='b', file_id='b-id',
+                          message='b msg %d' % rank)]
         return file_info
 
     def _get_file_info_revprops(self, rank):
@@ -1194,10 +1196,12 @@
         return {'file-info': bencode.bencode(file_info_prop).decode('UTF-8')}
 
     def _get_commit_message(self):
-        return self.config.get_user_option('gtk_global_commit_message')
+        return self.config.get_user_option(
+            'gtk_global_commit_message', expand=False)
 
     def _get_file_commit_messages(self):
-        return self.config.get_user_option('gtk_file_commit_messages')
+        return self.config.get_user_option(
+            'gtk_file_commit_messages', expand=False)
 
 
 class TestUncommitHook(TestSavedCommitMessages):
@@ -1219,29 +1223,31 @@
     def test_uncommit_one_by_one(self):
         uncommit.uncommit(self.tree.branch, tree=self.tree)
         self.assertEquals(u'three', self._get_commit_message())
-        self.assertEquals(u'd4:a-id7:a msg 34:b-id7:b msg 3e',
+        self.assertEquals(u'd4:a-id9:a {msg} 34:b-id7:b msg 3e',
                           self._get_file_commit_messages())
 
         uncommit.uncommit(self.tree.branch, tree=self.tree)
         self.assertEquals(u'two\n******\nthree', self._get_commit_message())
-        self.assertEquals(u'd4:a-id22:a msg 2\n******\na msg 3'
+        self.assertEquals(u'd4:a-id26:a {msg} 2\n******\na {msg} 3'
                           '4:b-id22:b msg 2\n******\nb msg 3e',
                           self._get_file_commit_messages())
 
         uncommit.uncommit(self.tree.branch, tree=self.tree)
         self.assertEquals(u'one\n******\ntwo\n******\nthree',
                           self._get_commit_message())
-        self.assertEquals(u'd4:a-id37:a msg 1\n******\na msg 2\n******\na msg 3'
-                          '4:b-id37:b msg 1\n******\nb msg 2\n******\nb msg 3e',
-                          self._get_file_commit_messages())
+        self.assertEquals(
+            u'd4:a-id43:a {msg} 1\n******\na {msg} 2\n******\na {msg} 3'
+            '4:b-id37:b msg 1\n******\nb msg 2\n******\nb msg 3e',
+            self._get_file_commit_messages())
 
     def test_uncommit_all_at_once(self):
         uncommit.uncommit(self.tree.branch, tree=self.tree, revno=1)
         self.assertEquals(u'one\n******\ntwo\n******\nthree',
                           self._get_commit_message())
-        self.assertEquals(u'd4:a-id37:a msg 1\n******\na msg 2\n******\na msg 3'
-                          '4:b-id37:b msg 1\n******\nb msg 2\n******\nb msg 3e',
-                          self._get_file_commit_messages())
+        self.assertEquals(
+            u'd4:a-id43:a {msg} 1\n******\na {msg} 2\n******\na {msg} 3'
+            '4:b-id37:b msg 1\n******\nb msg 2\n******\nb msg 3e',
+            self._get_file_commit_messages())
 
 
 class TestReusingSavedCommitMessages(TestSavedCommitMessages, QuestionHelpers):
@@ -1255,7 +1261,8 @@
         self.tree.add(['a'], ['a-id'])
         self.tree.add(['b'], ['b-id'])
         rev1 = self.tree.commit('one', revprops=self._get_file_info_revprops(1))
-        rev2 = self.tree.commit('two', revprops=self._get_file_info_revprops(2))
+        rev2 = self.tree.commit('two{x}',
+                                revprops=self._get_file_info_revprops(2))
         uncommit.uncommit(self.tree.branch, tree=self.tree)
         self.build_tree_contents([('tree/a', 'new a content\n'),
                                   ('tree/b', 'new b content'),])
@@ -1270,16 +1277,16 @@
 
     def test_setup_saved_messages(self):
         # Check the initial setup
-        self.assertEquals(u'two', self._get_commit_message())
-        self.assertEquals(u'd4:a-id7:a msg 24:b-id7:b msg 2e',
+        self.assertEquals(u'two{x}', self._get_commit_message())
+        self.assertEquals(u'd4:a-id9:a {msg} 24:b-id7:b msg 2e',
                           self._get_file_commit_messages())
 
     def test_messages_are_reloaded(self):
         dlg = self._get_commit_dialog(self.tree)
-        self.assertEquals(u'two', dlg._get_global_commit_message())
+        self.assertEquals(u'two{x}', dlg._get_global_commit_message())
         self.assertEquals(([u'a', u'b'],
                            [{ 'path': 'a',
-                             'file_id': 'a-id', 'message': 'a msg 2',},
+                             'file_id': 'a-id', 'message': 'a {msg} 2',},
                            {'path': 'b',
                             'file_id': 'b-id', 'message': 'b msg 2',}],),
                           dlg._get_specific_files())
@@ -1294,8 +1301,8 @@
         dlg = self._get_commit_dialog(self.tree)
         self._set_question_yes(dlg) # Save messages
         dlg._do_cancel()
-        self.assertEquals(u'two', self._get_commit_message())
-        self.assertEquals(u'd4:a-id7:a msg 24:b-id7:b msg 2e',
+        self.assertEquals(u'two{x}', self._get_commit_message())
+        self.assertEquals(u'd4:a-id9:a {msg} 24:b-id7:b msg 2e',
                           self._get_file_commit_messages())
 
     def test_messages_are_cleared_on_cancel_if_required(self):