[webkit-reviews] review granted: [Bug 30683] svn-apply's fixChangeLogPatch function seems broken : [Attachment 41956] Update after bug 30791

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Eric Seidel
<eric at webkit.org>'s request for review:
Bug 30683: svn-apply's fixChangeLogPatch function seems broken
https://bugs.webkit.org/show_bug.cgi?id=30683

Attachment 41956: Update after bug 30791
https://bugs.webkit.org/attachment.cgi?id=41956&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
> diff --git a/WebKitTools/Scripts/VCSUtils.pm
b/WebKitTools/Scripts/VCSUtils.pm
> index e1e0bc2..ebf1190 100644
> --- a/WebKitTools/Scripts/VCSUtils.pm
> +++ b/WebKitTools/Scripts/VCSUtils.pm
> @@ -347,56 +347,64 @@ sub gitdiff2svndiff($)
>      return $_;
>  }
>  
> +# The diff(1) command is greedy when matching lines, so a new ChangeLog
entry will
> +# have lines of context at the top of a patch when the existing entry has
the same
> +# date and author as the new entry.	Alter the ChangeLog patch so
> +# that the added lines ("+") in the patch always start at the beginning of
the
> +# patch and there are no initial lines of context.
>  sub fixChangeLogPatch($)
>  {
> +    my $patch = shift; # $patch will only contain patch fragments for
ChangeLog.
> +
> +    $patch =~ /(\r?\n)/;
> +    my $lineEnding = $1;
> +    my @patchLines = split(/$lineEnding/, $patch);
> +
> +    # e.g. 2009-06-03  Eric Seidel  <eric at webkit.org>
> +    my $dateLineRegexpString = '^\+(\d{4}-\d{2}-\d{2})' # Consume the
leading '+' and the date.
> +				. '\s+(.+)\s+' # Consume the name.
> +				. '<([^<>]+)>$'; # And finally the email
address.
> +
> +    # Figure out where the patch contents start and stop.
> +    my $patchHeaderIndex;
> +    my $firstContentIndex;
> +    my $trailingContextIndex;
> +    my $dateIndex;
> +    my $patchEndIndex = scalar(@patchLines);
> +    for (my $index = 0; $index < @patchLines; ++$index) {
> +	   my $line = $patchLines[$index];
> +	   if ($line =~ /^\@\@ -\d+,\d+ \+\d+,\d+ \@\@$/) { # e.g. @@ -1,5
+1,18 @@
> +	       if ($patchHeaderIndex) {
> +		   $patchEndIndex = $index; # We only bother to fix up the
first patch fragment.
> +		   last;
>	       }
> +	       $patchHeaderIndex = $index;
>	   }
> +	   $firstContentIndex = $index if ($patchHeaderIndex &&
!$firstContentIndex && $line =~ /^\+[^+]/); # Only match after finding
patchHeaderIndex, otherwise we'd match "+++".
> +	   $dateIndex = $index if ($line =~ /$dateLineRegexpString/);
> +	   $trailingContextIndex = $index if ($firstContentIndex &&
!$trailingContextIndex && $line =~ /^ /);
>      }
> +    my $contentLineCount = $trailingContextIndex - $firstContentIndex;
> +    my $trailingContextLineCount = $patchEndIndex - $trailingContextIndex;
> +
> +    # If we didn't find a date line in the content then this is not a patch
we should try and fix.
> +    return $patch if (!$dateIndex);
> +
> +    # We only need to do anything if the date line is not the first content
line.
> +    return $patch if ($dateIndex == $firstContentIndex);
> +
> +    # Write the new patch.
> +    my $totalNewContentLines = $contentLineCount +
$trailingContextLineCount;
> +    $patchLines[$patchHeaderIndex] = "@@ -1,$trailingContextLineCount
+1,$totalNewContentLines @@"; # Write a new header.
> +    my @repeatedLines = splice(@patchLines, $dateIndex,
$trailingContextIndex - $dateIndex); # The date line and all the content after
it that diff saw as repeated.
> +    splice(@patchLines, $firstContentIndex, 0, @repeatedLines); # Move the
repeated content to the top.
> +    foreach my $line (@repeatedLines) {
> +	   $line =~ s/^\+/ /;
> +    }
> +    splice(@patchLines, $trailingContextIndex, $patchEndIndex,
@repeatedLines); # Replace trailing context with the repeated content.
> +    splice(@patchLines, $patchHeaderIndex + 1, $firstContentIndex -
$patchHeaderIndex - 1); # Remove any leading context.
>  
> +    return join($lineEnding, @patchLines) . "\n"; # patch(1) expects an
extra trailing newline.
>  }

After thinking about this some more, there's an even easier way to mangle the
patch.	Basically, assuming the patch is 0-or-more unchanged lines, 1-or-more
added lines (+), and then 1-or-more unchanged lines, all you really have to do
is separate out the added lines from the unchanged lines, reorder the added
lines starting with the date line and wrapping around to the top, then output
the reordered added lines plus the first three lines of unchanged lines.  (This
works because the added lines always contain the complete patch, but because of
the vagaries of diff, they are sometimes out of order.)

r=me to unblock other issues.  We can revisit this later if needed.