[Webkit-unassigned] [Bug 39409] Make VCSUtils::parseDiff recognize an SVN footer

[bugzilla-daemon at webkit.org]

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





--- Comment #4 from Chris Jerdonek <cjerdonek at webkit.org>  2010-05-25 06:01:12 PST ---
(From update of attachment 56846)
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."

> +          - Modified function parseDiff to call parseSvnDiffProperties when
> +            it find the start of an SVN property change diff.

finds

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

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

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

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

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

> +            }
> +            ($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?

>  
> @@ -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".

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

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