[Webkit-unassigned] [Bug 39184] Add support for parsing a SVN property

[bugzilla-daemon at webkit.org]

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





--- Comment #10 from Daniel Bates <dbates at webkit.org>  2010-05-16 22:54:14 PST ---
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.

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