You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@subversion.apache.org by Michael Kefeder mi...@weird-birds.org on 2003/01/25 16:37:07 UTC
flaw in python bindings diff?
Hi
Today i went through viewcvs code to figure out why the heck diffing
doesn't work for me. It works for http://www.linux1394.org so i thought it
must work for me too.
In the end i tracked it down to the diff -L flag viewcvs uses to label the
temporary files. viewcvs creates a label string that looks like
Filename\tdate\trevision. And that label (at least on my machine with
diff 2.7.2) causes trouble.
The reason lies either in viewcvs or in subversions python bindings. Where
i'd like to vote for the svn-bindings. Because i think there's a flaw
within fs.py in FileDiff::get_pipe(). The diffoptions (passed to FileDiff
constructor) are string.split() which can result into strange behaviour
because split() uses all whitespace characters for splitting if not told
otherwise. Now when I pass the string '-L my file_label' to FileDiff then
split() would create an array [-L, \my, file_label\] which is a
incorrect parameter-list for popen2.popen2() to call diff.
One workaround i can think of is using a non-whitespace
param-string-separator (like | but that is a bit clumsy)
Another one is to accept a list object instead of string as argument to the
constructor of FileDiff to pass the diff-params
or to create an intelligent cmd-param parser to split the params ;)
What do you think?
Oh, i almost forgot - i run svn 0.17.0 r4468 server from tarball and a
recent checkout of viewcvs-1.0dev with python 2.2
Yours
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by cm...@collab.net.
Michael Kefeder ml@weird-birds.org writes:
Oh, i like the label and it's format - and it's not directly stopping
diff from working, it's the label split apart in get_pipe() and passed
to pipe2. I forgot to express that in a more clear way, sorry ;)
No problem.
I understand Greg Steins intention to use popen2 but then it becomes
tricky separating the cmd line params for diff that are given as a
string. I'd vote for a list object as parameter to FileDiff instead of
string, because a list is needed in the end for popen2 and imho it
isn't complicated to create a list in python (or it's done anyway like
in your viewcvs code). I think the right magic you mentioned will
only introduce new bugs and take up a fair amount of coding-time.
I'll give it a shot.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by Michael Kefeder ml...@weird-birds.org.
--On Montag, 27. Jänner 2003 11:42 -0600 cmpilato@collab.net
cmpilato@collab.net wrote:
Michael Kefeder ml@weird-birds.org writes:
what do you think about that? Is there a need to take a string as
parameter? if yes why? Or doesn't that remain consitent with
rcsdiff?
Get the latest Subversion and ViewCVS. See if all is well for you.
Even files with tabs in their names work for CVS. I just found a bug,
however, with running 'svn add filewith\ttab', which I'm off to fix now.
wow, now that's what i call fixing a bug in short time! thanks a lot! Now i
don't have to feel terrible for using that crazy patch of mine ;) Your new
code works for me now.
thanks
Yours
Mike
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by Michael Kefeder ml...@weird-birds.org.
--On Montag, 27. Jänner 2003 10:31 -0600 cmpilato@collab.net
cmpilato@collab.net wrote:
Michael Kefeder mike@weird-birds.org writes:
Hi
In the end i tracked it down to the diff -L flag viewcvs uses to label
the temporary files. viewcvs creates a label string that looks like
Filename\tdate\trevision. And that label (at least on my machine
with diff 2.7.2) causes trouble.
The reason lies either in viewcvs or in subversions python
bindings.
I made the bindings use that type of label because that's what rcsdiff
uses for CVS stuffs. And ViewCVS works fine for CVS files with
spaces.
Oh, i like the label and it's format - and it's not directly stopping diff
from working, it's the label split apart in get_pipe() and passed to pipe2.
I forgot to express that in a more clear way, sorry ;)
Where i'd like to vote for the svn-bindings. Because i think there's
a flaw within fs.py in FileDiff::get_pipe(). The diffoptions (passed
to FileDiff constructor) are string.split() which can result into
strange behaviour because split() uses all whitespace characters for
splitting if not told otherwise.
Ah...looks like Greg Stein busted this when he converted to popen2 in
revision 4343 (strangely enough because he claimed it fixed diffs on
files with spaces in them). Greg, can you help out a bit here?
Various updates to the FileDiff class.
* bindings/swig/python/svn/fs.py:
(FileDiff.either_binary): new method to detect whether either file
is a binary file, thus indicating whether the diff should be
skipped (FileDiff.get_pipe): use a list for the diff command, along
with the popen2 module (rather than os.popen). this avoids the
shell for security purposes, and allows spaces in file names.
(FileDiff.__del__): ignore errors while trying to remove the files.
One workaround i can think of is using a non-whitespace
param-string-separator (like | but that is a bit clumsy)
Nono. It just makes sense to have the output remain consistent with
rcsdiff's. We just need the right magic to do it.
I understand Greg Steins intention to use popen2 but then it becomes tricky
separating the cmd line params for diff that are given as a string. I'd
vote for a list object as parameter to FileDiff instead of string, because
a list is needed in the end for popen2 and imho it isn't complicated to
create a list in python (or it's done anyway like in your viewcvs code). I
think the right magic you mentioned will only introduce new bugs and take
up a fair amount of coding-time.
instead of (line 2607 viewcvs.py)
diffobj = vclib.svn.do_diff(request.repos, request.where,
int(rev1), int(rev2),
string.join(args, ))
i'd have no problem with
diffobj = vclib.svn.do_diff(request.repos, request.where,
int(rev1), int(rev2),
args)
what do you think about that? Is there a need to take a string as
parameter? if yes why? Or doesn't that remain consitent with rcsdiff?
Yours
Mike
p.s.: the | separator idea was ment as a joke, but it quick-fixed my
viewcvs troubles ;)
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
[PATCH] Add compatible popen2 function for win32 (Was Re: flaw in python bindings diff?)
Posted by Chris Foote Ch...@v21.me.uk.
----- Original Message -----
From: cmpilato@collab.net
To: Chris Foote Chris.Foote@v21.me.uk
Cc: dev@subversion.tigris.org
Sent: Tuesday, January 28, 2003 1:01 AM
Subject: Re: flaw in python bindings diff?
Chris, would you mind taking another run at this patch to incorporate
my and Greg's feedback? Thanks!
Sure, here goes.
Log:
For windows, add a version of popen2 that takes the same type of args
as on Unix platforms.
* fs.py:
(FileDiff.get_pipe): Use _compat_popen2.
(_compat_popen2): New function to wrap the Unix form of popen2.popen2
on windows.
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 4642)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -19,6 +19,7 @@
### hide these names?
import tempfile
import os
+import sys
import popen2
import _fs
@@ -110,7 +111,7 @@
# open the pipe, forget the end for writing to the child (we won't),
# and then return the file object for reading from the child.
- fromchild, tochild = popen2.popen2(cmd)
+ fromchild, tochild = _compat_popen2(cmd)
tochild.close()
return fromchild
@@ -127,3 +128,26 @@
os.remove(self.tempfile2)
except OSError:
pass
+
+# For windows, add a version of popen2 that takes the same type of args
+# as on Unix platforms.
+if sys.platform[:3] == 'win':
+ import types
+
+ def _compat_popen2(cmd, bufsize=-1, mode='t'):
+ Execute the shell command 'cmd' in a sub-process. If 'bufsize' is
+ specified, it sets the buffer size for the I/O pipes. The file objects
+ (child_stdout, child_stdin) are returned.
+ cmdstr = ''
+ if isinstance(cmd, types.StringTypes):
+ cmdstr = cmd
+ else:
+ cmdstr = cmd[0]
+ for arg in cmd[1:]: # build the command string
+ cmdstr = cmdstr + ' ' + str(arg) + ''
+
+ return popen2.popen2(cmdstr, bufsize, mode)
+
+else:
+ _compat_popen2 = popen2.popen2
+
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by cm...@collab.net.
Chris, would you mind taking another run at this patch to incorporate
my and Greg's feedback? Thanks!
Greg Stein gstein@lyra.org writes:
On Mon, Jan 27, 2003 at 05:12:20PM -0600, cmpilato@collab.net wrote:
Chris Foote Chris.Foote@v21.me.uk writes:
...
@@ -110,7 +111,10 @@
# open the pipe, forget the end for writing to the child (we won't),
# and then return the file object for reading from the child.
- fromchild, tochild = popen2.popen2(cmd)
+ if sys.platform[:3] == 'win':
+ fromchild, tochild = popen2.popen2(string.join(cmd))
+ else:
+ fromchild, tochild = popen2.popen2(cmd)
tochild.close()
return fromchild
Wow, I'm shocked that this code wasn't portable to windows. I hate to
sound like a skeptic ... but I am. Can someone else confirm that this
change is needed? Or point me to a doc so I can learn another new
thing today?
Looks like some badness in Python. The windows stuff uses some special
code to do the pipes, and it looks like the APIs aren't matched up.
So the basic concept of the change is good, but I'd break it out into
a local function called _compat_popen() or somesuch. That also
provides a place to document why this is needed. Further, if we end up
with a lot of compat stuff across the bindings, then we can create a
_compat.py (imported as svn._compat) which can hold code for Python
version-compatibility code.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by Greg Stein gs...@lyra.org.
On Mon, Jan 27, 2003 at 05:12:20PM -0600, cmpilato@collab.net wrote:
Chris Foote Chris.Foote@v21.me.uk writes:
...
@@ -110,7 +111,10 @@
# open the pipe, forget the end for writing to the child (we won't),
# and then return the file object for reading from the child.
- fromchild, tochild = popen2.popen2(cmd)
+ if sys.platform[:3] == 'win':
+ fromchild, tochild = popen2.popen2(string.join(cmd))
+ else:
+ fromchild, tochild = popen2.popen2(cmd)
tochild.close()
return fromchild
Wow, I'm shocked that this code wasn't portable to windows. I hate to
sound like a skeptic ... but I am. Can someone else confirm that this
change is needed? Or point me to a doc so I can learn another new
thing today?
Looks like some badness in Python. The windows stuff uses some special
code to do the pipes, and it looks like the APIs aren't matched up.
So the basic concept of the change is good, but I'd break it out into
a local function called _compat_popen() or somesuch. That also
provides a place to document why this is needed. Further, if we end up
with a lot of compat stuff across the bindings, then we can create a
_compat.py (imported as svn._compat) which can hold code for Python
version-compatibility code.
Cheers,
-g
--
Greg Stein, http://www.lyra.org/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by cm...@collab.net.
Chris Foote Chris.Foote@v21.me.uk writes:
There is one major problem with popen2.popen2() that I have found.
It takes different arguments on windows than what it does on unix.
The windows version expects a string, so I have applied the following patch
to get this working for ViewCVS on windows.
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 4614)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -19,6 +19,7 @@
### hide these names?
import tempfile
import os
+import sys
import popen2
Why import sys?
@@ -110,7 +111,10 @@
# open the pipe, forget the end for writing to the child (we won't),
# and then return the file object for reading from the child.
- fromchild, tochild = popen2.popen2(cmd)
+ if sys.platform[:3] == 'win':
+ fromchild, tochild = popen2.popen2(string.join(cmd))
+ else:
+ fromchild, tochild = popen2.popen2(cmd)
tochild.close()
return fromchild
Wow, I'm shocked that this code wasn't portable to windows. I hate to
sound like a skeptic ... but I am. Can someone else confirm that this
change is needed? Or point me to a doc so I can learn another new
thing today?
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by Chris Foote Ch...@v21.me.uk.
There is one major problem with popen2.popen2() that I have found.
It takes different arguments on windows than what it does on unix.
The windows version expects a string, so I have applied the following patch
to get this working for ViewCVS on windows.
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 4614)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -19,6 +19,7 @@
### hide these names?
import tempfile
import os
+import sys
import popen2
import _fs
@@ -110,7 +111,10 @@
# open the pipe, forget the end for writing to the child (we won't),
# and then return the file object for reading from the child.
- fromchild, tochild = popen2.popen2(cmd)
+ if sys.platform[:3] == 'win':
+ fromchild, tochild = popen2.popen2(string.join(cmd))
+ else:
+ fromchild, tochild = popen2.popen2(cmd)
tochild.close()
return fromchild
Regards,
Chris
----- Original Message -----
From: cmpilato@collab.net
To: Greg Stein gstein@collab.net
Cc: Michael Kefeder mike@weird-birds.org; Subversion Developers
dev@subversion.tigris.org
Sent: Monday, January 27, 2003 7:00 PM
Subject: Re: flaw in python bindings diff?
Greg Stein gstein@collab.net writes:
Ah...looks like Greg Stein busted this when he converted to popen2 in
revision 4343 (strangely enough because he claimed it fixed diffs on
files with spaces in them). Greg, can you help out a bit here?
Switching to popen2 meant that I passed a filename as a separate
argument, which fixes the spaces. I forgot that a filename might be
part of those diffoptions.
Yes, passing a list as the diffoptions is the best behavior.
The reason to use popen2.popen2() is for security. The shell never
sees the command string; we just directly exec() the new process with
the right arguments. In the standard popen(), that goes to the shell
which can then parse for redirections and whatnot -- major security
nightmare.
All good. Your reasons for the change were fine, and understood, and
the fix for spaces has already been made. Yay!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by cm...@collab.net.
Greg Stein gstein@collab.net writes:
Ah...looks like Greg Stein busted this when he converted to popen2 in
revision 4343 (strangely enough because he claimed it fixed diffs on
files with spaces in them). Greg, can you help out a bit here?
Switching to popen2 meant that I passed a filename as a separate
argument, which fixes the spaces. I forgot that a filename might be
part of those diffoptions.
Yes, passing a list as the diffoptions is the best behavior.
The reason to use popen2.popen2() is for security. The shell never
sees the command string; we just directly exec() the new process with
the right arguments. In the standard popen(), that goes to the shell
which can then parse for redirections and whatnot -- major security
nightmare.
All good. Your reasons for the change were fine, and understood, and
the fix for spaces has already been made. Yay!
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by Greg Stein gs...@collab.net.
On Mon, Jan 27, 2003 at 10:31:22AM -0600, cmpilato@collab.net wrote:
Michael Kefeder mike@weird-birds.org writes:
...
Where i'd like to vote for the svn-bindings. Because i think there's
a flaw within fs.py in FileDiff::get_pipe(). The diffoptions (passed
to FileDiff constructor) are string.split() which can result into
strange behaviour because split() uses all whitespace characters for
splitting if not told otherwise.
Ah...looks like Greg Stein busted this when he converted to popen2 in
revision 4343 (strangely enough because he claimed it fixed diffs on
files with spaces in them). Greg, can you help out a bit here?
Switching to popen2 meant that I passed a filename as a separate
argument, which fixes the spaces. I forgot that a filename might be
part of those diffoptions.
Yes, passing a list as the diffoptions is the best behavior.
The reason to use popen2.popen2() is for security. The shell never
sees the command string; we just directly exec() the new process with
the right arguments. In the standard popen(), that goes to the shell
which can then parse for redirections and whatnot -- major security
nightmare.
Cheers,
-g
--
Greg Stein, Director of Software Engineering
CollabNet, Inc. http://www.collab.net/
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Re: flaw in python bindings diff?
Posted by cm...@collab.net.
Michael Kefeder mike@weird-birds.org writes:
Hi
In the end i tracked it down to the diff -L flag viewcvs uses to label
the temporary files. viewcvs creates a label string that looks like
Filename\tdate\trevision. And that label (at least on my machine
with diff 2.7.2) causes trouble.
The reason lies either in viewcvs or in subversions python
bindings.
I made the bindings use that type of label because that's what rcsdiff
uses for CVS stuffs. And ViewCVS works fine for CVS files with
spaces.
Where i'd like to vote for the svn-bindings. Because i think there's
a flaw within fs.py in FileDiff::get_pipe(). The diffoptions (passed
to FileDiff constructor) are string.split() which can result into
strange behaviour because split() uses all whitespace characters for
splitting if not told otherwise.
Ah...looks like Greg Stein busted this when he converted to popen2 in
revision 4343 (strangely enough because he claimed it fixed diffs on
files with spaces in them). Greg, can you help out a bit here?
Various updates to the FileDiff class.
* bindings/swig/python/svn/fs.py:
(FileDiff.either_binary): new method to detect whether either file
is a binary file, thus indicating whether the diff should be skipped
(FileDiff.get_pipe): use a list for the diff command, along with the
popen2 module (rather than os.popen). this avoids the shell for
security purposes, and allows spaces in file names.
(FileDiff.__del__): ignore errors while trying to remove the files.
One workaround i can think of is using a non-whitespace
param-string-separator (like | but that is a bit clumsy)
Nono. It just makes sense to have the output remain consistent with
rcsdiff's. We just need the right magic to do it.
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org