[webkit-reviews] review granted: [Bug 118922] Add script to update Xcode build location when switching branches : [Attachment 207150] Patch

[bugzilla-daemon at webkit.org]

David Kilzer (:ddkilzer) <ddkilzer at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 118922: Add script to update Xcode build location when switching branches
https://bugs.webkit.org/show_bug.cgi?id=118922

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

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


r=me with the grammar fixes in the ChangeLog and error message.

> Tools/ChangeLog:10
> +	   location used by an Xcode workspace file and a post-checkout Git
hook script that
> +	   be used to run update-Xcode-workspace-build-location whenever a
person switches

Typo:  …that *can* be used...

> Tools/ChangeLog:18
> +

Maybe add a reference to the wiki page like this?

See also: <http://trac.webkit.org/wiki/UsingGitWithWebKit>

> Tools/Scripts/update-Xcode-workspace-build-location:50
> +    "no-symlink" => \$doNotUseSymlink,

Getopt::Long has automatic negation recognition which would allow --symlink,
--nosymlink and --no-symlink automatically.  See "man Getopt::Long".

    "symlink+" => \$useSymlink,

Then you can get rid of the negative logic required for $doNotUseSymlink.

Not required to land this patch; for a future update.

> Tools/Scripts/update-Xcode-workspace-build-location:57
> +  --no-symlink		       Modify the Xcode workspace to point to
the current WebKit build without using a symbolic link

if you switch to $useSymlink, you should print out the default here.

> Tools/Scripts/update-Xcode-workspace-build-location:112
> +    print "Cannot find neither file \"$xcodeWorkspaceSettings\"\n";

Grammar issue.	Should either be:

    Can find neither X nor Y.

or:

    Cannot find either X or Y.