[webkit-reviews] review denied: [Bug 39170] Add support function to parse SVN single-line and multi-line property value : [Attachment 56171] Patch with unit tests

[bugzilla-daemon at webkit.org]

Chris Jerdonek <cjerdonek at webkit.org> has denied Daniel Bates
<dbates at webkit.org>'s request for review:
Bug 39170: Add support function to parse SVN single-line and multi-line
property value
https://bugs.webkit.org/show_bug.cgi?id=39170

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

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Thanks, Dan!  Comments below.  r- to consider them:

> Index: WebKitTools/ChangeLog
> +	   Add function parseSvnPropertyPlusOrMinus to parse single-line and
multi-line
> +	   property values.

Would parseSvnPropertyValue() be more direct?

> +
> +	   * Scripts/VCSUtils.pm:
> +	       Added function parseSvnPropertyPlusOrMinus. We will use this as
part of
> +	       Bug #38885.

It might be helpful to include a bug URL for clickability.

> Index: WebKitTools/Scripts/VCSUtils.pm
>  
>  my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
>  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> +my $svnPropertyChangeStartRegEx = qr#^(Modified|Name|Added|Deleted):
([^\r\n]+)#; # $1 is the name of the property
> +my $svnPropertyValueRegEx = qr#^   (\+|-) ([^\r\n]+)#; # $2 is the start of
the property's value (which may span multiple lines).

Would $svnPropertyValueStartRegEx be better since it is just the first line?

> +# Parse the property value of an SVN property from the given file handle,
and advance
> +# the handle so the last line read is the first line after the property
value.

I would abbreviate this to, "Parse the value of an SVN property change from..."

and then in a separate paragraph include that an SVN property change is
a section in the footer of an SVN diff with one or two property value 
sub-sections.

> +# This subroutine dies if the first line is not a valid SVN property value
line
> +# (i.e. a line that begins with "   +" or "	 -").

"...not the beginning of a valid..."

> +#   $fileHandle: advanced so the last line read from the handle is the first

> +#		    line of the property value to parse.  This should be a line

> +#		    beginning with "   +" or "	 -".

You already said this above.  Probably just include it in one of the two
places.

> +# FIXME: This method is unused as of (05/15/2010). We will call this
function
> +#	    as part of parsing a property from the SVN footer. See Bug #38885.

Maybe include bug URL.

> +sub parseSvnPropertyPlusOrMinus($$)
> +{
> +    my ($fileHandle, $line) = @_;
> +
> +    $_ = $line;
> +
> +    my $propertyValue;
> +    if (/$svnPropertyValueRegEx/) {
> +	   $propertyValue = $2; # Does not include the end-of-line character(s)

> +    } else {
> +	   die("Failed to find '+' or '-' property: \"$_\".");

Maybe "Failed to find property value beginning with '+' or '-': ..."?

> +    }
> +
> +    $_ = <$fileHandle>;
> +    $propertyValue .= $POSTMATCH if defined($_); # Preserve the end-of-line
of the first line in a multi-line property value.

It looks like you're including the end-of-line characters even if the
property value has just one line.  Also, could you possibly rework things
so that the $POSTMATCH value is read from closer to the reg ex (e.g. right
after where $propertyValue is set to $2?

> +# The array of test cases.

This is probably redundant (even though it's in the other files).

> +my @testCaseHashRefs = (
> +{
> +    # New test

Is it necessary to write "# New test" at the beginning of each test case?