Bug 39184 – Add support for parsing a SVN property

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Daniel Bates]

We need to add support for parsing a SVN property so that we can use it to parse a SVN property change diff (Bug #38885) that contains one or more SVN properties.

[Daniel Bates]

Created attachment 56197 [details]
Patch with unit tests

[Chris Jerdonek]

Comment on attachment 56197 [details]
Patch with unit tests

Thanks for this patch, Dan!  Overall this looks pretty good, but several minor comments below.

> Index: WebKitTools/ChangeLog
> +        Adds function VCSUtils::parseSvnProperty to parse an SVN property with
> +        either a single-line or multi-line value.

I would add the word "change" at the end because the property can have two
"value" sections if there is both a "-" and "+".  So, "...with either a
single-line or multi-line value change."

> +
> +        * Scripts/VCSUtils.pm:
> +          - Added function parseSvnProperty. We will use this function
> +          towards resolving Bug #38885 <https://bugs.webkit.org/show_bug.cgi?id=38885>.

alignment

> +          - Removed FIXME comment above function parseSvnPropertyValue, since
> +            it is being used by parseSvnProperty.
> +          - (*) Modified function parseSvnPropertyValue to break out of "while (<$fileHandle>)"
> +            loop when it encounters the start of the next property so that it can be
> +            processed by its caller, parseSvnPropertyValue.

Instead of saying "(*)", I would say include an additional sentence saying,
"We reference this bullet below by (*)."  I was confused by what the "(*)"
meant until I read to the end of the entry.

> +        * Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl: Added.
> +          - Added unit tests.
> +        * Scripts/webkitperl/VCSUtils_unittest/parseSvnPropertyValue.pl:
> +          - Changed the name of the unit test "simple multi-line '-' change" to
> +            "single-line '-' change followed by empty line" since the former was an
> +            incorrect description of this test.
> +          - Added unit test "single-line '-' change followed by the next property", and
> +            "multi-line '-' change followed by the next property" to test (*).

I would insert the word "above" at the end as in "to test (*) above".

> Index: WebKitTools/Scripts/VCSUtils.pm
> +my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $1 is the name of the property.

Should the comment say $2?

> +# Parse the next SVN property from the given file handle, and advance the handle so the last
> +# line read is the first line after the property.
> +#
> +# This subroutine dies if the first line is an invalid SVN property
> +# (i.e. a line that does not match $svnPropertyStartRegEx) or
> +# the value of the property is missing.

I would rewrite this as "...if the first line is not a valid start of an
SVN property (e.g. if the value of the property is missing)."


> +# Returns ($propertyHashRef, $lastReadLine):
> +#   $propertyHashRef: a hash reference representing a SVN property.
> +#     name: the name of the property.
> +#     value: the last-most value of the property. 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).

I would be consistent with your uses above and use two spaces after each period.
Also, instead of "last-most", I would just say, "the last property value."


> +#     propertyChangeDelta: the value 1 or -1 if the property was added or
> +#                         removed, respectively.

alignment (from before)

> +#   $lastReadLine: the line last read from $fileHandle.
> +#
> +# FIXME: This method is unused as of (05/16/2010). We will call this function
> +#        as part of parsing a SVN footer. See <https://bugs.webkit.org/show_bug.cgi?id=38885>.

Two spaces after each period.  Also do this below (I will stop mentioning it.)

> +sub parseSvnProperty($$)
> +{
> +    my ($fileHandle, $line) = @_;
> +
> +    $_ = $line;
> +
> +    my $propertyName;
> +    my $propertyNamePrefix;

Maybe $propertyChangeType instead of prefix?  propertyNamePrefix makes it
seem like it's the prefix of the name -- e.g. the "svn" in "svn:executable".



> +    if (/$svnPropertyStartRegEx/) {
> +        $propertyNamePrefix = $1;
> +        $propertyName = $2;
> +    } else {
> +        die("Failed to find SVN property: \"$_\".");
> +    }
> +
> +    $_ = <$fileHandle>; # Not defined if end-of-file reached.
> +
> +    my $propertyValue;
> +    my $propertyValuePrefix;

Again, for similar reasons, maybe propertyValueType?

> +    while (defined($_)) {
> +        while (defined($_) && /$svnPropertyValueStartRegEx/) {
> +            # Note, a '-' property may be followed by '+' property in the case of a "Modified"
> +            # or "Name" property. We only care about the actual change (i.e. the '+' property)

Maybe "ending value" instead of "actual change" would be clearer?

> +            # in such circumstances. So, we take the property value for the property to be its
> +            # last parsed property value.
> +            # FIXME: We may want to consider strictly enforcing a '-', '+' property ordering or
> +            # add error checking to prevent '+', '+', ..., '+' and other invalid combinations.
> +            $propertyValuePrefix = $1;
> +            ($propertyValue, $_) = parseSvnPropertyValue($fileHandle, $_);

Does this while loop handle the case of empty lines between property values?
If we don't need to support that for some reason, it might be worth
mentioning commenting why.

> +        }
> +
> +        # We skip over everything that isn't an empty line or $svnPropertyStartRegEx. There is an
> +        # empty line before the contents of a binary patch.

I'm confused.  Once you have the last property value, why do you need to
keep looping through the while loop?  It seems like you can just exit
since you have the last value.  This also makes it seem like you only
need one while loop and not two nested loops.

> +        last if (!defined($_) || /^[\r\n]*\z/ || /$svnPropertyStartRegEx/);
> +        $_ = <$fileHandle>; # Not defined if end-of-file reached.
> +    }
> +
> +    if (!$propertyValue) {
> +        die("Failed to find the property value for the SVN property \"$propertyName\": \"$_\".");
> +    }
> +
> +    my $propertyChangeDelta;
> +    if ($propertyValuePrefix eq '+') {
> +        $propertyChangeDelta = 1;
> +    } elsif ($propertyValuePrefix eq '-') {
> +        $propertyChangeDelta = -1;
> +    } else {
> +        # Not reached.
> +        die;
> +    }
> +
> +    my $expectedChangeDelta;

I would add a comment here that you are doing additional validation, 
if that is what you're doing.

> +    if ($propertyNamePrefix eq "Name") {
> +        $expectedChangeDelta = 0;
> +    } elsif ($propertyNamePrefix eq "Modified") {
> +        $expectedChangeDelta = 0;
> +    } elsif ($propertyNamePrefix eq "Added") {
> +        $expectedChangeDelta = 1;
> +    } elsif ($propertyNamePrefix eq "Deleted") {
> +        $expectedChangeDelta = -1;
> +    } else {
> +        # Not reached.
> +        die;
> +    }
> +
> +    if ($propertyChangeDelta == -1 * $expectedChangeDelta) {

Why aren't you doing "if ($propertyChangeDelta != $expectedChangeDelta)"?

> +        die("Property change prefix \"$propertyValuePrefix\" does not match " .
> +            "the property name prefix \"$propertyNamePrefix\".");

I don't think "match" is the right word since they're not really supposed
to be the same.  Maybe "The final property value type found \"$propertyValueType\"
does not correspond to the property change type found \"$propertyChangeType\"."?

How hard would it be to include the correct property value type in the message?

> -#   $line: the line last read from $fileHandle
> +#   $line: the line last read from $fileHandle.

Thanks.

> -        if (/^$/ || /$svnPropertyValueStartRegEx/) {
> +        if (/^$/ || /$svnPropertyValueStartRegEx/ || /$svnPropertyStartRegEx/) {
>              # Note, we may encounter an empty line before the contents of a binary patch.
>              # Also, we check for $svnPropertyValueStartRegEx because a '-' property may be
>              # followed by a '+' property in the case of a "Modified" or "Name" property.
> +            # We check for $svnPropertyStartRegEx because it indicates the start of the
> +            # next property to parse.

Does this mean that there is not always an empty line separating one
property change from another?

> Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl

Thanks for your unit tests.  It seems like you should be putting lines
after the property changes in each of your test cases to test the
expectedNextLine and last read line in each case.

> +{
> +    # New test
> +    diffName => "simple: add svn:executable",
> +    inputText => <<'END',
> +Added: svn:executable
> +   + *

For example, include a blank line and a line marking the beginning of the
next diff here.  In most use cases, you will have stuff after the property
value.  This will also let you test your handling of empty lines, etc.
in relation to the while loops above.

> +{
> +    # New test
> +    diffName => "multi-line '+' change, followed by svn:executable",
> +    inputText => <<'END',
> +Name: documentation
> +   + A
> +long sentence that spans
> +multiple lines.
> +Name: svn:executable
> +   + *
> +END

I thought there was also an empty line between successive properties,
or did you recently find that that is not the case?

[Daniel Bates]

(In reply to comment #2)
> (From update of attachment 56197 [details])
> Thanks for this patch, Dan!  Overall this looks pretty good, but several minor comments below.
> 
> > Index: WebKitTools/ChangeLog
> > +        Adds function VCSUtils::parseSvnProperty to parse an SVN property with
> > +        either a single-line or multi-line value.
> 
> I would add the word "change" at the end because the property can have two
> "value" sections if there is both a "-" and "+".  So, "...with either a
> single-line or multi-line value change."

Will add.

> 
> > +
> > +        * Scripts/VCSUtils.pm:
> > +          - Added function parseSvnProperty. We will use this function
> > +          towards resolving Bug #38885 <https://bugs.webkit.org/show_bug.cgi?id=38885>.
> 
> alignment
> 
> > +          - Removed FIXME comment above function parseSvnPropertyValue, since
> > +            it is being used by parseSvnProperty.
> > +          - (*) Modified function parseSvnPropertyValue to break out of "while (<$fileHandle>)"
> > +            loop when it encounters the start of the next property so that it can be
> > +            processed by its caller, parseSvnPropertyValue.
> 
> Instead of saying "(*)", I would say include an additional sentence saying,
> "We reference this bullet below by (*)."  I was confused by what the "(*)"
> meant until I read to the end of the entry.

Will change. 

> > +        * Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl: Added.
> > +          - Added unit tests.
> > +        * Scripts/webkitperl/VCSUtils_unittest/parseSvnPropertyValue.pl:
> > +          - Changed the name of the unit test "simple multi-line '-' change" to
> > +            "single-line '-' change followed by empty line" since the former was an
> > +            incorrect description of this test.
> > +          - Added unit test "single-line '-' change followed by the next property", and
> > +            "multi-line '-' change followed by the next property" to test (*).
> 
> I would insert the word "above" at the end as in "to test (*) above".

Will change.

> 
> > Index: WebKitTools/Scripts/VCSUtils.pm
> > +my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $1 is the name of the property.
> 
> Should the comment say $2?

Good catch. Will change.

> 
> > +# Parse the next SVN property from the given file handle, and advance the handle so the last
> > +# line read is the first line after the property.
> > +#
> > +# This subroutine dies if the first line is an invalid SVN property
> > +# (i.e. a line that does not match $svnPropertyStartRegEx) or
> > +# the value of the property is missing.
> 
> I would rewrite this as "...if the first line is not a valid start of an
> SVN property (e.g. if the value of the property is missing)."

I am unclear if you are implying we should lump together, for documentation purposes, the cases where we die when the first line is invalid and also when the property value is missing?

Without loss of generality, this function dies if the the first line is not of the form "Added: svn:executable". Following from this, suppose the input to this function is:

Added: svn:executable

Index: Makefile.shared
==========================================================
[...]

This is a malformed patch since it is missing a property value. So, I chose to die if there is no property value. Hence, we also die if the value for the property is missing. Maybe we should assume this is just improbable and/or simply return a property hash whose value and propertyChangeDelta keys are undefined? 

Note, I inadvertently left out that we also die if there is disagreement between the $propertyNamePrefix (e.g. "Added" , "Deleted")  and the value prefix (e.g. '+', '-'), such as:


Added: svn:executable
   - *

> 
> 
> > +# Returns ($propertyHashRef, $lastReadLine):
> > +#   $propertyHashRef: a hash reference representing a SVN property.
> > +#     name: the name of the property.
> > +#     value: the last-most value of the property. 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).
> 
> I would be consistent with your uses above and use two spaces after each period.
> Also, instead of "last-most", I would just say, "the last property value."

Will change both.

> 
> 
> > +#     propertyChangeDelta: the value 1 or -1 if the property was added or
> > +#                         removed, respectively.
> 
> alignment (from before)

Will fix alignment of second line of comment.

> 
> > +#   $lastReadLine: the line last read from $fileHandle.
> > +#
> > +# FIXME: This method is unused as of (05/16/2010). We will call this function
> > +#        as part of parsing a SVN footer. See <https://bugs.webkit.org/show_bug.cgi?id=38885>.
> 
> Two spaces after each period.  Also do this below (I will stop mentioning it.)

Will change.

> 
> > +sub parseSvnProperty($$)
> > +{
> > +    my ($fileHandle, $line) = @_;
> > +
> > +    $_ = $line;
> > +
> > +    my $propertyName;
> > +    my $propertyNamePrefix;
> 
> Maybe $propertyChangeType instead of prefix?  propertyNamePrefix makes it
> seem like it's the prefix of the name -- e.g. the "svn" in "svn:executable".
> 

I agree. Will change.

> 
> 
> > +    if (/$svnPropertyStartRegEx/) {
> > +        $propertyNamePrefix = $1;
> > +        $propertyName = $2;
> > +    } else {
> > +        die("Failed to find SVN property: \"$_\".");
> > +    }
> > +
> > +    $_ = <$fileHandle>; # Not defined if end-of-file reached.
> > +
> > +    my $propertyValue;
> > +    my $propertyValuePrefix;
> 
> Again, for similar reasons, maybe propertyValueType?
> 
> > +    while (defined($_)) {
> > +        while (defined($_) && /$svnPropertyValueStartRegEx/) {
> > +            # Note, a '-' property may be followed by '+' property in the case of a "Modified"
> > +            # or "Name" property. We only care about the actual change (i.e. the '+' property)
> 
> Maybe "ending value" instead of "actual change" would be clearer?

Will change.

> 
> > +            # in such circumstances. So, we take the property value for the property to be its
> > +            # last parsed property value.
> > +            # FIXME: We may want to consider strictly enforcing a '-', '+' property ordering or
> > +            # add error checking to prevent '+', '+', ..., '+' and other invalid combinations.
> > +            $propertyValuePrefix = $1;
> > +            ($propertyValue, $_) = parseSvnPropertyValue($fileHandle, $_);
> 
> Does this while loop handle the case of empty lines between property values?
> If we don't need to support that for some reason, it might be worth
> mentioning commenting why.

The "svn diff" command does not insert newlines between property values or successive properties. So, these new lines would be part of the property value. This patch does not support property values that end with trailing newlines as it does not have support to disambiguate between these new lines and the new line that precedes the contents of a binary patch. I would suggest adding such support in a future patch. I'll add a FIXME to clarify this.

> 
> > +        }
> > +
> > +        # We skip over everything that isn't an empty line or $svnPropertyStartRegEx. There is an
> > +        # empty line before the contents of a binary patch.
> 
> I'm confused.  Once you have the last property value, why do you need to
> keep looping through the while loop?  It seems like you can just exit
> since you have the last value.  This also makes it seem like you only
> need one while loop and not two nested loops.

We don't need the outer while loop, will remove.

> 
> > +        last if (!defined($_) || /^[\r\n]*\z/ || /$svnPropertyStartRegEx/);
> > +        $_ = <$fileHandle>; # Not defined if end-of-file reached.
> > +    }
> > +
> > +    if (!$propertyValue) {
> > +        die("Failed to find the property value for the SVN property \"$propertyName\": \"$_\".");
> > +    }
> > +
> > +    my $propertyChangeDelta;
> > +    if ($propertyValuePrefix eq '+') {
> > +        $propertyChangeDelta = 1;
> > +    } elsif ($propertyValuePrefix eq '-') {
> > +        $propertyChangeDelta = -1;
> > +    } else {
> > +        # Not reached.
> > +        die;
> > +    }
> > +
> > +    my $expectedChangeDelta;
> 
> I would add a comment here that you are doing additional validation, 
> if that is what you're doing.

Will add comment.

> 
> > +    if ($propertyNamePrefix eq "Name") {
> > +        $expectedChangeDelta = 0;
> > +    } elsif ($propertyNamePrefix eq "Modified") {
> > +        $expectedChangeDelta = 0;
> > +    } elsif ($propertyNamePrefix eq "Added") {
> > +        $expectedChangeDelta = 1;
> > +    } elsif ($propertyNamePrefix eq "Deleted") {
> > +        $expectedChangeDelta = -1;
> > +    } else {
> > +        # Not reached.
> > +        die;
> > +    }
> > +
> > +    if ($propertyChangeDelta == -1 * $expectedChangeDelta) {
> 
> Why aren't you doing "if ($propertyChangeDelta != $expectedChangeDelta)"?

Take $propertyChangeDelta := 1, and $expectedChangeDelta := 0. Then we would die with a mismatch error.

Notice, $expectedChangeDelta is zero when the property type is "Name" or "Modified" since we cannot infer whether this is an addition or a removal of a property. Instead, we must inspect the +/- line(s) that follow this line in the diff.

Alternatively, we could write this as "if ($expectedChangeDelta && $propertyChangeDelta != $expectedChangeDelta)".

> 
> > +        die("Property change prefix \"$propertyValuePrefix\" does not match " .
> > +            "the property name prefix \"$propertyNamePrefix\".");
> 
> I don't think "match" is the right word since they're not really supposed
> to be the same.  Maybe "The final property value type found \"$propertyValueType\"
> does not correspond to the property change type found \"$propertyChangeType\"."?
> 
> How hard would it be to include the correct property value type in the message?

As aforementioned above, it is not always possible to infer what is the correct property value type.

> 
> > -#   $line: the line last read from $fileHandle
> > +#   $line: the line last read from $fileHandle.
> 
> Thanks.
> 
> > -        if (/^$/ || /$svnPropertyValueStartRegEx/) {
> > +        if (/^$/ || /$svnPropertyValueStartRegEx/ || /$svnPropertyStartRegEx/) {
> >              # Note, we may encounter an empty line before the contents of a binary patch.
> >              # Also, we check for $svnPropertyValueStartRegEx because a '-' property may be
> >              # followed by a '+' property in the case of a "Modified" or "Name" property.
> > +            # We check for $svnPropertyStartRegEx because it indicates the start of the
> > +            # next property to parse.
> 
> Does this mean that there is not always an empty line separating one
> property change from another?

Right. As aforementioned above, the "svn diff" command does not insert newlines between property values. 

> 
> > Index: WebKitTools/Scripts/webkitperl/VCSUtils_unittest/parseSvnProperty.pl
> 
> Thanks for your unit tests.  It seems like you should be putting lines
> after the property changes in each of your test cases to test the
> expectedNextLine and last read line in each case.
> 
> > +{
> > +    # New test
> > +    diffName => "simple: add svn:executable",
> > +    inputText => <<'END',
> > +Added: svn:executable
> > +   + *
> 
> For example, include a blank line and a line marking the beginning of the
> next diff here.  In most use cases, you will have stuff after the property
> value.  This will also let you test your handling of empty lines, etc.
> in relation to the while loops above.

Will add a case for the simple scenario. This is already covered for multi-lines in tests:
""multi-line '+' change and start of binary patch", and "multi-line '-' change followed by multi-line '+' change followed by start of binary patch".

> 
> > +{
> > +    # New test
> > +    diffName => "multi-line '+' change, followed by svn:executable",
> > +    inputText => <<'END',
> > +Name: documentation
> > +   + A
> > +long sentence that spans
> > +multiple lines.
> > +Name: svn:executable
> > +   + *
> > +END
> 
> I thought there was also an empty line between successive properties,
> or did you recently find that that is not the case?

As aforementioned, the "svn diff" command does not insert newlines between successive properties. The property value may contain trailing newlines. This patch does not support properties whose values end in trailing new lines (mentioned above).

[Daniel Bates]

(In reply to comment #3)
> (In reply to comment #2)
> > (From update of attachment 56197 [details] [details])
> 
> This is a malformed patch since it is missing a property value. So, I chose to die if there is no property
> value. Hence, we also die if the value for the property is missing. Maybe we should assume this is just
> improbable and/or simply return a property hash whose value and propertyChangeDelta keys are
> undefined? 

This should read:

This is a malformed patch since it is missing a property value. So, I chose to die if there is no property value. Maybe we should assume this is just improbable and/or simply return a property hash whose value and propertyChangeDelta keys are undefined?

[Daniel Bates]

Created attachment 56204 [details]
Patch with unit tests

Updated patch based on Chris's suggestions.

Added test cases:
"add svn:executable, followed by empty line"
"add svn:executable, followed by empty line and start of binary patch"
"single-line '-' change with trailing new line, and single-line '+' change"
"multi-line '-' change with trailing new line, and multi-line '+' change"
"single-line '+' with trailing new line, followed by empty line and start of binary patch"
"single-line '+' change followed by custom property with single-line '+' change"

[Daniel Bates]

Created attachment 56205 [details]
Patch with unit tests

Fix alignment issue in change log.

[Daniel Bates]

Created attachment 56212 [details]
Patch with unit tests

Spoke with Chris tonight, and we decided to change the line "if ($propertyChangeDelta == -1 * $expectedChangeDelta)" to  "if ($expectedChangeDelta && $propertyChangeDelta != $expectedChangeDelta)" to make it clearer to a reader that we should only die if the property change type (e.g. "Added") differs from the property value type (e.g. "+"), for "Added" and "Deleted" change types only.

[Chris Jerdonek]

(In reply to comment #3)
> > I would rewrite this as "...if the first line is not a valid start of an
> > SVN property (e.g. if the value of the property is missing)."
> 
> I am unclear if you are implying we should lump together, for documentation purposes, the cases where we die when the first line is invalid and also when the property value is missing?

No, I didn't mean to lump them together.  You can ignore the comment.  I just didn't understand from what you wrote that you were also checking the validity of the later lines.

> The "svn diff" command does not insert newlines between property values or successive properties. So, these new lines would be part of the property value. This patch does not support property values that end with trailing newlines as it does not have support to disambiguate between these new lines and the new line that precedes the contents of a binary patch. I would suggest adding such support in a future patch. I'll add a FIXME to clarify this.

Okay.  Yes, it might help in case people leave trailing empty lines when setting the svn:ignore value.

> > > +    if ($propertyChangeDelta == -1 * $expectedChangeDelta) {
> > 
> > Why aren't you doing "if ($propertyChangeDelta != $expectedChangeDelta)"?
> 
> Take $propertyChangeDelta := 1, and $expectedChangeDelta := 0. Then we would die with a mismatch error.
> 
> Notice, $expectedChangeDelta is zero when the property type is "Name" or "Modified" since we cannot infer whether this is an addition or a removal of a property. Instead, we must inspect the +/- line(s) that follow this line in the diff.

Okay.  Then I would consider using undef instead of 0 when it cannot be inferred.

> Alternatively, we could write this as "if ($expectedChangeDelta && $propertyChangeDelta != $expectedChangeDelta)".

Yes, this makes it a bit clearer.  And then you can say defined($expectedChangeDelta) instead of $expectedChangeDelta if you switch to undef.

[Chris Jerdonek]

Comment on attachment 56212 [details]
Patch with unit tests

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?

[Daniel Bates]

Thanks Chris for the review.

(In reply to comment #9)
> (From update of attachment 56212 [details])
> 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.

Will change.

> 
> > +    # FIXME: We do not support property values that contain tailing new line characters
> 
> trailing, newline (instead of new line)

Will change.

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

Will change.

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

Will change.

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

Will change.

> 
> > +    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. :)

Will change. Note, Die() already returns the file and line number where it was called.

> 
> > +    # 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?

I will add some more test cases.

The included test coverage is sufficient since the code does not explicitly check for the start of the next diff or a binary patch. Instead, it checks for the presence of an empty line, which always follows the SVN footer.

[Chris Jerdonek]

(In reply to comment #10)
> > Did you do any tests for property value followed by start of next diff?
> 
> I will add some more test cases.
> 
> The included test coverage is sufficient since the code does not explicitly check for the start of the next diff or a binary patch. Instead, it checks for the presence of an empty line, which always follows the SVN footer.

Okay, thanks.  I wasn't sure because it seemed like you were giving special emphasis to test cases involving binary patches.

[Daniel Bates]

Committed r59593: <http://trac.webkit.org/changeset/59593>