Merge lp://staging/~spiv/bzr/robust-cleanup-in-commit into lp://staging/bzr
- robust-cleanup-in-commit
- Merge into bzr.dev
| Status: | Merged |
|---|---|
| Approved by: | John A Meinel |
| Approved revision: | no longer in the source branch. |
| Merged at revision: | not available |
| Proposed branch: | lp://staging/~spiv/bzr/robust-cleanup-in-commit |
| Merge into: | lp://staging/bzr |
| Diff against target: |
863 lines 6 files modified
NEWS (+5/-0) bzrlib/cleanup.py (+177/-0) bzrlib/commit.py (+137/-148) bzrlib/repository.py (+1/-1) bzrlib/tests/__init__.py (+1/-0) bzrlib/tests/test_cleanup.py (+280/-0) |
| To merge this branch: | bzr merge lp://staging/~spiv/bzr/robust-cleanup-in-commit |
| Related bugs: |
| Reviewer | Review Type | Date Requested | Status |
|---|---|---|---|
| John A Meinel | Approve | ||
|
Review via email:
|
|||
This proposal supersedes a proposal from 2009-10-15.
Commit message
Description of the change
| Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal | # |
| John A Meinel (jameinel) wrote : Posted in a previous version of this proposal | # |
59 +def _run_cleanup(func, *args, **kwargs):
60 + """Run func(*args, **kwargs), logging but not propagating any error it
61 + raises.
62 +
63 + :returns: True if func raised no errors, else False.
64 + """
65 + try:
66 + func(*args, **kwargs)
67 + except KeyboardInterrupt:
68 + raise
69 + except Exception, exc:
70 + _log_cleanup_
71 + return False
72 + return True
73 +
74 +
75 +def _run_cleanup_
76 + try:
77 + func(*args, **kwargs)
78 + except KeyboardInterrupt:
79 + raise
80 + except Exception, exc:
81 + trace.mutter(
82 + trace.log_
83 + trace.warning(
84 + return False
85 + return True
Is it just me, or is "_run_cleanup_
thing as _run_cleanup, only it isn't calling out to a helper to log the error.
As such, I'm thinking one of them should be removed. Which should also simplify
the _run_cleanups() function.
I assume at one time one of them might have propagated the exception?
124 + def add_cleanup(self, cleanup_func):
125 + """Add a cleanup to run. Cleanups will be executed in LIFO order."""
126 + self.cleanups.
^- insert(0,) has to move all items in the list each time. Why not just do:
self.cleanups.
and then
128 + def run(self, *args, **kwargs):
129 + func = lambda: self.func(self, *args, **kwargs)
130 + return do_with_
Or alternatively, I think the standard idiom would be to build up state in
forward order and then walk it reversed. so I would think it reasonable to
change '_run_cleanups' from:
for func in funcs:
to
for func in reversed(funcs):
I'm pretty sure you always want to run cleanups in LIFO order, so it would make
sense to bias the apis to do that easily.
Also, if we are going to have an "add_cleanup()" helper, it seems like it would
be good to make it the twin of TestCase.
(now) allows you to pass in arguments to the cleanup functions.
I also think that would make for a cleaner api for 'do_with_cleanups', since
otherwise you have this:
func = lambda: self.func(self, *args, **kwargs)
which works, but means that the api isn't very nice to use.
695 +#class TestRunCleanupR
696 +#
697 +# def test_cleanup_
698 +# xxx
^- I'm assuming you just want to nuke this when you nuke
_run_cleanup_
I do think commit.py is nicer with an object managing the state of things that
needs to be cleaned up. And I'm flexible about api decisions. I figured I could
state my opinion, though.
| Andrew Bennetts (spiv) wrote : Posted in a previous version of this proposal | # |
John A Meinel wrote:
[...]
> Is it just me, or is "_run_cleanup_
> thing as _run_cleanup, only it isn't calling out to a helper to log the error.
> As such, I'm thinking one of them should be removed. Which should also simplify
> the _run_cleanups() function.
>
> I assume at one time one of them might have propagated the exception?
Yeah, I think so. Also note that _run_cleanup_
warns rather than checking for -Dcleanup, but I don't think that's particularly
valuable.
> 124 + def add_cleanup(self, cleanup_func):
> 125 + """Add a cleanup to run. Cleanups will be executed in LIFO order."""
> 126 + self.cleanups.
>
> ^- insert(0,) has to move all items in the list each time. Why not just do:
>
> self.cleanups.
>
> and then
>
> 128 + def run(self, *args, **kwargs):
> 129 + func = lambda: self.func(self, *args, **kwargs)
> 130 + return do_with_
The func passed to do_with_cleanups here is expected to mutate self.cleanups
(via calling add_cleanup). So passing in reversed(
mean no cleanups get run!
I'd be happy to make it a deque for clarity, but as the longest cleanup chain we
have so far is 4 elements I didn't think it really mattered much in terms of
performance :)
> Or alternatively, I think the standard idiom would be to build up state in
> forward order and then walk it reversed. so I would think it reasonable to
> change '_run_cleanups' from:
> for func in funcs:
> to
> for func in reversed(funcs):
Hmm. There's the competing concern that when calling do_with_cleanups directly
with a pre-built literal list I think it may be clearer to have the cleanups
forwards, i.e.:
do_
But you're right that it might be a net win to bias towards the other way. I'll
try it out and see what I think.
> I'm pretty sure you always want to run cleanups in LIFO order, so it would make
> sense to bias the apis to do that easily.
>
> Also, if we are going to have an "add_cleanup()" helper, it seems like it would
> be good to make it the twin of TestCase.
> (now) allows you to pass in arguments to the cleanup functions.
Ok, if you like. It held off doing that simply on the grounds that I hadn't
had use for it. It's easy to add.
> I also think that would make for a cleaner api for 'do_with_cleanups', since
> otherwise you have this:
> func = lambda: self.func(self, *args, **kwargs)
>
> which works, but means that the api isn't very nice to use.
I don't follow... what signature are you proposing do_with_cleanups should have
instead? You need some way to keep the cleanups list separate from the *args
and **kwargs, but that means either putting the cleanups list as the first arg,
or as a positional arg between the function and its *args. Both options feel
ugly to me.
> 695 +#class TestRunCleanupR
> 696 +#
> 697 +# def test_cleanup_
> 698 +# xxx
>
>
> ^- I'm assuming you just want to nuke this when you nuke
> _run_cleanup...
| John A Meinel (jameinel) wrote : Posted in a previous version of this proposal | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
...
>> Also, if we are going to have an "add_cleanup()" helper, it seems like it would
>> be good to make it the twin of TestCase.
>> (now) allows you to pass in arguments to the cleanup functions.
>
> Ok, if you like. It held off doing that simply on the grounds that I hadn't
> had use for it. It's easy to add.
>
>> I also think that would make for a cleaner api for 'do_with_cleanups', since
>> otherwise you have this:
>> func = lambda: self.func(self, *args, **kwargs)
>>
>> which works, but means that the api isn't very nice to use.
>
> I don't follow... what signature are you proposing do_with_cleanups should have
> instead? You need some way to keep the cleanups list separate from the *args
> and **kwargs, but that means either putting the cleanups list as the first arg,
> or as a positional arg between the function and its *args. Both options feel
> ugly to me.
So, IMO cleanups should be a list of tuples of the form:
[(func, args, kwargs), ...]
and do_with_cleanups would then be
do_with_
However, I also think the 'helper' form is cleaner, and that we should
make 'do_with_cleanups' a hidden detail of OperationWithCl
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkr
4bAAnj1QYPmAsX4
=elO0
-----END PGP SIGNATURE-----
| Andrew Bennetts (spiv) wrote : | # |
This I think addresses all the comments from the previous review.
I've tweaked docstrings to emphasise OperationWithCl
The only real difference from the suggestions in the last review is that I'm using deque.appendleft to add cleanups rather than appending to a list then reversing. I think this works out a little cleaner, as otherwise I would have had to either duplicate the calls to reversed(cleanups) or added more complication. It just works out simpler to build the sequence in the desired order.
You can see an incremental diff here: http://
| John A Meinel (jameinel) wrote : | # |
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Andrew Bennetts wrote:
> Andrew Bennetts has proposed merging lp:~spiv/bzr/robust-cleanup-in-commit into lp:bzr.
>
> Requested reviews:
> John A Meinel (jameinel):
> Related bugs:
> #429747 [master] errors during cleanup mask underlying errors (eg TooManyConcurre
> https:/
>
>
review: approve
merge: approve
John
=:->
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.9 (Cygwin)
Comment: Using GnuPG with Mozilla - http://
iEYEARECAAYFAkr
8BsAnilUJvwTwny
=iTSL
-----END PGP SIGNATURE-----
Preview Diff
| 1 | === modified file 'NEWS' |
| 2 | --- NEWS 2009-10-27 14:13:40 +0000 |
| 3 | +++ NEWS 2009-10-28 00:14:11 +0000 |
| 4 | @@ -27,6 +27,11 @@ |
| 5 | |
| 6 | * Diff parsing handles "Binary files differ" hunks. (Aaron Bentley, #436325) |
| 7 | |
| 8 | +* Errors during commit are handled more robustly so that knock-on errors |
| 9 | + are less likely to occur, and will not obscure the original error if |
| 10 | + they do occur. This fixes some causes of ``TooManyConcurrentRequests`` |
| 11 | + and similar errors. (Andrew Bennetts, #429747, #243391) |
| 12 | + |
| 13 | * Fetching from stacked pre-2a repository via a smart server no longer |
| 14 | fails intermittently with "second push failed to complete". |
| 15 | (Andrew Bennetts, #437626) |
| 16 | |
| 17 | === added file 'bzrlib/cleanup.py' |
| 18 | --- bzrlib/cleanup.py 1970-01-01 00:00:00 +0000 |
| 19 | +++ bzrlib/cleanup.py 2009-10-28 00:14:11 +0000 |
| 20 | @@ -0,0 +1,177 @@ |
| 21 | +# Copyright (C) 2009 Canonical Ltd |
| 22 | +# |
| 23 | +# This program is free software; you can redistribute it and/or modify |
| 24 | +# it under the terms of the GNU General Public License as published by |
| 25 | +# the Free Software Foundation; either version 2 of the License, or |
| 26 | +# (at your option) any later version. |
| 27 | +# |
| 28 | +# This program is distributed in the hope that it will be useful, |
| 29 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
| 30 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
| 31 | +# GNU General Public License for more details. |
| 32 | +# |
| 33 | +# You should have received a copy of the GNU General Public License |
| 34 | +# along with this program; if not, write to the Free Software |
| 35 | +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
| 36 | + |
| 37 | +"""Helpers for managing cleanup functions and the errors they might raise. |
| 38 | + |
| 39 | +The usual way to run cleanup code in Python is:: |
| 40 | + |
| 41 | + try: |
| 42 | + do_something() |
| 43 | + finally: |
| 44 | + cleanup_something() |
| 45 | + |
| 46 | +However if both `do_something` and `cleanup_something` raise an exception |
| 47 | +Python will forget the original exception and propagate the one from |
| 48 | +cleanup_something. Unfortunately, this is almost always much less useful than |
| 49 | +the original exception. |
| 50 | + |
| 51 | +If you want to be certain that the first, and only the first, error is raised, |
| 52 | +then use:: |
| 53 | + |
| 54 | + operation = OperationWithCleanups(lambda operation: do_something()) |
| 55 | + operation.add_cleanup(cleanup_something) |
| 56 | + operation.run() |
| 57 | + |
| 58 | +This is more inconvenient (because you need to make every try block a |
| 59 | +function), but will ensure that the first error encountered is the one raised, |
| 60 | +while also ensuring all cleanups are run. See OperationWithCleanups for more |
| 61 | +details. |
| 62 | +""" |
| 63 | + |
| 64 | + |
| 65 | +from collections import deque |
| 66 | +import sys |
| 67 | +from bzrlib import ( |
| 68 | + debug, |
| 69 | + trace, |
| 70 | + ) |
| 71 | + |
| 72 | +def _log_cleanup_error(exc): |
| 73 | + trace.mutter('Cleanup failed:') |
| 74 | + trace.log_exception_quietly() |
| 75 | + if 'cleanup' in debug.debug_flags: |
| 76 | + trace.warning('bzr: warning: Cleanup failed: %s', exc) |
| 77 | + |
| 78 | + |
| 79 | +def _run_cleanup(func, *args, **kwargs): |
| 80 | + """Run func(*args, **kwargs), logging but not propagating any error it |
| 81 | + raises. |
| 82 | + |
| 83 | + :returns: True if func raised no errors, else False. |
| 84 | + """ |
| 85 | + try: |
| 86 | + func(*args, **kwargs) |
| 87 | + except KeyboardInterrupt: |
| 88 | + raise |
| 89 | + except Exception, exc: |
| 90 | + _log_cleanup_error(exc) |
| 91 | + return False |
| 92 | + return True |
| 93 | + |
| 94 | + |
| 95 | +def _run_cleanups(funcs): |
| 96 | + """Run a series of cleanup functions.""" |
| 97 | + for func, args, kwargs in funcs: |
| 98 | + _run_cleanup(func, *args, **kwargs) |
| 99 | + |
| 100 | + |
| 101 | +class OperationWithCleanups(object): |
| 102 | + """A way to run some code with a dynamic cleanup list. |
| 103 | + |
| 104 | + This provides a way to add cleanups while the function-with-cleanups is |
| 105 | + running. |
| 106 | + |
| 107 | + Typical use:: |
| 108 | + |
| 109 | + operation = OperationWithCleanups(some_func) |
| 110 | + operation.run(args...) |
| 111 | + |
| 112 | + where `some_func` is:: |
| 113 | + |
| 114 | + def some_func(operation, args, ...) |
| 115 | + do_something() |
| 116 | + operation.add_cleanup(something) |
| 117 | + # etc |
| 118 | + |
| 119 | + Note that the first argument passed to `some_func` will be the |
| 120 | + OperationWithCleanups object. |
| 121 | + """ |
| 122 | + |
| 123 | + def __init__(self, func): |
| 124 | + self.func = func |
| 125 | + self.cleanups = deque() |
| 126 | + |
| 127 | + def add_cleanup(self, cleanup_func, *args, **kwargs): |
| 128 | + """Add a cleanup to run. |
| 129 | + |
| 130 | + Cleanups may be added at any time before or during the execution of |
| 131 | + self.func. Cleanups will be executed in LIFO order. |
| 132 | + """ |
| 133 | + self.cleanups.appendleft((cleanup_func, args, kwargs)) |
| 134 | + |
| 135 | + def run(self, *args, **kwargs): |
| 136 | + return _do_with_cleanups( |
| 137 | + self.cleanups, self.func, self, *args, **kwargs) |
| 138 | + |
| 139 | + |
| 140 | +def _do_with_cleanups(cleanup_funcs, func, *args, **kwargs): |
| 141 | + """Run `func`, then call all the cleanup_funcs. |
| 142 | + |
| 143 | + All the cleanup_funcs are guaranteed to be run. The first exception raised |
| 144 | + by func or any of the cleanup_funcs is the one that will be propagted by |
| 145 | + this function (subsequent errors are caught and logged). |
| 146 | + |
| 147 | + Conceptually similar to:: |
| 148 | + |
| 149 | + try: |
| 150 | + return func(*args, **kwargs) |
| 151 | + finally: |
| 152 | + for cleanup, cargs, ckwargs in cleanup_funcs: |
| 153 | + cleanup(*cargs, **ckwargs) |
| 154 | + |
| 155 | + It avoids several problems with using try/finally directly: |
| 156 | + * an exception from func will not be obscured by a subsequent exception |
| 157 | + from a cleanup. |
| 158 | + * an exception from a cleanup will not prevent other cleanups from |
| 159 | + running (but the first exception encountered is still the one |
| 160 | + propagated). |
| 161 | + |
| 162 | + Unike `_run_cleanup`, `_do_with_cleanups` can propagate an exception from a |
| 163 | + cleanup, but only if there is no exception from func. |
| 164 | + """ |
| 165 | + # As correct as Python 2.4 allows. |
| 166 | + try: |
| 167 | + result = func(*args, **kwargs) |
| 168 | + except: |
| 169 | + # We have an exception from func already, so suppress cleanup errors. |
| 170 | + _run_cleanups(cleanup_funcs) |
| 171 | + raise |
| 172 | + else: |
| 173 | + # No exception from func, so allow the first exception from |
| 174 | + # cleanup_funcs to propagate if one occurs (but only after running all |
| 175 | + # of them). |
| 176 | + exc_info = None |
| 177 | + for cleanup, c_args, c_kwargs in cleanup_funcs: |
| 178 | + # XXX: Hmm, if KeyboardInterrupt arrives at exactly this line, we |
| 179 | + # won't run all cleanups... perhaps we should temporarily install a |
| 180 | + # SIGINT handler? |
| 181 | + if exc_info is None: |
| 182 | + try: |
| 183 | + cleanup(*c_args, **c_kwargs) |
| 184 | + except: |
| 185 | + # This is the first cleanup to fail, so remember its |
| 186 | + # details. |
| 187 | + exc_info = sys.exc_info() |
| 188 | + else: |
| 189 | + # We already have an exception to propagate, so log any errors |
| 190 | + # but don't propagate them. |
| 191 | + _run_cleanup(cleanup, *c_args, **kwargs) |
| 192 | + if exc_info is not None: |
| 193 | + raise exc_info[0], exc_info[1], exc_info[2] |
| 194 | + # No error, so we can return the result |
| 195 | + return result |
| 196 | + |
| 197 | + |
| 198 | |
| 199 | === modified file 'bzrlib/commit.py' |
| 200 | --- bzrlib/commit.py 2009-08-28 05:00:33 +0000 |
| 201 | +++ bzrlib/commit.py 2009-10-28 00:14:11 +0000 |
| 202 | @@ -65,6 +65,7 @@ |
| 203 | xml_serializer, |
| 204 | ) |
| 205 | from bzrlib.branch import Branch |
| 206 | +from bzrlib.cleanup import OperationWithCleanups |
| 207 | import bzrlib.config |
| 208 | from bzrlib.errors import (BzrError, PointlessCommit, |
| 209 | ConflictsInTree, |
| 210 | @@ -234,6 +235,31 @@ |
| 211 | commit. Pending changes to excluded files will be ignored by the |
| 212 | commit. |
| 213 | """ |
| 214 | + operation = OperationWithCleanups(self._commit) |
| 215 | + return operation.run( |
| 216 | + message=message, |
| 217 | + timestamp=timestamp, |
| 218 | + timezone=timezone, |
| 219 | + committer=committer, |
| 220 | + specific_files=specific_files, |
| 221 | + rev_id=rev_id, |
| 222 | + allow_pointless=allow_pointless, |
| 223 | + strict=strict, |
| 224 | + verbose=verbose, |
| 225 | + revprops=revprops, |
| 226 | + working_tree=working_tree, |
| 227 | + local=local, |
| 228 | + reporter=reporter, |
| 229 | + config=config, |
| 230 | + message_callback=message_callback, |
| 231 | + recursive=recursive, |
| 232 | + exclude=exclude, |
| 233 | + possible_master_transports=possible_master_transports) |
| 234 | + |
| 235 | + def _commit(self, operation, message, timestamp, timezone, committer, |
| 236 | + specific_files, rev_id, allow_pointless, strict, verbose, revprops, |
| 237 | + working_tree, local, reporter, config, message_callback, recursive, |
| 238 | + exclude, possible_master_transports): |
| 239 | mutter('preparing to commit') |
| 240 | |
| 241 | if working_tree is None: |
| 242 | @@ -262,7 +288,6 @@ |
| 243 | self.exclude = [] |
| 244 | self.local = local |
| 245 | self.master_branch = None |
| 246 | - self.master_locked = False |
| 247 | self.recursive = recursive |
| 248 | self.rev_id = None |
| 249 | # self.specific_files is None to indicate no filter, or any iterable to |
| 250 | @@ -283,6 +308,7 @@ |
| 251 | self.verbose = verbose |
| 252 | |
| 253 | self.work_tree.lock_write() |
| 254 | + operation.add_cleanup(self.work_tree.unlock) |
| 255 | self.parents = self.work_tree.get_parent_ids() |
| 256 | # We can use record_iter_changes IFF iter_changes is compatible with |
| 257 | # the command line parameters, and the repository has fast delta |
| 258 | @@ -293,119 +319,118 @@ |
| 259 | (self.branch.repository._format.fast_deltas or |
| 260 | len(self.parents) < 2)) |
| 261 | self.pb = bzrlib.ui.ui_factory.nested_progress_bar() |
| 262 | + operation.add_cleanup(self.pb.finished) |
| 263 | self.basis_revid = self.work_tree.last_revision() |
| 264 | self.basis_tree = self.work_tree.basis_tree() |
| 265 | self.basis_tree.lock_read() |
| 266 | + operation.add_cleanup(self.basis_tree.unlock) |
| 267 | + # Cannot commit with conflicts present. |
| 268 | + if len(self.work_tree.conflicts()) > 0: |
| 269 | + raise ConflictsInTree |
| 270 | + |
| 271 | + # Setup the bound branch variables as needed. |
| 272 | + self._check_bound_branch(operation, possible_master_transports) |
| 273 | + |
| 274 | + # Check that the working tree is up to date |
| 275 | + old_revno, new_revno = self._check_out_of_date_tree() |
| 276 | + |
| 277 | + # Complete configuration setup |
| 278 | + if reporter is not None: |
| 279 | + self.reporter = reporter |
| 280 | + elif self.reporter is None: |
| 281 | + self.reporter = self._select_reporter() |
| 282 | + if self.config is None: |
| 283 | + self.config = self.branch.get_config() |
| 284 | + |
| 285 | + self._set_specific_file_ids() |
| 286 | + |
| 287 | + # Setup the progress bar. As the number of files that need to be |
| 288 | + # committed in unknown, progress is reported as stages. |
| 289 | + # We keep track of entries separately though and include that |
| 290 | + # information in the progress bar during the relevant stages. |
| 291 | + self.pb_stage_name = "" |
| 292 | + self.pb_stage_count = 0 |
| 293 | + self.pb_stage_total = 5 |
| 294 | + if self.bound_branch: |
| 295 | + self.pb_stage_total += 1 |
| 296 | + self.pb.show_pct = False |
| 297 | + self.pb.show_spinner = False |
| 298 | + self.pb.show_eta = False |
| 299 | + self.pb.show_count = True |
| 300 | + self.pb.show_bar = True |
| 301 | + |
| 302 | + self._gather_parents() |
| 303 | + # After a merge, a selected file commit is not supported. |
| 304 | + # See 'bzr help merge' for an explanation as to why. |
| 305 | + if len(self.parents) > 1 and self.specific_files is not None: |
| 306 | + raise errors.CannotCommitSelectedFileMerge(self.specific_files) |
| 307 | + # Excludes are a form of selected file commit. |
| 308 | + if len(self.parents) > 1 and self.exclude: |
| 309 | + raise errors.CannotCommitSelectedFileMerge(self.exclude) |
| 310 | + |
| 311 | + # Collect the changes |
| 312 | + self._set_progress_stage("Collecting changes", counter=True) |
| 313 | + self.builder = self.branch.get_commit_builder(self.parents, |
| 314 | + self.config, timestamp, timezone, committer, revprops, rev_id) |
| 315 | + |
| 316 | try: |
| 317 | - # Cannot commit with conflicts present. |
| 318 | - if len(self.work_tree.conflicts()) > 0: |
| 319 | - raise ConflictsInTree |
| 320 | - |
| 321 | - # Setup the bound branch variables as needed. |
| 322 | - self._check_bound_branch(possible_master_transports) |
| 323 | - |
| 324 | - # Check that the working tree is up to date |
| 325 | - old_revno, new_revno = self._check_out_of_date_tree() |
| 326 | - |
| 327 | - # Complete configuration setup |
| 328 | - if reporter is not None: |
| 329 | - self.reporter = reporter |
| 330 | - elif self.reporter is None: |
| 331 | - self.reporter = self._select_reporter() |
| 332 | - if self.config is None: |
| 333 | - self.config = self.branch.get_config() |
| 334 | - |
| 335 | - self._set_specific_file_ids() |
| 336 | - |
| 337 | - # Setup the progress bar. As the number of files that need to be |
| 338 | - # committed in unknown, progress is reported as stages. |
| 339 | - # We keep track of entries separately though and include that |
| 340 | - # information in the progress bar during the relevant stages. |
| 341 | - self.pb_stage_name = "" |
| 342 | - self.pb_stage_count = 0 |
| 343 | - self.pb_stage_total = 5 |
| 344 | - if self.bound_branch: |
| 345 | - self.pb_stage_total += 1 |
| 346 | - self.pb.show_pct = False |
| 347 | - self.pb.show_spinner = False |
| 348 | - self.pb.show_eta = False |
| 349 | - self.pb.show_count = True |
| 350 | - self.pb.show_bar = True |
| 351 | - |
| 352 | - self._gather_parents() |
| 353 | - # After a merge, a selected file commit is not supported. |
| 354 | - # See 'bzr help merge' for an explanation as to why. |
| 355 | - if len(self.parents) > 1 and self.specific_files is not None: |
| 356 | - raise errors.CannotCommitSelectedFileMerge(self.specific_files) |
| 357 | - # Excludes are a form of selected file commit. |
| 358 | - if len(self.parents) > 1 and self.exclude: |
| 359 | - raise errors.CannotCommitSelectedFileMerge(self.exclude) |
| 360 | - |
| 361 | - # Collect the changes |
| 362 | - self._set_progress_stage("Collecting changes", counter=True) |
| 363 | - self.builder = self.branch.get_commit_builder(self.parents, |
| 364 | - self.config, timestamp, timezone, committer, revprops, rev_id) |
| 365 | - |
| 366 | - try: |
| 367 | - self.builder.will_record_deletes() |
| 368 | - # find the location being committed to |
| 369 | - if self.bound_branch: |
| 370 | - master_location = self.master_branch.base |
| 371 | - else: |
| 372 | - master_location = self.branch.base |
| 373 | - |
| 374 | - # report the start of the commit |
| 375 | - self.reporter.started(new_revno, self.rev_id, master_location) |
| 376 | - |
| 377 | - self._update_builder_with_changes() |
| 378 | - self._check_pointless() |
| 379 | - |
| 380 | - # TODO: Now the new inventory is known, check for conflicts. |
| 381 | - # ADHB 2006-08-08: If this is done, populate_new_inv should not add |
| 382 | - # weave lines, because nothing should be recorded until it is known |
| 383 | - # that commit will succeed. |
| 384 | - self._set_progress_stage("Saving data locally") |
| 385 | - self.builder.finish_inventory() |
| 386 | - |
| 387 | - # Prompt the user for a commit message if none provided |
| 388 | - message = message_callback(self) |
| 389 | - self.message = message |
| 390 | - |
| 391 | - # Add revision data to the local branch |
| 392 | - self.rev_id = self.builder.commit(self.message) |
| 393 | - |
| 394 | - except Exception, e: |
| 395 | - mutter("aborting commit write group because of exception:") |
| 396 | - trace.log_exception_quietly() |
| 397 | - note("aborting commit write group: %r" % (e,)) |
| 398 | - self.builder.abort() |
| 399 | - raise |
| 400 | - |
| 401 | - self._process_pre_hooks(old_revno, new_revno) |
| 402 | - |
| 403 | - # Upload revision data to the master. |
| 404 | - # this will propagate merged revisions too if needed. |
| 405 | - if self.bound_branch: |
| 406 | - self._set_progress_stage("Uploading data to master branch") |
| 407 | - # 'commit' to the master first so a timeout here causes the |
| 408 | - # local branch to be out of date |
| 409 | - self.master_branch.import_last_revision_info( |
| 410 | - self.branch.repository, new_revno, self.rev_id) |
| 411 | - |
| 412 | - # and now do the commit locally. |
| 413 | - self.branch.set_last_revision_info(new_revno, self.rev_id) |
| 414 | - |
| 415 | - # Make the working tree be up to date with the branch. This |
| 416 | - # includes automatic changes scheduled to be made to the tree, such |
| 417 | - # as updating its basis and unversioning paths that were missing. |
| 418 | - self.work_tree.unversion(self.deleted_ids) |
| 419 | - self._set_progress_stage("Updating the working tree") |
| 420 | - self.work_tree.update_basis_by_delta(self.rev_id, |
| 421 | - self.builder.get_basis_delta()) |
| 422 | - self.reporter.completed(new_revno, self.rev_id) |
| 423 | - self._process_post_hooks(old_revno, new_revno) |
| 424 | - finally: |
| 425 | - self._cleanup() |
| 426 | + self.builder.will_record_deletes() |
| 427 | + # find the location being committed to |
| 428 | + if self.bound_branch: |
| 429 | + master_location = self.master_branch.base |
| 430 | + else: |
| 431 | + master_location = self.branch.base |
| 432 | + |
| 433 | + # report the start of the commit |
| 434 | + self.reporter.started(new_revno, self.rev_id, master_location) |
| 435 | + |
| 436 | + self._update_builder_with_changes() |
| 437 | + self._check_pointless() |
| 438 | + |
| 439 | + # TODO: Now the new inventory is known, check for conflicts. |
| 440 | + # ADHB 2006-08-08: If this is done, populate_new_inv should not add |
| 441 | + # weave lines, because nothing should be recorded until it is known |
| 442 | + # that commit will succeed. |
| 443 | + self._set_progress_stage("Saving data locally") |
| 444 | + self.builder.finish_inventory() |
| 445 | + |
| 446 | + # Prompt the user for a commit message if none provided |
| 447 | + message = message_callback(self) |
| 448 | + self.message = message |
| 449 | + |
| 450 | + # Add revision data to the local branch |
| 451 | + self.rev_id = self.builder.commit(self.message) |
| 452 | + |
| 453 | + except Exception, e: |
| 454 | + mutter("aborting commit write group because of exception:") |
| 455 | + trace.log_exception_quietly() |
| 456 | + note("aborting commit write group: %r" % (e,)) |
| 457 | + self.builder.abort() |
| 458 | + raise |
| 459 | + |
| 460 | + self._process_pre_hooks(old_revno, new_revno) |
| 461 | + |
| 462 | + # Upload revision data to the master. |
| 463 | + # this will propagate merged revisions too if needed. |
| 464 | + if self.bound_branch: |
| 465 | + self._set_progress_stage("Uploading data to master branch") |
| 466 | + # 'commit' to the master first so a timeout here causes the |
| 467 | + # local branch to be out of date |
| 468 | + self.master_branch.import_last_revision_info( |
| 469 | + self.branch.repository, new_revno, self.rev_id) |
| 470 | + |
| 471 | + # and now do the commit locally. |
| 472 | + self.branch.set_last_revision_info(new_revno, self.rev_id) |
| 473 | + |
| 474 | + # Make the working tree be up to date with the branch. This |
| 475 | + # includes automatic changes scheduled to be made to the tree, such |
| 476 | + # as updating its basis and unversioning paths that were missing. |
| 477 | + self.work_tree.unversion(self.deleted_ids) |
| 478 | + self._set_progress_stage("Updating the working tree") |
| 479 | + self.work_tree.update_basis_by_delta(self.rev_id, |
| 480 | + self.builder.get_basis_delta()) |
| 481 | + self.reporter.completed(new_revno, self.rev_id) |
| 482 | + self._process_post_hooks(old_revno, new_revno) |
| 483 | return self.rev_id |
| 484 | |
| 485 | def _select_reporter(self): |
| 486 | @@ -433,7 +458,7 @@ |
| 487 | return |
| 488 | raise PointlessCommit() |
| 489 | |
| 490 | - def _check_bound_branch(self, possible_master_transports=None): |
| 491 | + def _check_bound_branch(self, operation, possible_master_transports=None): |
| 492 | """Check to see if the local branch is bound. |
| 493 | |
| 494 | If it is bound, then most of the commit will actually be |
| 495 | @@ -474,7 +499,7 @@ |
| 496 | # so grab the lock |
| 497 | self.bound_branch = self.branch |
| 498 | self.master_branch.lock_write() |
| 499 | - self.master_locked = True |
| 500 | + operation.add_cleanup(self.master_branch.unlock) |
| 501 | |
| 502 | def _check_out_of_date_tree(self): |
| 503 | """Check that the working tree is up to date. |
| 504 | @@ -565,42 +590,6 @@ |
| 505 | old_revno, old_revid, new_revno, self.rev_id, |
| 506 | tree_delta, future_tree) |
| 507 | |
| 508 | - def _cleanup(self): |
| 509 | - """Cleanup any open locks, progress bars etc.""" |
| 510 | - cleanups = [self._cleanup_bound_branch, |
| 511 | - self.basis_tree.unlock, |
| 512 | - self.work_tree.unlock, |
| 513 | - self.pb.finished] |
| 514 | - found_exception = None |
| 515 | - for cleanup in cleanups: |
| 516 | - try: |
| 517 | - cleanup() |
| 518 | - # we want every cleanup to run no matter what. |
| 519 | - # so we have a catchall here, but we will raise the |
| 520 | - # last encountered exception up the stack: and |
| 521 | - # typically this will be useful enough. |
| 522 | - except Exception, e: |
| 523 | - found_exception = e |
| 524 | - if found_exception is not None: |
| 525 | - # don't do a plan raise, because the last exception may have been |
| 526 | - # trashed, e is our sure-to-work exception even though it loses the |
| 527 | - # full traceback. XXX: RBC 20060421 perhaps we could check the |
| 528 | - # exc_info and if its the same one do a plain raise otherwise |
| 529 | - # 'raise e' as we do now. |
| 530 | - raise e |
| 531 | - |
| 532 | - def _cleanup_bound_branch(self): |
| 533 | - """Executed at the end of a try/finally to cleanup a bound branch. |
| 534 | - |
| 535 | - If the branch wasn't bound, this is a no-op. |
| 536 | - If it was, it resents self.branch to the local branch, instead |
| 537 | - of being the master. |
| 538 | - """ |
| 539 | - if not self.bound_branch: |
| 540 | - return |
| 541 | - if self.master_locked: |
| 542 | - self.master_branch.unlock() |
| 543 | - |
| 544 | def _gather_parents(self): |
| 545 | """Record the parents of a merge for merge detection.""" |
| 546 | # TODO: Make sure that this list doesn't contain duplicate |
| 547 | |
| 548 | === modified file 'bzrlib/repository.py' |
| 549 | --- bzrlib/repository.py 2009-10-12 22:04:23 +0000 |
| 550 | +++ bzrlib/repository.py 2009-10-28 00:14:11 +0000 |
| 551 | @@ -50,7 +50,6 @@ |
| 552 | """) |
| 553 | |
| 554 | from bzrlib.decorators import needs_read_lock, needs_write_lock, only_raises |
| 555 | -from bzrlib.lock import _RelockDebugMixin |
| 556 | from bzrlib.inter import InterObject |
| 557 | from bzrlib.inventory import ( |
| 558 | Inventory, |
| 559 | @@ -58,6 +57,7 @@ |
| 560 | ROOT_ID, |
| 561 | entry_factory, |
| 562 | ) |
| 563 | +from bzrlib.lock import _RelockDebugMixin |
| 564 | from bzrlib import registry |
| 565 | from bzrlib.trace import ( |
| 566 | log_exception_quietly, note, mutter, mutter_callsite, warning) |
| 567 | |
| 568 | === modified file 'bzrlib/tests/__init__.py' |
| 569 | --- bzrlib/tests/__init__.py 2009-10-19 10:59:16 +0000 |
| 570 | +++ bzrlib/tests/__init__.py 2009-10-28 00:14:11 +0000 |
| 571 | @@ -3722,6 +3722,7 @@ |
| 572 | 'bzrlib.tests.test_chk_serializer', |
| 573 | 'bzrlib.tests.test_chunk_writer', |
| 574 | 'bzrlib.tests.test_clean_tree', |
| 575 | + 'bzrlib.tests.test_cleanup', |
| 576 | 'bzrlib.tests.test_commands', |
| 577 | 'bzrlib.tests.test_commit', |
| 578 | 'bzrlib.tests.test_commit_merge', |
| 579 | |
| 580 | === added file 'bzrlib/tests/test_cleanup.py' |
| 581 | --- bzrlib/tests/test_cleanup.py 1970-01-01 00:00:00 +0000 |
| 582 | +++ bzrlib/tests/test_cleanup.py 2009-10-28 00:14:11 +0000 |
| 583 | @@ -0,0 +1,280 @@ |
| 584 | +# Copyright (C) 2009 Canonical Ltd |
| 585 | +# |
| 586 | +# This program is free software; you can redistribute it and/or modify |
| 587 | +# it under the terms of the GNU General Public License as published by |
| 588 | +# the Free Software Foundation; either version 2 of the License, or |
| 589 | +# (at your option) any later version. |
| 590 | +# |
| 591 | +# This program is distributed in the hope that it will be useful, |
| 592 | +# but WITHOUT ANY WARRANTY; without even the implied warranty of |
| 593 | +# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the |
| 594 | +# GNU General Public License for more details. |
| 595 | +# |
| 596 | +# You should have received a copy of the GNU General Public License |
| 597 | +# along with this program; if not, write to the Free Software |
| 598 | +# Foundation, Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301 USA |
| 599 | + |
| 600 | +from cStringIO import StringIO |
| 601 | +import re |
| 602 | + |
| 603 | +from bzrlib.cleanup import ( |
| 604 | + _do_with_cleanups, |
| 605 | + _run_cleanup, |
| 606 | + OperationWithCleanups, |
| 607 | + ) |
| 608 | +from bzrlib.tests import TestCase |
| 609 | +from bzrlib import ( |
| 610 | + debug, |
| 611 | + trace, |
| 612 | + ) |
| 613 | + |
| 614 | + |
| 615 | +class CleanupsTestCase(TestCase): |
| 616 | + |
| 617 | + def setUp(self): |
| 618 | + super(CleanupsTestCase, self).setUp() |
| 619 | + self.call_log = [] |
| 620 | + |
| 621 | + def no_op_cleanup(self): |
| 622 | + self.call_log.append('no_op_cleanup') |
| 623 | + |
| 624 | + def assertLogContains(self, regex): |
| 625 | + log = self._get_log(keep_log_file=True) |
| 626 | + self.assertContainsRe(log, regex, re.DOTALL) |
| 627 | + |
| 628 | + def failing_cleanup(self): |
| 629 | + self.call_log.append('failing_cleanup') |
| 630 | + raise Exception("failing_cleanup goes boom!") |
| 631 | + |
| 632 | + |
| 633 | +class TestRunCleanup(CleanupsTestCase): |
| 634 | + |
| 635 | + def test_no_errors(self): |
| 636 | + """The function passed to _run_cleanup is run.""" |
| 637 | + self.assertTrue(_run_cleanup(self.no_op_cleanup)) |
| 638 | + self.assertEqual(['no_op_cleanup'], self.call_log) |
| 639 | + |
| 640 | + def test_cleanup_with_args_kwargs(self): |
| 641 | + def func_taking_args_kwargs(*args, **kwargs): |
| 642 | + self.call_log.append(('func', args, kwargs)) |
| 643 | + _run_cleanup(func_taking_args_kwargs, 'an arg', kwarg='foo') |
| 644 | + self.assertEqual( |
| 645 | + [('func', ('an arg',), {'kwarg': 'foo'})], self.call_log) |
| 646 | + |
| 647 | + def test_cleanup_error(self): |
| 648 | + """An error from the cleanup function is logged by _run_cleanup, but not |
| 649 | + propagated. |
| 650 | + |
| 651 | + This is there's no way for _run_cleanup to know if there's an existing |
| 652 | + exception in this situation:: |
| 653 | + try: |
| 654 | + some_func() |
| 655 | + finally: |
| 656 | + _run_cleanup(cleanup_func) |
| 657 | + So, the best _run_cleanup can do is always log errors but never raise |
| 658 | + them. |
| 659 | + """ |
| 660 | + self.assertFalse(_run_cleanup(self.failing_cleanup)) |
| 661 | + self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom') |
| 662 | + |
| 663 | + def test_cleanup_error_debug_flag(self): |
| 664 | + """The -Dcleanup debug flag causes cleanup errors to be reported to the |
| 665 | + user. |
| 666 | + """ |
| 667 | + log = StringIO() |
| 668 | + trace.push_log_file(log) |
| 669 | + debug.debug_flags.add('cleanup') |
| 670 | + self.assertFalse(_run_cleanup(self.failing_cleanup)) |
| 671 | + self.assertContainsRe( |
| 672 | + log.getvalue(), |
| 673 | + "bzr: warning: Cleanup failed:.*failing_cleanup goes boom") |
| 674 | + |
| 675 | + def test_prior_error_cleanup_succeeds(self): |
| 676 | + """Calling _run_cleanup from a finally block will not interfere with an |
| 677 | + exception from the try block. |
| 678 | + """ |
| 679 | + def failing_operation(): |
| 680 | + try: |
| 681 | + 1/0 |
| 682 | + finally: |
| 683 | + _run_cleanup(self.no_op_cleanup) |
| 684 | + self.assertRaises(ZeroDivisionError, failing_operation) |
| 685 | + self.assertEqual(['no_op_cleanup'], self.call_log) |
| 686 | + |
| 687 | + def test_prior_error_cleanup_fails(self): |
| 688 | + """Calling _run_cleanup from a finally block will not interfere with an |
| 689 | + exception from the try block even when the cleanup itself raises an |
| 690 | + exception. |
| 691 | + |
| 692 | + The cleanup exception will be logged. |
| 693 | + """ |
| 694 | + def failing_operation(): |
| 695 | + try: |
| 696 | + 1/0 |
| 697 | + finally: |
| 698 | + _run_cleanup(self.failing_cleanup) |
| 699 | + self.assertRaises(ZeroDivisionError, failing_operation) |
| 700 | + self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom') |
| 701 | + |
| 702 | + |
| 703 | +class TestDoWithCleanups(CleanupsTestCase): |
| 704 | + |
| 705 | + def trivial_func(self): |
| 706 | + self.call_log.append('trivial_func') |
| 707 | + return 'trivial result' |
| 708 | + |
| 709 | + def test_runs_func(self): |
| 710 | + """_do_with_cleanups runs the function it is given, and returns the |
| 711 | + result. |
| 712 | + """ |
| 713 | + result = _do_with_cleanups([], self.trivial_func) |
| 714 | + self.assertEqual('trivial result', result) |
| 715 | + |
| 716 | + def test_runs_cleanups(self): |
| 717 | + """Cleanup functions are run (in the given order).""" |
| 718 | + cleanup_func_1 = (self.call_log.append, ('cleanup 1',), {}) |
| 719 | + cleanup_func_2 = (self.call_log.append, ('cleanup 2',), {}) |
| 720 | + _do_with_cleanups([cleanup_func_1, cleanup_func_2], self.trivial_func) |
| 721 | + self.assertEqual( |
| 722 | + ['trivial_func', 'cleanup 1', 'cleanup 2'], self.call_log) |
| 723 | + |
| 724 | + def failing_func(self): |
| 725 | + self.call_log.append('failing_func') |
| 726 | + 1/0 |
| 727 | + |
| 728 | + def test_func_error_propagates(self): |
| 729 | + """Errors from the main function are propagated (after running |
| 730 | + cleanups). |
| 731 | + """ |
| 732 | + self.assertRaises( |
| 733 | + ZeroDivisionError, _do_with_cleanups, |
| 734 | + [(self.no_op_cleanup, (), {})], self.failing_func) |
| 735 | + self.assertEqual(['failing_func', 'no_op_cleanup'], self.call_log) |
| 736 | + |
| 737 | + def test_func_error_trumps_cleanup_error(self): |
| 738 | + """Errors from the main function a propagated even if a cleanup raises |
| 739 | + an error. |
| 740 | + |
| 741 | + The cleanup error is be logged. |
| 742 | + """ |
| 743 | + self.assertRaises( |
| 744 | + ZeroDivisionError, _do_with_cleanups, |
| 745 | + [(self.failing_cleanup, (), {})], self.failing_func) |
| 746 | + self.assertLogContains('Cleanup failed:.*failing_cleanup goes boom') |
| 747 | + |
| 748 | + def test_func_passes_and_error_from_cleanup(self): |
| 749 | + """An error from a cleanup is propagated when the main function doesn't |
| 750 | + raise an error. Later cleanups are still executed. |
| 751 | + """ |
| 752 | + exc = self.assertRaises( |
| 753 | + Exception, _do_with_cleanups, |
| 754 | + [(self.failing_cleanup, (), {}), (self.no_op_cleanup, (), {})], |
| 755 | + self.trivial_func) |
| 756 | + self.assertEqual('failing_cleanup goes boom!', exc.args[0]) |
| 757 | + self.assertEqual( |
| 758 | + ['trivial_func', 'failing_cleanup', 'no_op_cleanup'], |
| 759 | + self.call_log) |
| 760 | + |
| 761 | + def test_multiple_cleanup_failures(self): |
| 762 | + """When multiple cleanups fail (as tends to happen when something has |
| 763 | + gone wrong), the first error is propagated, and subsequent errors are |
| 764 | + logged. |
| 765 | + """ |
| 766 | + cleanups = self.make_two_failing_cleanup_funcs() |
| 767 | + self.assertRaises(ErrorA, _do_with_cleanups, cleanups, |
| 768 | + self.trivial_func) |
| 769 | + self.assertLogContains('Cleanup failed:.*ErrorB') |
| 770 | + log = self._get_log(keep_log_file=True) |
| 771 | + self.assertFalse('ErrorA' in log) |
| 772 | + |
| 773 | + def make_two_failing_cleanup_funcs(self): |
| 774 | + def raise_a(): |
| 775 | + raise ErrorA('Error A') |
| 776 | + def raise_b(): |
| 777 | + raise ErrorB('Error B') |
| 778 | + return [(raise_a, (), {}), (raise_b, (), {})] |
| 779 | + |
| 780 | + def test_multiple_cleanup_failures_debug_flag(self): |
| 781 | + log = StringIO() |
| 782 | + trace.push_log_file(log) |
| 783 | + debug.debug_flags.add('cleanup') |
| 784 | + cleanups = self.make_two_failing_cleanup_funcs() |
| 785 | + self.assertRaises(ErrorA, _do_with_cleanups, cleanups, |
| 786 | + self.trivial_func) |
| 787 | + self.assertContainsRe( |
| 788 | + log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n") |
| 789 | + self.assertEqual(1, log.getvalue().count('bzr: warning:'), |
| 790 | + log.getvalue()) |
| 791 | + |
| 792 | + def test_func_and_cleanup_errors_debug_flag(self): |
| 793 | + log = StringIO() |
| 794 | + trace.push_log_file(log) |
| 795 | + debug.debug_flags.add('cleanup') |
| 796 | + cleanups = self.make_two_failing_cleanup_funcs() |
| 797 | + self.assertRaises(ZeroDivisionError, _do_with_cleanups, cleanups, |
| 798 | + self.failing_func) |
| 799 | + self.assertContainsRe( |
| 800 | + log.getvalue(), "bzr: warning: Cleanup failed:.*Error A\n") |
| 801 | + self.assertContainsRe( |
| 802 | + log.getvalue(), "bzr: warning: Cleanup failed:.*Error B\n") |
| 803 | + self.assertEqual(2, log.getvalue().count('bzr: warning:')) |
| 804 | + |
| 805 | + def test_func_may_mutate_cleanups(self): |
| 806 | + """The main func may mutate the cleanups before it returns. |
| 807 | + |
| 808 | + This allows a function to gradually add cleanups as it acquires |
| 809 | + resources, rather than planning all the cleanups up-front. The |
| 810 | + OperationWithCleanups helper relies on this working. |
| 811 | + """ |
| 812 | + cleanups_list = [] |
| 813 | + def func_that_adds_cleanups(): |
| 814 | + self.call_log.append('func_that_adds_cleanups') |
| 815 | + cleanups_list.append((self.no_op_cleanup, (), {})) |
| 816 | + return 'result' |
| 817 | + result = _do_with_cleanups(cleanups_list, func_that_adds_cleanups) |
| 818 | + self.assertEqual('result', result) |
| 819 | + self.assertEqual( |
| 820 | + ['func_that_adds_cleanups', 'no_op_cleanup'], self.call_log) |
| 821 | + |
| 822 | + def test_cleanup_error_debug_flag(self): |
| 823 | + """The -Dcleanup debug flag causes cleanup errors to be reported to the |
| 824 | + user. |
| 825 | + """ |
| 826 | + log = StringIO() |
| 827 | + trace.push_log_file(log) |
| 828 | + debug.debug_flags.add('cleanup') |
| 829 | + self.assertRaises(ZeroDivisionError, _do_with_cleanups, |
| 830 | + [(self.failing_cleanup, (), {})], self.failing_func) |
| 831 | + self.assertContainsRe( |
| 832 | + log.getvalue(), |
| 833 | + "bzr: warning: Cleanup failed:.*failing_cleanup goes boom") |
| 834 | + self.assertEqual(1, log.getvalue().count('bzr: warning:')) |
| 835 | + |
| 836 | + |
| 837 | +class ErrorA(Exception): pass |
| 838 | +class ErrorB(Exception): pass |
| 839 | + |
| 840 | + |
| 841 | +class TestOperationWithCleanups(CleanupsTestCase): |
| 842 | + |
| 843 | + def test_cleanup_ordering(self): |
| 844 | + """Cleanups are added in LIFO order. |
| 845 | + |
| 846 | + So cleanups added before run is called are run last, and the last |
| 847 | + cleanup added during the func is run first. |
| 848 | + """ |
| 849 | + call_log = [] |
| 850 | + def func(op, foo): |
| 851 | + call_log.append(('func called', foo)) |
| 852 | + op.add_cleanup(call_log.append, 'cleanup 2') |
| 853 | + op.add_cleanup(call_log.append, 'cleanup 1') |
| 854 | + return 'result' |
| 855 | + owc = OperationWithCleanups(func) |
| 856 | + owc.add_cleanup(call_log.append, 'cleanup 4') |
| 857 | + owc.add_cleanup(call_log.append, 'cleanup 3') |
| 858 | + result = owc.run('foo') |
| 859 | + self.assertEqual('result', result) |
| 860 | + self.assertEqual( |
| 861 | + [('func called', 'foo'), 'cleanup 1', 'cleanup 2', 'cleanup 3', |
| 862 | + 'cleanup 4'], call_log) |
| 863 | + |
This branch makes commit.py simpler, shorter, and more robust.
It adds a simple OperationWithCl
This should make failures during commit (e.g. due to unexpected network issues during a commit of a lightweight checkout of a remote branch) always cleanup correctly *and* always report the original error.
Combined with the @only_raises(...) decorator that's already landed, this is the most valuable parts of the better cleanup code experiments in lp:~spiv/bzr/cleanup-hof. So this branch more-or-less supersedes cleanup-hof, which is now removed from the review queue.