[webkit-reviews] review denied: [Bug 54701] pathRelativeToSVNRepositoryRootForPath needs to handle svn+ssh URLs : [Attachment 82885] patch

[bugzilla-daemon at webkit.org]

Daniel Bates <dbates at webkit.org> has denied Matt Lilek (pewtermoose)
<mlilek at apple.com>'s request for review:
Bug 54701: pathRelativeToSVNRepositoryRootForPath needs to handle svn+ssh URLs
https://bugs.webkit.org/show_bug.cgi?id=54701

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

------- Additional Comments from Daniel Bates <dbates at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=82885&action=review

This change looks good to me, but we are missing a unit test.

We should separate the parsing logic from the file system/command execution
logic in this function so that we can unit test the parsing logic. See
Tools/Scripts/webkitperl/VCSUtils_unittest for example unit tests. In
particular, look at Tools/Scripts/webkitperl/VCSUtils_unittest/removeEOL.pl,
which is a unit test for a parsing function.

> Tools/Scripts/VCSUtils.pm:389
> +    $svnURL =~ s/\Q$repositoryRoot\E\///;

We should take this opportunity to change the substitution delimiter from '/'
to '|' so that we don't have to escape the the '/':

$svnURL =~ s|\Q$repositoryRoot\E/||;