[Webkit-unassigned] [Bug 39409] Make VCSUtils::parseDiff recognize an SVN footer
bugzilla-daemon at webkit.org
bugzilla-daemon at webkit.org
Mon May 31 14:00:41 PDT 2010
https://bugs.webkit.org/show_bug.cgi?id=39409
--- Comment #6 from Daniel Bates <dbates at webkit.org> 2010-05-31 14:00:41 PST ---
(In reply to comment #4)
> (From update of attachment 56846 [details])
> Some initial comments on the first part of the patch:
>
> > Index: WebKitTools/ChangeLog
> > + Make VCSUtils::parseDiff recognize an SVN footer
> > + https://bugs.webkit.org/show_bug.cgi?id=39409
>
> It seems like a better summary would be to say that you are adding executable-bit
> support to svn-apply and svn-unapply, since that's the end result people would
> care about more. SVN footer support is just a means to that end in this patch.
> By the way, since you stopped using the word "footer" in the code, you might
> want to change that terminology in the ChangeLog as well.
>
> > + Connect up the Git, and SVN executable bit support in the function parseDiff
>
> "Connect up Git and SVN executable-bit support in parseDiff() so that..."
> (no "the" and no comma).
>
> A suggestion: I usually append "()" to function names in my ChangeLog descriptions so
> I don't have to type the word "function."
Will change.
>
> > + - Modified function parseDiff to call parseSvnDiffProperties when
> > + it find the start of an SVN property change diff.
>
> finds
Will change.
>
> > Index: WebKitTools/Scripts/VCSUtils.pm
> > my $svnDiffStartRegEx = qr#^Index: ([^\r\n]+)#;
> > +my $svnDiffPropertiesStartRegEx = qr#Property changes on: ([^\r\n]+)#; # $1 is normally the same as the index path.
> > my $svnPropertyStartRegEx = qr#^(Modified|Name|Added|Deleted): ([^\r\n]+)#; # $2 is the name of the property.
> > my $svnPropertyValueStartRegEx = qr#^ (\+|-) ([^\r\n]+)#; # $2 is the start of the property's value (which may span multiple lines).
>
> It seems $svnPropertiesStartRegEx would be more consistent with the other
> regex variables here (even though the function it's actually used in is called
> parseSvnDiffProperties()). If you think the other regexes should be changed to
> begin with $svnDiff..., you might want to add a FIXME.
Will change.
>
> > +# executableBitDelta: the value 1 or -1 if the executable bit was added or
> > +# removed from the target file, respectively.
>
> By the way, are you maintaining in your code the convention that
> deleted executable files have delta = -1? I think I did that for the
> Git code. That will help for unapplying since adding the file back
> should have delta = +1.
SVN deleted file diffs don't include a property change entry. When you unapply a deleted file diff then we perform an svn revert, which restores the file mode.
>
> > my $headerHashRef; # Last header found, as returned by parseDiffHeader().
> > + my $propertyHashRef; # Last SVN properties diff found, as returned by parseSvnDiffProperties().
>
> Would $svnPropertiesHashRef be a better name?
Will change.
>
> > + if ($line =~ $svnDiffPropertiesStartRegEx) {
> > + if ($propertyHashRef) {
> > + # This is the start of the second consecutive property diff of
> > + # this while loop (i.e. we just processed a property diff and
> > + # just encountered the start of another property diff).
> > + last;
> > + }
> > + if ($headerHashRef && ($1 ne $headerHashRef->{indexPath})) {
>
> Could you perhaps read $1 much closer to the regex (possibly setting a variable)?
Will change.
>
> > + # This is the start of the second diff of this while loop.
> > + # In particular, this is the start of a property diff for
> > + # a file that only has property changes.
> > + last;
>
> It seems like it would be better to combine these two if blocks that both call "last".
Will change.
>
> > + }
> > + ($propertyHashRef, $line) = parseSvnDiffProperties($fileHandle, $line);
> > + next;
> > + }
> > if ($line !~ $headerStartRegEx) {
> > # Then we are in the body of the diff.
> > $svnText .= $line;
> > @@ -789,8 +809,9 @@ sub parseDiff($$)
> > next;
> > } # Otherwise, we found a diff header.
> >
> > - if ($headerHashRef) {
> > - # Then this is the second diff header of this while loop.
> > + if ($propertyHashRef || $headerHashRef) {
> > + # Then either we just processed an SVN property change or this
> > + # is the start of the second diff header of this while loop.
> > last;
> > }
>
> Something tells me the "if" logic in the while loop can be made simpler somehow...
> For example, if either property changes or a header can begin the diff, then
> why is parseDiffHeader() called at the end of the while loop, whereas
> parseSvnDiffProperties() is called towards the beginning of the loop.
> There's an asymmetry here. Can this be made simpler?
We can make this simpler. I would suggest we do this in a separate patch as this would requires changes to parseDiffHeader(). In particular, so that it does not die when it does not find a line that begins with "Index" or "diff --git".
>
> >
> > @@ -812,12 +833,30 @@ sub parseDiff($$)
> > $copyHash{copiedFromPath} = $headerHashRef->{copiedFromPath};
> > $copyHash{indexPath} = $headerHashRef->{indexPath};
> > $copyHash{sourceRevision} = $headerHashRef->{sourceRevision} if $headerHashRef->{sourceRevision};
> > + if ($headerHashRef->{isSvn}) {
> > + # SVN represents a copy of a file with an executable bit as two distinct diffs: the actual copy, and setting the executable bit.
> > + $copyHash{executableBitDelta} = $propertyHashRef->{executableBitDelta} if $propertyHashRef->{executableBitDelta};
> > + }
>
> If this is the case, then why would this not cause a problem for parseDiff()?
> If svn-create-patch creates two diffs for that case, then wouldn't this result
> in two calls to parseDiff() -- which would throw off your logic here?
> Is parseDiff() treating the "Property Changes" section as a separate diff
> or part of the same diff? We need to clarify what we are calling a "diff".
This comment was ambiguous, so I removed it.
I was trying to describe the composition of an SVN copy file diff. Our implementation considers a header for file A followed by property change for file A to be part of the same diff (i.e. pass one patch to patch()). Hence, we don't treat these as separate diffs.
>
> It might be worth putting a comment somewhere explaining the relationship/ordering
> between the header and property changes section (and possibly also contents and
> binary contents).
Will add.
--
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