pathRelativeToSVNRepositoryRootForPath needs to handle svn+ssh URLs

[Matt Lilek]

The regex used to strip the repository root in pathRelativeToSVNRepositoryRootForPath in VCSUtils.pm needs to handle svn+ssh URLs.

[Matt Lilek]

Created attachment 82885 [details]
patch

[Daniel Bates]

Comment on attachment 82885 [details]
patch

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/||;

[Matt Lilek]

Created attachment 84506 [details]
Take 2

[Daniel Bates]

Comment on attachment 84506 [details]
Take 2

View in context: https://bugs.webkit.org/attachment.cgi?id=84506&action=review

Thank you Matt for separating out the parsing logic and adding a unit test!

> Tools/ChangeLog:8
> +        * Scripts/VCSUtils.pm:

By convention we usually document the functions we've added here because prepare-ChangeLog doesn't know how to generate this information for Perl scripts. (We should teach prepare-ChangeLog to do so). See <http://trac.webkit.org/changeset/77028> and <http://trac.webkit.org/changeset/52692> for examples of the format we use.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPathFromRepositoryRoot.pl:15
> +#     * Neither the name of Research In Motion Limited nor the names of

Nit: "Research In Motion Limited" => "Apple Inc."

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPathFromRepositoryRoot.pl:53
> +my $svnInfo_http = "Path: update-webkit
> +Name: update-webkit
> +URL: http://svn.webkit.org/repository/webkit/trunk/Tools/Scripts/update-webkit
> +Repository Root: http://svn.webkit.org/repository/webkit
> +Repository UUID: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
> +Revision: 80194
> +Node Kind: file
> +Schedule: normal
> +Last Changed Author: dpranke\@chromium.org
> +Last Changed Rev: 78520
> +Last Changed Date: 2011-02-14 15:59:48 -0800 (Mon, 14 Feb 2011)
> +Text Last Updated: 2011-03-02 18:26:56 -0800 (Wed, 02 Mar 2011)
> +Checksum: 7639a3c28d7d1828af0aad8587cf9e6b
> +
> +";

So that we don't have to escape the '@' we should use non-interpolated "here-document" syntax:

my $svnInfo = <<'END';
Path: update-webkit
Name: update-webkit
...
END

I explicitly wrote the ';' on the first line, did not put a space between "<<" and 'END', and single-quote END in the aforementioned example (that is, these are not typos). For more information on "here-document" syntax see <http://perldoc.perl.org/perlop.html>. You can also see examples of here-document syntax in some of the existing unit tests, say parseGitDiffHeader.pl.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseSvnPathFromRepositoryRoot.pl:72
> +my $svnInfo_svnPlusSSH = "Path: update-webkit
> +Name: update-webkit
> +URL: svn+ssh://username\@svn.webkit.org/repository/webkit/trunk/Tools/Scripts/update-webkit
> +Repository Root: svn+ssh://username\@svn.webkit.org/repository/webkit
> +Repository UUID: 268f45cc-cd09-0410-ab3c-d52691b4dbfc
> +Revision: 80194
> +Node Kind: file
> +Schedule: normal
> +Last Changed Author: dpranke\@chromium.org
> +Last Changed Rev: 78520
> +Last Changed Date: 2011-02-14 15:59:48 -0800 (Mon, 14 Feb 2011)
> +Text Last Updated: 2011-03-02 18:26:56 -0800 (Wed, 02 Mar 2011)
> +Checksum: 7639a3c28d7d1828af0aad8587cf9e6b
> +
> +";

Ditto.

[Darin Adler]

Comment on attachment 84506 [details]
Take 2

Clearing review flag on this months-old patch that probably can’t be landed as is.