[darcs-users] problem with --sendmail-command
benjamin.franksen at bessy.de
Tue Oct 7 11:36:13 UTC 2008
Eric Kow wrote:
> On Tue, Oct 07, 2008 at 01:12:32 +0200, Ben Franksen wrote:
>> ...or so I thought. I find the code in Darcs.Command.Send resp
>> Darcs.Externals.hs very, ahem, hard to understand.
> I have always wondered if there was some way we could clean up this
> function. I have a sneaking suspicion that its arguments list is
> complicated because of how we call it, and how we call it is complicated
> of the arguments list... i.e. that there is some sort of self-cancelling
> complexity that we can work on between the two functions. Maybe one
> good refactor would be to generateEmail to a document instead of to a
> handle. I think Florent might be interested in that too.
This is easily done, and is one of the more obvious improvements. There are
more problems. One is that the argument 'body' to sendEmailDoc in fact
contains header lines, see Darcs.Email.make_email which is called e.g. in
Darcs.Commands.Send.send_to_them. It also may contain the patch bundle
inlined into the body. At other call sites the bundle is attached
separately, passed in argument 'mbundle' (which may be Nothing). It is
unclear to me why darcs supports both ways to send patches and what the
heuristics are to chose one method over the other. In any case, unifying
and cleaning up the code in Darcs.Email, Darcs.Externals, etc seems like a
good idea. I'll see if I can come up with something...
>> Anyway, I /think/ there is a problem in sendEmailDoc calling
>> execSendmail, because execSendmail gets a 'fn' argument (last arg) and a
>> /different/ filename (from yet another invocation of withtempFile) via
>> the ftable (index 'a' which I suppose is connected to the %a
>> replacement), /if/ mbundle is not Nothing. There are all-too-many
>> withTempFile calls there and I think filenames for all these temp files
>> got badly mixed up somehow...
> So two temp files get created, as I can see. The 'fn' file is written
> to by generateEmail, and the 'a' file contains just the bundle.
Yes, this is ok, I was just confused. What is not ok is that execSendmail
gets called /outside/ the body of the withOpenTemp that creates the
attached file. That is, the file gets deleted before the sendmail command
is called. I have a patch that should fix this, which I will send later
today (after some testing). This bug, BTW, nicely illustrates a weakness of
the withXYZ idiom: it cannot statically prevent that some resource
allocated (in this case the name of the tempfile) 'escapes' the body, i.e.
its scope of validity. I strongly support the idea to investigate whether
parts of the darcs code can be refactored to use Oleg's latest enumerator
stuff, as suggested recently on this list.
More information about the darcs-users