On Mon, May 4, 2009 at 7:08 AM, Aaron Bentley <span dir="ltr"><<a href="mailto:aaron@aaronbentley.com">aaron@aaronbentley.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Of these, only diff and merge use external programs, so I don't see how<br>
the rest can be subject to the same race condition. Unless what you<br>
mean is that any arbitrary deletion on Windows can fail because some<br>
random program may be holding a file open.<br>
<div class="im"></div></blockquote><div class="im"><br><span style="color: rgb(51, 0, 153);">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.<br>
<br>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. :)</span><br style="color: rgb(51, 0, 153);">
<br>
> (A) still give several chances in _win323_delete_readonly because the<br>
> best and most maintainable fixes are where the problem lies;<br>
<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Not yet convinced.<br>
<div class="im"></div></blockquote><div><br><span style="color: rgb(51, 0, 153);">I understand your opinion. Again, I accept that my paranoia is more than bzr needs.</span><br> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im"><br>
> (B) raise only generic exceptions in osutils<br>
<br>
</div>Don't understand. If you fail to handle the error, you should raise it.<br>
<div class="im"></div></blockquote><div class="im"><br><span style="color: rgb(51, 0, 153);">This really only made sense if (A) made sense. Since we are rejecting (A) we also agree to reject (B).</span><br> <br>
> (C) defer exception handling to the callers (diff, merge, remote,<br>
> /etc./) because those routines may need to do some cleanup and only<br>
> those routines know the "context" of the failed rmtree.<br>
><br>
> So I need someone to tell me how to formally withdraw/reject my request<br>
> at bundlebuggy<br>
<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">Actually, only reviewers can do that. I've just done it for you.<br><div class="im">
</div></blockquote><div><br><span style="color: rgb(51, 0, 153);">Thanks.</span><br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div class="im">
> and I need feedback on plan ABC above.<br>
<br>
</div>Please catch specific exceptions, not using a bare except clause. Many<br>
causes of deletion failure are unrecoverable (file does not exist, file<br>
is a directory, etc), so they should propagate immediately.</blockquote><div><br><span style="color: rgb(51, 0, 153);">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. </span><br style="color: rgb(51, 0, 153);">
</div><div> </div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
Because OSError and WinError use error codes, I typically write code like:<br>
<br>
try:<br>
os.unlink('foo')<br>
except OSError, e:<br>
if e.errno != errno.ENOENT:<br>
raise</blockquote><div><br><span style="color: rgb(51, 0, 153);">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.</span><br style="color: rgb(51, 0, 153);">
<br style="color: rgb(51, 0, 153);"><span style="color: rgb(51, 0, 153);">Thank you, Alexander, for an informative and helpful discussion.</span><br style="color: rgb(51, 0, 153);"><br style="color: rgb(51, 0, 153);"><span style="color: rgb(51, 0, 153);">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.</span><br style="color: rgb(51, 0, 153);">
<br style="color: rgb(51, 0, 153);"><span style="color: rgb(51, 0, 153);">Does anyone else have opinions on this?</span><br style="color: rgb(51, 0, 153);"><br style="color: rgb(51, 0, 153);"><span style="color: rgb(51, 0, 153);">Gracias!</span><br style="color: rgb(51, 0, 153);">
<span style="color: rgb(51, 0, 153);">-M</span><br><br></div></div><br>