[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 #10 from Daniel Bates <dbates at webkit.org>  2010-05-11 15:16:01 PST ---
(In reply to comment #8)
> (In reply to comment #6)
> > Created an attachment (id=55669)
 --> (https://bugs.webkit.org/attachment.cgi?id=55669) [details] [details]
> 
> Some comments on the first portion of the patch:
> 
> > Index: WebKitTools/ChangeLog
> > +        * Scripts/VCSUtils.pm:
> 
> In the ChangeLog, it is generally good to comment on the per-file changes
> for each file.  For example, can you at least include the names of the new
> subroutines added to VCSUtils.pm?

Yes, I agree. Will change.

> 
> > Index: WebKitTools/Scripts/VCSUtils.pm
> >  my $gitDiffStartRegEx = qr#^diff --git (\w/)?(.+) (\w/)?([^\r\n]+)#;
> >  my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> > +my $svnFooterStartRegEx = qr#^Property changes on: ([^\r\n]+)#;
> > +my $svnPropertyNameRegEx = qr#^(Name|Added|Deleted): ([^\r\n]+)#;
> 
> It might be good to comment these two variables.  For example, it might be
> nice to say here what $1 is (will normally be the same as the index path).

Will add comment.

> For the second, it might be good to state what $1 is and also clarify that the
> reg ex marks the beginning of a change to a particular property instead of
> to a section of additions (for example).

Will add comment.

> 
> For the second, might $svnPropertyChangeStartRegEx or $svnPropertyStartRegEx
> be clearer since the line marks the beginning of a property change in the
> "Property changes" section and when parseSvnProperty() should be called?

Yeah, I was not sure what to call this. I'll go with $svnPropertyChangeStartRegEx.

> 
> > +# Args:
> > +#   $fileHandle: advanced so the last line read from the handle is either the
> > +#                the first occurence of an empty line or the first line of the
> 
> occurrence
> 
> Since this is in the args section, the $fileHandle comment should be explaining
> the state of the file handle going into the subroutine.  Can you clarify the scoping
> in the phrase "first occurrence of an empty line"?  This could be taken to mean
> the first empty line in the diff, which is obviously not right.  Also, do we really
> want to be saying that an empty line is what marks the beginning of the footer?
> It seems like we can always require it to be the "Property changes" line.

Oops, this used to describe the state of the fileHandle after the subroutine returns (in the old patch: <https://bugs.webkit.org/attachment.cgi?id=55655>). The empty line comment no longer applies to <https://bugs.webkit.org/attachment.cgi?id=55669>. Will update comment. 

> 
> > +#                next header to parse.  For SVN-formatted diffs, this is a line
> > +#                beginning with "Index:".  For Git, this is a line beginning
> > +#                with "diff --git".
> 
> It looks like you need to rewrite this in terms of footer language.

See above response. Will update comment.

> 
> > +#   $line: the line last read from $fileHandle
> 
> Missing a period.

Will fix.

> 
> > +# Returns ($footerHashRef, $lastReadLine):
> > +#   $footerHashRef: a hash reference representing a diff footer
> > +#     propertyPath: the path of the target file.
> 
> It might be good to add that this should normally be the same as the
> "index path" for the diff.

Will update comment to reflect this.

> 
> > +sub parseSvnDiffFooter($$)
> 
> > +        if (/$svnFooterStartRegEx/) {
> > +            $propertyPath = $1;
> 
> Would something like "filePath" be clearer?  propertyPath makes it seem like
> it is a path to a property.
> 
> > +        } elsif (/$svnPropertyNameRegEx/) {
> > +            ($propertyHashRef, $_) = parseSvnProperty($fileHandle, $_);
> > +            $footer{executableBitDelta} = $propertyHashRef->{propertyChangeDelta} if $propertyHashRef->{name} eq "svn:executable";
> 
> Since it doesn't look like you're storing any property changes other than
> the executable bit one, it might be good to add a FIXME somewhere (like in
> or near the description of the subroutine) to add support for parsing
> all properties.

Will do.

> 
> When this method is modified to support all properties, it seems like it
> would make sense to store all the properties as a generic hash of key-value
> pairs, so that this subroutine will not need to have any special knowledge of
> the property representing the executable bit.  Then the caller could be
> responsible for examining the hash and converting that information into an
> executableBitDelta for the diff.  Maybe you can include a FIXME here
> suggesting at future changes to this subroutine.

Yes, this was my original thought as well. Will add a FIXME for now.

> 
> > +        } elsif (/^\s*$/x || $_ eq '_' x 67) {
> > +            # Skip empty/whitespace-only lines and the divider line.
> > +        } else {
> > +            # Some other content, such as the binary portion of an SVN binary diff.
> > +            last;
> 
> Since the binary lines are getting skipped over, how are they getting preserved
> so the binary changes can later be applied?

The binary lines should not be skipped over. Disregarding the case where the caller of parseSvnFooterDiff does not pass the start of an SVN property change entry (i.e. starts with "Property changes on ..."), by the last "else" clause we break out of the loop and will return this line so that the caller (say, parseDiff() can continue processing the diff). Therefore, it will be the caller's responsibility to to preserve the binary patch contents. See test cases in parseSvnDiffFooter.pl: "binary file (diff snippet) with executable bit set", and  "binary file (diff snippet) with executable bit set (with empty line)".

> 
> > +        }
> > +
> > +        $_ = <$fileHandle>; # Not defined if end-of-file reached.
> > +
> > +        last if (!defined($_) || /$svnDiffStartRegEx/ || /$gitDiffStartRegEx/);
> 
> Are we supporting patches with Git and SVN diffs mixed together?  It doesn't
> seem like this SVN-specific method needs to know about Git diffs.

I agree, we shouldn't have mixing. Will change.

> 
> > +    }
> > +
> > +    if (!$propertyPath) {
> > +        die("First line of SVN diff does not begin with \"Property changes on \": \"$_\".");
> 
> It seems like the logic above doesn't require the first line to be the
> "Property changes" line.  It seems like it only requires that some line
> be a "Property changes" line.  Is there any reason not to check the first
> line at the beginning of the method?

In the process of refactoring this, I wound up moving it into the loop. I believe we can factor this out of the loop and do the testing at the beginning of the method.

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