[darcs-users] darcs patch: resolve issue784: fix file handle leak and check for exceptions on process running.

[Jason Dagit]

Eric, David here is a patch review:

On Sun, Oct 19, 2008 at 11:08 PM, Salvatore Insalaco <kirby81 at gmail.com>wrote:

> I'm sorry for not using darcs send: unfortunately my mail are blocked
> by the mailing list spam filter.
>
> This patch should hopefully fix issue 784 (Windows complaining while
> paging the help).
>
> There where three things involved:
> 1) There is a file handle leak in pipeDocToPager when an exception is
> raised. This is the direct cause of the "permission denied" errors, as
> you can't delete an opened file in Windows.


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 :)


>
> 2) ortryrunning doesn't check for exceptions: in Windows a failed
> runProcess returns an exception.


So it looks like you made this changes:
+ortryrunning :: IO ExitCode -> IO ExitCode -> IO ExitCode
+a `ortryrunning` b = do ret <- try a
+                        case ret of
+                          (Right ExitSuccess) -> return ExitSuccess
+                          _ -> b

Just checking that I understand, this could also be:
a `ortryrunning` b = try a >>= either (const b) return

I didn't check to make sure that passes the type checker.  The important bit
is that you added 'try' it would seem.


> 3) runProcess on Windows require explicit ".com" suffix for .com
> executables. We should add "more.com" to the list of possible pagers.


I think others have looked into which program to execute so I'll let the
expert handle this.

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!

Thanks Salvatore.

Jason
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.osuosl.org/pipermail/darcs-users/attachments/20081019/9b96069a/attachment.htm