Bug 41333 – VCSUtils.pm complains about uninitialized value $newLine

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Dumitru Daniliuc]

Every once in a while when I run svn-create-patch (svn client, in cygwin), I get this error:

Use of uninitialized value $newLine in substr at /cygdrive/d/webkit5/WebKitTools/Scripts/VCSUtils.pm line 1310.
substr outside of string at /cygdrive/d/webkit5/WebKitTools/Scripts/VCSUtils.pm line 1310.
Use of uninitialized value in string ne at /cygdrive/d/webkit5/WebKitTools/Scripts/VCSUtils.pm line 1310.

[Dumitru Daniliuc]

I was able to get past this problem by changing

my $newLine = pop(@overlappingLines);
if ($text ne substr($newLine, 1)) {

to

my $newLine = (scalar @overlappingLines > 0 ? pop(@overlappingLines) : "");
if ((length($newLine) == 0) || ($text ne substr($newLine, 1))) {

The patch file I got seems to be OK.

[Dumitru Daniliuc]

Created attachment 59990 [details]
ChangeLogs files + patch

[Daniel Bates]

Created attachment 60679 [details]
[Patch] Work in progress

[Daniel Bates]

Created attachment 60832 [details]
Patch with unit test

We run off the end of an array when processing a change log entry that was inserted earlier in the file, but after an entry with the same author and date (because of the blank line of context at the top of the change log entry).

Currently, when we detect that the new change log entry is earlier in the change log file we do not move this entry to the top of the file as the entry may have been explicitly placed earlier by the author of the patch. This situation seems rare and we may want to re-consider this decision and/or provide some kind of optional boolean flag to forcefully move a new change log entry to the top of the change log to prevent inadvertently landing a change with a change log entry in the wrong place. Alternatively, we may want to consider adding a warning message of the form "WARNING: The new change log entry is not at the top of the change log. Make sure this is what you intended" when we detect such a change log entry.

For the purpose of this patch, I have decided to only fix the array issue as I think it is best that we resolve the placement issue/print a warning in a separate patch.

[Dumitru Daniliuc]

Comment on attachment 60832 [details]
Patch with unit test

r=me.

FWIW, I think we we should always push new ChangeLogs entries to the top.

[Daniel Bates]

Comment on attachment 60832 [details]
Patch with unit test

Clearing flags on attachment: 60832

Committed r62755: <http://trac.webkit.org/changeset/62755>

[Daniel Bates]

All reviewed patches have been landed.  Closing bug.

[Daniel Bates]

(In reply to comment #5)
> (From update of attachment 60832 [details])
> r=me.
> 
> FWIW, I think we we should always push new ChangeLogs entries to the top.

I filed bug #41833 to track this issue.

[WebKit Review Bot]

http://trac.webkit.org/changeset/62755 might have broken GTK Linux 64-bit Debug

[Daniel Bates]

(In reply to comment #9)
> http://trac.webkit.org/changeset/62755 might have broken GTK Linux 64-bit Debug

The error message appear unrelated to this change.

On another note, this bot has run out of disk space as per the error messages in the Perl and Python unit test stdio, <http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/9397/steps/webkitperl-test/logs/stdio> and <http://build.webkit.org/builders/GTK%20Linux%2064-bit%20Debug/builds/9397/steps/webkitpy-test/logs/stdio>, respectively.