Review Request: svn.fs.FileDiff

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

Review Request: svn.fs.FileDiff

Troy Curtis Jr
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

FileDiff.patch (6K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: svn.fs.FileDiff

Daniel Shahaf-2
Troy Curtis Jr wrote on Sun, 11 Feb 2018 16:10 +0000:
> I'm asking for comments because the actual diff output given when
> 'diffoptions' is None, is changed.
...
> Thoughts?

1. We have two consumers of FileDiff in tools/.  Check whether they are affected?

2. We might include the change in an alpha/beta release to give it more testing.

(#2 applies to FSFS's new verify-before-commit knob, too.)

Cheers,

Daniel
Reply | Threaded
Open this post in threaded view
|

verify-before-commit knob [was: Review Request: svn.fs.FileDiff]

Julian Foad-5
Daniel Shahaf wrote:
> 2. We might include the change in an alpha/beta release to give it more testing.
>
> (#2 applies to FSFS's new verify-before-commit knob, too.)

I noticed a 'verify-before-commit' option is included in fsfs.conf
nowadays, "enabled by default in debug builds". (On unix that's
--enable-maintainer-mode, not --enable-debug.)

- Julian
Reply | Threaded
Open this post in threaded view
|

Re: Review Request: svn.fs.FileDiff

Troy Curtis Jr
In reply to this post by Daniel Shahaf-2


On Sun, Feb 11, 2018 at 4:33 PM Daniel Shahaf <[hidden email]> wrote:
Troy Curtis Jr wrote on Sun, 11 Feb 2018 16:10 +0000:
> I'm asking for comments because the actual diff output given when
> 'diffoptions' is None, is changed.
...
> Thoughts?

1. We have two consumers of FileDiff in tools/.  Check whether they are affected?


tools/examples/svnlook.py calls with diffoptions, so it is unaffected as using args will call the executable (since that is obviously the desire having given arguments).

tools/hook-scripts/mailer/mailer.py's output would be changed. There are also other headers output on various conditions that mailer.py detects, like adding and removing files.  The more I look at it the more I think that perhaps the backwards compatible change of defaulting diff options to '[]' and checking specifically for 'None' to use the internal tool would be the way to go.  That way you can actually get to the internal subversion diff implementation if you want, but the default behavior doesn't change.

Troy
2. We might include the change in an alpha/beta release to give it more testing.

(#2 applies to FSFS's new verify-before-commit knob, too.)

Cheers,

Daniel