Merge lp:~vila/bzr/673637-temp-commit-file into lp:bzr

[Vincent Ladeuil]

Create commit message files in TMPDIR instead of the current dir

This brings in the patch proposed for bug #673637 with some small tweaks to keep the test coverage.

The issue is that:

  msgeditor.py creates the temporary log file in the directory where "bzr ci" is invoked,
  which is normally inside the branch. When you save the log message inside Emacs, Emacs
  runs "bzr status", because the log message file is in a versioned directory, and Emacs
  tries to get its revision and status, to show that in the modeline. But because "bzr ci"
  is still running, the branch is locked, and "bzr status" fails, this failing the commit.

Long story short: using the working directory for a temporary file is not needed there and the above is true only on windows so OLMD (bug #98836).

Since the previous related fix was about making sure we could create such files in a directory with a fancy name, I kept the test but tweaked it to still force a specified directory.

[John A Meinel (jameinel)]

I'm "ok" with this fix. It seems to solve a problem that someone was having.

The concerns are:

1) it seems like the problem is actually in emacs, and we are papering around it in bzr
2) It means that you won't use the cwd for staging the commit log, in case someone wants to save it off somewhere for later (in case of failure to commit/rollback/etc.)
3) I don't have any personal stake, because I always use "-m" and shell as my commit message editor. (With the other benefit that 'bzr diff' always continues to work.) As such, I don't have a feel for how this will impact people, since I don't use it.

4) I think this might also solve a bug people were having, where they wanted to commit it /etc, and had rights to /etc/.bzr but *not* rights to /etc/ itself. (sysadmins not wanting to commit as root, I guess.)

On the whole, I think I'm for it, just concerned about possible fallout.

review: Approve

[Vincent Ladeuil (vila)]

>>>>> John A Meinel <email address hidden> writes:

    > Review: Approve
    > I'm "ok" with this fix. It seems to solve a problem that someone was having.

    > The concerns are:

    > 1) it seems like the problem is actually in emacs, and we are
    > papering around it in bzr

Right, I chose to avoid the finger syndrom, hence the 'using the working
directory for a temporary file is not needed there' in the cover letter.

    > 2) It means that you won't use the cwd for staging the commit log,
    > in case someone wants to save it off somewhere for later (in case
    > of failure to commit/rollback/etc.)

I think we want to address this differently: both qbzr and bzr-gtk are
already saving it in branch.conf so:
- we need the savng to be done by bzr itself,
- we really want to save it in tree.conf instead.

In the mean time, this fix avoids the infamous OLMD (OS Locks Must
Die). I'm prety sure there is at least one bug asking for the above so
we won't forget.

    > 3) I don't have any personal stake, because I always use "-m" and
    > shell as my commit message editor. (With the other benefit that
    > 'bzr diff' always continues to work.) As such, I don't have a feel
    > for how this will impact people, since I don't use it.

Same here.

    > 4) I think this might also solve a bug people were having, where
    > they wanted to commit it /etc, and had rights to /etc/.bzr but
    > *not* rights to /etc/ itself. (sysadmins not wanting to commit as
    > root, I guess.)

Indeed.

    > On the whole, I think I'm for it, just concerned about possible fallout.

I was too, that's why I kept the test and tweaked it instead of just
removing it.

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

[Vincent Ladeuil (vila)]

sent to pqm by email

=== modified file 'bzrlib/msgeditor.py'
--- bzrlib/msgeditor.py	2010-09-13 10:04:19 +0000
+++ bzrlib/msgeditor.py	2010-11-11 13:47:08 +0000
@@ -208,28 +208,25 @@
 
 def _create_temp_file_with_commit_template(infotext,
                                            ignoreline=DEFAULT_IGNORE_LINE,
-                                           start_message=None):
+                                           start_message=None,
+                                           tmpdir=None):
     """Create temp file and write commit template in it.
 
-    :param infotext:    Text to be displayed at bottom of message
-                        for the user's reference;
-                        currently similar to 'bzr status'.
-                        The text is already encoded.
+    :param infotext: Text to be displayed at bottom of message for the
+        user's reference; currently similar to 'bzr status'.  The text is
+        already encoded.
 
     :param ignoreline:  The separator to use above the infotext.
 
-    :param start_message:   The text to place above the separator, if any.
-                            This will not be removed from the message
-                            after the user has edited it.
-                            The string is already encoded
+    :param start_message: The text to place above the separator, if any.
+        This will not be removed from the message after the user has edited
+        it.  The string is already encoded
 
     :return:    2-tuple (temp file name, hasinfo)
     """
     import tempfile
     tmp_fileno, msgfilename = tempfile.mkstemp(prefix='bzr_log.',
-                                               dir='.',
-                                               text=True)
-    msgfilename = osutils.basename(msgfilename)
+                                               dir=tmpdir, text=True)
     msgfile = os.fdopen(tmp_fileno, 'w')
     try:
         if start_message is not None:

=== modified file 'bzrlib/tests/test_msgeditor.py'
--- bzrlib/tests/test_msgeditor.py	2010-08-29 14:32:45 +0000
+++ bzrlib/tests/test_msgeditor.py	2010-11-11 13:47:08 +0000
@@ -301,9 +301,12 @@
     def test__create_temp_file_with_commit_template_in_unicode_dir(self):
         self.requireFeature(tests.UnicodeFilenameFeature)
         if hasattr(self, 'info'):
-            os.mkdir(self.info['directory'])
-            os.chdir(self.info['directory'])
-            msgeditor._create_temp_file_with_commit_template('infotext')
+            tmpdir = self.info['directory']
+            os.mkdir(tmpdir)
+            # Force the creation of temp file in a directory whose name
+            # requires some encoding support
+            msgeditor._create_temp_file_with_commit_template('infotext',
+                                                             tmpdir=tmpdir)
         else:
             raise TestNotApplicable('Test run elsewhere with non-ascii data.')
 

=== modified file 'doc/en/release-notes/bzr-2.3.txt'
--- doc/en/release-notes/bzr-2.3.txt	2010-11-10 11:15:50 +0000
+++ doc/en/release-notes/bzr-2.3.txt	2010-11-11 13:47:08 +0000
@@ -44,6 +44,10 @@
 * Better message if there is an error while setting ownership of
   ``.bazaar`` directory. (Parth Malwankar, #657553)
 
+* Don't create commit message files in the current directory to avoid nasty
+  interactions with emacs (which tries to establish the status of the file
+  during the commit which breaks on windows). (Vincent Ladeuil, #673637)
+
 * ``bzr resolve --take-other <file>`` will not crash anymore if ``<file>``
   is involved in a text conflict (but the conflict is still not
   resolved). (Vincent Ladeuil, #646961)