[darcs-users] DRAFT: comments on hPut and hGet in source code

[Eric Kow]

Hmm, I should actually have sent that to darcs-users instead of the
patch tracker

Following up on myself, looking more carefully at this, I get the
impression that nothing here is release-critical.  So if nobody says
anything, I'll put 2.4.4 online for packagers by tomorrow.

Let this one be right this time...

On Sat, May 15, 2010 at 10:44:51 +0000, Eric Kow wrote:
> Is there anybody who can spare a moment to look at what I've found in my
> quick IO audit?

I wonder if there's something we need to be doing in the bigger picture
so that these sorts of audits aren't needed.  Seems like we have a hard
time knowing what our IO is doing :-(

>            append_info f oldname =
>                do fc <- readBinFile f
> +                 -- AUDIT: seems incongruous that that we're reading bin file
> +                 -- and writing back out in text mode

appendToFile uses bin mode underneath, so that's OK

> hunk ./src/Darcs/External.hs 321
>         (i,o,e,pid) <- runInteractiveProcess c args Nothing Nothing
>         hSetBinaryMode i True
>         hSetBinaryMode o True
> +       -- AUDIT: should e be also in binary mode (probably doesn't matter)
> +       -- what about stdout and stderr?

This is in pipeDoc... and the answer doesn't really seem to affect
correctness (just user output)

> hunk ./src/Darcs/External.hs 364
>       putHeader "Subject" s
>       when (not (null cc)) (putHeader "Cc" cc)
>       putHeader "X-Mail-Originator" "Darcs Version Control System"
> +     -- AUDIT: should this be binary mode? what's the body?
> +     -- if the body is string-based it will use hPutStrLn underneath
> +     -- if the body is PS-based, it will use hPut
>       hPutDocLn h body

This is generateEmail.  All the uses I see for generate email open the
handle in bin mode.  Maybe that's something we should haddock?

'generateEmail expects handles open in bin mode'

> hunk ./src/Darcs/External.hs 505
>  execDocPipe :: String -> [String] -> Doc -> IO Doc
>  execDocPipe c args instr = withoutProgress $
>      do (i,o,e,pid) <- runInteractiveProcess c args Nothing Nothing
> +       -- AUDIT: seems like we should also set ioe binary mode here
> +       -- in case we have string-based docs underneath
>         forkIO $ hPutDoc i instr >> hClose i
>         mvare <- newEmptyMVar
>         forkIO ((Ratified.hGetContents e >>= -- ratify: immediately consumed

This is used for signPGP stuff

> hunk ./src/Darcs/External.hs 525
>  execPipeIgnoreError :: String -> [String] -> Doc -> IO Doc
>  execPipeIgnoreError c args instr = withoutProgress $
>      do (i,o,e,pid) <- runInteractiveProcess c args Nothing Nothing
> +       -- AUDIT: seems like we should also set ioe binary mode here
>         forkIO $ hPutDoc i instr >> hClose i
>         mvare <- newEmptyMVar
>         forkIO ((Ratified.hGetContents e >>= -- ratify: immediately consumed

This is used for talking to external diff tools by the Darcs diff
command.

>  -- | @hPrintPrintable h@ prints a 'Printable' to the handle h.
>  hPrintPrintable :: Handle -> Printable -> IO ()
> -hPrintPrintable h (S ps) = hPutStr h ps
> +hPrintPrintable h (S ps) = hPutStr h ps -- AUDIT: is this really the right thing?
>  hPrintPrintable h (PS ps) = B.hPut h ps
>  hPrintPrintable h (Both _ ps) = B.hPut h ps

Seems like it's your business how you open the handle here

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 195 bytes
Desc: not available
URL: <http://lists.osuosl.org/pipermail/darcs-users/attachments/20100515/0db144bd/attachment.pgp>