[Webkit-unassigned] [Bug 38885] Add infrastructure to parse SVN property changes

[bugzilla-daemon at webkit.org]

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





--- Comment #15 from Daniel Bates <dbates at webkit.org>  2010-05-21 21:27:50 PST ---
(In reply to comment #14)
> (From update of attachment 56567 [details])
> 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.

Will change.

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

I can add a comment about the binary diff. With regards to the name, what are your thoughts on parseSvnDiffPropertyChanges? This would follow our naming of parseDiff, parseDiffHeader, parseSvnDiffHeader, et cetera.

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

I'll remove the \Q and \E.

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

Will change.

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

Sure, I'll elaborate a bit further in the FIXME.

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

Will remove.

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

Yes, it can happen for the reason you stated. That is, a property change may be 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.

Will change.

> 
> > +Added: documentation
> > +   + A sentence.
> 
> This is not a complete sentence.  JK. :)

Will change.

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