[Svnmerge] [PATCH] performance fix for some platforms

[Giovanni Bajo]

On Sat, 30 Jun 2007 15:10:22 -0600, lsuvkne at onemodel.org wrote:

> You probably know some of this already but I'll repeat it for
> completeness.  The problem this patch addresses is that some OSes (some
> *nix flavors, but not others, and not windows) have larger numbers of
> file descriptors, and the older popen() code loops through trying to
> close all of them when you execute a subprocess. This makes the launch()
> command very slow.  The newer subprocess code fixes the issue, so this
> patch uses that for versions of python that support it.  This makes a
> huge difference on my box:  1-4 minutes vs. hours to run the tests; the
> svnmerge.py code itself naturally runs faster as well.

Hi Luke,

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


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.

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

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

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.

Please, explicitally CC me on next revision of this patch to get a quick
review.

Thanks!
--
Giovanni Bajo