Merge lp:~lifeless/bzr/commit into lp:bzr

[Robert Collins]

(robertc) bzr commit will prompt before using a commit message that was generated by a template and not edited by the user. (Robert Collins)

This branch slightly cleans up the cmd layer of commit, which I did while figuring out the best way to make the change. The functional change is to add a prompt before accepting an unaltered template file.

[John A Meinel (jameinel)]

You don't seem to have a test case for the 'failing' side. Where the user is prompted but they request to *not* commit.

That code path seems to:
  return ""

Whereas the other code paths when the template file doesn't exist, or _run_editor fails:
  return None

Otherwise it seems ok.

review: Needs Fixing

[lp:~lifeless/bzr/commit]

5138. By Canonical.com Patch Queue Manager <email address hidden> on 2010-04-07

(robertc) bzr commit will prompt before using a commit message that was
 generated by a template and not edited by the user. (Robert Collins)

=== modified file 'NEWS'
--- NEWS	2010-04-06 11:29:06 +0000
+++ NEWS	2010-04-07 22:25:45 +0000
@@ -29,6 +29,10 @@
 Improvements
 ************
 
+* ``bzr commit`` will prompt before using a commit message that was
+  generated by a template and not edited by the user.
+  (Robert Collins, #530265)
+
 * Less code is loaded at startup.  (Cold-cache start time is about 10-20%
   less.)
   (Martin Pool, #553017)

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-04-06 21:46:47 +0000
+++ bzrlib/builtins.py	2010-04-07 22:25:45 +0000
@@ -3150,31 +3150,37 @@
                     '(use --file "%(f)s" to take commit message from that file)'
                     % { 'f': message })
                 ui.ui_factory.show_warning(warning_msg)
+            if '\r' in message:
+                message = message.replace('\r\n', '\n')
+                message = message.replace('\r', '\n')
+            if file:
+                raise errors.BzrCommandError(
+                    "please specify either --message or --file")
 
         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 is the status of the tree
-                t = make_commit_message_template_encoded(tree,
+            if file:
+                my_message = codecs.open(
+                    file, 'rt', osutils.get_user_encoding()).read()
+            elif message is not None:
+                my_message = message
+            else:
+                # No message supplied: make one up.
+                # text is the status of the tree
+                text = make_commit_message_template_encoded(tree,
                         selected_list, diff=show_diff,
                         output_encoding=osutils.get_user_encoding())
                 # start_message is the template generated from hooks
+                # XXX: Warning - looks like hooks return unicode,
+                # make_commit_message_template_encoded returns user encoding.
+                # We probably want to be using edit_commit_message instead to
+                # avoid this.
                 start_message = generate_commit_message_template(commit_obj)
-                my_message = edit_commit_message_encoded(t,
+                my_message = edit_commit_message_encoded(text,
                     start_message=start_message)
                 if my_message is None:
                     raise errors.BzrCommandError("please specify a commit"
                         " message with either --message or --file")
-            elif my_message and file:
-                raise errors.BzrCommandError(
-                    "please specify either --message or --file")
-            if file:
-                my_message = codecs.open(file, 'rt',
-                                         osutils.get_user_encoding()).read()
             if my_message == "":
                 raise errors.BzrCommandError("empty commit message specified")
             return my_message
@@ -3192,8 +3198,6 @@
                         timezone=offset,
                         exclude=safe_relpath_files(tree, exclude))
         except PointlessCommit:
-            # FIXME: This should really happen before the file is read in;
-            # perhaps prepare the commit; get the message; then actually commit
             raise errors.BzrCommandError("No changes to commit."
                               " Use --unchanged to commit anyhow.")
         except ConflictsInTree:

=== modified file 'bzrlib/msgeditor.py'
--- bzrlib/msgeditor.py	2010-02-17 17:11:16 +0000
+++ bzrlib/msgeditor.py	2010-04-07 22:25:45 +0000
@@ -27,6 +27,8 @@
     config,
     osutils,
     trace,
+    transport,
+    ui,
     )
 from bzrlib.errors import BzrError, BadCommitMessageEncoding
 from bzrlib.hooks import HookPoint, Hooks
@@ -139,10 +141,21 @@
     try:
         msgfilename, hasinfo = _create_temp_file_with_commit_template(
                                     infotext, ignoreline, start_message)
-
-        if not msgfilename or not _run_editor(msgfilename):
-            return None
-
+        if not msgfilename:
+            return None
+        basename = osutils.basename(msgfilename)
+        msg_transport = transport.get_transport(osutils.dirname(msgfilename))
+        reference_content = msg_transport.get_bytes(basename)
+        if not _run_editor(msgfilename):
+            return None
+        edited_content = msg_transport.get_bytes(basename)
+        if edited_content == reference_content:
+            if not ui.ui_factory.get_boolean(
+                "Commit message was not edited, use anyway"):
+                # Returning "" makes cmd_commit raise 'empty commit message
+                # specified' which is a reasonable error, given the user has
+                # rejected using the unedited template.
+                return ""
         started = False
         msg = []
         lastline, nlines = 0, 0

=== modified file 'bzrlib/tests/blackbox/test_commit.py'
--- bzrlib/tests/blackbox/test_commit.py	2010-02-23 07:43:11 +0000
+++ bzrlib/tests/blackbox/test_commit.py	2010-04-07 22:25:45 +0000
@@ -675,7 +675,7 @@
         self.assertContainsRe(err,
             r'^bzr: ERROR: Cannot lock.*readonly transport')
 
-    def test_commit_hook_template(self):
+    def setup_editor(self):
         # Test that commit template hooks work
         if sys.platform == "win32":
             f = file('fed.bat', 'w')
@@ -688,11 +688,25 @@
             f.close()
             os.chmod('fed.sh', 0755)
             osutils.set_or_unset_env('BZR_EDITOR', "./fed.sh")
+
+    def setup_commit_with_template(self):
+        self.setup_editor()
         msgeditor.hooks.install_named_hook("commit_message_template",
                 lambda commit_obj, msg: "save me some typing\n", None)
         tree = self.make_branch_and_tree('tree')
         self.build_tree(['tree/hello.txt'])
         tree.add('hello.txt')
-        out, err = self.run_bzr("commit tree/hello.txt")
+        return tree
+
+    def test_commit_hook_template_accepted(self):
+        tree = self.setup_commit_with_template()
+        out, err = self.run_bzr("commit tree/hello.txt", stdin="y\n")
         last_rev = tree.branch.repository.get_revision(tree.last_revision())
         self.assertEqual('save me some typing\n', last_rev.message)
+
+    def test_commit_hook_template_rejected(self):
+        tree = self.setup_commit_with_template()
+        expected = tree.last_revision()
+        out, err = self.run_bzr_error(["empty commit message"],
+            "commit tree/hello.txt", stdin="n\n")
+        self.assertEqual(expected, tree.last_revision())