[webkit-reviews] review granted: [Bug 74469] svn scripts cannot handle files with @ symbols : [Attachment 119133] second patch

[bugzilla-daemon at webkit.org]

Adam Roben (:aroben) <aroben at apple.com> has granted Stephanie Lewis
<slewis at apple.com>'s request for review:
Bug 74469: svn scripts cannot handle files with @ symbols
https://bugs.webkit.org/show_bug.cgi?id=74469

Attachment 119133: second patch
https://bugs.webkit.org/attachment.cgi?id=119133&action=review

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


> Tools/Scripts/VCSUtils.pm:1997
> +sub escapePath($)
> +{
> +    my ($path) = @_;
> +    $path .= "@" if $path =~ /@/;
> +    return $path;
> +}

Since this function only makes sense for Subversion, it should probably either
do nothing for git or have a name that makes it clear it's Subversion-only.
Maybe escapeSubversionPath?

> Tools/Scripts/prepare-ChangeLog:1340
> +	   my @escapedPaths;
> +	   foreach my $path (@paths) { push @escapedPaths, escapePath($path); }


You can do this more succinctly with map:

my @escapedPaths = map(escapePath, @paths);

Seems like $pathsString should become a variable that's only used in the git
codepath.

> Tools/Scripts/prepare-ChangeLog:1361
> +	   my @escapedFiles;
> +	   foreach my $file (@files) { push @escapedFiles, escapePath($file); }

> +	   my $escapedFilesString = "'" . join("' '", @escapedFiles) . "'";
> +	   $command = "$SVN stat $escapedFilesString";

Same comments as above.

> Tools/Scripts/svn-apply:455
> +	   open SVN, "svn rm --force '$escapedPath' |" or die "svn rm --force
'$path' failed!";

You should use $escapedPath in the error message too.