[webkit-reviews] review granted: [Bug 135161] Correct auto-version.pl handling of __VERSION_TEXT__, __BUILD_NUMBER__, and __BUILD_NUMBER_SHORT__ for 4+-tuple versions : [Attachment 235294] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Brent Fulgham
<bfulgham at webkit.org>'s request for review:
Bug 135161: Correct auto-version.pl handling of __VERSION_TEXT__,
__BUILD_NUMBER__, and __BUILD_NUMBER_SHORT__ for 4+-tuple versions
https://bugs.webkit.org/show_bug.cgi?id=135161

Attachment 235294: Patch
https://bugs.webkit.org/attachment.cgi?id=235294&action=review

------- Additional Comments from David Kilzer (:ddkilzer) <ddkilzer at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=235294&action=review


r=me

> WebKitLibraries/win/tools/scripts/auto-version.pl:126
> +    $PROPOSED_VERSION =~ s/^\s*(.*)\s*$/$1/; # Get rid of any
leading/trailing whitespace
> +

Please add one test with trailing and leading whitespace.  (I didn't see any
added to this patch.)

> WebKitLibraries/win/tools/scripts/auto-version.pl:127
>      # Split out the three components of the dotted version number.  We pad

Nit:  "three components" isn't accurate; maybe just remove the word "three".

> WebKitLibraries/win/tools/scripts/auto-version.pl:153
> +    $PROPOSED_VERSION = substr $PROPOSED_VERSION, $charactersToRemove;

Nit:  Kind of weird to omit parens here for substr() when they're used
everywhere else.