Bug 48196 – Fix Perl uninitialized warnings in VCSUtils::svnStatus() and VCSUtils::removeEOL().

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

VCSUtils::svnStatus() concatenate the output of svn status with new line character. svn status may return no ouput (say for a file that has not been added, deleted, or modified). Currently, VCSUtils::svnStatus() assumes svn status always returns output. Instead, we should check that svn status returns some output before trying to concatenate it with the new line character.

Also, VCSUtils::removeEOL() should return if its argument is uninitialized.

[Daniel Bates]

For completeness, I came across this issues when performing the following within the WebKit directory:

1. Download the patch for bug 6751 <https://bug-6751-attachments.webkit.org/attachment.cgi?id=71377> to ~/Desktop as Bug6751_2.patch

2. WebKitTools/Scripts/svn-apply ~/Desktop/Bug6751_2.patch.

Then the output of svn-apply will contain:
...
patching file LayoutTests/fast/frames/resources/frame-element-name-left.html
patching file LayoutTests/fast/frames/resources/frame-element-name-right.html
Use of uninitialized value $line in substitution (s///) at /Users/dbates/Desktop/WebKit/WebKitTools/Scripts/VCSUtils.pm line 417.
Use of uninitialized value in concatenation (.) or string at /Users/dbates/Desktop/WebKit/WebKitTools/Scripts/VCSUtils.pm line 447.

Notice the uninitialized value warnings.

[Daniel Bates]

Created attachment 71660 [details]
Patch

[David Kilzer (:ddkilzer)]

Comment on attachment 71660 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=71660&action=review

Is it possible to write a test for this?  Maybe verify that removeEOL(undef) returns ""?

> WebKitTools/Scripts/VCSUtils.pm:416
> +    return unless $line;

Why not do this?

    return "" unless $line;

> WebKitTools/Scripts/VCSUtils.pm:447
> +        $svnStatus = (removeEOL(<SVN>) || "") . "\n";

Then you don't have to do anything special here.

[Daniel Bates]

(In reply to comment #3)
> (From update of attachment 71660 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=71660&action=review
> 
> Is it possible to write a test for this?  Maybe verify that removeEOL(undef) returns ""?

Yes, we can write a test for this. We would need to export the function removeEOL.

> 
> > WebKitTools/Scripts/VCSUtils.pm:416
> > +    return unless $line;
> 
> Why not do this?
> 
>     return "" unless $line;

Will change.

[Daniel Bates]

Created attachment 71668 [details]
Patch with unit test

Updated patch based on David Kilzer's comments. Added unit test.

[Daniel Bates]

Created attachment 71669 [details]
Patch with unit test

Fix copyright in unit test file removeEOL.pl.

[David Kilzer (:ddkilzer)]

Comment on attachment 71669 [details]
Patch with unit test

Thanks Dan!  r=me

[Daniel Bates]

Comment on attachment 71669 [details]
Patch with unit test

Clearing flags on attachment: 71669

Committed r70415: <http://trac.webkit.org/changeset/70415>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.