<div class="gmail_quote">On Fri, May 1, 2009 at 11:50 PM, Alexander Belchenko <<a href="mailto:bialix@ukr.net" target="_blank">bialix@ukr.net</a>> wrote: <blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"> You have modified base behavior of delete on win32. I think it's the wrong approach. Instead you'd better catch the error in the place where it occurs an print fine error message.</blockquote><div> Alexander:  Thanks for your feedback.  I believe I understand your comment.  I designed my patch this way for the following reasons which I should have mentioned sooner. First, I read the HACKING file which specifically recommends using the win32-specific rmtree replacement for shutil.rmtree in all cases.  This alerted me that any change to osutil.rmtree (for sysplatform=='win32') must be suitable for all win32 use-cases. Next, I grepped for all references to osutils.rmtree in bzrlib.  What I found was (besides osutil.py) <div style="margin-left: 40px;">clean_tree.py:                shutil.rmtree(path) </div><div style="margin-left: 40px;">diff.py:        osutils.rmtree(self._root) </div><div style="margin-left: 40px;">merge.py:            osutils.rmtree(temp_dir) remote.py:                osutils.rmtree(tmpdir) shelf_ui.py:            shutil.rmtree(self.tempdir) workingtree.py:                            osutils.rmtree(abs_path) </div> Your recommendation (as I understand it) is to catch the problem only in diff.py.  That was my initial plan, also.  But then I actually looked at the other uses of osutils.rmtree and concluded those cases are just as susceptible to a race condition as diff.  The fact that the race condition has (so far) manifested only in diff notwithstanding, the existence of a race condiion is intrinsic to all of these.  Because race conditions can be hard to debug, I thought it would be good to catch and correct them all at the source.  I can see how the scope of the patch being larger than the scope of the bug report can and should raise concern. I am beginning to see your point better, also.  By raising a BzrError in osutils.py, my patch denies the (present and future) callers of rmtree the opportunity to do any cleanup.  That is bad design.  Even if the error is unrecoverable, the caller may still need to clean up. So I withdraw my request and will submit another.  Here is my new plan, I would appreciate everyone's comments: (A) still give several chances in _win323_delete_readonly because the best and most maintainable fixes are where the problem lies; but (B) raise only generic exceptions in osutils and (C) defer exception handling to the callers (diff, merge, remote, etc.) because those routines may need to do some cleanup and only those routines know the "context" of the failed rmtree. So I need someone to tell me how to formally withdraw/reject my request at bundlebuggy and I need feedback on plan ABC above. Thanks, Martitza </div></div>