[webkit-reviews] review granted: [Bug 32919] svn-create-patch: fixChangeLogPatch does not work correctly in certain circumstances : [Attachment 45532] Proposed patch 2

[bugzilla-daemon at webkit.org]

David Kilzer (ddkilzer) <ddkilzer at webkit.org> has granted Chris Jerdonek
<chris.jerdonek at gmail.com>'s request for review:
Bug 32919: svn-create-patch: fixChangeLogPatch does not work correctly in
certain circumstances
https://bugs.webkit.org/show_bug.cgi?id=32919

Attachment 45532: Proposed patch 2
https://bugs.webkit.org/attachment.cgi?id=45532&action=review

------- Additional Comments from David Kilzer (ddkilzer) <ddkilzer at webkit.org>
Chris, thanks for writing test cases and tackling this code!

> Index: WebKitTools/Scripts/VCSUtils.pm
> +    my $i = 0; # We reuse the same index throughout.
> +
> +    # Skip to beginning of first chunk.
> +    for (; $i < @lines; ++$i) {
> +	   if (substr($lines[$i], 0, 1) eq "@") {
> +	       last;
> +	   }
> +    }
> +    my $chunkStart = ++$i;

I'd like to see this renamed to $chunkStartIndex.  Since Perl doesn't have
variable types, it's important to make sure the variable names are descriptive.


> +    # Skip to first line of newly added ChangeLog entry.
> +    # For example, +2009-06-03  Eric Seidel	<eric at webkit.org>
> +    my $dateStartRegEx = '^\+(\d{4}-\d{2}-\d{2})' # leading + and date
> +			    . '\s+(.+)\s+' # name
> +			    . '<([^<>]+)>$'; # e-mail address
> +
> +    for (; $i < @lines; ++$i) {
> +	   my $line = $lines[$i];
> +	   my $first = substr($line, 0, 1);

I'd like to see $first renamed to $firstChar or $firstCharacter.

> +	   if ($line =~ /$dateStartRegEx/) {
> +	       last;
> +	   } elsif ($first eq " " or $first eq "+") {
> +	       next;
> +	   }
> +	   return $patch; # Do not change if, for example, "-" or "@" found.
> +    }
> +    if ($i >= @lines) {
> +	   return $patch; # Do not change if date not found.
> +    }
> +    my $dateStart = $i;

Similar to $chunkStart, this should be renamed to $dateStartIndex.

> +    # Rewrite overlapping lines to lead with " ".
> +    my @overlappingNewLines = (); # These will include a leading +.

The words "NewLines" in @overlappingNewLines makes this variable name confusing
to me.	(I think of overlapping "\n" characters when I read it.)  Maybe just
@overlappingLine?

The "+" should be quoted in the comment.

> +    for (; $i < @lines; ++$i) {
> +	   my $line = $lines[$i];
> +	   if (substr($line, 0, 1) ne "+") {
> +	     last;
> +	   }
> +	   push(@overlappingNewLines, $line);
> +	   $lines[$i] = " " . substr($line, 1);
> +    }
> +
> +    # Remove excess ending context, if necessary.
> +    my $trimContext = 1;

Please rename $trimContext to $shouldTrimContext as it's used as a boolean
value.

> +    for (; $i < @lines; ++$i) {
> +	   my $first = substr($lines[$i], 0, 1);

Again, please change $first to $firstChar or $firstCharacter.

> +	   if ($first eq " ") {
> +	       next;
> +	   } elsif ($first eq "@") {
> +	       last;
> +	   }
> +	   $trimContext = 0; # For example, if "+" or "-" encountered.
> +	   last;
> +    }
> +    my $numLinesDeletedContext = 0;

How about $deletedLineCount instead of $numLinesDeletedContext?

> +    if ($trimContext) { # Also occurs if end of file reached.
> +	   splice(@lines, $i - @overlappingNewLines, @overlappingNewLines);
> +	   $numLinesDeletedContext = @overlappingNewLines;
> +    }
> +
> +    # Work backwards, shifting overlapping lines towards front
> +    # while checking that patch stays equivalent.
> +    for ($i = $dateStart - 1; $i >= $chunkStart; --$i) {
> +	   my $line = $lines[$i];
> +	   if (substr($line, 0, 1) ne " ") {
> +	       next;
> +	   }
> +	   my $text = substr($line, 1);
> +	   my $newLine = pop(@overlappingNewLines);

Again, $newLine makes me think of "\n" characters.  How about using
$overlappingLine instead?

> +	   if ($text ne substr($newLine, 1)) {
> +	       return $patch; # Unexpected difference.
> +	   }
> +	   $lines[$i] = "+$text";
> +    }
> +
> +    # Finish moving whatever overlapping lines remain, and update
> +    # the initial chunk range.
> +    my $chunkRangeRegEx = '^\@\@ -(\d+),(\d+) \+\d+,(\d+) \@\@$'; # e.g. @@
-2,6 +2,18 @@
> +    if ($lines[$chunkStart - 1] !~ /$chunkRangeRegEx/) {
> +	   # FIXME: Handle errors differently from ChangeLog files that
> +	   # are okay but should not be altered. That way we can find out
> +	   # if improvements to the script ever become necessary.
> +	   return $patch; # Error: unexpected patch string format.
> +    }
> +    my $numSkippedFirstLines = $1 - 1;
> +    my $oldNumSourceLines = $2;
> +    my $oldNumTargetLines = $3;

I prefer $fooLineCount over $numFooLines.

> +    if (@overlappingNewLines != $numSkippedFirstLines) {
> +	   # This can happen, for example, when deliberately inserting
> +	   # a new ChangeLog entry earlier in the file.
> +	   return $patch;
>      }
> +    # If @overlappingNewLines > 0, this is where we make use of the
> +    # assumption that the beginning of the source file was not modified.
> +    splice(@lines, $chunkStart, 0, @overlappingNewLines);
> +
> +    my $numSourceLines = $oldNumSourceLines + @overlappingNewLines
> +			    - $numLinesDeletedContext;
> +    my $numTargetLines = $oldNumTargetLines + @overlappingNewLines
> +			    - $numLinesDeletedContext;

No need to wrap these two lines.

Using $sourceLineCount and $targetLineCount is a little clearer, I think.

> +    $lines[$chunkStart - 1] = "@@ -1,$numSourceLines +1,$numTargetLines @@";

>  
> +    return join($lineEnding, @lines) . "\n"; # patch(1) expects an extra
trailing newline.
>  }

> Index: WebKitTools/Scripts/VCSUtils_unittest.pl
> +#!/usr/bin/perl
> +#
> +# Copyright (C) 2009 Apple Inc. All rights reserved.

You don't have to assign the copyright to Apple.  You can assign it to yourself
instead.

> +use VCSUtils;
> +
> +sub fixChangeLogPatch($);

Redeclaring this should not be necessary.  Just use VCSUtils::
fixChangeLogPatch() in the tests below.

> +# New test
> +$title = "fixChangeLogPatch: [no change] New entry inserted in middle.";
> +
> +$in = <<'END';
> +--- ChangeLog
> ++++ ChangeLog
> +@@ -11,6 +11,14 @@
> + 
> +	    Reviewed by Ray.
> + 
> ++	    Changed some more code on 2009-12-21.
> ++
> ++	    * File:
> ++
> ++2009-12-21	Alice  <alice at email.address>
> ++
> ++	    Reviewed by Ray.
> ++
> +	    Changed some code on 2009-12-21.
> + 
> +	    * File:
> +END
> +
> +ok(fixChangeLogPatch($in) eq $in, $title);

This test case should work.  In this situation, it's okay to assume the
previous ChangeLog entry started with the same date line.  All the context
lines are ignored anyway when the patch is applied--the important part is to
get the changed lines in order at the top of the patch.

> Index: WebKitTools/Scripts/run-webkit-unittests-perl
> +#!/usr/bin/perl
> +#
> +# Copyright (C) 2009 Apple Inc. All rights reserved.

Again, you can assign the copyright to yourself instead of Apple here.

r=me with the variable name changes.

I'd really like to see the one test case above fixed, but if it doesn't work
today, it's fine to land this patch first and open a new bug for this issue.