Bug 33119 – svn-apply breaks on patch files with leading white space

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Chris Jerdonek]

I noticed this one while reading svn-apply. If a patch file begins with white space, for instance:

--BEGIN--



Index: WebKitTools/ChangeLog
===================================================================
...

Then svn-apply terminates with the following, even though "Index" is present in the file:

$ svn-apply ~/dev-my/test-patch.txt
Failed to find 'Index:' in:
-------------------------------------------------------------------





-------------------------------------------------------------------
Died at .../WebKit/WebKitTools/Scripts/svn-apply line 339, <> line 575.

Fixing this will eliminate one case where --force needs to be used.  It's not a big deal, but it's something I was going to fix anyways in the course of some refactoring.

It's because the main while loop starts writing to $patch even before it encounters Index:

while (<>) {
    ...
    if (/^Index: (.+)/) {
        $indexPath = $1;
        if ($patch) {
            if (!$copiedFromPath) {
                push @patches, $patch;
            }
            $copiedFromPath = "";
            $patch = "";
        }
    }
    ...
    $patch .= $_;
    $patch .= $eol;
}

Let me know if you don't want to be CC'ed on future Perl issues.

[Zoltan Horvath]

Created attachment 51019 [details]
Patch

[Zoltan Horvath]

With using of --force we might lose useful warnings, so this solution should be better.

[Eric Seidel (no email)]

Comment on attachment 51019 [details]
Patch

We have perl unit tests now.  We should be able to test this.

If the perl unit tests aren't strong enough to test this, scm_unitttest.py certainly is.  You can run it using test-webkitpy --all

[Zoltan Horvath]

Created attachment 52855 [details]
proposed patch with unittests

[Chris Jerdonek]

Comment on attachment 52855 [details]
proposed patch with unittests

Marking this patch r- because it no longer applies.

Also, some high-level comments regarding the directory being created here:

> +++ WebKitTools/Scripts/webkitperl/tools_unittest/svn-apply.pl	(revision 0)

Might it make more sense to continue the existing pattern of one *_unittest/
directory per file to be unit-tested?  For example, svn-apply_unittest/ and
svn-unapply_unittest/?

If code can be shared between tests of code in svn-apply and svn-unapply,
maybe that can be handled by adding a test-support file in webkitperl/.

Finally, in cases where svn-apply and svn-unapply define the same or nearly
the same methods (because of cut-and-paste), it may also make sense to
refactor those areas to use the same code.  Then for each refactored
function, one function can be unit-testted instead of two.  That's not
something you necessarily need to be the one to do, but it's something
to consider.

[Zoltan Horvath]

(In reply to comment #5)
> (From update of attachment 52855 [details])
> Marking this patch r- because it no longer applies.
> 
> Also, some high-level comments regarding the directory being created here:
> 
> > +++ WebKitTools/Scripts/webkitperl/tools_unittest/svn-apply.pl	(revision 0)
> 
> Might it make more sense to continue the existing pattern of one *_unittest/
> directory per file to be unit-tested?  For example, svn-apply_unittest/ and
> svn-unapply_unittest/?

I don't like dir/unittest direction. I think it's more perspicuous to have only one unittest for a file, so directories're needless.
 
> If code can be shared between tests of code in svn-apply and svn-unapply,
> maybe that can be handled by adding a test-support file in webkitperl/.
>
> Finally, in cases where svn-apply and svn-unapply define the same or nearly
> the same methods (because of cut-and-paste), it may also make sense to
> refactor those areas to use the same code.  Then for each refactored
> function, one function can be unit-testted instead of two.  That's not
> something you necessarily need to be the one to do, but it's something
> to consider.

I considered this earlier, and I agree with merging the two files into one. (I opened a bug: bug #38388 ) So first, we should try to refactoring it, and then as you said there will be only unit-test. If it's okay, next week I'll try to allocate some time for this. :-)

[Chris Jerdonek]

(In reply to comment #6)
> > Might it make more sense to continue the existing pattern of one *_unittest/
> > directory per file to be unit-tested?  For example, svn-apply_unittest/ and
> > svn-unapply_unittest/?
> 
> I don't like dir/unittest direction. I think it's more perspicuous to have only
> one unittest for a file, so directories're needless.

A directory was created for VCSUtils.pm's unit tests to avoid having a single monster file.  VCSUtils.pm is up to around 37 subroutines, and some of those subroutines need many lines to test since the inputs and outputs are patch strings.  For example, the unit test file for the parseDiff() subroutine is over 300 lines not counting the initial license text.  And that may grow as we add support for other patch syntax like SVN properties, etc.

[Chris Jerdonek]

Resolved in:

https://bugs.webkit.org/show_bug.cgi?id=38507