On Thu, Oct 5, 2023 at 3:36 AM <dsahlb...@apache.org> wrote: > Author: dsahlberg > Date: Thu Oct 5 07:36:08 2023 > New Revision: 1912743 > > URL: http://svn.apache.org/viewvc?rev=1912743&view=rev > Log: > Fix issue #1778: Better handling if diff is not available. > > r1824410 solves the basic issue, to use the internal diff functions > when available. However if diffoptions is not None, an external > diff command is still called. If diff (or diff.exe) is not found > in PATH, Python2 will raise an OSError and Python3 will raise a > FileNotFoundError (which inherits OSError). > > r1912724 adds a docstring to FileDiff.get_pipe() documenting this > behaviour. > > This revision add an improved error message. When dropping Python2 > support, the code can catch FileNotFoundError and remove the check > for ENOENT. > > * subversion/bindings/swig/python/svn/fs.py > (FileDiff.get_pipe): Catch OSError/ENOENT and improve error msg > > * subversion/bindings/swig/python/tests/fs.py > (test_diff_repos_paths_external): Add note to change code when > droping Python2 support. No functional change. > > > Modified: > subversion/trunk/subversion/bindings/swig/python/svn/fs.py > subversion/trunk/subversion/bindings/swig/python/tests/fs.py > > Modified: subversion/trunk/subversion/bindings/swig/python/svn/fs.py > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/python/svn/fs.py?rev=1912743&r1=1912742&r2=1912743&view=diff > > ============================================================================== > --- subversion/trunk/subversion/bindings/swig/python/svn/fs.py (original) > +++ subversion/trunk/subversion/bindings/swig/python/svn/fs.py Thu Oct 5 > 07:36:08 2023 > @@ -23,6 +23,7 @@ > # under the License. > ###################################################################### > > +import errno > from libsvn.fs import * > > ###################################################################### > @@ -182,8 +183,17 @@ class FileDiff: > + [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") > + try: > + p = _subprocess.Popen(cmd, stdout=_subprocess.PIPE, bufsize=-1, > + close_fds=_sys.platform != "win32") > + # When removing Python 2 support: Change to FileNotFoundError and > + # remove check for ENOENT (FileNotFoundError "Corresponds to errno > + # ENOENT" according to documentation) > + except OSError as err: > + if err.errno == errno.ENOENT: > + err.strerror = "External diff command not found in PATH" > + raise err > + > return _PopenStdoutWrapper(p) > > else: > > Modified: subversion/trunk/subversion/bindings/swig/python/tests/fs.py > URL: > http://svn.apache.org/viewvc/subversion/trunk/subversion/bindings/swig/python/tests/fs.py?rev=1912743&r1=1912742&r2=1912743&view=diff > > ============================================================================== > --- subversion/trunk/subversion/bindings/swig/python/tests/fs.py (original) > +++ subversion/trunk/subversion/bindings/swig/python/tests/fs.py Thu Oct > 5 07:36:08 2023 > @@ -308,6 +308,9 @@ class SubversionFSTestCase(unittest.Test > try: > diffout, differr = Popen(["diff"], stdin=PIPE, > stderr=PIPE).communicate() > > + # When removing Python 2 support: Change to FileNotFoundError and > + # remove check for ENOENT (FileNotFoundError "Corresponds to errno > + # ENOENT" according to documentation) > except OSError as err: > if err.errno == errno.ENOENT: > self.skipTest("'diff' command not present")
Thanks for taking care of that. Looks good to me. I am considering nominating this for backport to 1.14.x. (At least, I don't see a reason not to, since Python2 remains supported.) Cheers, Nathan
