Bug 38388 – svn-apply: merge its implementation with svn-unapply

• Status: NEW
• Component: Tools / Tests

[Zoltan Horvath]

Merge svn-apply and svn-unapply into one file, since these define nearly the same methods.

[Chris Jerdonek]

Another approach that can be taken in conjunction with this approach is to move/refactor methods defined in both files to a third location.  The third location can be VCSUtils.pm or a new library file in webkitperl/ named something like svnApplyUtils.pm.  I started doing this myself here, for instance:

http://trac.webkit.org/changeset/52692 

Question: are you suggesting that, in the end, svn-unapply should be invoked by calling svn-apply (e.g. by passing a -R/--reverse flag)?  That sounds good.  If so, svn-unapply could be a wrapper for svn-apply until the callers are modified.

[Zoltan Horvath]

(In reply to comment #1)
> Another approach that can be taken in conjunction with this approach is to
> move/refactor methods defined in both files to a third location.  The third
> location can be VCSUtils.pm or a new library file in webkitperl/ named
> something like svnApplyUtils.pm.  I started doing this myself here, for
> instance:
> 
> http://trac.webkit.org/changeset/52692 

If we follow this direction the unit-testing can be more precise not by testing the whole file(s), but subroutines directly. If the merged functions can be used in other files I like the idea to put it into VCSUtils, if not, separated file would be better.

> Question: are you suggesting that, in the end, svn-unapply should be invoked by
> calling svn-apply (e.g. by passing a -R/--reverse flag)?  That sounds good.  If
> so, svn-unapply could be a wrapper for svn-apply until the callers are
> modified.

Yes. I was thinking on the same.

[Chris Jerdonek]

(In reply to comment #2)
> (In reply to comment #1)
> > Another approach that can be taken in conjunction with this approach is to
> > move/refactor methods defined in both files to a third location.
> 
> If we follow this direction the unit-testing can be more precise not by testing
> the whole file(s), but subroutines directly.

We can still do this either way.  For example, if you look at VCSUtils_unittest/, you will see that it contains unit tests for individual subroutines defined in VCSUtils.pm.

In the end, perhaps svn-apply can have a top-level "main()" method.  Then if one wants to go the route of testing the whole file, testing that single subroutine is almost as good from a code coverage standpoint as testing the "whole file".  It won't be as necessary to test the script by invoking it as a script.

[Sam Weinig]

(In reply to comment #1)
> Question: are you suggesting that, in the end, svn-unapply should be invoked by
> calling svn-apply (e.g. by passing a -R/--reverse flag)?  That sounds good.  If
> so, svn-unapply could be a wrapper for svn-apply until the callers are
> modified.

Please don't remove the ability to call svn-apply and svn-unapply.  I have been happily using these scripts for many years and really don't want to change my habits.  Merging the implementations sounds fine.

[Chris Jerdonek]

Retitling with "svn-apply" at the beginning of the title.