<div dir="ltr">Eric, David here is a patch review:<br><br><div class="gmail_quote">On Sun, Oct 19, 2008 at 11:08 PM, Salvatore Insalaco <span dir="ltr"><<a href="mailto:kirby81@gmail.com">kirby81@gmail.com</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">I'm sorry for not using darcs send: unfortunately my mail are blocked<br>
by the mailing list spam filter.<br>
<br>
This patch should hopefully fix issue 784 (Windows complaining while<br>
paging the help).<br>
<br>
There where three things involved:<br>
1) There is a file handle leak in pipeDocToPager when an exception is<br>
raised. This is the direct cause of the "permission denied" errors, as<br>
you can't delete an opened file in Windows.</blockquote><div><br>Thanks for spotting that. Adding bracket looks like a good idea. This would be yet another place where we could use either iteratees or light weight monadic regions to make this resource leak impossible! We could use an Oleg refactoring day where we apply cool things he's come up with :)<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><br>
2) ortryrunning doesn't check for exceptions: in Windows a failed<br>
runProcess returns an exception.</blockquote><div> </div><div>So it looks like you made this changes:<br>+ortryrunning :: IO ExitCode -> IO ExitCode -> IO ExitCode<br>+a `ortryrunning` b = do ret <- try a<br>+ case ret of<br>
+ (Right ExitSuccess) -> return ExitSuccess<br>+ _ -> b<br><br>Just checking that I understand, this could also be:<br>a `ortryrunning` b = try a >>= either (const b) return<br>
<br>I didn't check to make sure that passes the type checker. The important bit is that you added 'try' it would seem.<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;">
<br>
3) runProcess on Windows require explicit ".com" suffix for .com<br>
executables. We should add "<a href="http://more.com" target="_blank">more.com</a>" to the list of possible pagers.</blockquote><div><br>I think others have looked into which program to execute so I'll let the expert handle this. <br>
<br></div></div>The changes look fine to me and if Salvatore says they work and the build bots keep running then I say it should go in!<br><br>Thanks Salvatore.<br><br>Jason<br></div>