[PATCH stable-v2] convert: fix a test failure due to git change (issue3428)

[Patrick Mézard]

Le 03/08/12 00:54, Mads Kiilerich a écrit :
> Ross Lagerwall wrote, On 08/01/2012 06:44 PM:
>> # HG changeset patch
>> # User Ross Lagerwall <rosslagerwall at gmail.com>
>> # Date 1343838158 -7200
>> # Branch stable
>> # Node ID 8df5a0dfab4cde5e936b63d631842becb989ce1f
>> # Parent  3ccb49ed1cc13cf71cd780e998f200ed974ec271
>> convert: fix a test failure due to git change (issue3428)

[...]

> My comment:
> 1: we have some subprocess / popen abstractions in util
> 2: we regularly have issues with the places that don't use these abstractions
> 3: ...

I am afraid I have to disagree with this comment:

1- We have popen abstractions in util.py but in my experience they always get in the way. First, they are not equivalent, util.popen calls wrappers around os.popen while util.popen2 uses subprocess. They are two different beasts and on Windows, os.popen has always been problematic (note that only the Windows version of popen messes with the command argument, not the subprocess one). Now, util.popen2 gives you the streams but you cannot retrieve the call status code, not terribly useful. And you cannot do redirections like subprocess let you do.

2- We regularly have issues with code not using subprocess or a derivative. And most of the time, the util wrappers get in the way. See how *none* of the non-mercurial subrepos use the util abstractions but heavily tweaked subprocess calls instead. Because it works much better.

Interestingly, the subversion sink does not work at all on Windows using util.popen but is fine when I replace that with a subprocess call.

What the util abstractions have are sane default values for subprocess parameters. Things like close_fds, bufsize, shell, etc. But subprocess.Popen is a better API than os.popen, maybe we should really wrap it in util, only rewriting the default values.

Ross, I would like to take a closer look at your patch before asking you any changes, I hope I will have time to do that in the next days.

--
Patrick Mézard