[Svnmerge] [PATCH] performance fix for some platforms

[Giovanni Bajo]

On 03/07/2007 17.54, lsuvkne at onemodel.org wrote:

> Thanks to both for the reviews; I will try to reply to both
> though I'm quoting only one.

Please do not incorporate two patches in one mail. It makes everything harder. 
Mails are cheap :)

>> 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.
> 
> I don't like it either but without it, many tests fail on linux.

You should investigate on why they fail. For instance, do they pass if you 
specify shell=True? Which tests, specifically, fail? What is the command line 
they try to execute and fail to?

I'm -1 on the patch with the quote parsing stuff. I'm positive it can be made 
to work without that code (or subprocess would be the most useless module 
ever), so you'll forgive me if I ask you again to investigate the issue.

> suggestion is a good one I'd like to implement ... but I think instead
> I should work on things like getting the rest of my patches in, to
> address parts of the "rename + merge = dataloss" problem, testing out
> more scenarios to see what else could go wrong with merges and think of
> how to address them.  Even better yet, I'd like to get up to speed and
> work on a couple of things in svn itself to help address "tree
> collisions" and to avoid the problem of silent dataloss with
> merge+rename, altogether.  As time permits.

I'm happy that you're going to work on these things, but I don't think we 
should let code that I believe being wrong into the tree. If you think that 
your time is better spent elsewhere than on this "subprocess" patch, you are 
free to reschedule yourself as you want. But I really believe the patch as-is 
should not be committed.

>> 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.
> 
> It simply didn't work on windows until I did.  Various errors.

Again, this really doesn't answer my question. For instance, the subprocess 
documentation for "executable" says:

"""
On Windows, the default shell is specified by the COMSPEC environment variable
"""

So, at the very least, I'd assume that if you specified shell=True, you don't 
need to specify executable=os.environ['COMSPEC'].

> Thanks for the review and suggestions!  I ran the tests on Windows 
> also, but unfortunately not with an older python version.

If you provide me a new patch that addresses my concerns, I'm happy to help 
you testing on old and new Python versions on all platforms (linux, mac, 
windows).

> (Any thoughts on my other patch, for fixing the older regression test
> errors?  I'd like to put these to rest if possible so I can start
> sending in others...)

I'll have a look in the following days. If I don't send a review in the next 3 
days, please send me a personal mail reminding me that I need to review your 
patch.

Thanks!
-- 
Giovanni Bajo