[webkit-reviews] review granted: [Bug 38885] Add infrastructure to parse SVN property changes : [Attachment 56567] 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 38885: Add infrastructure to parse SVN property changes
https://bugs.webkit.org/show_bug.cgi?id=38885

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

------- Additional Comments from Chris Jerdonek <cjerdonek at webkit.org>
Thanks for the revised patch.  Some suggestions (mostly about comments) to
consider before landing.

> +# Parse the next SVN diff footer from the given file handle, and advance
> +# the handle so the last line read is the first line after the footer.
> +#
> +# This subroutine dies if given leading junk.

I would just say "Parse an SVN diff footer...."  I would also not use the
phrase "leading junk" here since it is usually used in the context of
leading junk at the beginning of a patch (e.g. the e-mail stuff that appears
at the beginning of git-format-patch) rather than in the middle.

It might be nice to add a comment here that for the special case of an
SVN binary diff, there can be binary contents after the footer (so
technically speaking, the "Property changes" section is not the full
footer).  Should we be calling this parseSvnPropertyChanges() since it's
not really always the footer?

> +    if (defined($_) && /^\Q$separator\E[\r\n]+$/) {

Do you need to be using \Q/\E here?  If so, I would add a comment why since
it's a bit more obscure.

> +    # FIXME: We should expand this to support other SVN properties.
> +    # Notice, we keep processing until we hit end-of-file or some

I would add a blank line after FIXME line so it's clearer that the second
and subsequent lines is not part of the FIXME.

Also, did you want to hint at possible implementation for other SVN
properties -- e.g. by returning a hash of property key-values that
represents all properties?  The line you wrote makes it seem like the 
FIXME is to make the function explicitly more aware of other properties.
In some sense, returning a hash makes the function less aware of the 
specific properties since the special handling of svn:executable will be
going in the calling code with the implementation we've discussed
(returning a hash).

> +	       # FIXME: Should the diff specify multiple values for
svn:executable
> +	       #	(hence be malformed) then we set the value to the last
> +	       #	svn:executable property. We may want to consider some
> +	       #	better error handling.

I probably wouldn't bother with this FIXME.  We shouldn't need to be validating

every aspect of a patch.  And "last wins" is a valid logic to follow.

> +####
> +# Simple test cases
> +##

This looks good.  I like how you start the text closer in. :)

> +{
> +    # New test
> +    diffName => "add svn:executable, followed by empty line and start of
next property diff",
> +    inputText => <<'END',
> +Property changes on: FileA
> +___________________________________________________________________
> +Added: svn:executable
> +   + *
> +
> +Property changes on: Makefile.shared
> +END

Can this case ever happen?  Is this what happens if a property change
is the only change to a file?

> +####
> +# Property value followed by empty line and start of binary patch
> +##

I would say "binary contents" as binary patch makes it seem like it's the
start of a new diff.

> +Added: documentation
> +   + A sentence.

This is not a complete sentence.  JK. :)