Merge lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt into lp:~bzr/bzr/trunk-old

Proposed by John A Meinel on 2009-06-08
Status: Merged
Merged at revision: not available
Proposed branch: lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt
Merge into: lp:~bzr/bzr/trunk-old
Diff against target: 50 lines
To merge this branch: bzr merge lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt
Reviewer Review Type Date Requested Status
John A Meinel Approve on 2009-06-08
Review via email: mp+7200@code.launchpad.net

This proposal supersedes a proposal from 2009-06-06.

To post a comment you must log in.
Edwin Grubbs (edwin-grubbs) wrote : Posted in a previous version of this proposal

This branch adds the ability to pass the email body into mutt. I tried using a NamedTemporaryFile as Martin suggested, but the file got deleted immediately by garbage collection.

Martin Pool (mbp) wrote : Posted in a previous version of this proposal

> This branch adds the ability to pass the email body into mutt. I tried using a
> NamedTemporaryFile as Martin suggested, but the file got deleted immediately
> by garbage collection.

Is that because bzr send exits before mutt is finished? If not, is it possible to hold a reference to the variable, but perhaps there's no easy scope to do so. It's not a big deal, but having a comment might be good.

Edwin Grubbs (edwin-grubbs) wrote : Posted in a previous version of this proposal

> > This branch adds the ability to pass the email body into mutt. I tried using
> a
> > NamedTemporaryFile as Martin suggested, but the file got deleted immediately
> > by garbage collection.
>
> Is that because bzr send exits before mutt is finished? If not, is it
> possible to hold a reference to the variable, but perhaps there's no easy
> scope to do so. It's not a big deal, but having a comment might be good.

I added a comment. Here is the incremental diff:

{{{
=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py 2009-06-06 03:25:19 +0000
+++ bzrlib/mail_client.py 2009-06-07 01:16:59 +0000
@@ -271,6 +271,9 @@
             message_options.extend(['-a',
                 self._encode_path(attach_path, 'attachment')])
         if body is not None:
+ # mkstemp() is used instead of NamedTemporaryFile, so that
+ # the file won't be deleted when the object gets garbage
+ # collected, which may be before mutt can read it.
             fd, temp_file = tempfile.mkstemp(prefix="mutt-body-",
                                              suffix=".txt")
             try:

}}}

John A Meinel (jameinel) wrote : Posted in a previous version of this proposal

So I think to do this correctly, we would need to have a way to define the lifetime of the NamedTemporaryFile, versus the lifetime of running the mutt command.

I *think* if you just assigned the temp file to 'self' then it would survive long enough for mutt to find it.

If you just did

temp_file = NamedTemporaryFile(...)
temp_file.write()
...

args.extend(...)
...
return args

Then the temp file will be destroyed as soon as the function returns.

However if you did:

self._temp_file = NamedTemporaryFile(...)

then the file will be around until the Mutt class is destroyed, which should be long enough.

I'll also note that NamedTemporaryFile won't work on windows (it gives you the path to a file, but that file cannot be opened even within the same process... :(

Even so, I'm not worried about people using mutt on Windows, so I'd still like to try.

Anyway, I think this is good enough to merge, but it would be nice if you could try the NamedTemporaryFile trick again, with a longer lifetime.

review: Approve
Edwin Grubbs (edwin-grubbs) wrote : Posted in a previous version of this proposal

Storing the temp file in the Mutt object worked. Here is the incremental diff.

=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py 2009-06-07 01:17:13 +0000
+++ bzrlib/mail_client.py 2009-06-08 17:43:56 +0000
@@ -271,16 +271,13 @@
             message_options.extend(['-a',
                 self._encode_path(attach_path, 'attachment')])
         if body is not None:
- # mkstemp() is used instead of NamedTemporaryFile, so that
- # the file won't be deleted when the object gets garbage
- # collected, which may be before mutt can read it.
- fd, temp_file = tempfile.mkstemp(prefix="mutt-body-",
- suffix=".txt")
- try:
- os.write(fd, body)
- finally:
- os.close(fd)
- message_options.extend(['-i', temp_file])
+ # Store the temp file object in self, so that it does not get
+ # garbage collected and delete the file before mutt can read it.
+ self._temp_file = tempfile.NamedTemporaryFile(
+ prefix="mutt-body-", suffix=".txt")
+ self._temp_file.write(body)
+ self._temp_file.flush()
+ message_options.extend(['-i', self._temp_file.name])
         if to is not None:
             message_options.extend(['--', self._encode_safe(to)])
         return message_options

John A Meinel (jameinel) :
review: Approve

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
=== modified file 'bzrlib/mail_client.py'
--- bzrlib/mail_client.py 2009-06-04 21:05:44 +0000
+++ bzrlib/mail_client.py 2009-06-08 18:35:18 +0000
@@ -257,12 +257,12 @@
257 help=Evolution.__doc__)257 help=Evolution.__doc__)
258258
259259
260class Mutt(ExternalMailClient):260class Mutt(BodyExternalMailClient):
261 """Mutt mail client."""261 """Mutt mail client."""
262262
263 _client_commands = ['mutt']263 _client_commands = ['mutt']
264264
265 def _get_compose_commandline(self, to, subject, attach_path):265 def _get_compose_commandline(self, to, subject, attach_path, body=None):
266 """See ExternalMailClient._get_compose_commandline"""266 """See ExternalMailClient._get_compose_commandline"""
267 message_options = []267 message_options = []
268 if subject is not None:268 if subject is not None:
@@ -270,6 +270,14 @@
270 if attach_path is not None:270 if attach_path is not None:
271 message_options.extend(['-a',271 message_options.extend(['-a',
272 self._encode_path(attach_path, 'attachment')])272 self._encode_path(attach_path, 'attachment')])
273 if body is not None:
274 # Store the temp file object in self, so that it does not get
275 # garbage collected and delete the file before mutt can read it.
276 self._temp_file = tempfile.NamedTemporaryFile(
277 prefix="mutt-body-", suffix=".txt")
278 self._temp_file.write(body)
279 self._temp_file.flush()
280 message_options.extend(['-i', self._temp_file.name])
273 if to is not None:281 if to is not None:
274 message_options.extend(['--', self._encode_safe(to)])282 message_options.extend(['--', self._encode_safe(to)])
275 return message_options283 return message_options
276284
=== modified file 'bzrlib/tests/test_mail_client.py'
--- bzrlib/tests/test_mail_client.py 2009-06-04 19:56:42 +0000
+++ bzrlib/tests/test_mail_client.py 2009-06-08 18:35:18 +0000
@@ -28,8 +28,10 @@
2828
29 def test_commandline(self):29 def test_commandline(self):
30 mutt = mail_client.Mutt(None)30 mutt = mail_client.Mutt(None)
31 commandline = mutt._get_compose_commandline(None, None, 'file%')31 commandline = mutt._get_compose_commandline(
32 self.assertEqual(['-a', 'file%'], commandline)32 None, None, 'file%', body="hello")
33 # The temporary filename is randomly generated, so it is not matched.
34 self.assertEqual(['-a', 'file%', '-i'], commandline[:-1])
33 commandline = mutt._get_compose_commandline('jrandom@example.org',35 commandline = mutt._get_compose_commandline('jrandom@example.org',
34 'Hi there!', None)36 'Hi there!', None)
35 self.assertEqual(['-s', 'Hi there!', '--', 'jrandom@example.org'],37 self.assertEqual(['-s', 'Hi there!', '--', 'jrandom@example.org'],