[Webkit-unassigned] [Bug 111066] svn-apply cannot apply patches which is generated by git to files that contain space characters in their path

[bugzilla-daemon at webkit.org]

https://bugs.webkit.org/show_bug.cgi?id=111066





--- Comment #22 from Daniel Bates <dbates at webkit.org>  2013-06-04 14:54:00 PST ---
(From update of attachment 203682)
View in context: https://bugs.webkit.org/attachment.cgi?id=203682&action=review

This patch looks good. I have some minor nits.

> Tools/ChangeLog:8
> +        If the path in a diff has space characters, parseGitDiffHeader() considers last nonspace characters as file path.

I'm unclear what you mean by this sentence.

> Tools/ChangeLog:9
> +        When the diff have prefix, we consider next characters after "b/" as file path.

Is this sentence correct? I thought we currently considered the next characters after "b/" as part of a file path?

> Tools/ChangeLog:12
> +        We only support --src-prefix and --dst-prefix don't have a \W and end with '/' because we cannot distinguish the prefix from directory path.

For your consideration, I suggest we write out the meaning of \W in English and hence change "have a \W" to "contain a non-word character (\W)".

> Tools/Scripts/VCSUtils.pm:111
> +my $gitDiffStartWithPrefixRegEx = qr#^diff --git \w/(.+) \w/([^\r\n]+)#; # We suppose that --src-prefix and --dst-prefix don't have a \W and end with '/'.

For your consideration, I suggest we write out the meaning of \W in English and hence change "have a \W" to "contain a non-word character (\W)".

> Tools/Scripts/VCSUtils.pm:114
> +my $gitDiffPathPrefix = qr#^diff --git ([^\/]+\/)#;

It's unnecessary to escape the '/' character in this regular expression since we use '#' as the delimiter. Thus, we can simplify the regular expression in this line to read: ^diff --git ([^/]+/)

On another note, I wish we could come up with a more descriptive name for this variable. Maybe $gitDiffStartWithoutPrefixFirstDirectoryNameRegExp? or $gitDiffStartWithoutPrefixSourceDirectoryPrefixRegExp? or $gitDiffStartPathPrefixRegExp?

> Tools/Scripts/VCSUtils.pm:624
> +#   $line: the line last read from $fileHandle

The description of this argument is incorrect. We expect $line to be the diff --git line.

> Tools/Scripts/VCSUtils.pm:626
> +# Returns $indexPath: the path of the target file.

There is no such local variable called $indexPath. I would update this line to read:

Returns the path of the target file.

> Tools/Scripts/VCSUtils.pm:636
> +        # Moving top directory file is not supported (e.g diff --git A.txt B.txt).

We should make this a FIXME comment.

> Tools/Scripts/VCSUtils.pm:637
> +        die("Could not find '/' in  \"diff --git\" line: \"$line\".\nPlease try git diff without --no-prefix");

I suggest we further elaborate on the reason we failed similar to the comment on the line above. Maybe something like:

"Could not find '/' in \"diff --git\" line: \"$line\"; only non-prefixed git diffs (i.e. not generated with --no-prefix) that move a top-level directory file are supported."

> Tools/Scripts/VCSUtils.pm:640
> +    if (!/^diff --git $pathPrefix.+ ($pathPrefix.+)$/) {

We should disable all metacharacters in $pathPrefix when interpolating its value by surrounding it with the special escapes \Q, \E:

/^diff --git \Q$pathPrefix\E.+ (\Q$pathPrefix\E.+)$/

For completeness, the special escape characters \Q and \E are discussed in <http://perldoc.perl.org/perlop.html#Quote-and-Quote-like-Operators>.

> Tools/Scripts/VCSUtils.pm:641
> +        # Moving a file through sub directories of top directory is not supported (e.g diff --git A/B.txt C/B.txt).

We should make this a FIXME coment.

> Tools/Scripts/VCSUtils.pm:642
> +        die("Could not find '/' in  \"diff --git\" line: \"$line\".\nPlease try git diff without --no-prefix");

I suggest we further elaborate on the reason we failed similar to the comment on the line above. Maybe something like:

"Could not find '/' in \"diff --git\" line: \"$line\"; only non-prefixed git diffs (i.e. not generated with --no-prefix) that move a file between top-level directories are supported."

> Tools/Scripts/VCSUtils.pm:740
> +            # This suffix make our diff more closely match the SVN diff format.

Nit: make => makes

> Tools/Scripts/VCSUtils.pm:746
> +            # This suffix make our diff more closely match the SVN diff format.

Nit: make => makes

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:84
> +    diffName => "Modified file which have empty characters in path",

Nit: "empty" => "space"

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:107
> +{   # New test

These test cases are good. For your consideration, I suggest we create a test case that has a diff --git line identical to one in attachment #190663 (which is the patch that revealed the bug in our Git diff parsing code):

diff --git a/WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme b/WebKit.xcworkspace/xcshareddata/xcschemes/All Source (target WebProcess).xcscheme

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:108
> +    diffName => "Modified file which have empty characters in path using --no-prefix",

Nit: "empty" => "space"

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:186
> +    diffName => "delete file which have empty characters in path using --no-prefix",

Nit: "empty" => "space"

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:264
> +    diffName => "copy file which have empty characters in path using --no-prefix (with similarity index 100%)",

Ditto.

> Tools/Scripts/webkitperl/VCSUtils_unittest/parseGitDiffHeader.pl:338
> +    diffName => "rename file which have empty characters in path using --no-prefix (with similarity index 100%)",

Ditto.

-- 
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.