Code review comment for lp:~vila/bzr/525571-lock-bazaar-conf-files

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