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.
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.
Created attachment 71660 [details] Patch
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.
(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.
Created attachment 71668 [details] Patch with unit test Updated patch based on David Kilzer's comments. Added unit test.
Created attachment 71669 [details] Patch with unit test Fix copyright in unit test file removeEOL.pl.
Comment on attachment 71669 [details] Patch with unit test Thanks Dan! r=me
Comment on attachment 71669 [details] Patch with unit test Clearing flags on attachment: 71669 Committed r70415: <http://trac.webkit.org/changeset/70415>
All reviewed patches have been landed. Closing bug.