[MERGE] [BUG 363837] Catch _win32_delete_readonly failure to remove file or directory and try to recover

[Aaron Bentley]

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

martitzam at gmail.com wrote:
> 
> 
> On Fri, May 1, 2009 at 11:50 PM, Alexander Belchenko <bialix at ukr.net
> <mailto:bialix at ukr.net>> wrote:
> 
> 
>     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.
> 
> 
> 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)
> 
> clean_tree.py:                shutil.rmtree(path)
> diff.py:        osutils.rmtree(self._root)
> merge.py:            osutils.rmtree(temp_dir)
> remote.py:                osutils.rmtree(tmpdir)
> shelf_ui.py:            shutil.rmtree(self.tempdir)
> workingtree.py:                            osutils.rmtree(abs_path)
> 
> I concluded those cases are just as susceptible
> to a race condition as diff.

Of these, only diff and merge use external programs, so I don't see how
the rest can be subject to the same race condition.  Unless what you
mean is that any arbitrary deletion on Windows can fail because some
random program may be holding a file open.

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

Not yet convinced.

> (B) raise only generic exceptions in osutils

Don't understand.  If you fail to handle the error, you should raise it.

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

Actually, only reviewers can do that.  I've just done it for you.

> and I need feedback on plan ABC above.

Please catch specific exceptions, not using a bare except clause.  Many
causes of deletion failure are unrecoverable (file does not exist, file
is a directory, etc), so they should propagate immediately.

Because OSError and WinError use error codes, I typically write code like:

try:
    os.unlink('foo')
except OSError, e:
    if e.errno != errno.ENOENT:
        raise

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

iEYEARECAAYFAkn+9r4ACgkQ0F+nu1YWqI1skgCfeRgQXLf1CFp4WenO232djQM4
5ZcAnAuLllfFuXb8RIrj+2mKWZ2U96xm
=Wcr4
-----END PGP SIGNATURE-----