Den tis 3 okt. 2023 kl 02:44 skrev Nathan Hartman <hartman.nat...@gmail.com
>:
> On Mon, Oct 2, 2023 at 5:39 AM Daniel Sahlberg
> <daniel.l.sahlb...@gmail.com> wrote:
> > Den sön 1 okt. 2023 kl 21:50 skrev Nathan Hartman <
> hartman.nat...@gmail.com>:
> (snip)
> >> This (somewhat platform-specific) "gotcha" isn't completely obvious
> from a first glance at the code.
> >>
> >> Your explanation above makes the failure mode and its reasons more
> clear.
> >>
> >> So, to close SVN-1778, I would suggest only to add some documentation
> of the above fact to the function. I'll be happy to compose a suggested
> docstring.
> >
> >
> > Please do!
>
> Suggested docstring (also attached as a patch):
>
> """Perform diff and return a file object from which the output can
> be read.
>
> When DIFFOPTIONS is None (the default), use svn's internal diff.
>
> With any other DIFFOPTIONS, exec the external diff found on PATH,
> passing it DIFFOPTIONS. On Windows, exec diff.exe rather than
> diff. If a diff utility is not installed or found on PATH, throws
> FileNotFoundError. Caveat: On some systems, including Windows, an
> external diff may not be available unless installed and added to
> PATH manually.
> """
>
> More below:
>
This looks good to me!
>
> >> I don't know how it fails (with a cryptic traceback message?) but if we
> want to get fancy, perhaps a failure could give some user-friendly hint
> about things to check for (such as whether a diff tool is available and
> diffoptions are correct).
> >
> >
> > It fails with a FileNotFoundError error so it should be trivial to
> catch. I don't know the proper conventions on error handling in the
> bindings (or in Python in general), I guess we should still throw an error,
> hoping that whoever use the FileDiff class will catch it and handle in an
> intelligent way.
> >
> > Another way would be to use the internal diff in case of an exception,
> but it would be less apparent (I would rather raise an error, even if it
> just means ignoring the FileNotFoundError and documenting the cause).
>
> In this case, I would rather leave the implementation as it is
> currently. I think that calling the internal diff when an external
> diff is expected (with some unknown options) would be worse than
> failing with FileNotFoundError here. Even if the exception is not
> caught, the docstring should provide the useful hint. :-)
>
Agreed! Being fancy, how about:
[[[
Index: subversion/bindings/swig/python/svn/fs.py
===================================================================
--- subversion/bindings/swig/python/svn/fs.py (revision 1912696)
+++ subversion/bindings/swig/python/svn/fs.py (working copy)
@@ -170,8 +170,13 @@
+ [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")
+ except FileNotFoundError as e:
+ e.strerror = "External diff command not found in PATH"
+ raise
+
return _PopenStdoutWrapper(p)
else:
]]]
In my testing that gave me a FileNotFoundError with a custom error message
instead of the generic "No such file or directory".
Kind regards,
Daniel
