[webkit-reviews] review denied: [Bug 85791] Remove CYGWIN=tty from environment variable as its no longer supported : [Attachment 140997] Patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at webkit.org> has denied Vivek Galatage
<vivekgalatage at gmail.com>'s request for review:
Bug 85791: Remove CYGWIN=tty from environment variable as its no longer
supported
https://bugs.webkit.org/show_bug.cgi?id=85791

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

------- Additional Comments from Adam Roben (:aroben) <aroben at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=140997&action=review


This is looking close!

> Tools/ChangeLog:6
> +	   Added check to support older versions of Cygwin 1.7.9 or lesser

This doesn't really describe the entire change; it only describes the
differences from the last version of your patch.

> Tools/Scripts/webkitdirs.pm:1491
> +	   # FIXME: The setting CYGWIN=tty is supported only till cygwin
version 1.7.9

Why is this a FIXME? I don't think there's anything we need to "fix" here.

> Tools/Scripts/webkitdirs.pm:1495
> +	   my $currentCygwinVersion = version->parse(`uname -r`);
> +	   my $minCygwinVersion = version->parse("1.7.10");
> +	   if ($currentCygwinVersion < $minCygwinVersion) {

I didn't know about the version module! Seems less mysterious than using
"v1.7.10" strings.

I'd rename $minCygwinVersion to $firstCygwinVersionWithoutTTYVariable for
clarity.