[Webkit-unassigned] [Bug 39184] Add support for parsing a SVN property
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Sun May 16 15:32:03 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39184
Chris Jerdonek <cjerdonek at webkit.org> changed:
What |Removed |Added
----------------------------------------------------------------------------
Attachment #56197|review? |review-
Flag| |
--- Comment #2 from Chris Jerdonek <cjerdonek at webkit.org> 2010-05-16 15:32:02 PST ---
(From update of attachment 56197)
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?
--
Configure bugmail: https://bugs.webkit.org/userprefs.cgi?tab=email
------- You are receiving this mail because: -------
You are the assignee for the bug.
More information about the webkit-unassigned
mailing list