Merge lp:~vila/bzr/525571-lock-bazaar-conf-files into lp:bzr

[Vincent Ladeuil]

The precise circumstances leading to the bug remained unclear, so
here it my best guess: somehow one process was able to read a
conf file *while* another process was writing it and failed
because the file was incomplete leading to parse errors.

ConfigObj does:

                h = open(infile, 'rb')
                infile = h.read() or []
                h.close()

to read a config file.

and
            h = open(self.filename, 'wb')
            h.write(output)
            h.close()

to write a config file.

I.e. the read or write can results in several physical IOs that
could explain partial files being read.

Addressing this is done with:

- using an atomic operation to write the config file so readers
  can't get a partial file,

- using a lock to guarantee that a single writer is allowed at any time.

The data model we use for config files is that they are used a
database where the records are the (name, value) pairs.

When a value is modified, the whole config file is written to
disk.

Config objects themselve are rarely cached (if ever) so to access
a value we generally load the whole file.

From there, it makes sense to re-read the whole file before
setting a new value so that other updates are taken into account
(*not* doing so means we will reset the values ignoring the
concurrent updates).

So the sequence to set a new value is:

- take a write lock,
- re-read the config file (to take concurrent updates into
  account),
- set the new value,
- write the config file,
- unlock

The proposed implementation means that all methods that ends up
writing a config file should be decorated with
@needs_write_lock. This is generally set_user_option() and its
variants (notably set_alias() and unset_alias() for
GlobalConfig).

Note that since we use an atomic write we neglect to protect
readers against concurrent writers. This shouldn't be a problem
in practice and I've been yelled at for suggesting it in a
paranoid attempt to cover all cases. The case at point being:

- a reader opens a file,
- starts to read it (unlikely to not complete in a single IO),
- a writer somehow manages to replace the opened file (unlikely since we
  do a rename),
- the OS presents the new content to the reader.

Since I'm not even sure this scenario is valid, I'll wait for
evidence before considering it.

Anyway, machines can crash while a lock is hold so break-lock has
to be updated to handle the config locks. After discussing the
possible implementations with lifeless, we settled on adding a
--conf option that must be specified to break a config lock. The
actual implementation succeeds when asked to break a lock in a
bzrdir where no lock exist, I did the same. We may want to
revisit the behavior for both cases (failing when no lock are
present) but I consider this out of scope for this bug fix.

I didn't enforce the config directory to be '~/.bazaar', it could
as well be '.bzrmeta' or anything.

[John A Meinel (jameinel)]

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

Vincent Ladeuil wrote:
> Vincent Ladeuil has proposed merging lp:~vila/bzr/525571-lock-bazaar-conf-files into lp:bzr.
>
> Requested reviews:
> bzr-core (bzr-core)
> Related bugs:
> #525571 No locking when updating files in ~/.bazaar
> https://bugs.launchpad.net/bugs/525571
>
>
> This implements a write lock on LocationConfig and GlobalConfig and add support for them in break-lock
> as required for bug #525571.
>
> There is no user nor dev visible change but I'll welcome feedback from plugin authors.
>
> There is a new bzrlib.config.LockableConfig that plugins authors using config files in ~/.bazaar may want to inherit (as LocationConfig and GlobalConfig do now).
>
> I had to do some cleanup in the tests as modifying the model made quite a few of them fail
> (congrats to test authors: failing tests are good tests ! :)
>
> So I made a bit more cleanup than strictly necessary (during failure analysis),
> my apologies to the reviewers.
>

As a comment, without having really read the code thoroughly.

How does this handle stuff like 2 branches locking concurrently
locations.conf. I don't know how often we do it internally, though.

I think lots of filesystem locks on the bazaar directory could adversely
affect performance on Windows. IME locking isn't too expensive if you do
it 1 or 2 times. But if you lock and unlock on every attribute that gets
set, then it probably starts to be an issue.

On a Windows host:
  $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
  10.5msec

On an Ubuntu VM on the same machine:
  $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
  1.55msec

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

iEYEARECAAYFAkwqMNkACgkQJdeBCYSNAAO7WACfYZOte5LfqA4Ro4J6U/3ZA2Cf
ZhkAoLy3d0+tQjcgx047AYI0sJMiZlfY
=mMJj
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

07:04 -!- guilhembi
[~<email address hidden>] has quit
[Quit: Client exiting]
07:11 < lifeless> vila:
07:11 < lifeless> + __doc__ = """\
07:11 < lifeless> + Break a dead lock on a repository, branch,
working directory or config file.
07:11 < lifeless> I'd prefer to see that spelt as
07:11 < lifeless> __doc__ = \
07:11 < lifeless> """Break...
07:12 < lifeless> because having to actually think about what you were
escaping hurt my brain

[Robert Collins (lifeless)]

Meta: This seems like a case where two threads would be nice:
a) fix the tests that are currently a bit gratuitous.
b) make the behaviour change

[Robert Collins (lifeless)]

Ok, actual review stuff:
 the docstring layout is wrong, please nuke the \.

We should check for branches first, not config files, because branch locks are the common case and break-lock doesn't need to be slow.

This change is suspicous:

152 def _write_config_file(self):
153 - f = file(self._get_filename(), "wb")
154 + fname = self._get_filename()
155 + conf_dir = os.path.dirname(fname)
156 + ensure_config_dir_exists(conf_dir)
157 + f = file(fname, "wb")
158 try:
159 - osutils.copy_ownership_from_path(f.name)
160 + osutils.copy_ownership_from_path(fname)
161 self._get_parser().write(f)
162 finally:
163 f.close()
164

It appears to be adding a new stat/mkdir check, at the wrong layer.

missing VWS:

172 + """
173 + lock_name = 'lock'

Ditto here:

181 + def lock_write(self, token=None):
182 + if self.lock is None:
183 + ensure_config_dir_exists(self.dir)
184 + self._lock = lockdir.LockDir(self.transport, self.lock_name)
185 + self._lock.lock_write(token)
186 + return lock.LogicalLockResult(self.unlock)

If the dir doesn't exist, something is wrong - we really shouldn't have gotten this far without it, should we?

Docstrings!!!

I'll review the tests after the core is good.

Uh, you raise NoLockDir, but use it to mean 'A config directory that is not locked' which is very odd. Please consider something that means what you need. Also see my ordering suggestion which may help you not to need this at all.

review: Needs Fixing

[Vincent Ladeuil (vila)]

> Ok, actual review stuff:
> the docstring layout is wrong, please nuke the \.
>
> We should check for branches first, not config files, because branch locks are
> the common case and break-lock doesn't need to be slow.

break_lock() for wt, branch, repo gives no indication about whether it fails or succeeds.
Trying the conf files first was the easiest.

Regarding performance, I think we don't care at all, the user is supposed to first check that
the lock is not hold by a running process (or someone else) which requires seconds in the best case
or occur long after the lock has been created.

>
> This change is suspicous:
>
> 152 def _write_config_file(self):
> 153 - f = file(self._get_filename(), "wb")
> 154 + fname = self._get_filename()
> 155 + conf_dir = os.path.dirname(fname)
> 156 + ensure_config_dir_exists(conf_dir)
> 157 + f = file(fname, "wb")
> 158 try:
> 159 - osutils.copy_ownership_from_path(f.name)
> 160 + osutils.copy_ownership_from_path(fname)
> 161 self._get_parser().write(f)
> 162 finally:
> 163 f.close()
> 164
>
> It appears to be adding a new stat/mkdir check, at the wrong layer.

No adding there, code duplication removal instead, ensure_config_dir_exists() was called anyway.

>
> missing VWS:
>
> 172 + """
> 173 + lock_name = 'lock'

Fixed.
>
>
>
> Ditto here:
>
> 181 + def lock_write(self, token=None):
> 182 + if self.lock is None:
> 183 + ensure_config_dir_exists(self.dir)
> 184 + self._lock = lockdir.LockDir(self.transport,
> self.lock_name)
> 185 + self._lock.lock_write(token)
> 186 + return lock.LogicalLockResult(self.unlock)
>
> If the dir doesn't exist, something is wrong - we really shouldn't have gotten
> this far without it, should we?

When the config file didn't exist, the config dir needs to be created.

> Docstrings!!!

A bit terse but I will add some.

> Uh, you raise NoLockDir, but use it to mean 'A config directory that is not
> locked' which is very odd.
> Please consider something that means what you need.

I need something that means: "Oh, I wanted to break a lock there but there is no lock dir there,
surely I can't break a lock in that case". I fail to see the oddness :-/

[Vincent Ladeuil (vila)]

> As a comment, without having really read the code thoroughly.
>
> How does this handle stuff like 2 branches locking concurrently
> locations.conf. I don't know how often we do it internally, though.

TestLockableConfig.test_writes_are_serialized

>
> I think lots of filesystem locks on the bazaar directory could adversely
> affect performance on Windows. IME locking isn't too expensive if you do
> it 1 or 2 times. But if you lock and unlock on every attribute that gets
> set, then it probably starts to be an issue.

The actual code is not correct, it allows concurrent writers. If higher levels do too many updates of config variables it's a problem in the higher levels, we could imagine holding the updates until the last one, but
this can't addressed here.

Correctness comes before performance, there are many things we can do to address performance but it's way out of scope for this proposal IMHO.

>
> On a Windows host:
> $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
> 10.5msec
>
> On an Ubuntu VM on the same machine:
> $ TIMEIT -s "b = Branch.open('.')" "b.lock_write(); b.unlock()"
> 1.55msec

Thanks, good data point, but still, I haven't seen code doing massive updates of config variables either...

[John A Meinel (jameinel)]

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

...

> Note that since we use an atomic write we neglect to protect
> readers against concurrent writers. This shouldn't be a problem
> in practice and I've been yelled at for suggesting it in a
> paranoid attempt to cover all cases. The case at point being:
>
> - a reader opens a file,
> - starts to read it (unlikely to not complete in a single IO),
> - a writer somehow manages to replace the opened file (unlikely since we
> do a rename),
> - the OS presents the new content to the reader.
>
> Since I'm not even sure this scenario is valid, I'll wait for
> evidence before considering it.

POSIX systems require that the currently open file stays pointing at the
same content (same inode). While the rename replaces the directory
information, the open file handle does not point to a new file. (You
would have to have 'read' be implemented as (open(), seek(), read(),
close(), to get this behavior.)

Windows says that open() creates a lock on the *path*, which means that
you cannot rename a new file over a file which is already open. (So no
atomic rename operation.)

There is probably a CreateFile flag that might allow it (as there is one
that allows you to delete an open file IIRC). But we don't use it ourselves.

I'm about 95% sure that this scenario is invalid.

John
=:->

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

iEYEARECAAYFAkwrrRMACgkQJdeBCYSNAANG5QCeNjIy3IeDz2fFMW0KPHEUp5qv
gdYAoMG9ymYJX32g3tVRWCsIXF5vke8r
=nu81
-----END PGP SIGNATURE-----

[Robert Collins (lifeless)]

16:24 < lifeless> vila: --config please, not --conf
16:25 < lifeless> --conf is a strange abbreviation
16:25 < lifeless> c = config.LockableConfig(lambda : location)
16:25 < lifeless> reads really strangely
16:25 < lifeless> perhaps the LockableConfig constructor should be different
16:27 < vila> lifeless: I don't want to break the chain of constructors, I can add a def get_filename if you prefer

When a class provokes ugly code in users, the class is wrong. Perhaps the class could take two keyword constructors, and then only the new code in builtins uses a keyword, and uses the second one to supply a name.

the class can then use it.

The ironic thing here is the first thing the class does is make the filename by calling the constructor.

Please note in the (otherwise excellent) docstring that the reason callers need to lock_write, rather than the innermost writer function, is to ensure that the file is both *read* and *written* in the same lock.

Perhaps that would be best tracked - when doing a read, if the object is not locked, set self._can_write = False, and if it is, set it to True. Then check that as well as whether it is locked, in _write_config.

The file=None to infile=None change seems to just generate spurious noise, and is not done in a backward compatible way.

We are likely going to want to backport this for launchpad, so it really should be a focused patch.

Likewise the test changes that are not related to locking consistency.

Please roll those things back, and I think we'll have a much more focused patch that we can confidently get LP to cherrypick.

review: Needs Fixing

[Robert Collins (lifeless)]

There's also a number of opaque variable names like 'c', 'p' and 'lc_tests' - while I figured out what they were, they could be a lot clearer.

[Vincent Ladeuil (vila)]

> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation
> 16:25 < lifeless> c = config.LockableConfig(lambda : location)
> 16:25 < lifeless> reads really strangely
> 16:25 < lifeless> perhaps the LockableConfig constructor should be different
> 16:27 < vila> lifeless: I don't want to break the chain of constructors, I can
> add a def get_filename if you prefer
>
>
> When a class provokes ugly code in users, the class is wrong. Perhaps the
> class could take two keyword constructors, and then only the new code in
> builtins uses a keyword, and uses the second one to supply a name.

I.e. __init__ should accept file_name and get_filename and deprecate the later ?

>
> the class can then use it.

Conflicts with making a focused patch. I'll keep that for an updated submission focused on cleanup.

>
> The ironic thing here is the first thing the class does is make the filename
> by calling the constructor.

Yup, it's needed to build the lock directory (I'm sure you got that). The other classes postpone the get_filename() evaluation until it's needed, but given the functions used to provide the names, I don't think postponing is really required (some env variables are involved though, so there may be edge cases).

>
> Please note in the (otherwise excellent) docstring that the reason callers
> need to lock_write, rather than the innermost writer function, is to ensure
> that the file is both *read* and *written* in the same lock.
>

I'll mention that. And yes, it's the real reason for requiring a larger lock scope.

> Perhaps that would be best tracked - when doing a read, if the object is not
> locked, set self._can_write = False, and if it is, set it to True. Then check
> that as well as whether it is locked, in _write_config.

_can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

>
>
> The file=None to infile=None change seems to just generate spurious noise, and
> is not done in a backward compatible way.

It's on a private method and used only by tests (and provide a poor way to build test configs anyway).

>
> We are likely going to want to backport this for launchpad, so it really
> should be a focused patch.

I'll restart from scratch then.

>
> Likewise the test changes that are not related to locking consistency.

Most of them were required to address the flawed assumption that a config object (with content)
can be created without an actual file existing, I had to fix the failing tests.
The duplicated ensure_config_dir_exists() came in the way to.

And I mention that to explain that many cleanups were not for the sake of pure cleaning but driven by problems making the fix harder.

I'll look into an alternative solution to minimise the patch but that would likely end up in either weird code or intrusive changes, we'll see.

[Vincent Ladeuil (vila)]

> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation

:-P

[Robert Collins (lifeless)]

Vila, I think I need to expand on this:

>> Perhaps that would be best tracked - when doing a read, if the object is not
>> locked, set self._can_write = False, and if it is, set it to True. Then check
>> that as well as whether it is locked, in _write_config.

> _can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

They are quite different.

If when you *read* you are unlocked, and you permit a *write* without forcing a new read, changes from other processes are discarded.

Here:
conf.read_something()
--client 2 writes to the file
conf.set_something()
--*boom* client 2's changes are lost

What we need is two fold: detection of this case (which setting 'can write' during *read* if it is write locked at the time will permit), and easy api use (which may be less immediate depending on impact).

Alternatively lock_write could possibly trigger a read after locking before returning, which would cause properly lock_write() guarded mutators to work correctly - as they would apply the mutation within the locked context.

In short - concurrency is hard, lets drink beer.

review: Needs Fixing

[John Szakmeister (jszakmeister)]

On Sun, Jul 4, 2010 at 6:39 PM, Robert Collins
<email address hidden> wrote:
[snip]
> In short - concurrency is hard, lets drink beer.

I think that's the best quote I've seen in a while! :-)

-John

[Vincent Ladeuil (vila)]

>>>>> Robert Collins <email address hidden> writes:

    > Review: Needs Fixing
    > Vila, I think I need to expand on this:

    >>> Perhaps that would be best tracked - when doing a read, if the object is not
    >>> locked, set self._can_write = False, and if it is, set it to True. Then check
    >>> that as well as whether it is locked, in _write_config.

    >> _can_write == is_locked() I see the idea but it won't change the end result nor the feedback we can give.

    > They are quite different.

    > If when you *read* you are unlocked, and you permit a *write*
    > without forcing a new read,

Which this proposal guards against.

    > changes from other processes are discarded.

Yes, that's a delicate problem. This proposal isn't perfect but good
enough to cover most of our use cases.

    > Here:
    > conf.read_something()
    > --client 2 writes to the file
    > conf.set_something()
    > --*boom* client 2's changes are lost

Yup, I added a test for that and saw it fail.

    > What we need is two fold: detection of this case (which setting
    > 'can write' during *read* if it is write locked at the time will
    > permit),

Right, but what will we gain here ? The ability to *not* force the
refreshing read if someone already did it while write locked ?

Yet, it goes in the same direction as implementing a no-op read lock,
which we may want to add too, so, worth considering.

    > and easy api use (which may be less immediate depending on
    > impact).

    > Alternatively lock_write could possibly trigger a read after
    > locking before returning, which would cause properly lock_write()
    > guarded mutators to work correctly - as they would apply the
    > mutation within the locked context.

Inside the lock scope, a write should always happen *after* a refreshing
read or you'll lose changes for variables you're not changing (the
current API change one variable at a time).

Of course you always lose any change to the variable you're changing and
that in itself could be a problem if processes try to use config
variables for synchronization (a really bad idea), like an incremented
counter.

    > In short - concurrency is hard, lets drink beer.

+1

There are various lock schemes with different scopes that can be tried,
but so far, I haven't found a good way to address:

- client1 read 'var'
- client1 starts a "long" process based on 'var'
- client2 set a new value for 'var'
- client1 finished its process with a now outdated value for 'var'

Short of blocking client2 for the duration of client1 process, I can't
see how to address this.

But do we care ?

It's hard to decide without concrete examples, if 'var' is a parent
branch location and the process is pull, chances are the user really
want its pull to finish and changing 'var' to point to a new branch
should just be considered the next time we try to pull (alternate
scenarios includes: abort the pull (and rollback the already fetched
revisions (eeerk)), block the write (i.e. block the whole config file
for the duration of client1 process (omg are you crazy ?))).

Overall, I agree that we should continue thinking about which lock model
we want here, the proposed...

[Vincent Ladeuil (vila)]

Marking as WIP pending a discussion during our next sprint on larger issues.
A minimal fix has landed for 2.1 using atomic writes which should be a good enough fix in the mean time.

[John A Meinel (jameinel)]

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

...

>
> Overall, I agree that we should continue thinking about which lock model
> we want here, the proposed one is incrementally better and at least
> address the bug better than the minimal one landed for 2.1.
>
> Orthogonal to the lock scope, sharing a config files at the
> wt/branch/repo level could at least avoid reading the config file for
> each variable which could make read locks less painful, but still, I
> don't think it would be enough.

I think defining a scope is reasonable, and just sticking with that.

IMO, configs could be treated just like the rest of the branch data. So
if you '.lock_read()' at the beginning of a process, you read the config
state at that point in time, and then never read the config file again.

When you go to write, you probably need to re-read the file, because a
given text file has more content than what you are likely to be adjusting.

We *could* implement some sort of CAS to make config updates atomic. I
don't think that is worthwhile. (I tried to overwrite parent X with
parent Y, but parent was already Z.)

I agree that you shouldn't try to abuse configs to store incremental
counters, etc.

I honestly don't know what state was getting trashed by concurrent
updates, but I have the feeling that all of the bzr core data doesn't
really care. (Yes, you might get the wrong parent pointer, or submit
pointer, but we don't really store 'live' data in conf files.)

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

iEYEARECAAYFAkwzP7gACgkQJdeBCYSNAANa5QCdFVLPjMHJLyKn7MvR9NdkiAhp
6r8An3u2enhCMDQVbPS/wOmeo5LUO6X3
=Kmd7
-----END PGP SIGNATURE-----

[lp:~vila/bzr/525571-lock-bazaar-conf-files]

5334. By Vincent Ladeuil on 2010-07-02

Clarify lock scope.

5335. By Vincent Ladeuil on 2010-07-02

Use --config instead of --conf for break-lock.

[Vincent Ladeuil (vila)]

Superseded by the loom ending in https://code.edge.launchpad.net/~vila/bzr/remove-gratuitous-ensure-config-dir-exist-calls/+merge/33426

Unmerged revisions

5335. By Vincent Ladeuil on 2010-07-02

Use --config instead of --conf for break-lock.

5334. By Vincent Ladeuil on 2010-07-02

Clarify lock scope.

5333. By Vincent Ladeuil on 2010-06-30

Final cleanup.

5332. By Vincent Ladeuil on 2010-06-30

Implement the --conf option for break-lock as per lifeless suggestion.

* bzrlib/errors.py:
(NoLockDir): Useless, deleted.

* bzrlib/config.py:
(LockableConfig.unlock): NoLockDir is useless, break_lock()
silenty succeeds if the directory doesn't exist.

* bzrlib/tests/blackbox/test_break_lock.py:
Tweak the tests.

* bzrlib/builtins.py:
(cmd_break_lock): Fix docstring, add a --conf option to deal with
config files.

5331. By Vincent Ladeuil on 2010-06-30

Add a test to help LockableConfig daughter classes identify methods that needs to be decorated.

5330. By Vincent Ladeuil on 2010-06-30

Further clarify NEWS entry.

5329. By Vincent Ladeuil on 2010-06-30

Fix docstring.

5328. By Vincent Ladeuil on 2010-06-30

Fix wrong bug number and clarify NEWS entries.

5327. By Vincent Ladeuil on 2010-06-30

Revert the lock scope to a sane value.

* bzrlib/tests/test_config.py:
(TestLockableConfig.test_writes_are_serialized)
(TestLockableConfig.test_read_while_writing): Fix the fallouts.

* bzrlib/config.py:
(LockableConfig): Wrong idea, the lock needs to be taken arond the
whole value update call, reducing the lock scope to
_write_config_file exclude the config file re-read.
(GlobalConfig.set_user_option, GlobalConfig.set_alias)
(GlobalConfig.unset_alias, LocationConfig.set_user_option): These
methods needs to be decorated with needs_write_lock to enforce the
design constraints (lock, re-read config, set new value, write
config, unlock).

5326. By Vincent Ladeuil on 2010-06-29

Add some comments.

=== modified file 'NEWS'
--- NEWS	2010-07-13 21:25:03 +0000
+++ NEWS	2010-07-14 08:34:44 +0000
@@ -119,6 +119,15 @@
 New Features
 ************
 
+* ``bzr break-lock --config [location]`` can now break config files
+  locks. (Vincent Ladeuil, #525571)
+
+* ``bzrlib.config.LockableConfig`` is a base class for config files that
+  needs to be protected against multiple writers. All methods that
+  change a configuration variable value must be decorated with
+  @needs_write_lock (set_option() for example).
+  (Vincent Ladeuil,  #525571)
+
 * Support ``--directory`` option for a number of additional commands:
   conflicts, merge-directive, missing, resolve, shelve, switch,
   unshelve, whoami. (Martin von Gagern, #527878)
@@ -145,6 +154,7 @@
   or pull location in locations.conf or branch.conf.
   (Gordon Tyler, #534787)
 
+<<<<<<< TREE
 * ``bzr reconfigure --unstacked`` now works with branches accessed via a
   smart server. (Andrew Bennetts, #551525)
 
@@ -154,6 +164,11 @@
 * ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied.
   (Marius Kruger, Robert Collins)
 
+=======
+* Configuration files in ``${BZR_HOME}`` are now protected
+  against concurrent writers. (Vincent Ladeuil, #525571)
+
+>>>>>>> MERGE-SOURCE
 * Ensure that wrong path specifications in ``BZR_PLUGINS_AT`` display
   proper error messages. (Vincent Ladeuil, #591215)
 

=== modified file 'bzrlib/builtins.py'
--- bzrlib/builtins.py	2010-06-30 12:13:14 +0000
+++ bzrlib/builtins.py	2010-07-14 08:34:44 +0000
@@ -33,7 +33,7 @@
     bzrdir,
     directory_service,
     delta,
-    config,
+    config as _mod_config,
     errors,
     globbing,
     hooks,
@@ -3332,7 +3332,7 @@
                 try:
                     c = Branch.open_containing(u'.')[0].get_config()
                 except errors.NotBranchError:
-                    c = config.GlobalConfig()
+                    c = _mod_config.GlobalConfig()
             else:
                 c = Branch.open(directory).get_config()
             if email:
@@ -3343,7 +3343,7 @@
 
         # display a warning if an email address isn't included in the given name.
         try:
-            config.extract_email_address(name)
+            _mod_config.extract_email_address(name)
         except errors.NoEmailInUsername, e:
             warning('"%s" does not seem to contain an email address.  '
                     'This is allowed, but not recommended.', name)
@@ -3355,7 +3355,7 @@
             else:
                 c = Branch.open(directory).get_config()
         else:
-            c = config.GlobalConfig()
+            c = _mod_config.GlobalConfig()
         c.set_user_option('email', name)
 
 
@@ -3428,13 +3428,13 @@
                 'bzr alias --remove expects an alias to remove.')
         # If alias is not found, print something like:
         # unalias: foo: not found
-        c = config.GlobalConfig()
+        c = _mod_config.GlobalConfig()
         c.unset_alias(alias_name)
 
     @display_command
     def print_aliases(self):
         """Print out the defined aliases in a similar format to bash."""
-        aliases = config.GlobalConfig().get_aliases()
+        aliases = _mod_config.GlobalConfig().get_aliases()
         for key, value in sorted(aliases.iteritems()):
             self.outf.write('bzr alias %s="%s"\n' % (key, value))
 
@@ -3450,7 +3450,7 @@
 
     def set_alias(self, alias_name, alias_command):
         """Save the alias in the global config."""
-        c = config.GlobalConfig()
+        c = _mod_config.GlobalConfig()
         c.set_alias(alias_name, alias_command)
 
 
@@ -4829,7 +4829,10 @@
 
 
 class cmd_break_lock(Command):
-    __doc__ = """Break a dead lock on a repository, branch or working directory.
+    __doc__ = """Break a dead lock.
+
+    This command breaks a lock on a repository, branch, working directory or
+    config file.
 
     CAUTION: Locks should only be broken when you are sure that the process
     holding the lock has been stopped.
@@ -4840,17 +4843,27 @@
     :Examples:
         bzr break-lock
         bzr break-lock bzr+ssh://example.com/bzr/foo
+        bzr break-lock --conf ~/.bazaar
     """
+
     takes_args = ['location?']
+    takes_options = [
+        Option('config',
+               help='LOCATION is the directory where the config lock is.'),
+        ]
 
-    def run(self, location=None, show=False):
+    def run(self, location=None, config=False):
         if location is None:
             location = u'.'
-        control, relpath = bzrdir.BzrDir.open_containing(location)
-        try:
-            control.break_lock()
-        except NotImplementedError:
-            pass
+        if config:
+            conf = _mod_config.LockableConfig(lambda : location)
+            conf.break_lock()
+        else:
+            control, relpath = bzrdir.BzrDir.open_containing(location)
+            try:
+                control.break_lock()
+            except NotImplementedError:
+                pass
 
 
 class cmd_wait_until_signalled(Command):

=== modified file 'bzrlib/config.py'
--- bzrlib/config.py	2010-07-13 16:10:20 +0000
+++ bzrlib/config.py	2010-07-14 08:34:44 +0000
@@ -62,9 +62,11 @@
 up=pull
 """
 
+from cStringIO import StringIO
 import os
 import sys
 
+from bzrlib.decorators import needs_write_lock
 from bzrlib.lazy_import import lazy_import
 lazy_import(globals(), """
 import errno
@@ -77,11 +79,14 @@
     atomicfile,
     debug,
     errors,
+    lock,
+    lockdir,
     mail_client,
     osutils,
     registry,
     symbol_versioning,
     trace,
+    transport,
     ui,
     urlutils,
     win32utils,
@@ -357,18 +362,35 @@
         self._get_filename = get_filename
         self._parser = None
 
-    def _get_parser(self, file=None):
+    def _get_parser(self, infile=None):
         if self._parser is not None:
             return self._parser
-        if file is None:
-            input = self._get_filename()
-        else:
-            input = file
+        # Do we have a file name for the config file
+        if self._get_filename is None:
+            fname = None
+        else:
+            fname = self._get_filename()
+        # Do we have a content for the config file ?
+        if infile is None:
+            fname_or_content = fname
+        else:
+            fname_or_content = infile
+        # Build the ConfigObj
+        p = None
         try:
-            self._parser = ConfigObj(input, encoding='utf-8')
+            p = ConfigObj(fname_or_content, encoding='utf-8')
         except configobj.ConfigObjError, e:
             raise errors.ParseConfigError(e.errors, e.config.filename)
-        return self._parser
+        if p is not None and fname is not None:
+            # Make sure p.reload() will use the right file name
+            p.filename = fname
+        self._parser = p
+        return p
+
+    def reload(self):
+        """Reload the config file from disk."""
+        if self._parser is not None:
+            self._parser.reload()
 
     def _get_matching_sections(self):
         """Return an ordered list of (section_name, extra_path) pairs.
@@ -479,38 +501,122 @@
         return self.get_user_option('nickname')
 
     def _write_config_file(self):
+<<<<<<< TREE
         atomic_file = atomicfile.AtomicFile(self._get_filename())
         self._get_parser().write(atomic_file)
         atomic_file.commit()
         atomic_file.close()
-
-
-class GlobalConfig(IniBasedConfig):
+=======
+        fname = self._get_filename()
+        conf_dir = os.path.dirname(fname)
+        ensure_config_dir_exists(conf_dir)
+        f = file(fname, "wb")
+        try:
+            osutils.copy_ownership_from_path(fname)
+            self._get_parser().write(f)
+        finally:
+            f.close()
+>>>>>>> MERGE-SOURCE
+
+
+class LockableConfig(IniBasedConfig):
+    """A configuration needing explicit locking for access.
+
+    If several processes try to write the config file, the accesses need to be
+    serialized.
+
+    Daughter classes should decorate all methods that update a config with the
+    ``@needs_write_lock`` decorator (they call, directly or indirectly, the
+    ``_write_config_file()`` method. These methods (typically ``set_option()``
+    and variants must reload the config file from disk before calling
+    ``_write_config_file()``), this can be achieved by calling the
+    ``self.reload()`` method. Note that the lock scope should cover both the
+    reading and the writing of the config file which is why the decorator can't
+    be applied to ``_write_config_file()`` only.
+
+    This should be enough to implement the following logic:
+    - lock for exclusive write access,
+    - reload the config file from disk,
+    - set the new value
+    - unlock
+
+    This logic guarantees that a writer can update a value without erasing an
+    update made by another writer.
+    """
+
+    lock_name = 'lock'
+
+    def __init__(self, get_filename):
+        super(LockableConfig, self).__init__(get_filename)
+        self.dir = osutils.dirname(osutils.safe_unicode(self._get_filename()))
+        self.transport = transport.get_transport(self.dir)
+        self._lock = None
+
+    def lock_write(self, token=None):
+        """Takes a write lock in the directory containing the config file.
+
+        If the directory doesn't exist it is created.
+        """
+        if self._lock is None:
+            ensure_config_dir_exists(self.dir)
+            self._lock = lockdir.LockDir(self.transport, self.lock_name)
+        self._lock.lock_write(token)
+        return lock.LogicalLockResult(self.unlock)
+
+    def unlock(self):
+        self._lock.unlock()
+
+    def break_lock(self):
+        if self._lock is None:
+            self._lock = lockdir.LockDir(self.transport, self.lock_name)
+        self._lock.break_lock()
+
+    def _write_config_file(self):
+        if self._lock is None or not self._lock.is_held:
+            # NB: if the following exception is raised it probably means a
+            # missing @needs_write_lock decorator on one of the callers.
+            raise errors.ObjectNotLocked(self)
+        fname = self._get_filename()
+        f = StringIO()
+        p = self._get_parser()
+        p.write(f)
+        self.transport.put_bytes(os.path.basename(fname), f.getvalue())
+        # Make sure p.reload() will use the right file name
+        p.filename = fname
+        osutils.copy_ownership_from_path(fname)
+
+
+class GlobalConfig(LockableConfig):
     """The configuration that should be used for a specific location."""
 
-    def get_editor(self):
-        return self._get_user_option('editor')
-
     def __init__(self):
         super(GlobalConfig, self).__init__(config_filename)
 
+    @needs_write_lock
     def set_user_option(self, option, value):
         """Save option and its value in the configuration."""
         self._set_option(option, value, 'DEFAULT')
 
+    def get_editor(self):
+        return self._get_user_option('editor')
+
     def get_aliases(self):
         """Return the aliases section."""
-        if 'ALIASES' in self._get_parser():
-            return self._get_parser()['ALIASES']
+        p = self._get_parser()
+        if 'ALIASES' in p:
+            return p['ALIASES']
         else:
             return {}
 
+    @needs_write_lock
     def set_alias(self, alias_name, alias_command):
         """Save the alias in the configuration."""
         self._set_option(alias_name, alias_command, 'ALIASES')
 
+    @needs_write_lock
     def unset_alias(self, alias_name):
         """Unset an existing alias."""
+        self.reload()
         aliases = self._get_parser().get('ALIASES')
         if not aliases or alias_name not in aliases:
             raise errors.NoSuchAlias(alias_name)
@@ -518,15 +624,12 @@
         self._write_config_file()
 
     def _set_option(self, option, value, section):
-        # FIXME: RBC 20051029 This should refresh the parser and also take a
-        # file lock on bazaar.conf.
-        conf_dir = os.path.dirname(self._get_filename())
-        ensure_config_dir_exists(conf_dir)
+        self.reload()
         self._get_parser().setdefault(section, {})[option] = value
         self._write_config_file()
 
 
-class LocationConfig(IniBasedConfig):
+class LocationConfig(LockableConfig):
     """A configuration object that gives the policy for a location."""
 
     def __init__(self, location):
@@ -642,6 +745,7 @@
             if policy_key in self._get_parser()[section]:
                 del self._get_parser()[section][policy_key]
 
+    @needs_write_lock
     def set_user_option(self, option, value, store=STORE_LOCATION):
         """Save option and its value in the configuration."""
         if store not in [STORE_LOCATION,
@@ -649,19 +753,16 @@
                          STORE_LOCATION_APPENDPATH]:
             raise ValueError('bad storage policy %r for %r' %
                 (store, option))
-        # FIXME: RBC 20051029 This should refresh the parser and also take a
-        # file lock on locations.conf.
-        conf_dir = os.path.dirname(self._get_filename())
-        ensure_config_dir_exists(conf_dir)
+        self.reload()
+        p = self._get_parser()
         location = self.location
         if location.endswith('/'):
             location = location[:-1]
-        if (not location in self._get_parser() and
-            not location + '/' in self._get_parser()):
-            self._get_parser()[location]={}
-        elif location + '/' in self._get_parser():
+        if not location in p and not location + '/' in p:
+            p[location] = {}
+        elif location + '/' in p:
             location = location + '/'
-        self._get_parser()[location][option]=value
+        p[location][option] = value
         # the allowed values of store match the config policies
         self._set_option_policy(location, option, store)
         self._write_config_file()

=== modified file 'bzrlib/tests/blackbox/test_break_lock.py'
--- bzrlib/tests/blackbox/test_break_lock.py	2010-06-11 07:32:12 +0000
+++ bzrlib/tests/blackbox/test_break_lock.py	2010-07-14 08:34:44 +0000
@@ -18,17 +18,18 @@
 
 import os
 
-import bzrlib
 from bzrlib import (
+    branch,
+    bzrdir,
+    config,
     errors,
     lockdir,
+    osutils,
+    tests,
     )
-from bzrlib.branch import Branch
-from bzrlib.bzrdir import BzrDir
-from bzrlib.tests import TestCaseWithTransport
-
-
-class TestBreakLock(TestCaseWithTransport):
+
+
+class TestBreakLock(tests.TestCaseWithTransport):
 
     # General principal for break-lock: All the elements that might be locked
     # by a bzr operation on PATH, are candidates that break-lock may unlock.
@@ -52,14 +53,14 @@
              'repo/',
              'repo/branch/',
              'checkout/'])
-        bzrlib.bzrdir.BzrDir.create('master-repo').create_repository()
-        self.master_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience(
+        bzrdir.BzrDir.create('master-repo').create_repository()
+        self.master_branch = bzrdir.BzrDir.create_branch_convenience(
             'master-repo/master-branch')
-        bzrlib.bzrdir.BzrDir.create('repo').create_repository()
-        local_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience('repo/branch')
+        bzrdir.BzrDir.create('repo').create_repository()
+        local_branch = bzrdir.BzrDir.create_branch_convenience('repo/branch')
         local_branch.bind(self.master_branch)
-        checkoutdir = bzrlib.bzrdir.BzrDir.create('checkout')
-        bzrlib.branch.BranchReferenceFormat().initialize(
+        checkoutdir = bzrdir.BzrDir.create('checkout')
+        branch.BranchReferenceFormat().initialize(
             checkoutdir, target_branch=local_branch)
         self.wt = checkoutdir.create_workingtree()
 
@@ -73,7 +74,7 @@
         # however, we dont test breaking the working tree because we
         # cannot accurately do so right now: the dirstate lock is held
         # by an os lock, and we need to spawn a separate process to lock it
-        # thne kill -9 it.
+        # then kill -9 it.
         # sketch of test:
         # lock most of the dir:
         self.wt.branch.lock_write()
@@ -82,22 +83,45 @@
         # we need 5 yes's - wt, branch, repo, bound branch, bound repo.
         self.run_bzr('break-lock checkout', stdin="y\ny\ny\ny\n")
         # a new tree instance should be lockable
-        branch = bzrlib.branch.Branch.open('checkout')
-        branch.lock_write()
-        branch.unlock()
+        b = branch.Branch.open('checkout')
+        b.lock_write()
+        b.unlock()
         # and a new instance of the master branch
-        mb = branch.get_master_branch()
+        mb = b.get_master_branch()
         mb.lock_write()
         mb.unlock()
         self.assertRaises(errors.LockBroken, self.wt.unlock)
         self.assertRaises(errors.LockBroken, self.master_branch.unlock)
 
 
-class TestBreakLockOldBranch(TestCaseWithTransport):
+class TestBreakLockOldBranch(tests.TestCaseWithTransport):
 
     def test_break_lock_format_5_bzrdir(self):
         # break lock on a format 5 bzrdir should just return
-        self.make_branch_and_tree('foo', format=bzrlib.bzrdir.BzrDirFormat5())
+        self.make_branch_and_tree('foo', format=bzrdir.BzrDirFormat5())
         out, err = self.run_bzr('break-lock foo')
         self.assertEqual('', out)
         self.assertEqual('', err)
+
+
+class TestConfigBreakLock(tests.TestCaseWithTransport):
+
+    def create_pending_lock(self):
+        def config_file_name():
+            return './my.conf'
+        self.build_tree_contents([(config_file_name(), '[DEFAULT]\none=1\n')])
+        conf = config.LockableConfig(config_file_name)
+        conf.lock_write()
+        return conf
+
+    def test_create_pending_lock(self):
+        conf = self.create_pending_lock()
+        self.addCleanup(conf.unlock)
+        self.assertTrue(conf._lock.is_held)
+
+    def test_break_lock(self):
+        conf = self.create_pending_lock()
+        self.run_bzr('break-lock --config %s'
+                     % osutils.dirname(conf._get_filename()),
+                     stdin="y\n")
+        self.assertRaises(errors.LockBroken, conf.unlock)

=== modified file 'bzrlib/tests/test_commands.py'
--- bzrlib/tests/test_commands.py	2010-05-27 21:16:48 +0000
+++ bzrlib/tests/test_commands.py	2010-07-14 08:34:44 +0000
@@ -97,7 +97,7 @@
     def _get_config(self, config_text):
         my_config = config.GlobalConfig()
         config_file = StringIO(config_text.encode('utf-8'))
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_simple(self):

=== modified file 'bzrlib/tests/test_config.py'
--- bzrlib/tests/test_config.py	2010-04-23 07:15:23 +0000
+++ bzrlib/tests/test_config.py	2010-07-14 08:34:44 +0000
@@ -19,6 +19,7 @@
 from cStringIO import StringIO
 import os
 import sys
+import threading
 
 #import bzrlib specific imports here
 from bzrlib import (
@@ -38,6 +39,32 @@
 from bzrlib.util.configobj import configobj
 
 
+def lockable_config_scenarios():
+    return [
+        ('global',
+         {'config_file_name': config.config_filename,
+          'config_class': config.GlobalConfig,
+          'config_args': [],
+          'config_section': 'DEFAULT'}),
+        ('locations',
+         {'config_file_name': config.locations_config_filename,
+          'config_class': config.LocationConfig,
+          'config_args': ['.'],
+          'config_section': '.'}),]
+
+
+def load_tests(standard_tests, module, loader):
+    suite = loader.suiteClass()
+
+    lc_tests, remaining_tests = tests.split_suite_by_condition(
+        standard_tests, tests.condition_isinstance((
+                TestLockableConfig,
+                )))
+    tests.multiply_tests(lc_tests, lockable_config_scenarios(), suite)
+    suite.addTest(remaining_tests)
+    return suite
+
+
 sample_long_alias="log -r-15..-1 --line"
 sample_config_text = u"""
 [DEFAULT]
@@ -129,6 +156,9 @@
         self._calls.append(('keys',))
         return []
 
+    def reload(self):
+        self._calls.append(('reload',))
+
     def write(self, arg):
         self._calls.append(('write',))
 
@@ -371,7 +401,7 @@
 
     def make_config_parser(self, s):
         conf = config.IniBasedConfig(None)
-        parser = conf._get_parser(file=StringIO(s.encode('utf-8')))
+        parser = conf._get_parser(infile=StringIO(s.encode('utf-8')))
         return conf, parser
 
 
@@ -384,16 +414,144 @@
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.IniBasedConfig(None)
         self.failUnless(
-            isinstance(my_config._get_parser(file=config_file),
+            isinstance(my_config._get_parser(infile=config_file),
                         configobj.ConfigObj))
 
     def test_cached(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.IniBasedConfig(None)
-        parser = my_config._get_parser(file=config_file)
+        parser = my_config._get_parser(infile=config_file)
         self.failUnless(my_config._get_parser() is parser)
 
 
+class TestLockableConfig(tests.TestCaseInTempDir):
+
+    # Set by load_tests
+    config_file_name = None
+    config_class = None
+    config_args = None
+    config_section = None
+
+    def setUp(self):
+        super(TestLockableConfig, self).setUp()
+        config.ensure_config_dir_exists()
+        text = '[%s]\none=1\ntwo=2\n' % (self.config_section,)
+        self.build_tree_contents([(self.config_file_name(), text)])
+
+    def create_config(self):
+        return self.config_class(*self.config_args)
+
+    def test_simple_read_access(self):
+        c = self.create_config()
+        self.assertEquals('1', c.get_user_option('one'))
+
+    def test_simple_write_access(self):
+        c = self.create_config()
+        c.set_user_option('one', 'one')
+        self.assertEquals('one', c.get_user_option('one'))
+
+    def test_unlocked_config(self):
+        c = self.create_config()
+        self.assertRaises(errors.ObjectNotLocked, c._write_config_file)
+
+    def test_listen_to_the_last_speaker(self):
+        c1 = self.create_config()
+        c2 = self.create_config()
+        c1.set_user_option('one', 'ONE')
+        c2.set_user_option('two', 'TWO')
+        self.assertEquals('ONE', c1.get_user_option('one'))
+        self.assertEquals('TWO', c2.get_user_option('two'))
+        # The second update respect the first one
+        self.assertEquals('ONE', c2.get_user_option('one'))
+
+    def test_last_speaker_wins(self):
+        # If the same config is not shared, the same variable modified twice
+        # can only see a single result.
+        c1 = self.create_config()
+        c2 = self.create_config()
+        c1.set_user_option('one', 'c1')
+        c2.set_user_option('one', 'c2')
+        self.assertEquals('c2', c2.get_user_option('one'))
+        # The first modification is still available until another refresh
+        # occur
+        self.assertEquals('c1', c1.get_user_option('one'))
+        c1.set_user_option('two', 'done')
+        self.assertEquals('c2', c1.get_user_option('one'))
+
+    def test_writes_are_serialized(self):
+        c1 = self.create_config()
+        c2 = self.create_config()
+
+        # We spawn a thread that will pause *during* the write
+        before_writing = threading.Event()
+        after_writing = threading.Event()
+        writing_done = threading.Event()
+        c1_orig = c1._write_config_file
+        def c1_write_config_file():
+            before_writing.set()
+            c1_orig()
+            # The lock is held we wait for the main thread to decide when to
+            # continue
+            after_writing.wait()
+        c1._write_config_file = c1_write_config_file
+        def c1_set_option():
+            c1.set_user_option('one', 'c1')
+            writing_done.set()
+        t1 = threading.Thread(target=c1_set_option)
+        # Collect the thread after the test
+        self.addCleanup(t1.join)
+        # Be ready to unblock the thread if the test goes wrong
+        self.addCleanup(after_writing.set)
+        t1.start()
+        before_writing.wait()
+        self.assertTrue(c1._lock.is_held)
+        self.assertRaises(errors.LockContention,
+                          c2.set_user_option, 'one', 'c2')
+        self.assertEquals('c1', c1.get_user_option('one'))
+        # Let the lock be released
+        after_writing.set()
+        writing_done.wait()
+        c2.set_user_option('one', 'c2')
+        self.assertEquals('c2', c2.get_user_option('one'))
+
+    def test_read_while_writing(self):
+        c1 = self.create_config()
+        # We spawn a thread that will pause *during* the write
+        ready_to_write = threading.Event()
+        do_writing = threading.Event()
+        writing_done = threading.Event()
+        c1_orig = c1._write_config_file
+        def c1_write_config_file():
+            ready_to_write.set()
+            # The lock is held we wait for the main thread to decide when to
+            # continue
+            do_writing.wait()
+            c1_orig()
+            writing_done.set()
+        c1._write_config_file = c1_write_config_file
+        def c1_set_option():
+            c1.set_user_option('one', 'c1')
+        t1 = threading.Thread(target=c1_set_option)
+        # Collect the thread after the test
+        self.addCleanup(t1.join)
+        # Be ready to unblock the thread if the test goes wrong
+        self.addCleanup(do_writing.set)
+        t1.start()
+        # Ensure the thread is ready to write
+        ready_to_write.wait()
+        self.assertTrue(c1._lock.is_held)
+        self.assertEquals('c1', c1.get_user_option('one'))
+        # If we read during the write, we get the old value
+        c2 = self.create_config()
+        self.assertEquals('1', c2.get_user_option('one'))
+        # Let the writing occur and ensure it occurred
+        do_writing.set()
+        writing_done.wait()
+        # Now we get the updated value
+        c3 = self.create_config()
+        self.assertEquals('c1', c3.get_user_option('one'))
+
+
 class TestGetUserOptionAs(TestIniConfig):
 
     def test_get_user_option_as_bool(self):
@@ -583,26 +741,26 @@
     def test_user_id(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(u"Erik B\u00e5gfors <[e-mail]>",
                          my_config._get_user_id())
 
     def test_absent_user_id(self):
         config_file = StringIO("")
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(None, my_config._get_user_id())
 
     def test_configured_editor(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual("vim", my_config.get_editor())
 
     def test_signatures_always(self):
         config_file = StringIO(sample_always_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_ALWAYS,
@@ -612,7 +770,7 @@
     def test_signatures_if_possible(self):
         config_file = StringIO(sample_maybe_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_NEVER,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_WHEN_REQUIRED,
@@ -622,7 +780,7 @@
     def test_signatures_ignore(self):
         config_file = StringIO(sample_ignore_signatures)
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         self.assertEqual(config.CHECK_ALWAYS,
                          my_config.signature_checking())
         self.assertEqual(config.SIGN_NEVER,
@@ -632,7 +790,7 @@
     def _get_sample_config(self):
         config_file = StringIO(sample_config_text.encode('utf-8'))
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_gpg_signing_command(self):
@@ -643,7 +801,7 @@
     def _get_empty_config(self):
         config_file = StringIO("")
         my_config = config.GlobalConfig()
-        my_config._parser = my_config._get_parser(file=config_file)
+        my_config._parser = my_config._get_parser(infile=config_file)
         return my_config
 
     def test_gpg_signing_command_unset(self):
@@ -745,10 +903,11 @@
                          [('__init__', config.locations_config_filename(),
                            'utf-8')])
         config.ensure_config_dir_exists()
-        #os.mkdir(config.config_dir())
         f = file(config.branches_config_filename(), 'wb')
-        f.write('')
-        f.close()
+        try:
+            f.write('')
+        finally:
+            f.close()
         oldparserclass = config.ConfigObj
         config.ConfigObj = InstrumentedConfigObj
         try:
@@ -995,10 +1154,15 @@
         else:
             global_file = StringIO(global_config.encode('utf-8'))
         branches_file = StringIO(sample_branches_text.encode('utf-8'))
+        # Make sure the locations config can be reloaded
+        config.ensure_config_dir_exists()
+        f = file(config.locations_config_filename(), 'wb')
+        try:
+            f.write(branches_file.getvalue())
+        finally:
+            f.close
         self.my_config = config.BranchConfig(FakeBranch(location))
-        # Force location config to use specified file
         self.my_location_config = self.my_config._get_location_config()
-        self.my_location_config._get_parser(branches_file)
         # Force global config to use specified file
         self.my_config._get_global_config()._get_parser(global_file)
 
@@ -1007,25 +1171,14 @@
         record = InstrumentedConfigObj("foo")
         self.my_location_config._parser = record
 
-        real_mkdir = os.mkdir
-        self.created = False
-        def checked_mkdir(path, mode=0777):
-            self.log('making directory: %s', path)
-            real_mkdir(path, mode)
-            self.created = True
-
-        os.mkdir = checked_mkdir
-        try:
-            self.callDeprecated(['The recurse option is deprecated as of '
-                                 '0.14.  The section "/a/c" has been '
-                                 'converted to use policies.'],
-                                self.my_config.set_user_option,
-                                'foo', 'bar', store=config.STORE_LOCATION)
-        finally:
-            os.mkdir = real_mkdir
-
-        self.failUnless(self.created, 'Failed to create ~/.bazaar')
-        self.assertEqual([('__contains__', '/a/c'),
+        self.callDeprecated(['The recurse option is deprecated as of '
+                             '0.14.  The section "/a/c" has been '
+                             'converted to use policies.'],
+                            self.my_config.set_user_option,
+                            'foo', 'bar', store=config.STORE_LOCATION)
+
+        self.assertEqual([('reload',),
+                          ('__contains__', '/a/c'),
                           ('__contains__', '/a/c/'),
                           ('__setitem__', '/a/c', {}),
                           ('__getitem__', '/a/c'),
@@ -1083,10 +1236,13 @@
         if global_config is not None:
             global_file = StringIO(global_config.encode('utf-8'))
             my_config._get_global_config()._get_parser(global_file)
-        self.my_location_config = my_config._get_location_config()
+        lconf = my_config._get_location_config()
         if location_config is not None:
             location_file = StringIO(location_config.encode('utf-8'))
-            self.my_location_config._get_parser(location_file)
+            lconf._get_parser(location_file)
+            # Make sure the config can be reloaded
+            lconf._parser.filename = config.locations_config_filename()
+        self.my_location_config = lconf
         if branch_data_config is not None:
             my_config.branch.control_files.files['branch.conf'] = \
                 branch_data_config