Patchwork [4,of,5,STABLE] mail: use popen4 instead of popen for portability

# HG changeset patch
# User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
# Date 1477678088 -32400
#      Sat Oct 29 03:08:08 2016 +0900
# Branch stable
# Node ID 51b14d2127fbd214c5ec57f9c1ef17457c61f93d
# Parent  141cb12c0175d9e4fbdab1f69d99be24d50ce3f4
mail: use popen4 instead of popen for portability

On Windows platform, the process spawned by util.popen("w") doesn't
work as expected, if it writes anything into stdout of it. In such
case, spawned child process terminates with status code 68864 (at
least, simple .bat script does so).

Maybe, this is a variant of python issue below, which is handled in
popen() in windows.py of Mercurial.

    http://bugs.python.org/issue1366

This patch shows stderr output of spawned process at first, to prevent
users from overlooking serious information after long stdout output.

This ui.warn() invocation causes flushing messages buffered by ui
before popen4(). Therefore, this patch also moves ui.note() output in
test-patchbomb.t.

This patch chooses util.popen4(), because other util.popen* family
doesn't provide the way to get exit code of spawned process.

BTW, other clients except for spawning pager process in patchbomb use
util.popen() only in "read" mode. They (spawning pager, too) should be
safe and portable enough, even though discarding stderr on Windows
might have to be fixed for safety.

On Sat, 29 Oct 2016 03:16:49 +0900, FUJIWARA Katsunori wrote:
> # HG changeset patch
> # User FUJIWARA Katsunori <foozy@lares.dti.ne.jp>
> # Date 1477678088 -32400
> #      Sat Oct 29 03:08:08 2016 +0900
> # Branch stable
> # Node ID 51b14d2127fbd214c5ec57f9c1ef17457c61f93d
> # Parent  141cb12c0175d9e4fbdab1f69d99be24d50ce3f4
> mail: use popen4 instead of popen for portability
> 
> On Windows platform, the process spawned by util.popen("w") doesn't
> work as expected, if it writes anything into stdout of it. In such
> case, spawned child process terminates with status code 68864 (at
> least, simple .bat script does so).
> 
> Maybe, this is a variant of python issue below, which is handled in
> popen() in windows.py of Mercurial.
> 
>     http://bugs.python.org/issue1366
> 
> This patch shows stderr output of spawned process at first, to prevent
> users from overlooking serious information after long stdout output.
> 
> This ui.warn() invocation causes flushing messages buffered by ui
> before popen4(). Therefore, this patch also moves ui.note() output in
> test-patchbomb.t.
> 
> This patch chooses util.popen4(), because other util.popen* family
> doesn't provide the way to get exit code of spawned process.

I don't think this change is good for stable considering how many people
would actually use the sendmail method on Windows.

> --- a/mercurial/mail.py
> +++ b/mercurial/mail.py
> @@ -160,9 +160,15 @@ def _sendmail(ui, sender, recipients, ms
>      cmdline = '%s -f %s %s' % (program, util.email(sender),
>                                 ' '.join(map(util.email, recipients)))
>      ui.note(_('sending mail: %s\n') % cmdline)
> -    fp = util.popen(cmdline, 'w')
> -    fp.write(msg)
> -    ret = fp.close()
> +    p = util.popen4(cmdline)[3]
> +    outdata, errdata = p.communicate(msg)
> +    errdata = errdata.rstrip()
> +    if errdata:
> +        ui.warn(errdata + '\n')
> +    outdata = outdata.rstrip()
> +    if outdata:
> +        ui.write(outdata + '\n')
> +    ret = p.returncode

Maybe we can factor out a helper to run an external command with ui.fout and
ui.ferr. util.system() does a similar thing in a slightly different way. It
would be a bit unfortunate we have many variants of process handling.