[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

Re: flaw in python bindings diff?

From: <ml_at_weird-birds.org>
Date: 2003-01-27 18:47:41 CET

--On Montag, 27. Jšnner 2003 10:31 -0600 cmpilato@collab.net
cmpilato@collab.net wrote:

 Michael Kefeder mike@weird-birds.org writes:


 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

 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

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),

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?

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
Received on Sat Oct 14 02:19:50 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.