[Svnmerge] [PATCH] performance fix for some platforms
lsuvkne at onemodel.org
lsuvkne at onemodel.org
Fri Jul 6 09:21:23 PDT 2007
>> 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 :)
Sorry for being unclear. I meant that I was trying to word my comments
about my single patch (in two files), such that they accounted for the
points made by both you and Dustin about the patch, since my replies
were similar. Next time I'll reply separately to each.
There are two patch files (created with "diff -u..." as per the Hacker's
Guide to Subversion). This change requires changes to svnmerge_test.py,
because some tests send blank strings as commands (the old launch()
version must have tolerated this); I simply changed the tests not to
send the blank strings. They were being sent because of the carriage
returns in the multiline strings given as parameters to
"multilaunch()"). It also required me to change some string quoting in
the tests; the code comments for launch() explain further.
>>> 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.
Thanks for pointing out shlex.split() to me. This enabled me to
simplify the code. This points out my lack of python experience. I
hope the comments that are now in the code indicate why I had to use it.
>>> 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:
> ...
I found that the command string by itself works fine on windows without
the extra work I did: I just had to not try to pass the same "args"
parameter as I was passing to Popen() on linux, and all was well. Again
I'm hoping the code comments clarify the reasons better than I did last
time.
I usually test this under QEMU instead of rebooting into Windows. For
some reason on Windows XP under QEMU I had to specify the parameter
"executable=os.environ['COMSPEC']" to the Popen() call, or it would not
find "dir". I didn't look to see what shell it was using, but as you
say the default should be the one specified by COMSPEC anyway, and an
easy solution was to change TestCase_launch to use "xcopy /?" instead of
"dir", so it ran the same in both places without problems caused somehow
by "dir" being a shell built-in. Now I can test that in Windows without
rebooting.
>> 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).
Thanks for that offer. If you like the patch now, your help with tests
on those permutations would be great; all but the 3 tests pass, on suse
10.0 and XP; the only 3 test failures are older ones I fixed in my other
patch, that you saw come through a few days ago. If this patch is still
lacking, let me know.
Best,
-Luke
-------------- next part --------------
--- /home/callla/dnload/subversion/trunk/contrib/client-side/svnmerge/svnmerge_test.py 2007-07-05 15:17:16.000000000 -0600
+++ svnmerge_test.py 2007-07-05 15:54:42.000000000 -0600
@@ -456,7 +496,8 @@
def multilaunch(self, cmds):
for cmd in cmds.split("\n"):
cmd = cmd.strip()
- svnmerge.launch(cmd % self.command_dict())
+ if len(cmd) > 0:
+ svnmerge.launch(cmd % self.command_dict())
def revert(self):
self.multilaunch("svn revert -R .")
@@ -944,10 +1030,10 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
open("anothernewfile", "w").close()
self.launch("svn add anothernewfile")
- self.launch("svn commit -m 'Adding anothernewfile'", match=r"Committed revision 16")
+ self.launch('svn commit -m "Adding anothernewfile"', match=r"Committed revision 16")
# Svnmerge block r15,16
os.chdir("../test-branch")
@@ -977,7 +1063,7 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
# Svnmerge merge r15
os.chdir("../test-branch")
@@ -1002,10 +1088,10 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
open("anothernewfile", "w").close()
self.launch("svn add anothernewfile")
- self.launch("svn commit -m 'Adding anothernewfile'", match=r"Committed revision 16")
+ self.launch('svn commit -m "Adding anothernewfile"', match=r"Committed revision 16")
# Svnmerge block r16, merge r15
os.chdir("../test-branch")
@@ -1039,7 +1125,7 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'",
+ self.launch('svn commit -m "Adding newfile"',
match=r"Committed revision 15")
# Merge a change from trunk to test-branch.
-------------- next part --------------
--- /home/callla/dnload/subversion/trunk/contrib/client-side/svnmerge/svnmerge_test.py 2007-07-05 15:17:16.000000000 -0600
+++ svnmerge_test.py 2007-07-05 15:54:42.000000000 -0600
@@ -456,7 +496,8 @@
def multilaunch(self, cmds):
for cmd in cmds.split("\n"):
cmd = cmd.strip()
- svnmerge.launch(cmd % self.command_dict())
+ if len(cmd) > 0:
+ svnmerge.launch(cmd % self.command_dict())
def revert(self):
self.multilaunch("svn revert -R .")
@@ -944,10 +1030,10 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
open("anothernewfile", "w").close()
self.launch("svn add anothernewfile")
- self.launch("svn commit -m 'Adding anothernewfile'", match=r"Committed revision 16")
+ self.launch('svn commit -m "Adding anothernewfile"', match=r"Committed revision 16")
# Svnmerge block r15,16
os.chdir("../test-branch")
@@ -977,7 +1063,7 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
# Svnmerge merge r15
os.chdir("../test-branch")
@@ -1002,10 +1088,10 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'", match=r"Committed revision 15")
+ self.launch('svn commit -m "Adding newfile"', match=r"Committed revision 15")
open("anothernewfile", "w").close()
self.launch("svn add anothernewfile")
- self.launch("svn commit -m 'Adding anothernewfile'", match=r"Committed revision 16")
+ self.launch('svn commit -m "Adding anothernewfile"', match=r"Committed revision 16")
# Svnmerge block r16, merge r15
os.chdir("../test-branch")
@@ -1039,7 +1125,7 @@
os.chdir("../trunk")
open("newfile", "w").close()
self.launch("svn add newfile")
- self.launch("svn commit -m 'Adding newfile'",
+ self.launch('svn commit -m "Adding newfile"',
match=r"Committed revision 15")
# Merge a change from trunk to test-branch.
More information about the Svnmerge
mailing list