On Sat, Feb 10, 2018 at 8:28 PM Troy Curtis Jr <troycurti...@gmail.com> wrote:
> On Thu, Feb 8, 2018 at 12:22 PM Kenneth Porter <sh...@sewingwitch.com> > wrote: > >> On 2/8/2018 4:35 AM, Daniel Shahaf wrote: >> > So, just to be clear, the problem is that svn/fs.py is not py3 >> > compatible, and having the 'builtins' module under py2 merely >> > exposes that? I.e., we have no reason to suspect a bug in the 'future' >> > package's implementation of builtins.open() under py2. >> >> That's my interpretation. As I said at the start of the thread, it was >> never clear why the internal temporary file was opened in text mode and >> update mode when the code was first written. >> >> > I committed the fix to the bindings in > https://svn.apache.org/viewvc?view=revision&revision=1823802 . In > addition to Kenneth's suggestion of opening in binary mode, I switched the > imports so that the python2-future's implementation would not get > inadvertently pulled in. Everything looked fine with the how > python2-future's open worked (since it did in fact use the Python 3's > open() semantics), but I think it best that on the intended modules are > included. I also added a test which duplicated the issue (with > python2-future installed at least), and confirms the fix. > > This is a relatively isolated change, but fixes surprising behavior (as > Kenneth can attest to), does something like this make sense to propose for > the 1.10 branch? > > https://ci.apache.org/builders/svn-windows-ra/builds/1998 It did occur to me at one point, "I wonder how well this works on Windows", since it shells out to external 'diff' command. I presume fs.FileDiff has never worked on Windows (well presumably it would if a compatible 'diff' command was in the PATH), but there was never a test for it previously. I believe that Subversion has an internal diff generation utility, could someone point me in the right direction? I think just changing the implementation to using an internal diff would make this more portable. Troy
