Merge lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt into lp:~bzr/bzr/trunk-old
Status: | Superseded | ||||
---|---|---|---|---|---|
Proposed branch: | lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt | ||||
Merge into: | lp:~bzr/bzr/trunk-old | ||||
Diff against target: | 53 lines | ||||
To merge this branch: | bzr merge lp:~edwin-grubbs/bzr/bug-384158-passing-body-to-mutt | ||||
Related bugs: |
|
Reviewer | Review Type | Date Requested | Status |
---|---|---|---|
John A Meinel | Approve on 2009-06-08 | ||
bzr-core | code | 2009-06-06 | Pending |
Review via email:
|
This proposal has been superseded by a proposal from 2009-06-08.
Edwin Grubbs (edwin-grubbs) wrote : | # |
Martin Pool (mbp) wrote : | # |
> 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 : | # |
> > 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/
--- bzrlib/
+++ bzrlib/
@@ -271,6 +271,9 @@
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.
try:
}}}
- 4418. By Edwin Grubbs on 2009-06-07
-
Added comment.
John A Meinel (jameinel) wrote : | # |
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 = NamedTemporaryF
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 = NamedTemporaryF
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.
Edwin Grubbs (edwin-grubbs) wrote : | # |
Storing the temp file in the Mutt object worked. Here is the incremental diff.
=== modified file 'bzrlib/
--- bzrlib/
+++ bzrlib/
@@ -271,16 +271,13 @@
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.
- suffix=".txt")
- try:
- os.write(fd, body)
- finally:
- os.close(fd)
- message_
+ # 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.
+ prefix=
+ self._temp_
+ self._temp_
+ message_
if to is not None:
return message_options
- 4419. By Edwin Grubbs on 2009-06-08
-
Stored temp file in self to allow using NamedTemporaryFile.
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.