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

[Maritza Mendez]

Aaron y Alexander...  Sorry!  I have been talking with both of you about
this, and I misattributed some of Aaron's comments to Alexander.  I
apologize for the error!

Thank you both!

-M


On Mon, May 4, 2009 at 1:35 PM, Maritza Mendez <martitzam at gmail.com> wrote:

> On Mon, May 4, 2009 at 7:08 AM, Aaron Bentley <aaron at aaronbentley.com>wrote:
>
>> 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.
>>
>
> Yes, that's exactly what I meant.  I don't think any of us would accept
> this behavior on Unix, and support for Windows should be held to the same
> standard.  For me this comes down to whether bzr is viable in combination
> with common Windows environments.
>
> I'm also conditioned to catch these things as soon as possible, before they
> propagate.  This is a low-level problem which should be prevented whenever
> possible.  In my line of work, low-level problems which are allowed to
> propagate (and perhaps not caught by every caller) have been known to cost
> lives, so I'm paranoid.  Of course we don't write production code in python,
> either...  But that level of paranoia is not justified for bzr.  :)
>
> > (A) still give several chances in _win323_delete_readonly because the
> > best and most maintainable fixes are where the problem lies;
>
> Not yet convinced.
>>
>
> I understand your opinion.  Again, I accept that my paranoia is more than
> bzr needs.
>
>
>>
>> > (B) raise only generic exceptions in osutils
>>
>> Don't understand.  If you fail to handle the error, you should raise it.
>>
>
> This really only made sense if (A) made sense.  Since we are rejecting (A)
> we also agree to reject (B).
>
> > (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.
>>
>
> Thanks.
>
>  > 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.
>
>
> That's really my point also.  My objections to what I found in osutils are
> (1) a bare exception comes back to the caller (notice the bare "raise" in
> the released code) and (2) no attempt to do anything useful about it is
> made.  I see why you don't like my first attempt at improving this, and I
> think we can still do better.
>
>
>> 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
>
>
> So I am prepared to create and submit a different patch when I get spare
> time.  I still intend to add second-chance code with a small delay, but
> these deltas will then be in the high-level callers (diff and merge).  This
> will result in some duplication and maybe some small obfuscation.  It also
> means that diff and merge will deal with a low-level problem which so far
> has been seen only on Windows.  I suspect someone will have a strong opinion
> on this.  Ultimately, all I need is *one* acceptable solution.  It does not
> have to be my preferred solution.
>
> Thank you, Alexander, for an informative and helpful discussion.
>
> To recap, my original patch was based on the opinion that the best place to
> mitigate a low-level win32 error is in the low-level win32 layer so that
> high-level code becomes involved only if the low-level can't handle it.
> Alexander makes a good counter-proposal, albeit spilling over into non-win32
> code.
>
> Does anyone else have opinions on this?
>
> Gracias!
> -M
>
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: https://lists.ubuntu.com/archives/bazaar/attachments/20090504/75731435/attachment-0001.htm