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

Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 5541
Proposed branch: lp:~vila/bzr/673637-temp-commit-file
Merge into: lp:bzr
Diff against target: 76 lines (+19/-15)
3 files modified
bzrlib/msgeditor.py (+9/-12)
bzrlib/tests/test_msgeditor.py (+6/-3)
doc/en/release-notes/bzr-2.3.txt (+4/-0)
To merge this branch: bzr merge lp:~vila/bzr/673637-temp-commit-file
Reviewer Review Type Date Requested Status
John A Meinel Approve
Review via email: mp+40625@code.launchpad.net

Commit message

Create commit message files in TMPDIR instead of the current dir

Description of the change

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.

To post a comment you must log in.
Revision history for this message
John A Meinel (jameinel) wrote :

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
Revision history for this message
Vincent Ladeuil (vila) wrote :

>>>>> 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.

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Revision history for this message
Vincent Ladeuil (vila) wrote :

sent to pqm by email

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== 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 @@
208208
209def _create_temp_file_with_commit_template(infotext,209def _create_temp_file_with_commit_template(infotext,
210 ignoreline=DEFAULT_IGNORE_LINE,210 ignoreline=DEFAULT_IGNORE_LINE,
211 start_message=None):211 start_message=None,
212 tmpdir=None):
212 """Create temp file and write commit template in it.213 """Create temp file and write commit template in it.
213214
214 :param infotext: Text to be displayed at bottom of message215 :param infotext: Text to be displayed at bottom of message for the
215 for the user's reference;216 user's reference; currently similar to 'bzr status'. The text is
216 currently similar to 'bzr status'.217 already encoded.
217 The text is already encoded.
218218
219 :param ignoreline: The separator to use above the infotext.219 :param ignoreline: The separator to use above the infotext.
220220
221 :param start_message: The text to place above the separator, if any.221 :param start_message: The text to place above the separator, if any.
222 This will not be removed from the message222 This will not be removed from the message after the user has edited
223 after the user has edited it.223 it. The string is already encoded
224 The string is already encoded
225224
226 :return: 2-tuple (temp file name, hasinfo)225 :return: 2-tuple (temp file name, hasinfo)
227 """226 """
228 import tempfile227 import tempfile
229 tmp_fileno, msgfilename = tempfile.mkstemp(prefix='bzr_log.',228 tmp_fileno, msgfilename = tempfile.mkstemp(prefix='bzr_log.',
230 dir='.',229 dir=tmpdir, text=True)
231 text=True)
232 msgfilename = osutils.basename(msgfilename)
233 msgfile = os.fdopen(tmp_fileno, 'w')230 msgfile = os.fdopen(tmp_fileno, 'w')
234 try:231 try:
235 if start_message is not None:232 if start_message is not None:
236233
=== 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 @@
301 def test__create_temp_file_with_commit_template_in_unicode_dir(self):301 def test__create_temp_file_with_commit_template_in_unicode_dir(self):
302 self.requireFeature(tests.UnicodeFilenameFeature)302 self.requireFeature(tests.UnicodeFilenameFeature)
303 if hasattr(self, 'info'):303 if hasattr(self, 'info'):
304 os.mkdir(self.info['directory'])304 tmpdir = self.info['directory']
305 os.chdir(self.info['directory'])305 os.mkdir(tmpdir)
306 msgeditor._create_temp_file_with_commit_template('infotext')306 # Force the creation of temp file in a directory whose name
307 # requires some encoding support
308 msgeditor._create_temp_file_with_commit_template('infotext',
309 tmpdir=tmpdir)
307 else:310 else:
308 raise TestNotApplicable('Test run elsewhere with non-ascii data.')311 raise TestNotApplicable('Test run elsewhere with non-ascii data.')
309312
310313
=== 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 @@
44* Better message if there is an error while setting ownership of44* Better message if there is an error while setting ownership of
45 ``.bazaar`` directory. (Parth Malwankar, #657553)45 ``.bazaar`` directory. (Parth Malwankar, #657553)
4646
47* Don't create commit message files in the current directory to avoid nasty
48 interactions with emacs (which tries to establish the status of the file
49 during the commit which breaks on windows). (Vincent Ladeuil, #673637)
50
47* ``bzr resolve --take-other <file>`` will not crash anymore if ``<file>``51* ``bzr resolve --take-other <file>`` will not crash anymore if ``<file>``
48 is involved in a text conflict (but the conflict is still not52 is involved in a text conflict (but the conflict is still not
49 resolved). (Vincent Ladeuil, #646961)53 resolved). (Vincent Ladeuil, #646961)