[webkit-reviews] review granted: [Bug 39184] Add support for parsing a SVN property : [Attachment 56212] Patch with unit tests

[bugzilla-daemon at webkit.org]

Chris Jerdonek <cjerdonek at webkit.org> has granted Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 39184: Add support for parsing a SVN property
https://bugs.webkit.org/show_bug.cgi?id=39184

Attachment 56212: Patch with unit tests
https://bugs.webkit.org/attachment.cgi?id=56212&action=review

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Feel free to consider these nits and minor comments below before landing. r=me.



> +#	 value: the last property value.  For instance, suppose the property is
"Modified".
> +#		Then it has both a '-' and '+' property value in that order. 
Therefore,
> +#		the value of this key is the value of the '+' property by
ordering (since
> +#		it is the last-most value).

Nit: last instead of last-most.

> +    # FIXME: We do not support property values that contain tailing new line
characters

trailing, newline (instead of new line)

> +    #	as it is difficult to disambiguate these trailing new lines
from the empty

newline

> +    #	line that preceeds the contents of a binary patch.

precedes


> +    my $propertyValue;
> +    my $propertyValueType;
> +    while (defined($_) && /$svnPropertyValueStartRegEx/) {
> +	   # Note, a '-' property may be followed by '+' property in the case
of a "Modified"

a '+' property

> +    my $propertyChangeDelta;
> +    if ($propertyValueType eq '+') {
> +	   $propertyChangeDelta = 1;
> +    } elsif ($propertyValueType eq '-') {
> +	   $propertyChangeDelta = -1;
> +    } else {
> +	   # Not reached.
> +	   die;

Would die("Not reached.") be better?  Just in case. :)

> +    # New test
> +    diffName => "single-line '+' with trailing new line, followed by empty
line and start of binary patch",

Did you do any tests for property value followed by start of next diff?