[Svnmerge] [PATCH] performance fix for some platforms

[Dustin J. Mitchell]

On Mon, Jul 02, 2007 at 02:41:36PM +0200, Giovanni Bajo wrote:
> thanks for the patch. I have some comments on it:
> 
> 1) You can't import subprocess at the top of the file without guarding it
> with try/except. Please, do the import immediately above the launch()
> function and totally replace its definition in case the module is
> available. Something like:
> 
> try:
>    import subprocess
>    def launch(...):
>        # subprocess version
> 
> except ImportError:
>    def launch(...):
>        # old version

Agreed -- this would be clearer.  You can also avoid the explicit
Python-version checks this way.  This is a fairly common idiom.

> 2) I dislike your manual argument quoting code. The splitting part should
> be done with shlex.split() (but that follows POSIX, not Windows CMD), and
> the merging with subprocess.list2cmdline (under Windows). Anyway, I don't
> get why you need to do *anything* at all, since Popen() accepts both a
> single string or a list of strings.

Also agreed -- any kind of shell parsing is just likely to cause
spurious trouble for test writers.  Is there a problem with command
lines in existing tests being incorrectly interpreted on Windows?

> 3) As a followup patch, please do change all launch's users to get a list
> of strings.

This would be great, but on the assumption that your (Luke's) time is
limited, it's not urgent.

> 4) Explicitally specify close_fds=False in the Popen() call, since that's
> the "raison d'etre" of the whole patch.

There was a lengthy discussion of this issue on python-dev recently.
It's interesting that you see a speedup even without setting
close_fds=False..

> 5) p.returncode==0 is a sufficient test for success. Why do you also check
> that stderr output is empty? I find that irrelevant.
> 
> 6) Why do you need to specify a different "executable=" argument for the
> two operating systems? Isn't the default OK? Also, why do you think that we
> need to go through the shell under nt? I thought that wasn't required.

Agreed on both.  One thing to add:

+                # Setting keepends=True for compatibility with previous logic 
+                # (where file.readlines() preserves newlines)
+                # and the test TestCase_launch.test_basic, whose purpose in 
+                # checking for newlines I don't yet know.

The test_basic test is just to make sure that launch is still acting the way
the remainder of the tests think it is :).  You can reduce this comment to just
the first two lines.

Overall, I'd like to see the shell quoting stuff removed if possible, and to
know that the result works on Windows (which I can't test).  The rest is
negotiable.

Dustin

-- 
        Dustin J. Mitchell
        Storage Software Engineer, Zmanda, Inc.
        http://www.zmanda.com/