Merge lp:~vila/bzr/525571-lock-bazaar-conf-files into lp:bzr
- 525571-lock-bazaar-conf-files
- Merge into bzr.dev
Status: | Rejected | ||||
---|---|---|---|---|---|
Rejected by: | Vincent Ladeuil | ||||
Proposed branch: | lp:~vila/bzr/525571-lock-bazaar-conf-files | ||||
Merge into: | lp:bzr | ||||
Diff against target: |
893 lines (+410/-101) (has conflicts) 6 files modified
NEWS (+15/-0) bzrlib/builtins.py (+27/-14) bzrlib/config.py (+130/-29) bzrlib/tests/blackbox/test_break_lock.py (+44/-20) bzrlib/tests/test_commands.py (+1/-1) bzrlib/tests/test_config.py (+193/-37) Text conflict in NEWS Text conflict in bzrlib/config.py |
||||
To merge this branch: | bzr merge lp:~vila/bzr/525571-lock-bazaar-conf-files | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
Robert Collins (community) | Needs Fixing | ||
Review via email: mp+28898@code.launchpad.net |
This proposal supersedes a proposal from 2010-06-29.
Commit message
Description of the change
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')
to read a config file.
and
h = open(self.filename, 'wb')
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) wrote : Posted in a previous version of this proposal | # |
Robert Collins (lifeless) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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) wrote : Posted in a previous version of this proposal | # |
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_
153 - f = file(self.
154 + fname = self._get_
155 + conf_dir = os.path.
156 + ensure_
157 + f = file(fname, "wb")
158 try:
159 - osutils.
160 + osutils.
161 self._get_
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_
184 + self._lock = lockdir.
185 + self._lock.
186 + return lock.LogicalLoc
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.
Vincent Ladeuil (vila) wrote : Posted in a previous version of this proposal | # |
> 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_
> 153 - f = file(self.
> 154 + fname = self._get_
> 155 + conf_dir = os.path.
> 156 + ensure_
> 157 + f = file(fname, "wb")
> 158 try:
> 159 - osutils.
> 160 + osutils.
> 161 self._get_
> 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_
>
> 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_
> 184 + self._lock = lockdir.
> self.lock_name)
> 185 + self._lock.
> 186 + return lock.LogicalLoc
>
> 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) wrote : Posted in a previous version of this proposal | # |
> 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.
TestLockableCon
>
> 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) wrote : | # |
-----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://
iEYEARECAAYFAkw
gdYAoMG9ymYJX32
=nu81
-----END PGP SIGNATURE-----
Robert Collins (lifeless) wrote : | # |
16:24 < lifeless> vila: --config please, not --conf
16:25 < lifeless> --conf is a strange abbreviation
16:25 < lifeless> c = config.
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.
Robert Collins (lifeless) wrote : | # |
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) wrote : | # |
> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation
> 16:25 < lifeless> c = config.
> 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_
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) wrote : | # |
> 16:24 < lifeless> vila: --config please, not --conf
> 16:25 < lifeless> --conf is a strange abbreviation
:-P
Robert Collins (lifeless) wrote : | # |
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_
--client 2 writes to the file
conf.set_
--*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.
John Szakmeister (jszakmeister) wrote : | # |
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) wrote : | # |
>>>>> 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_
> --client 2 writes to the file
> conf.set_
> --*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) wrote : | # |
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) wrote : | # |
-----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://
iEYEARECAAYFAkw
6r8An3u2enhCMDQ
=Kmd7
-----END PGP SIGNATURE-----
- 5334. By Vincent Ladeuil
-
Clarify lock scope.
- 5335. By Vincent Ladeuil
-
Use --config instead of --conf for break-lock.
Vincent Ladeuil (vila) wrote : | # |
Superseded by the loom ending in https:/
Unmerged revisions
- 5335. By Vincent Ladeuil
-
Use --config instead of --conf for break-lock.
- 5334. By Vincent Ladeuil
-
Clarify lock scope.
- 5333. By Vincent Ladeuil
-
Final cleanup.
- 5332. By Vincent Ladeuil
-
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
-
Add a test to help LockableConfig daughter classes identify methods that needs to be decorated.
- 5330. By Vincent Ladeuil
-
Further clarify NEWS entry.
- 5329. By Vincent Ladeuil
-
Fix docstring.
- 5328. By Vincent Ladeuil
-
Fix wrong bug number and clarify NEWS entries.
- 5327. By Vincent Ladeuil
-
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
-
Add some comments.
Preview Diff
1 | === modified file 'NEWS' | |||
2 | --- NEWS 2010-07-13 21:25:03 +0000 | |||
3 | +++ NEWS 2010-07-14 08:34:44 +0000 | |||
4 | @@ -119,6 +119,15 @@ | |||
5 | 119 | New Features | 119 | New Features |
6 | 120 | ************ | 120 | ************ |
7 | 121 | 121 | ||
8 | 122 | * ``bzr break-lock --config [location]`` can now break config files | ||
9 | 123 | locks. (Vincent Ladeuil, #525571) | ||
10 | 124 | |||
11 | 125 | * ``bzrlib.config.LockableConfig`` is a base class for config files that | ||
12 | 126 | needs to be protected against multiple writers. All methods that | ||
13 | 127 | change a configuration variable value must be decorated with | ||
14 | 128 | @needs_write_lock (set_option() for example). | ||
15 | 129 | (Vincent Ladeuil, #525571) | ||
16 | 130 | |||
17 | 122 | * Support ``--directory`` option for a number of additional commands: | 131 | * Support ``--directory`` option for a number of additional commands: |
18 | 123 | conflicts, merge-directive, missing, resolve, shelve, switch, | 132 | conflicts, merge-directive, missing, resolve, shelve, switch, |
19 | 124 | unshelve, whoami. (Martin von Gagern, #527878) | 133 | unshelve, whoami. (Martin von Gagern, #527878) |
20 | @@ -145,6 +154,7 @@ | |||
21 | 145 | or pull location in locations.conf or branch.conf. | 154 | or pull location in locations.conf or branch.conf. |
22 | 146 | (Gordon Tyler, #534787) | 155 | (Gordon Tyler, #534787) |
23 | 147 | 156 | ||
24 | 157 | <<<<<<< TREE | ||
25 | 148 | * ``bzr reconfigure --unstacked`` now works with branches accessed via a | 158 | * ``bzr reconfigure --unstacked`` now works with branches accessed via a |
26 | 149 | smart server. (Andrew Bennetts, #551525) | 159 | smart server. (Andrew Bennetts, #551525) |
27 | 150 | 160 | ||
28 | @@ -154,6 +164,11 @@ | |||
29 | 154 | * ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied. | 164 | * ``BzrDir.find_bzrdirs`` should ignore dirs that raises PermissionDenied. |
30 | 155 | (Marius Kruger, Robert Collins) | 165 | (Marius Kruger, Robert Collins) |
31 | 156 | 166 | ||
32 | 167 | ======= | ||
33 | 168 | * Configuration files in ``${BZR_HOME}`` are now protected | ||
34 | 169 | against concurrent writers. (Vincent Ladeuil, #525571) | ||
35 | 170 | |||
36 | 171 | >>>>>>> MERGE-SOURCE | ||
37 | 157 | * Ensure that wrong path specifications in ``BZR_PLUGINS_AT`` display | 172 | * Ensure that wrong path specifications in ``BZR_PLUGINS_AT`` display |
38 | 158 | proper error messages. (Vincent Ladeuil, #591215) | 173 | proper error messages. (Vincent Ladeuil, #591215) |
39 | 159 | 174 | ||
40 | 160 | 175 | ||
41 | === modified file 'bzrlib/builtins.py' | |||
42 | --- bzrlib/builtins.py 2010-06-30 12:13:14 +0000 | |||
43 | +++ bzrlib/builtins.py 2010-07-14 08:34:44 +0000 | |||
44 | @@ -33,7 +33,7 @@ | |||
45 | 33 | bzrdir, | 33 | bzrdir, |
46 | 34 | directory_service, | 34 | directory_service, |
47 | 35 | delta, | 35 | delta, |
49 | 36 | config, | 36 | config as _mod_config, |
50 | 37 | errors, | 37 | errors, |
51 | 38 | globbing, | 38 | globbing, |
52 | 39 | hooks, | 39 | hooks, |
53 | @@ -3332,7 +3332,7 @@ | |||
54 | 3332 | try: | 3332 | try: |
55 | 3333 | c = Branch.open_containing(u'.')[0].get_config() | 3333 | c = Branch.open_containing(u'.')[0].get_config() |
56 | 3334 | except errors.NotBranchError: | 3334 | except errors.NotBranchError: |
58 | 3335 | c = config.GlobalConfig() | 3335 | c = _mod_config.GlobalConfig() |
59 | 3336 | else: | 3336 | else: |
60 | 3337 | c = Branch.open(directory).get_config() | 3337 | c = Branch.open(directory).get_config() |
61 | 3338 | if email: | 3338 | if email: |
62 | @@ -3343,7 +3343,7 @@ | |||
63 | 3343 | 3343 | ||
64 | 3344 | # display a warning if an email address isn't included in the given name. | 3344 | # display a warning if an email address isn't included in the given name. |
65 | 3345 | try: | 3345 | try: |
67 | 3346 | config.extract_email_address(name) | 3346 | _mod_config.extract_email_address(name) |
68 | 3347 | except errors.NoEmailInUsername, e: | 3347 | except errors.NoEmailInUsername, e: |
69 | 3348 | warning('"%s" does not seem to contain an email address. ' | 3348 | warning('"%s" does not seem to contain an email address. ' |
70 | 3349 | 'This is allowed, but not recommended.', name) | 3349 | 'This is allowed, but not recommended.', name) |
71 | @@ -3355,7 +3355,7 @@ | |||
72 | 3355 | else: | 3355 | else: |
73 | 3356 | c = Branch.open(directory).get_config() | 3356 | c = Branch.open(directory).get_config() |
74 | 3357 | else: | 3357 | else: |
76 | 3358 | c = config.GlobalConfig() | 3358 | c = _mod_config.GlobalConfig() |
77 | 3359 | c.set_user_option('email', name) | 3359 | c.set_user_option('email', name) |
78 | 3360 | 3360 | ||
79 | 3361 | 3361 | ||
80 | @@ -3428,13 +3428,13 @@ | |||
81 | 3428 | 'bzr alias --remove expects an alias to remove.') | 3428 | 'bzr alias --remove expects an alias to remove.') |
82 | 3429 | # If alias is not found, print something like: | 3429 | # If alias is not found, print something like: |
83 | 3430 | # unalias: foo: not found | 3430 | # unalias: foo: not found |
85 | 3431 | c = config.GlobalConfig() | 3431 | c = _mod_config.GlobalConfig() |
86 | 3432 | c.unset_alias(alias_name) | 3432 | c.unset_alias(alias_name) |
87 | 3433 | 3433 | ||
88 | 3434 | @display_command | 3434 | @display_command |
89 | 3435 | def print_aliases(self): | 3435 | def print_aliases(self): |
90 | 3436 | """Print out the defined aliases in a similar format to bash.""" | 3436 | """Print out the defined aliases in a similar format to bash.""" |
92 | 3437 | aliases = config.GlobalConfig().get_aliases() | 3437 | aliases = _mod_config.GlobalConfig().get_aliases() |
93 | 3438 | for key, value in sorted(aliases.iteritems()): | 3438 | for key, value in sorted(aliases.iteritems()): |
94 | 3439 | self.outf.write('bzr alias %s="%s"\n' % (key, value)) | 3439 | self.outf.write('bzr alias %s="%s"\n' % (key, value)) |
95 | 3440 | 3440 | ||
96 | @@ -3450,7 +3450,7 @@ | |||
97 | 3450 | 3450 | ||
98 | 3451 | def set_alias(self, alias_name, alias_command): | 3451 | def set_alias(self, alias_name, alias_command): |
99 | 3452 | """Save the alias in the global config.""" | 3452 | """Save the alias in the global config.""" |
101 | 3453 | c = config.GlobalConfig() | 3453 | c = _mod_config.GlobalConfig() |
102 | 3454 | c.set_alias(alias_name, alias_command) | 3454 | c.set_alias(alias_name, alias_command) |
103 | 3455 | 3455 | ||
104 | 3456 | 3456 | ||
105 | @@ -4829,7 +4829,10 @@ | |||
106 | 4829 | 4829 | ||
107 | 4830 | 4830 | ||
108 | 4831 | class cmd_break_lock(Command): | 4831 | class cmd_break_lock(Command): |
110 | 4832 | __doc__ = """Break a dead lock on a repository, branch or working directory. | 4832 | __doc__ = """Break a dead lock. |
111 | 4833 | |||
112 | 4834 | This command breaks a lock on a repository, branch, working directory or | ||
113 | 4835 | config file. | ||
114 | 4833 | 4836 | ||
115 | 4834 | CAUTION: Locks should only be broken when you are sure that the process | 4837 | CAUTION: Locks should only be broken when you are sure that the process |
116 | 4835 | holding the lock has been stopped. | 4838 | holding the lock has been stopped. |
117 | @@ -4840,17 +4843,27 @@ | |||
118 | 4840 | :Examples: | 4843 | :Examples: |
119 | 4841 | bzr break-lock | 4844 | bzr break-lock |
120 | 4842 | bzr break-lock bzr+ssh://example.com/bzr/foo | 4845 | bzr break-lock bzr+ssh://example.com/bzr/foo |
121 | 4846 | bzr break-lock --conf ~/.bazaar | ||
122 | 4843 | """ | 4847 | """ |
123 | 4848 | |||
124 | 4844 | takes_args = ['location?'] | 4849 | takes_args = ['location?'] |
125 | 4850 | takes_options = [ | ||
126 | 4851 | Option('config', | ||
127 | 4852 | help='LOCATION is the directory where the config lock is.'), | ||
128 | 4853 | ] | ||
129 | 4845 | 4854 | ||
131 | 4846 | def run(self, location=None, show=False): | 4855 | def run(self, location=None, config=False): |
132 | 4847 | if location is None: | 4856 | if location is None: |
133 | 4848 | location = u'.' | 4857 | location = u'.' |
139 | 4849 | control, relpath = bzrdir.BzrDir.open_containing(location) | 4858 | if config: |
140 | 4850 | try: | 4859 | conf = _mod_config.LockableConfig(lambda : location) |
141 | 4851 | control.break_lock() | 4860 | conf.break_lock() |
142 | 4852 | except NotImplementedError: | 4861 | else: |
143 | 4853 | pass | 4862 | control, relpath = bzrdir.BzrDir.open_containing(location) |
144 | 4863 | try: | ||
145 | 4864 | control.break_lock() | ||
146 | 4865 | except NotImplementedError: | ||
147 | 4866 | pass | ||
148 | 4854 | 4867 | ||
149 | 4855 | 4868 | ||
150 | 4856 | class cmd_wait_until_signalled(Command): | 4869 | class cmd_wait_until_signalled(Command): |
151 | 4857 | 4870 | ||
152 | === modified file 'bzrlib/config.py' | |||
153 | --- bzrlib/config.py 2010-07-13 16:10:20 +0000 | |||
154 | +++ bzrlib/config.py 2010-07-14 08:34:44 +0000 | |||
155 | @@ -62,9 +62,11 @@ | |||
156 | 62 | up=pull | 62 | up=pull |
157 | 63 | """ | 63 | """ |
158 | 64 | 64 | ||
159 | 65 | from cStringIO import StringIO | ||
160 | 65 | import os | 66 | import os |
161 | 66 | import sys | 67 | import sys |
162 | 67 | 68 | ||
163 | 69 | from bzrlib.decorators import needs_write_lock | ||
164 | 68 | from bzrlib.lazy_import import lazy_import | 70 | from bzrlib.lazy_import import lazy_import |
165 | 69 | lazy_import(globals(), """ | 71 | lazy_import(globals(), """ |
166 | 70 | import errno | 72 | import errno |
167 | @@ -77,11 +79,14 @@ | |||
168 | 77 | atomicfile, | 79 | atomicfile, |
169 | 78 | debug, | 80 | debug, |
170 | 79 | errors, | 81 | errors, |
171 | 82 | lock, | ||
172 | 83 | lockdir, | ||
173 | 80 | mail_client, | 84 | mail_client, |
174 | 81 | osutils, | 85 | osutils, |
175 | 82 | registry, | 86 | registry, |
176 | 83 | symbol_versioning, | 87 | symbol_versioning, |
177 | 84 | trace, | 88 | trace, |
178 | 89 | transport, | ||
179 | 85 | ui, | 90 | ui, |
180 | 86 | urlutils, | 91 | urlutils, |
181 | 87 | win32utils, | 92 | win32utils, |
182 | @@ -357,18 +362,35 @@ | |||
183 | 357 | self._get_filename = get_filename | 362 | self._get_filename = get_filename |
184 | 358 | self._parser = None | 363 | self._parser = None |
185 | 359 | 364 | ||
187 | 360 | def _get_parser(self, file=None): | 365 | def _get_parser(self, infile=None): |
188 | 361 | if self._parser is not None: | 366 | if self._parser is not None: |
189 | 362 | return self._parser | 367 | return self._parser |
194 | 363 | if file is None: | 368 | # Do we have a file name for the config file |
195 | 364 | input = self._get_filename() | 369 | if self._get_filename is None: |
196 | 365 | else: | 370 | fname = None |
197 | 366 | input = file | 371 | else: |
198 | 372 | fname = self._get_filename() | ||
199 | 373 | # Do we have a content for the config file ? | ||
200 | 374 | if infile is None: | ||
201 | 375 | fname_or_content = fname | ||
202 | 376 | else: | ||
203 | 377 | fname_or_content = infile | ||
204 | 378 | # Build the ConfigObj | ||
205 | 379 | p = None | ||
206 | 367 | try: | 380 | try: |
208 | 368 | self._parser = ConfigObj(input, encoding='utf-8') | 381 | p = ConfigObj(fname_or_content, encoding='utf-8') |
209 | 369 | except configobj.ConfigObjError, e: | 382 | except configobj.ConfigObjError, e: |
210 | 370 | raise errors.ParseConfigError(e.errors, e.config.filename) | 383 | raise errors.ParseConfigError(e.errors, e.config.filename) |
212 | 371 | return self._parser | 384 | if p is not None and fname is not None: |
213 | 385 | # Make sure p.reload() will use the right file name | ||
214 | 386 | p.filename = fname | ||
215 | 387 | self._parser = p | ||
216 | 388 | return p | ||
217 | 389 | |||
218 | 390 | def reload(self): | ||
219 | 391 | """Reload the config file from disk.""" | ||
220 | 392 | if self._parser is not None: | ||
221 | 393 | self._parser.reload() | ||
222 | 372 | 394 | ||
223 | 373 | def _get_matching_sections(self): | 395 | def _get_matching_sections(self): |
224 | 374 | """Return an ordered list of (section_name, extra_path) pairs. | 396 | """Return an ordered list of (section_name, extra_path) pairs. |
225 | @@ -479,38 +501,122 @@ | |||
226 | 479 | return self.get_user_option('nickname') | 501 | return self.get_user_option('nickname') |
227 | 480 | 502 | ||
228 | 481 | def _write_config_file(self): | 503 | def _write_config_file(self): |
229 | 504 | <<<<<<< TREE | ||
230 | 482 | atomic_file = atomicfile.AtomicFile(self._get_filename()) | 505 | atomic_file = atomicfile.AtomicFile(self._get_filename()) |
231 | 483 | self._get_parser().write(atomic_file) | 506 | self._get_parser().write(atomic_file) |
232 | 484 | atomic_file.commit() | 507 | atomic_file.commit() |
233 | 485 | atomic_file.close() | 508 | atomic_file.close() |
237 | 486 | 509 | ======= | |
238 | 487 | 510 | fname = self._get_filename() | |
239 | 488 | class GlobalConfig(IniBasedConfig): | 511 | conf_dir = os.path.dirname(fname) |
240 | 512 | ensure_config_dir_exists(conf_dir) | ||
241 | 513 | f = file(fname, "wb") | ||
242 | 514 | try: | ||
243 | 515 | osutils.copy_ownership_from_path(fname) | ||
244 | 516 | self._get_parser().write(f) | ||
245 | 517 | finally: | ||
246 | 518 | f.close() | ||
247 | 519 | >>>>>>> MERGE-SOURCE | ||
248 | 520 | |||
249 | 521 | |||
250 | 522 | class LockableConfig(IniBasedConfig): | ||
251 | 523 | """A configuration needing explicit locking for access. | ||
252 | 524 | |||
253 | 525 | If several processes try to write the config file, the accesses need to be | ||
254 | 526 | serialized. | ||
255 | 527 | |||
256 | 528 | Daughter classes should decorate all methods that update a config with the | ||
257 | 529 | ``@needs_write_lock`` decorator (they call, directly or indirectly, the | ||
258 | 530 | ``_write_config_file()`` method. These methods (typically ``set_option()`` | ||
259 | 531 | and variants must reload the config file from disk before calling | ||
260 | 532 | ``_write_config_file()``), this can be achieved by calling the | ||
261 | 533 | ``self.reload()`` method. Note that the lock scope should cover both the | ||
262 | 534 | reading and the writing of the config file which is why the decorator can't | ||
263 | 535 | be applied to ``_write_config_file()`` only. | ||
264 | 536 | |||
265 | 537 | This should be enough to implement the following logic: | ||
266 | 538 | - lock for exclusive write access, | ||
267 | 539 | - reload the config file from disk, | ||
268 | 540 | - set the new value | ||
269 | 541 | - unlock | ||
270 | 542 | |||
271 | 543 | This logic guarantees that a writer can update a value without erasing an | ||
272 | 544 | update made by another writer. | ||
273 | 545 | """ | ||
274 | 546 | |||
275 | 547 | lock_name = 'lock' | ||
276 | 548 | |||
277 | 549 | def __init__(self, get_filename): | ||
278 | 550 | super(LockableConfig, self).__init__(get_filename) | ||
279 | 551 | self.dir = osutils.dirname(osutils.safe_unicode(self._get_filename())) | ||
280 | 552 | self.transport = transport.get_transport(self.dir) | ||
281 | 553 | self._lock = None | ||
282 | 554 | |||
283 | 555 | def lock_write(self, token=None): | ||
284 | 556 | """Takes a write lock in the directory containing the config file. | ||
285 | 557 | |||
286 | 558 | If the directory doesn't exist it is created. | ||
287 | 559 | """ | ||
288 | 560 | if self._lock is None: | ||
289 | 561 | ensure_config_dir_exists(self.dir) | ||
290 | 562 | self._lock = lockdir.LockDir(self.transport, self.lock_name) | ||
291 | 563 | self._lock.lock_write(token) | ||
292 | 564 | return lock.LogicalLockResult(self.unlock) | ||
293 | 565 | |||
294 | 566 | def unlock(self): | ||
295 | 567 | self._lock.unlock() | ||
296 | 568 | |||
297 | 569 | def break_lock(self): | ||
298 | 570 | if self._lock is None: | ||
299 | 571 | self._lock = lockdir.LockDir(self.transport, self.lock_name) | ||
300 | 572 | self._lock.break_lock() | ||
301 | 573 | |||
302 | 574 | def _write_config_file(self): | ||
303 | 575 | if self._lock is None or not self._lock.is_held: | ||
304 | 576 | # NB: if the following exception is raised it probably means a | ||
305 | 577 | # missing @needs_write_lock decorator on one of the callers. | ||
306 | 578 | raise errors.ObjectNotLocked(self) | ||
307 | 579 | fname = self._get_filename() | ||
308 | 580 | f = StringIO() | ||
309 | 581 | p = self._get_parser() | ||
310 | 582 | p.write(f) | ||
311 | 583 | self.transport.put_bytes(os.path.basename(fname), f.getvalue()) | ||
312 | 584 | # Make sure p.reload() will use the right file name | ||
313 | 585 | p.filename = fname | ||
314 | 586 | osutils.copy_ownership_from_path(fname) | ||
315 | 587 | |||
316 | 588 | |||
317 | 589 | class GlobalConfig(LockableConfig): | ||
318 | 489 | """The configuration that should be used for a specific location.""" | 590 | """The configuration that should be used for a specific location.""" |
319 | 490 | 591 | ||
320 | 491 | def get_editor(self): | ||
321 | 492 | return self._get_user_option('editor') | ||
322 | 493 | |||
323 | 494 | def __init__(self): | 592 | def __init__(self): |
324 | 495 | super(GlobalConfig, self).__init__(config_filename) | 593 | super(GlobalConfig, self).__init__(config_filename) |
325 | 496 | 594 | ||
326 | 595 | @needs_write_lock | ||
327 | 497 | def set_user_option(self, option, value): | 596 | def set_user_option(self, option, value): |
328 | 498 | """Save option and its value in the configuration.""" | 597 | """Save option and its value in the configuration.""" |
329 | 499 | self._set_option(option, value, 'DEFAULT') | 598 | self._set_option(option, value, 'DEFAULT') |
330 | 500 | 599 | ||
331 | 600 | def get_editor(self): | ||
332 | 601 | return self._get_user_option('editor') | ||
333 | 602 | |||
334 | 501 | def get_aliases(self): | 603 | def get_aliases(self): |
335 | 502 | """Return the aliases section.""" | 604 | """Return the aliases section.""" |
338 | 503 | if 'ALIASES' in self._get_parser(): | 605 | p = self._get_parser() |
339 | 504 | return self._get_parser()['ALIASES'] | 606 | if 'ALIASES' in p: |
340 | 607 | return p['ALIASES'] | ||
341 | 505 | else: | 608 | else: |
342 | 506 | return {} | 609 | return {} |
343 | 507 | 610 | ||
344 | 611 | @needs_write_lock | ||
345 | 508 | def set_alias(self, alias_name, alias_command): | 612 | def set_alias(self, alias_name, alias_command): |
346 | 509 | """Save the alias in the configuration.""" | 613 | """Save the alias in the configuration.""" |
347 | 510 | self._set_option(alias_name, alias_command, 'ALIASES') | 614 | self._set_option(alias_name, alias_command, 'ALIASES') |
348 | 511 | 615 | ||
349 | 616 | @needs_write_lock | ||
350 | 512 | def unset_alias(self, alias_name): | 617 | def unset_alias(self, alias_name): |
351 | 513 | """Unset an existing alias.""" | 618 | """Unset an existing alias.""" |
352 | 619 | self.reload() | ||
353 | 514 | aliases = self._get_parser().get('ALIASES') | 620 | aliases = self._get_parser().get('ALIASES') |
354 | 515 | if not aliases or alias_name not in aliases: | 621 | if not aliases or alias_name not in aliases: |
355 | 516 | raise errors.NoSuchAlias(alias_name) | 622 | raise errors.NoSuchAlias(alias_name) |
356 | @@ -518,15 +624,12 @@ | |||
357 | 518 | self._write_config_file() | 624 | self._write_config_file() |
358 | 519 | 625 | ||
359 | 520 | def _set_option(self, option, value, section): | 626 | def _set_option(self, option, value, section): |
364 | 521 | # FIXME: RBC 20051029 This should refresh the parser and also take a | 627 | self.reload() |
361 | 522 | # file lock on bazaar.conf. | ||
362 | 523 | conf_dir = os.path.dirname(self._get_filename()) | ||
363 | 524 | ensure_config_dir_exists(conf_dir) | ||
365 | 525 | self._get_parser().setdefault(section, {})[option] = value | 628 | self._get_parser().setdefault(section, {})[option] = value |
366 | 526 | self._write_config_file() | 629 | self._write_config_file() |
367 | 527 | 630 | ||
368 | 528 | 631 | ||
370 | 529 | class LocationConfig(IniBasedConfig): | 632 | class LocationConfig(LockableConfig): |
371 | 530 | """A configuration object that gives the policy for a location.""" | 633 | """A configuration object that gives the policy for a location.""" |
372 | 531 | 634 | ||
373 | 532 | def __init__(self, location): | 635 | def __init__(self, location): |
374 | @@ -642,6 +745,7 @@ | |||
375 | 642 | if policy_key in self._get_parser()[section]: | 745 | if policy_key in self._get_parser()[section]: |
376 | 643 | del self._get_parser()[section][policy_key] | 746 | del self._get_parser()[section][policy_key] |
377 | 644 | 747 | ||
378 | 748 | @needs_write_lock | ||
379 | 645 | def set_user_option(self, option, value, store=STORE_LOCATION): | 749 | def set_user_option(self, option, value, store=STORE_LOCATION): |
380 | 646 | """Save option and its value in the configuration.""" | 750 | """Save option and its value in the configuration.""" |
381 | 647 | if store not in [STORE_LOCATION, | 751 | if store not in [STORE_LOCATION, |
382 | @@ -649,19 +753,16 @@ | |||
383 | 649 | STORE_LOCATION_APPENDPATH]: | 753 | STORE_LOCATION_APPENDPATH]: |
384 | 650 | raise ValueError('bad storage policy %r for %r' % | 754 | raise ValueError('bad storage policy %r for %r' % |
385 | 651 | (store, option)) | 755 | (store, option)) |
390 | 652 | # FIXME: RBC 20051029 This should refresh the parser and also take a | 756 | self.reload() |
391 | 653 | # file lock on locations.conf. | 757 | p = self._get_parser() |
388 | 654 | conf_dir = os.path.dirname(self._get_filename()) | ||
389 | 655 | ensure_config_dir_exists(conf_dir) | ||
392 | 656 | location = self.location | 758 | location = self.location |
393 | 657 | if location.endswith('/'): | 759 | if location.endswith('/'): |
394 | 658 | location = location[:-1] | 760 | location = location[:-1] |
399 | 659 | if (not location in self._get_parser() and | 761 | if not location in p and not location + '/' in p: |
400 | 660 | not location + '/' in self._get_parser()): | 762 | p[location] = {} |
401 | 661 | self._get_parser()[location]={} | 763 | elif location + '/' in p: |
398 | 662 | elif location + '/' in self._get_parser(): | ||
402 | 663 | location = location + '/' | 764 | location = location + '/' |
404 | 664 | self._get_parser()[location][option]=value | 765 | p[location][option] = value |
405 | 665 | # the allowed values of store match the config policies | 766 | # the allowed values of store match the config policies |
406 | 666 | self._set_option_policy(location, option, store) | 767 | self._set_option_policy(location, option, store) |
407 | 667 | self._write_config_file() | 768 | self._write_config_file() |
408 | 668 | 769 | ||
409 | === modified file 'bzrlib/tests/blackbox/test_break_lock.py' | |||
410 | --- bzrlib/tests/blackbox/test_break_lock.py 2010-06-11 07:32:12 +0000 | |||
411 | +++ bzrlib/tests/blackbox/test_break_lock.py 2010-07-14 08:34:44 +0000 | |||
412 | @@ -18,17 +18,18 @@ | |||
413 | 18 | 18 | ||
414 | 19 | import os | 19 | import os |
415 | 20 | 20 | ||
416 | 21 | import bzrlib | ||
417 | 22 | from bzrlib import ( | 21 | from bzrlib import ( |
418 | 22 | branch, | ||
419 | 23 | bzrdir, | ||
420 | 24 | config, | ||
421 | 23 | errors, | 25 | errors, |
422 | 24 | lockdir, | 26 | lockdir, |
423 | 27 | osutils, | ||
424 | 28 | tests, | ||
425 | 25 | ) | 29 | ) |
432 | 26 | from bzrlib.branch import Branch | 30 | |
433 | 27 | from bzrlib.bzrdir import BzrDir | 31 | |
434 | 28 | from bzrlib.tests import TestCaseWithTransport | 32 | class TestBreakLock(tests.TestCaseWithTransport): |
429 | 29 | |||
430 | 30 | |||
431 | 31 | class TestBreakLock(TestCaseWithTransport): | ||
435 | 32 | 33 | ||
436 | 33 | # General principal for break-lock: All the elements that might be locked | 34 | # General principal for break-lock: All the elements that might be locked |
437 | 34 | # by a bzr operation on PATH, are candidates that break-lock may unlock. | 35 | # by a bzr operation on PATH, are candidates that break-lock may unlock. |
438 | @@ -52,14 +53,14 @@ | |||
439 | 52 | 'repo/', | 53 | 'repo/', |
440 | 53 | 'repo/branch/', | 54 | 'repo/branch/', |
441 | 54 | 'checkout/']) | 55 | 'checkout/']) |
444 | 55 | bzrlib.bzrdir.BzrDir.create('master-repo').create_repository() | 56 | bzrdir.BzrDir.create('master-repo').create_repository() |
445 | 56 | self.master_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience( | 57 | self.master_branch = bzrdir.BzrDir.create_branch_convenience( |
446 | 57 | 'master-repo/master-branch') | 58 | 'master-repo/master-branch') |
449 | 58 | bzrlib.bzrdir.BzrDir.create('repo').create_repository() | 59 | bzrdir.BzrDir.create('repo').create_repository() |
450 | 59 | local_branch = bzrlib.bzrdir.BzrDir.create_branch_convenience('repo/branch') | 60 | local_branch = bzrdir.BzrDir.create_branch_convenience('repo/branch') |
451 | 60 | local_branch.bind(self.master_branch) | 61 | local_branch.bind(self.master_branch) |
454 | 61 | checkoutdir = bzrlib.bzrdir.BzrDir.create('checkout') | 62 | checkoutdir = bzrdir.BzrDir.create('checkout') |
455 | 62 | bzrlib.branch.BranchReferenceFormat().initialize( | 63 | branch.BranchReferenceFormat().initialize( |
456 | 63 | checkoutdir, target_branch=local_branch) | 64 | checkoutdir, target_branch=local_branch) |
457 | 64 | self.wt = checkoutdir.create_workingtree() | 65 | self.wt = checkoutdir.create_workingtree() |
458 | 65 | 66 | ||
459 | @@ -73,7 +74,7 @@ | |||
460 | 73 | # however, we dont test breaking the working tree because we | 74 | # however, we dont test breaking the working tree because we |
461 | 74 | # cannot accurately do so right now: the dirstate lock is held | 75 | # cannot accurately do so right now: the dirstate lock is held |
462 | 75 | # by an os lock, and we need to spawn a separate process to lock it | 76 | # by an os lock, and we need to spawn a separate process to lock it |
464 | 76 | # thne kill -9 it. | 77 | # then kill -9 it. |
465 | 77 | # sketch of test: | 78 | # sketch of test: |
466 | 78 | # lock most of the dir: | 79 | # lock most of the dir: |
467 | 79 | self.wt.branch.lock_write() | 80 | self.wt.branch.lock_write() |
468 | @@ -82,22 +83,45 @@ | |||
469 | 82 | # we need 5 yes's - wt, branch, repo, bound branch, bound repo. | 83 | # we need 5 yes's - wt, branch, repo, bound branch, bound repo. |
470 | 83 | self.run_bzr('break-lock checkout', stdin="y\ny\ny\ny\n") | 84 | self.run_bzr('break-lock checkout', stdin="y\ny\ny\ny\n") |
471 | 84 | # a new tree instance should be lockable | 85 | # a new tree instance should be lockable |
475 | 85 | branch = bzrlib.branch.Branch.open('checkout') | 86 | b = branch.Branch.open('checkout') |
476 | 86 | branch.lock_write() | 87 | b.lock_write() |
477 | 87 | branch.unlock() | 88 | b.unlock() |
478 | 88 | # and a new instance of the master branch | 89 | # and a new instance of the master branch |
480 | 89 | mb = branch.get_master_branch() | 90 | mb = b.get_master_branch() |
481 | 90 | mb.lock_write() | 91 | mb.lock_write() |
482 | 91 | mb.unlock() | 92 | mb.unlock() |
483 | 92 | self.assertRaises(errors.LockBroken, self.wt.unlock) | 93 | self.assertRaises(errors.LockBroken, self.wt.unlock) |
484 | 93 | self.assertRaises(errors.LockBroken, self.master_branch.unlock) | 94 | self.assertRaises(errors.LockBroken, self.master_branch.unlock) |
485 | 94 | 95 | ||
486 | 95 | 96 | ||
488 | 96 | class TestBreakLockOldBranch(TestCaseWithTransport): | 97 | class TestBreakLockOldBranch(tests.TestCaseWithTransport): |
489 | 97 | 98 | ||
490 | 98 | def test_break_lock_format_5_bzrdir(self): | 99 | def test_break_lock_format_5_bzrdir(self): |
491 | 99 | # break lock on a format 5 bzrdir should just return | 100 | # break lock on a format 5 bzrdir should just return |
493 | 100 | self.make_branch_and_tree('foo', format=bzrlib.bzrdir.BzrDirFormat5()) | 101 | self.make_branch_and_tree('foo', format=bzrdir.BzrDirFormat5()) |
494 | 101 | out, err = self.run_bzr('break-lock foo') | 102 | out, err = self.run_bzr('break-lock foo') |
495 | 102 | self.assertEqual('', out) | 103 | self.assertEqual('', out) |
496 | 103 | self.assertEqual('', err) | 104 | self.assertEqual('', err) |
497 | 105 | |||
498 | 106 | |||
499 | 107 | class TestConfigBreakLock(tests.TestCaseWithTransport): | ||
500 | 108 | |||
501 | 109 | def create_pending_lock(self): | ||
502 | 110 | def config_file_name(): | ||
503 | 111 | return './my.conf' | ||
504 | 112 | self.build_tree_contents([(config_file_name(), '[DEFAULT]\none=1\n')]) | ||
505 | 113 | conf = config.LockableConfig(config_file_name) | ||
506 | 114 | conf.lock_write() | ||
507 | 115 | return conf | ||
508 | 116 | |||
509 | 117 | def test_create_pending_lock(self): | ||
510 | 118 | conf = self.create_pending_lock() | ||
511 | 119 | self.addCleanup(conf.unlock) | ||
512 | 120 | self.assertTrue(conf._lock.is_held) | ||
513 | 121 | |||
514 | 122 | def test_break_lock(self): | ||
515 | 123 | conf = self.create_pending_lock() | ||
516 | 124 | self.run_bzr('break-lock --config %s' | ||
517 | 125 | % osutils.dirname(conf._get_filename()), | ||
518 | 126 | stdin="y\n") | ||
519 | 127 | self.assertRaises(errors.LockBroken, conf.unlock) | ||
520 | 104 | 128 | ||
521 | === modified file 'bzrlib/tests/test_commands.py' | |||
522 | --- bzrlib/tests/test_commands.py 2010-05-27 21:16:48 +0000 | |||
523 | +++ bzrlib/tests/test_commands.py 2010-07-14 08:34:44 +0000 | |||
524 | @@ -97,7 +97,7 @@ | |||
525 | 97 | def _get_config(self, config_text): | 97 | def _get_config(self, config_text): |
526 | 98 | my_config = config.GlobalConfig() | 98 | my_config = config.GlobalConfig() |
527 | 99 | config_file = StringIO(config_text.encode('utf-8')) | 99 | config_file = StringIO(config_text.encode('utf-8')) |
529 | 100 | my_config._parser = my_config._get_parser(file=config_file) | 100 | my_config._parser = my_config._get_parser(infile=config_file) |
530 | 101 | return my_config | 101 | return my_config |
531 | 102 | 102 | ||
532 | 103 | def test_simple(self): | 103 | def test_simple(self): |
533 | 104 | 104 | ||
534 | === modified file 'bzrlib/tests/test_config.py' | |||
535 | --- bzrlib/tests/test_config.py 2010-04-23 07:15:23 +0000 | |||
536 | +++ bzrlib/tests/test_config.py 2010-07-14 08:34:44 +0000 | |||
537 | @@ -19,6 +19,7 @@ | |||
538 | 19 | from cStringIO import StringIO | 19 | from cStringIO import StringIO |
539 | 20 | import os | 20 | import os |
540 | 21 | import sys | 21 | import sys |
541 | 22 | import threading | ||
542 | 22 | 23 | ||
543 | 23 | #import bzrlib specific imports here | 24 | #import bzrlib specific imports here |
544 | 24 | from bzrlib import ( | 25 | from bzrlib import ( |
545 | @@ -38,6 +39,32 @@ | |||
546 | 38 | from bzrlib.util.configobj import configobj | 39 | from bzrlib.util.configobj import configobj |
547 | 39 | 40 | ||
548 | 40 | 41 | ||
549 | 42 | def lockable_config_scenarios(): | ||
550 | 43 | return [ | ||
551 | 44 | ('global', | ||
552 | 45 | {'config_file_name': config.config_filename, | ||
553 | 46 | 'config_class': config.GlobalConfig, | ||
554 | 47 | 'config_args': [], | ||
555 | 48 | 'config_section': 'DEFAULT'}), | ||
556 | 49 | ('locations', | ||
557 | 50 | {'config_file_name': config.locations_config_filename, | ||
558 | 51 | 'config_class': config.LocationConfig, | ||
559 | 52 | 'config_args': ['.'], | ||
560 | 53 | 'config_section': '.'}),] | ||
561 | 54 | |||
562 | 55 | |||
563 | 56 | def load_tests(standard_tests, module, loader): | ||
564 | 57 | suite = loader.suiteClass() | ||
565 | 58 | |||
566 | 59 | lc_tests, remaining_tests = tests.split_suite_by_condition( | ||
567 | 60 | standard_tests, tests.condition_isinstance(( | ||
568 | 61 | TestLockableConfig, | ||
569 | 62 | ))) | ||
570 | 63 | tests.multiply_tests(lc_tests, lockable_config_scenarios(), suite) | ||
571 | 64 | suite.addTest(remaining_tests) | ||
572 | 65 | return suite | ||
573 | 66 | |||
574 | 67 | |||
575 | 41 | sample_long_alias="log -r-15..-1 --line" | 68 | sample_long_alias="log -r-15..-1 --line" |
576 | 42 | sample_config_text = u""" | 69 | sample_config_text = u""" |
577 | 43 | [DEFAULT] | 70 | [DEFAULT] |
578 | @@ -129,6 +156,9 @@ | |||
579 | 129 | self._calls.append(('keys',)) | 156 | self._calls.append(('keys',)) |
580 | 130 | return [] | 157 | return [] |
581 | 131 | 158 | ||
582 | 159 | def reload(self): | ||
583 | 160 | self._calls.append(('reload',)) | ||
584 | 161 | |||
585 | 132 | def write(self, arg): | 162 | def write(self, arg): |
586 | 133 | self._calls.append(('write',)) | 163 | self._calls.append(('write',)) |
587 | 134 | 164 | ||
588 | @@ -371,7 +401,7 @@ | |||
589 | 371 | 401 | ||
590 | 372 | def make_config_parser(self, s): | 402 | def make_config_parser(self, s): |
591 | 373 | conf = config.IniBasedConfig(None) | 403 | conf = config.IniBasedConfig(None) |
593 | 374 | parser = conf._get_parser(file=StringIO(s.encode('utf-8'))) | 404 | parser = conf._get_parser(infile=StringIO(s.encode('utf-8'))) |
594 | 375 | return conf, parser | 405 | return conf, parser |
595 | 376 | 406 | ||
596 | 377 | 407 | ||
597 | @@ -384,16 +414,144 @@ | |||
598 | 384 | config_file = StringIO(sample_config_text.encode('utf-8')) | 414 | config_file = StringIO(sample_config_text.encode('utf-8')) |
599 | 385 | my_config = config.IniBasedConfig(None) | 415 | my_config = config.IniBasedConfig(None) |
600 | 386 | self.failUnless( | 416 | self.failUnless( |
602 | 387 | isinstance(my_config._get_parser(file=config_file), | 417 | isinstance(my_config._get_parser(infile=config_file), |
603 | 388 | configobj.ConfigObj)) | 418 | configobj.ConfigObj)) |
604 | 389 | 419 | ||
605 | 390 | def test_cached(self): | 420 | def test_cached(self): |
606 | 391 | config_file = StringIO(sample_config_text.encode('utf-8')) | 421 | config_file = StringIO(sample_config_text.encode('utf-8')) |
607 | 392 | my_config = config.IniBasedConfig(None) | 422 | my_config = config.IniBasedConfig(None) |
609 | 393 | parser = my_config._get_parser(file=config_file) | 423 | parser = my_config._get_parser(infile=config_file) |
610 | 394 | self.failUnless(my_config._get_parser() is parser) | 424 | self.failUnless(my_config._get_parser() is parser) |
611 | 395 | 425 | ||
612 | 396 | 426 | ||
613 | 427 | class TestLockableConfig(tests.TestCaseInTempDir): | ||
614 | 428 | |||
615 | 429 | # Set by load_tests | ||
616 | 430 | config_file_name = None | ||
617 | 431 | config_class = None | ||
618 | 432 | config_args = None | ||
619 | 433 | config_section = None | ||
620 | 434 | |||
621 | 435 | def setUp(self): | ||
622 | 436 | super(TestLockableConfig, self).setUp() | ||
623 | 437 | config.ensure_config_dir_exists() | ||
624 | 438 | text = '[%s]\none=1\ntwo=2\n' % (self.config_section,) | ||
625 | 439 | self.build_tree_contents([(self.config_file_name(), text)]) | ||
626 | 440 | |||
627 | 441 | def create_config(self): | ||
628 | 442 | return self.config_class(*self.config_args) | ||
629 | 443 | |||
630 | 444 | def test_simple_read_access(self): | ||
631 | 445 | c = self.create_config() | ||
632 | 446 | self.assertEquals('1', c.get_user_option('one')) | ||
633 | 447 | |||
634 | 448 | def test_simple_write_access(self): | ||
635 | 449 | c = self.create_config() | ||
636 | 450 | c.set_user_option('one', 'one') | ||
637 | 451 | self.assertEquals('one', c.get_user_option('one')) | ||
638 | 452 | |||
639 | 453 | def test_unlocked_config(self): | ||
640 | 454 | c = self.create_config() | ||
641 | 455 | self.assertRaises(errors.ObjectNotLocked, c._write_config_file) | ||
642 | 456 | |||
643 | 457 | def test_listen_to_the_last_speaker(self): | ||
644 | 458 | c1 = self.create_config() | ||
645 | 459 | c2 = self.create_config() | ||
646 | 460 | c1.set_user_option('one', 'ONE') | ||
647 | 461 | c2.set_user_option('two', 'TWO') | ||
648 | 462 | self.assertEquals('ONE', c1.get_user_option('one')) | ||
649 | 463 | self.assertEquals('TWO', c2.get_user_option('two')) | ||
650 | 464 | # The second update respect the first one | ||
651 | 465 | self.assertEquals('ONE', c2.get_user_option('one')) | ||
652 | 466 | |||
653 | 467 | def test_last_speaker_wins(self): | ||
654 | 468 | # If the same config is not shared, the same variable modified twice | ||
655 | 469 | # can only see a single result. | ||
656 | 470 | c1 = self.create_config() | ||
657 | 471 | c2 = self.create_config() | ||
658 | 472 | c1.set_user_option('one', 'c1') | ||
659 | 473 | c2.set_user_option('one', 'c2') | ||
660 | 474 | self.assertEquals('c2', c2.get_user_option('one')) | ||
661 | 475 | # The first modification is still available until another refresh | ||
662 | 476 | # occur | ||
663 | 477 | self.assertEquals('c1', c1.get_user_option('one')) | ||
664 | 478 | c1.set_user_option('two', 'done') | ||
665 | 479 | self.assertEquals('c2', c1.get_user_option('one')) | ||
666 | 480 | |||
667 | 481 | def test_writes_are_serialized(self): | ||
668 | 482 | c1 = self.create_config() | ||
669 | 483 | c2 = self.create_config() | ||
670 | 484 | |||
671 | 485 | # We spawn a thread that will pause *during* the write | ||
672 | 486 | before_writing = threading.Event() | ||
673 | 487 | after_writing = threading.Event() | ||
674 | 488 | writing_done = threading.Event() | ||
675 | 489 | c1_orig = c1._write_config_file | ||
676 | 490 | def c1_write_config_file(): | ||
677 | 491 | before_writing.set() | ||
678 | 492 | c1_orig() | ||
679 | 493 | # The lock is held we wait for the main thread to decide when to | ||
680 | 494 | # continue | ||
681 | 495 | after_writing.wait() | ||
682 | 496 | c1._write_config_file = c1_write_config_file | ||
683 | 497 | def c1_set_option(): | ||
684 | 498 | c1.set_user_option('one', 'c1') | ||
685 | 499 | writing_done.set() | ||
686 | 500 | t1 = threading.Thread(target=c1_set_option) | ||
687 | 501 | # Collect the thread after the test | ||
688 | 502 | self.addCleanup(t1.join) | ||
689 | 503 | # Be ready to unblock the thread if the test goes wrong | ||
690 | 504 | self.addCleanup(after_writing.set) | ||
691 | 505 | t1.start() | ||
692 | 506 | before_writing.wait() | ||
693 | 507 | self.assertTrue(c1._lock.is_held) | ||
694 | 508 | self.assertRaises(errors.LockContention, | ||
695 | 509 | c2.set_user_option, 'one', 'c2') | ||
696 | 510 | self.assertEquals('c1', c1.get_user_option('one')) | ||
697 | 511 | # Let the lock be released | ||
698 | 512 | after_writing.set() | ||
699 | 513 | writing_done.wait() | ||
700 | 514 | c2.set_user_option('one', 'c2') | ||
701 | 515 | self.assertEquals('c2', c2.get_user_option('one')) | ||
702 | 516 | |||
703 | 517 | def test_read_while_writing(self): | ||
704 | 518 | c1 = self.create_config() | ||
705 | 519 | # We spawn a thread that will pause *during* the write | ||
706 | 520 | ready_to_write = threading.Event() | ||
707 | 521 | do_writing = threading.Event() | ||
708 | 522 | writing_done = threading.Event() | ||
709 | 523 | c1_orig = c1._write_config_file | ||
710 | 524 | def c1_write_config_file(): | ||
711 | 525 | ready_to_write.set() | ||
712 | 526 | # The lock is held we wait for the main thread to decide when to | ||
713 | 527 | # continue | ||
714 | 528 | do_writing.wait() | ||
715 | 529 | c1_orig() | ||
716 | 530 | writing_done.set() | ||
717 | 531 | c1._write_config_file = c1_write_config_file | ||
718 | 532 | def c1_set_option(): | ||
719 | 533 | c1.set_user_option('one', 'c1') | ||
720 | 534 | t1 = threading.Thread(target=c1_set_option) | ||
721 | 535 | # Collect the thread after the test | ||
722 | 536 | self.addCleanup(t1.join) | ||
723 | 537 | # Be ready to unblock the thread if the test goes wrong | ||
724 | 538 | self.addCleanup(do_writing.set) | ||
725 | 539 | t1.start() | ||
726 | 540 | # Ensure the thread is ready to write | ||
727 | 541 | ready_to_write.wait() | ||
728 | 542 | self.assertTrue(c1._lock.is_held) | ||
729 | 543 | self.assertEquals('c1', c1.get_user_option('one')) | ||
730 | 544 | # If we read during the write, we get the old value | ||
731 | 545 | c2 = self.create_config() | ||
732 | 546 | self.assertEquals('1', c2.get_user_option('one')) | ||
733 | 547 | # Let the writing occur and ensure it occurred | ||
734 | 548 | do_writing.set() | ||
735 | 549 | writing_done.wait() | ||
736 | 550 | # Now we get the updated value | ||
737 | 551 | c3 = self.create_config() | ||
738 | 552 | self.assertEquals('c1', c3.get_user_option('one')) | ||
739 | 553 | |||
740 | 554 | |||
741 | 397 | class TestGetUserOptionAs(TestIniConfig): | 555 | class TestGetUserOptionAs(TestIniConfig): |
742 | 398 | 556 | ||
743 | 399 | def test_get_user_option_as_bool(self): | 557 | def test_get_user_option_as_bool(self): |
744 | @@ -583,26 +741,26 @@ | |||
745 | 583 | def test_user_id(self): | 741 | def test_user_id(self): |
746 | 584 | config_file = StringIO(sample_config_text.encode('utf-8')) | 742 | config_file = StringIO(sample_config_text.encode('utf-8')) |
747 | 585 | my_config = config.GlobalConfig() | 743 | my_config = config.GlobalConfig() |
749 | 586 | my_config._parser = my_config._get_parser(file=config_file) | 744 | my_config._parser = my_config._get_parser(infile=config_file) |
750 | 587 | self.assertEqual(u"Erik B\u00e5gfors <erik@bagfors.nu>", | 745 | self.assertEqual(u"Erik B\u00e5gfors <erik@bagfors.nu>", |
751 | 588 | my_config._get_user_id()) | 746 | my_config._get_user_id()) |
752 | 589 | 747 | ||
753 | 590 | def test_absent_user_id(self): | 748 | def test_absent_user_id(self): |
754 | 591 | config_file = StringIO("") | 749 | config_file = StringIO("") |
755 | 592 | my_config = config.GlobalConfig() | 750 | my_config = config.GlobalConfig() |
757 | 593 | my_config._parser = my_config._get_parser(file=config_file) | 751 | my_config._parser = my_config._get_parser(infile=config_file) |
758 | 594 | self.assertEqual(None, my_config._get_user_id()) | 752 | self.assertEqual(None, my_config._get_user_id()) |
759 | 595 | 753 | ||
760 | 596 | def test_configured_editor(self): | 754 | def test_configured_editor(self): |
761 | 597 | config_file = StringIO(sample_config_text.encode('utf-8')) | 755 | config_file = StringIO(sample_config_text.encode('utf-8')) |
762 | 598 | my_config = config.GlobalConfig() | 756 | my_config = config.GlobalConfig() |
764 | 599 | my_config._parser = my_config._get_parser(file=config_file) | 757 | my_config._parser = my_config._get_parser(infile=config_file) |
765 | 600 | self.assertEqual("vim", my_config.get_editor()) | 758 | self.assertEqual("vim", my_config.get_editor()) |
766 | 601 | 759 | ||
767 | 602 | def test_signatures_always(self): | 760 | def test_signatures_always(self): |
768 | 603 | config_file = StringIO(sample_always_signatures) | 761 | config_file = StringIO(sample_always_signatures) |
769 | 604 | my_config = config.GlobalConfig() | 762 | my_config = config.GlobalConfig() |
771 | 605 | my_config._parser = my_config._get_parser(file=config_file) | 763 | my_config._parser = my_config._get_parser(infile=config_file) |
772 | 606 | self.assertEqual(config.CHECK_NEVER, | 764 | self.assertEqual(config.CHECK_NEVER, |
773 | 607 | my_config.signature_checking()) | 765 | my_config.signature_checking()) |
774 | 608 | self.assertEqual(config.SIGN_ALWAYS, | 766 | self.assertEqual(config.SIGN_ALWAYS, |
775 | @@ -612,7 +770,7 @@ | |||
776 | 612 | def test_signatures_if_possible(self): | 770 | def test_signatures_if_possible(self): |
777 | 613 | config_file = StringIO(sample_maybe_signatures) | 771 | config_file = StringIO(sample_maybe_signatures) |
778 | 614 | my_config = config.GlobalConfig() | 772 | my_config = config.GlobalConfig() |
780 | 615 | my_config._parser = my_config._get_parser(file=config_file) | 773 | my_config._parser = my_config._get_parser(infile=config_file) |
781 | 616 | self.assertEqual(config.CHECK_NEVER, | 774 | self.assertEqual(config.CHECK_NEVER, |
782 | 617 | my_config.signature_checking()) | 775 | my_config.signature_checking()) |
783 | 618 | self.assertEqual(config.SIGN_WHEN_REQUIRED, | 776 | self.assertEqual(config.SIGN_WHEN_REQUIRED, |
784 | @@ -622,7 +780,7 @@ | |||
785 | 622 | def test_signatures_ignore(self): | 780 | def test_signatures_ignore(self): |
786 | 623 | config_file = StringIO(sample_ignore_signatures) | 781 | config_file = StringIO(sample_ignore_signatures) |
787 | 624 | my_config = config.GlobalConfig() | 782 | my_config = config.GlobalConfig() |
789 | 625 | my_config._parser = my_config._get_parser(file=config_file) | 783 | my_config._parser = my_config._get_parser(infile=config_file) |
790 | 626 | self.assertEqual(config.CHECK_ALWAYS, | 784 | self.assertEqual(config.CHECK_ALWAYS, |
791 | 627 | my_config.signature_checking()) | 785 | my_config.signature_checking()) |
792 | 628 | self.assertEqual(config.SIGN_NEVER, | 786 | self.assertEqual(config.SIGN_NEVER, |
793 | @@ -632,7 +790,7 @@ | |||
794 | 632 | def _get_sample_config(self): | 790 | def _get_sample_config(self): |
795 | 633 | config_file = StringIO(sample_config_text.encode('utf-8')) | 791 | config_file = StringIO(sample_config_text.encode('utf-8')) |
796 | 634 | my_config = config.GlobalConfig() | 792 | my_config = config.GlobalConfig() |
798 | 635 | my_config._parser = my_config._get_parser(file=config_file) | 793 | my_config._parser = my_config._get_parser(infile=config_file) |
799 | 636 | return my_config | 794 | return my_config |
800 | 637 | 795 | ||
801 | 638 | def test_gpg_signing_command(self): | 796 | def test_gpg_signing_command(self): |
802 | @@ -643,7 +801,7 @@ | |||
803 | 643 | def _get_empty_config(self): | 801 | def _get_empty_config(self): |
804 | 644 | config_file = StringIO("") | 802 | config_file = StringIO("") |
805 | 645 | my_config = config.GlobalConfig() | 803 | my_config = config.GlobalConfig() |
807 | 646 | my_config._parser = my_config._get_parser(file=config_file) | 804 | my_config._parser = my_config._get_parser(infile=config_file) |
808 | 647 | return my_config | 805 | return my_config |
809 | 648 | 806 | ||
810 | 649 | def test_gpg_signing_command_unset(self): | 807 | def test_gpg_signing_command_unset(self): |
811 | @@ -745,10 +903,11 @@ | |||
812 | 745 | [('__init__', config.locations_config_filename(), | 903 | [('__init__', config.locations_config_filename(), |
813 | 746 | 'utf-8')]) | 904 | 'utf-8')]) |
814 | 747 | config.ensure_config_dir_exists() | 905 | config.ensure_config_dir_exists() |
815 | 748 | #os.mkdir(config.config_dir()) | ||
816 | 749 | f = file(config.branches_config_filename(), 'wb') | 906 | f = file(config.branches_config_filename(), 'wb') |
819 | 750 | f.write('') | 907 | try: |
820 | 751 | f.close() | 908 | f.write('') |
821 | 909 | finally: | ||
822 | 910 | f.close() | ||
823 | 752 | oldparserclass = config.ConfigObj | 911 | oldparserclass = config.ConfigObj |
824 | 753 | config.ConfigObj = InstrumentedConfigObj | 912 | config.ConfigObj = InstrumentedConfigObj |
825 | 754 | try: | 913 | try: |
826 | @@ -995,10 +1154,15 @@ | |||
827 | 995 | else: | 1154 | else: |
828 | 996 | global_file = StringIO(global_config.encode('utf-8')) | 1155 | global_file = StringIO(global_config.encode('utf-8')) |
829 | 997 | branches_file = StringIO(sample_branches_text.encode('utf-8')) | 1156 | branches_file = StringIO(sample_branches_text.encode('utf-8')) |
830 | 1157 | # Make sure the locations config can be reloaded | ||
831 | 1158 | config.ensure_config_dir_exists() | ||
832 | 1159 | f = file(config.locations_config_filename(), 'wb') | ||
833 | 1160 | try: | ||
834 | 1161 | f.write(branches_file.getvalue()) | ||
835 | 1162 | finally: | ||
836 | 1163 | f.close | ||
837 | 998 | self.my_config = config.BranchConfig(FakeBranch(location)) | 1164 | self.my_config = config.BranchConfig(FakeBranch(location)) |
838 | 999 | # Force location config to use specified file | ||
839 | 1000 | self.my_location_config = self.my_config._get_location_config() | 1165 | self.my_location_config = self.my_config._get_location_config() |
840 | 1001 | self.my_location_config._get_parser(branches_file) | ||
841 | 1002 | # Force global config to use specified file | 1166 | # Force global config to use specified file |
842 | 1003 | self.my_config._get_global_config()._get_parser(global_file) | 1167 | self.my_config._get_global_config()._get_parser(global_file) |
843 | 1004 | 1168 | ||
844 | @@ -1007,25 +1171,14 @@ | |||
845 | 1007 | record = InstrumentedConfigObj("foo") | 1171 | record = InstrumentedConfigObj("foo") |
846 | 1008 | self.my_location_config._parser = record | 1172 | self.my_location_config._parser = record |
847 | 1009 | 1173 | ||
867 | 1010 | real_mkdir = os.mkdir | 1174 | self.callDeprecated(['The recurse option is deprecated as of ' |
868 | 1011 | self.created = False | 1175 | '0.14. The section "/a/c" has been ' |
869 | 1012 | def checked_mkdir(path, mode=0777): | 1176 | 'converted to use policies.'], |
870 | 1013 | self.log('making directory: %s', path) | 1177 | self.my_config.set_user_option, |
871 | 1014 | real_mkdir(path, mode) | 1178 | 'foo', 'bar', store=config.STORE_LOCATION) |
872 | 1015 | self.created = True | 1179 | |
873 | 1016 | 1180 | self.assertEqual([('reload',), | |
874 | 1017 | os.mkdir = checked_mkdir | 1181 | ('__contains__', '/a/c'), |
856 | 1018 | try: | ||
857 | 1019 | self.callDeprecated(['The recurse option is deprecated as of ' | ||
858 | 1020 | '0.14. The section "/a/c" has been ' | ||
859 | 1021 | 'converted to use policies.'], | ||
860 | 1022 | self.my_config.set_user_option, | ||
861 | 1023 | 'foo', 'bar', store=config.STORE_LOCATION) | ||
862 | 1024 | finally: | ||
863 | 1025 | os.mkdir = real_mkdir | ||
864 | 1026 | |||
865 | 1027 | self.failUnless(self.created, 'Failed to create ~/.bazaar') | ||
866 | 1028 | self.assertEqual([('__contains__', '/a/c'), | ||
875 | 1029 | ('__contains__', '/a/c/'), | 1182 | ('__contains__', '/a/c/'), |
876 | 1030 | ('__setitem__', '/a/c', {}), | 1183 | ('__setitem__', '/a/c', {}), |
877 | 1031 | ('__getitem__', '/a/c'), | 1184 | ('__getitem__', '/a/c'), |
878 | @@ -1083,10 +1236,13 @@ | |||
879 | 1083 | if global_config is not None: | 1236 | if global_config is not None: |
880 | 1084 | global_file = StringIO(global_config.encode('utf-8')) | 1237 | global_file = StringIO(global_config.encode('utf-8')) |
881 | 1085 | my_config._get_global_config()._get_parser(global_file) | 1238 | my_config._get_global_config()._get_parser(global_file) |
883 | 1086 | self.my_location_config = my_config._get_location_config() | 1239 | lconf = my_config._get_location_config() |
884 | 1087 | if location_config is not None: | 1240 | if location_config is not None: |
885 | 1088 | location_file = StringIO(location_config.encode('utf-8')) | 1241 | location_file = StringIO(location_config.encode('utf-8')) |
887 | 1089 | self.my_location_config._get_parser(location_file) | 1242 | lconf._get_parser(location_file) |
888 | 1243 | # Make sure the config can be reloaded | ||
889 | 1244 | lconf._parser.filename = config.locations_config_filename() | ||
890 | 1245 | self.my_location_config = lconf | ||
891 | 1090 | if branch_data_config is not None: | 1246 | if branch_data_config is not None: |
892 | 1091 | my_config.branch.control_files.files['branch.conf'] = \ | 1247 | my_config.branch.control_files.files['branch.conf'] = \ |
893 | 1092 | branch_data_config | 1248 | branch_data_config |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Vincent Ladeuil wrote: /bugs.launchpad .net/bugs/ 525571 config. LockableConfig that plugins authors using config files in ~/.bazaar may want to inherit (as LocationConfig and GlobalConfig do now).
> 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:/
>
>
> 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.
>
> 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 enigmail. mozdev. org/
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkw qMNkACgkQJdeBCY SNAAO7WACfYZOte 5LfqA4Ro4J6U/ 3ZA2Cf tQjcgx047AYI0sJ MiZlfY
ZhkAoLy3d0+
=mMJj
-----END PGP SIGNATURE-----