Re: mailer.py propchange2 bugs and [PATCH]

techtonik wrote:
> Hi there!
>
> Enclosed you will find a patch for mailer.py from 1.3.2
> It lists two parts. First one demonstrates obscure bug I will describe
> later.
>
> Second part fixes invocation of 'diff' utility. Second part is easy to
> explain - mailer.py uses NamedTemporaryFile class, which does not
> allow another process - i.e. diff to reopen created temp files - hence
> 'Permission denied' error is produced - this is well described here in
> last post - http://www.thescripts.com/forum/thread439775.html
> I changed the name NamedTemporaryFile to MailerTemporaryFile just
> because I see the python for the first time and do not want to collect
> possible bugs arising from name clashes. The second part also fixes
> bug when temporary files were not deleted by custom TemporaryFile
> class because they were not closed yet .
>
> As for the first part - this is where am I looking for advice.
> -----
> """Override this method, if the default implementation is not
> sufficient.
> Execute CMD, writing the stdout produced to the output
> representation."""
> # By default we choose to incorporate child stderr into the output
> + os.system(svn.core.argv_to_command_string(cmd))
> pipe_ob = Popen4(cmd)
> -----
> without this line diff results in mail with are blank. With it
> everything is ok. I failed to find any logical explanation for this
> fact. Any ideas why this can happen?
>
> Win2003 server, python 2.3.5
>
>
> ------------------------------------------------------------------------
>
> --- mailer.py Mon Aug 07 14:47:48 2006
> +++ ../mailer.py Thu Aug 17 14:38:58 2006
> @@ -167,10 +167,11 @@
> """Override this method, if the default implementation is not sufficient.
> Execute CMD, writing the stdout produced to the output representation."""
> # By default we choose to incorporate child stderr into the output
> + os.system(svn.core.argv_to_command_string(cmd))

I really don't get this. Of course, you really didn't get it either, as you
stated above.

> pipe_ob = Popen4(cmd)
>
> buf = pipe_ob.fromchild.read(self._CHUNKSIZE)
> - while buf:
> + while len(buf) > 0:

This change is, in this context, entirely unnecessary. 'while buf' will
only return something "true" if buf is a non-empty string.

> try:
> + # NamedTemporaryFile on windows does not allow diff to reopen temp files
> + # http://www.thescripts.com/forum/thread439775.html
> + if sys.platform == 'win32':
> + raise ImportError, 'NamedTemporaryFile is useless on win32'
> +
> from tempfile import NamedTemporaryFile
> + class MailerTemporaryFile(NamedTemporaryFile):
> + pass
> except ImportError:
> # NamedTemporaryFile was added in Python 2.3, so we need to emulate it
> # for older Pythons.
> - class NamedTemporaryFile:
> + class MailerTemporaryFile:
> def __init__(self):
> self.name = tempfile.mktemp()
> self.file = open(self.name, 'w+b')
> def __del__(self):
> + self.file.close()
> os.remove(self.name)
> def write(self, data):
> self.file.write(data)

If we're providing a custom class for older Pythons and newer Pythons on
Windows ... why do we even bother trying to use the stock NamedTemporaryFile
at all? The addition of the self.file.close() looks (to me) like a sensible
change, but one that seems useful on non-Windows systems, too.

-- 
C. Michael Pilato <cmpilato@collab.net>
CollabNet   <>   www.collab.net   <>   Distributed Development On Demand