In parallel to the other discussion on import order of svn.fs.FileDiff, I'd
like some comments on the attached patch. Mostly, this is to address the
failing tests on Windows [0], since there isn't a 'diff' executable
necessarily available. The attached patch uses the internal Subversion
diff implementation (which IMHO is actually better all around anyway),
unless the 'diffoptions' constructor argument is provided. If diffoptions
are given, then the old behavior of shelling out to 'diff' is still used.
I'm asking for comments because the actual diff output given when
'diffoptions' is None, is changed. Currently a no argument call to 'diff'
is used, which results in a 'normal' diff output. Using
svn_diff_file_output_unified4(), as I have in the patch, results in a
unified diff output. This output is more consistent with other subversion
diff output, but it *is* different than what svn.fs.FileDiff.get_pipe()
currently returns.
Another option would be to default diffoptions to [ '--normal' ], so that
it takes an explicit argument of 'None' or '[]' to get the new behavior,
which we could use in the test.
Thoughts?
Troy
0: https://ci.apache.org/builders/svn-windows-ra/builds/1998
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 1823802)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -42,6 +42,7 @@
# Python >=3.0
import builtins
import svn.core as _svncore
+import svn.diff as _svndiff
def entries(root, path, pool=None):
@@ -58,6 +59,7 @@
self.tempfile1 = None
self.tempfile2 = None
+ self.difftemp = None
self.root1 = root1
self.path1 = path1
@@ -110,27 +112,49 @@
def get_pipe(self):
self.get_files()
- # use an array for the command to avoid the shell and potential
- # security exposures
- cmd = ["diff"] \
- + self.diffoptions \
- + [self.tempfile1, self.tempfile2]
+ # If diffoptions were provided, then the diff command needs to be
+ # called in preference to using the internal Subversion diff.
+ if self.diffoptions:
+ # use an array for the command to avoid the shell and potential
+ # security exposures
+ cmd = ["diff"] \
+ + self.diffoptions \
+ + [self.tempfile1, self.tempfile2]
- # open the pipe, and return the file object for reading from the child.
- p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1,
- close_fds=_sys.platform != "win32")
- return p.stdout
+ # open the pipe, and return the file object for reading from the child.
+ p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1,
+ close_fds=_sys.platform != "win32")
+ return p.stdout
+ else:
+ if self.difftemp is None:
+ self.difftemp = _tempfile.mktemp()
+
+ with builtins.open(self.difftemp, "wb") as fp:
+ diffopt = _svndiff.file_options_create()
+ diffobj = _svndiff.file_diff_2(self.tempfile1,
+ self.tempfile2,
+ diffopt)
+
+ _svndiff.file_output_unified4(fp,
+ diffobj,
+ self.tempfile1,
+ self.tempfile2,
+ None, None,
+ "utf8",
+ None,
+ diffopt.show_c_function,
+ diffopt.context_size,
+ None, None)
+
+ return builtins.open(self.difftemp, "r")
+
def __del__(self):
# it seems that sometimes the files are deleted, so just ignore any
# failures trying to remove them
- if self.tempfile1 is not None:
- try:
- _os.remove(self.tempfile1)
- except OSError:
- pass
- if self.tempfile2 is not None:
- try:
- _os.remove(self.tempfile2)
- except OSError:
- pass
+ for tmpfile in [self.tempfile1, self.tempfile2, self.difftemp]:
+ if tmpfile is not None:
+ try:
+ _os.remove(tmpfile)
+ except OSError:
+ pass
Index: subversion/bindings/swig/python/tests/fs.py
===================================================================
--- subversion/bindings/swig/python/tests/fs.py (revision 1823802)
+++ subversion/bindings/swig/python/tests/fs.py (working copy)
@@ -19,7 +19,7 @@
# under the License.
#
#
-import os, unittest
+import os, unittest, sys
from tempfile import mkstemp
try:
# Python >=3.0
@@ -83,6 +83,7 @@
clientctx)
self.assertEqual(commitinfo.revision, self.rev + 1)
+ # Test standard internal diff
fdiff = fs.FileDiff(fs.revision_root(self.fs, commitinfo.revision), "/trunk/UniTest.txt",
None, None)
@@ -89,8 +90,19 @@
diffp = fdiff.get_pipe()
diffoutput = diffp.read().decode('utf8')
- self.assertTrue(diffoutput.find(u'< ⊙_ʘ') > 0)
+ self.assertTrue(diffoutput.find(u'-⊙_ʘ') > 0)
+ # Test passing diffoptions to an external 'diff' executable.
+ # It is unusual to have the 'diff' tool on Windows, so do not
+ # try the test there.
+ if sys.platform != "win32":
+ fdiff = fs.FileDiff(fs.revision_root(self.fs, commitinfo.revision), "/trunk/UniTest.txt",
+ None, None, None, ['--normal'])
+ diffp = fdiff.get_pipe()
+ diffoutput = diffp.read().decode('utf8')
+
+ self.assertTrue(diffoutput.find(u'< ⊙_ʘ') > 0)
+
def suite():
return unittest.defaultTestLoader.loadTestsFromTestCase(
SubversionFSTestCase)
