Merge lp:~vila/bzr/525571-minimal-atomic-write-bazaar-conf-files-2.1 into lp:bzr/2.1

Proposed by Vincent Ladeuil
Status: Merged
Approved by: John A Meinel
Approved revision: no longer in the source branch.
Merged at revision: 4849
Proposed branch: lp:~vila/bzr/525571-minimal-atomic-write-bazaar-conf-files-2.1
Merge into: lp:bzr/2.1
Diff against target: 52 lines (+13/-4)
2 files modified
NEWS (+4/-0)
bzrlib/config.py (+9/-4)
To merge this branch: bzr merge lp:~vila/bzr/525571-minimal-atomic-write-bazaar-conf-files-2.1
Reviewer Review Type Date Requested Status
John A Meinel Needs Fixing
Review via email: mp+29086@code.launchpad.net

Commit message

Minimal fix for bug #525571 to ease backport to launchpad.

Description of the change

This is a minimal patch to address bug #525571 (concurrent config writers) without any attempt to cleanup anything ;)

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

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/525571-minimal-lock-bazaar-conf-files-2.1 into lp:bzr/2.1.
>
> Requested reviews:
> bzr-core (bzr-core)
>
>
> This is a minimal patch to address bug #525571 (concurrent config writers) without any attempt to cleanup anything ;)
>
>

1) If we know we are accessing a local file (since you are using open()
directly), then we could just use AtomicFile directly.

2) I'm a bit concerned about the need for
safe_unicode(self._get_filename()).

3) You can do

dirname, basename = osutils.split(fname)

rather than doing it 2x.

Personally, I'd really rather see AtomicFile than Transport for this.

Specifically, if fname was ever Unicode, this would break. As Transport
expects the 'relpath' to be a URL, and you are handing it a Unicode
string. Oh, and it would probably break if fname had any funny
characters like % in it.

 review: needsfixing

John
=:->

-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org/

iEYEARECAAYFAkwt9DoACgkQJdeBCYSNAAOE7ACgzcbdLqynPzKMStWmEZkVgOtA
2jYAn0BNItWLFxQCecwo/NHh2x7+jOCG
=EmU/
-----END PGP SIGNATURE-----

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

> 1) If we know we are accessing a local file (since you are using open()
> directly), then we could just use AtomicFile directly.

Done.

>
> 2) I'm a bit concerned about the need for
> safe_unicode(self._get_filename()).

This was triggered in my cleanup branch by a test in a unicode home dir, Ill look into it again there.

>
> 3) You can do
>
> dirname, basename = osutils.split(fname)
>
> rather than doing it 2x.

1) I was aiming minimal, 2) different encodings were needed.

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 'NEWS'
--- NEWS 2010-06-16 12:12:56 +0000
+++ NEWS 2010-07-02 16:10:46 +0000
@@ -20,6 +20,10 @@
20Bug Fixes20Bug Fixes
21*********21*********
2222
23* Configuration files in ``${BZR_HOME}`` are now written in an atomic
24 way which should help avoid problems with concurrent writers.
25 (Vincent Ladeuil, #525571)
26
23* Raise ValueError instead of a string exception.27* Raise ValueError instead of a string exception.
24 (John Arbash Meinel, #586926)28 (John Arbash Meinel, #586926)
2529
2630
=== modified file 'bzrlib/config.py'
--- bzrlib/config.py 2010-02-17 17:11:16 +0000
+++ bzrlib/config.py 2010-07-02 16:10:46 +0000
@@ -74,6 +74,7 @@
7474
75import bzrlib75import bzrlib
76from bzrlib import (76from bzrlib import (
77 atomicfile,
77 debug,78 debug,
78 errors,79 errors,
79 mail_client,80 mail_client,
@@ -510,9 +511,10 @@
510 self._write_config_file()511 self._write_config_file()
511512
512 def _write_config_file(self):513 def _write_config_file(self):
513 f = open(self._get_filename(), 'wb')514 atomic_file = atomicfile.AtomicFile(self._get_filename())
514 self._get_parser().write(f)515 self._get_parser().write(atomic_file)
515 f.close()516 atomic_file.commit()
517 atomic_file.close()
516518
517519
518class LocationConfig(IniBasedConfig):520class LocationConfig(IniBasedConfig):
@@ -653,7 +655,10 @@
653 self._get_parser()[location][option]=value655 self._get_parser()[location][option]=value
654 # the allowed values of store match the config policies656 # the allowed values of store match the config policies
655 self._set_option_policy(location, option, store)657 self._set_option_policy(location, option, store)
656 self._get_parser().write(file(self._get_filename(), 'wb'))658 atomic_file = atomicfile.AtomicFile(self._get_filename())
659 self._get_parser().write(atomic_file)
660 atomic_file.commit()
661 atomic_file.close()
657662
658663
659class BranchConfig(Config):664class BranchConfig(Config):

Subscribers

People subscribed via source and target branches