Bug 32919

Created attachment 45524 [details] Proposed patch As a further check of the proposed revision, I ran the current fixChangeLogPatch against the 7 unit test cases in the patch. The current version did not pass the last 5 for various reasons. I inspected the output of each as a comparison.

style-queue ran check-webkit-style on attachment 45524 [details] without any errors.

Comment on attachment 45524 [details] Proposed patch Thanks for this patch! Especially, adding a unittest for VCSUtils.pm is great, I think. I'd like to put a few nitpicks. I'm not good at Perl and I don't think they are very important. Please feel free to ignore some of my comments if they don's sound good to you. > + $lines[$chunkStart - 1] =~ /^\@\@ -(\d+),(\d+) \+\d+,(\d+) \@\@/; # e.g. @@ -2,6 +2,18 @@ We may want to return $patch if this regexp doesn't match? > + # 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); > + # Update chunk range. > + my $numOriginalLines = $2 - $numLinesDeletedContext + @overlappingNewLines; > + my $newChunkLength = $3 - $numLinesDeletedContext + @overlappingNewLines; I slightly prefer to put these two lines before the splice call because I think the use of $2 and $3 should be near to the corresponding regexp as possible. > +# New test > +$text = "fixChangeLogPatch: [no change] First line is new line."; $description or something like this would be slightly better?

Created attachment 45532 [details] Proposed patch 2 Incorporated Shinichiro's suggestions.

style-queue ran check-webkit-style on attachment 45532 [details] without any errors.

I think this is more perl than my little brain can handle. Thank you for adding the unit tests. I should have done so originally. I instinct was to avoid adding more perl code, but I should have made an exception for unit tests and added them anyway. If we eventually convert this to another language, the unit tests will still be useful. :)

(In reply to comment #6) > instinct was to avoid adding more perl code, > ... > If we eventually convert this > to another language, the unit tests will still be useful. :) Thanks, Eric. A few questions about this since this is new to me. Has WebKit made a conscious decision to move away from Perl? If so, can you give a sense of the current status and plan of conversion to Python -- even if it's just your perspective. :) Lastly, out of curiosity, what else would have to be rewritten before this function would be useful in another language, or would it be useful now? In terms of converting, I think the learned logic might be useful, too. I was originally aiming for something elegant, but after coming across some of the test cases, there are certain things that just can't be gotten around (without making rigid assumptions about the form of the input patch string).

(In reply to comment #7) > Thanks, Eric. A few questions about this since this is new to me. Has WebKit > made a conscious decision to move away from Perl? No, not that I'm aware of. The shift to using Python for tools has been more about the people writing the tools than any conscious decision (IMO). Python is an approved language at Google, and more Google and/or Chromium developers have been writing/contributing new tools recently. > If so, can you give a sense > of the current status and plan of conversion to Python -- even if it's just > your perspective. :) This question is probably best discussed on <webkit-dev@lists.webkit.org> rather than in this bug. I'll try to review the patch tomorrow. Thanks!

For what it's worth, I agree with David Kilzer completely. As far as I know there has never been an "official" script language for WebKit. Just the personal preferences of whoever happens to be working on scripts at the time. My current personal preference is Python, but I speak (and incidentally according to ohloh have touched 18k lines of WebKit's) perl.

The real perl experts in WebKit are Darin Adler, David Kilzer, Adam Roben and Mark Rowe. Those are your most likely reviewers for this patch.

Removed brackets from title to avoid confusion with patches needing specialized review.

A couple quick things: Could someone explain what the original reasons were to have new ChangeLog entries at the top of patch files? Is it just to improve human readability, or was there a more technical reason? I'd like to add this information to the comments at the beginning of fixChangeLogPatch. Also, I'm going to wait to hear back from webkit-dev before changing the name of run-webkit-unittests-perl to test-webkit-perl.

(In reply to comment #12) > Could someone explain what the original reasons were to have new ChangeLog > entries at the top of patch files? Is it just to improve human readability, or > was there a more technical reason? I'd like to add this information to the > comments at the beginning of fixChangeLogPatch. The ChangeLog entry is supposed to explain the changes that follow it, hence the reason for the ChangeLog patch being listed before other patches within the same section (JavaScriptCore, WebCore, WebKit/mac, etc.). > Also, I'm going to wait to hear back from webkit-dev before changing the name > of run-webkit-unittests-perl to test-webkit-perl. This can be landed and then renamed if necessary. Sorry I didn't get the patch reviewed today. Will try again tomorrow.

(In reply to comment #13) > Will try again tomorrow. Thanks, David. > (In reply to comment #12) > > Could someone explain what the original reasons were to have new ChangeLog > > entries at the top of patch files? > The ChangeLog entry is supposed to explain the changes that follow it, hence > the reason for the ChangeLog patch being listed before other patches within the > same section (JavaScriptCore, WebCore, WebKit/mac, etc.). I meant at the top of the portion of the patch corresponding to the diff of the ChangeLog files, so there is no leading context in that portion (i.e. what fixChangeLogPatch is meant to fix).

(In reply to comment #14) > > (In reply to comment #12) > > > Could someone explain what the original reasons were to have new ChangeLog > > > entries at the top of patch files? > > > > The ChangeLog entry is supposed to explain the changes that follow it, hence > > the reason for the ChangeLog patch being listed before other patches within the > > same section (JavaScriptCore, WebCore, WebKit/mac, etc.). > > I meant at the top of the portion of the patch corresponding to the diff of the > ChangeLog files, so there is no leading context in that portion (i.e. what > fixChangeLogPatch is meant to fix). Each new ChangeLog entry should appear at the top of the ChangeLog file since the file is meant to be a local, in-order description of commits to the project. (If you're thinking it's redundant to 'svn log' or that git already provides local, offline history, it's all been discussed before on webkit-dev. See the list archive at <http://lists.webkit.org/>.) Fixing the patch so that it always applies to the top of the ChangeLog file makes it trivial to always apply in the correct location with the --fuzz=3 command-line switch for the patch command, which is what the svn-apply script does.

Comment on attachment 45532 [details] Proposed patch 2 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@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@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.

(In reply to comment #16) > (From update of attachment 45532 [details]) > > + # 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? Err...make that @overlappingLines.

(In reply to comment #16) > (From update of attachment 45532 [details]) Great suggestions, Dave. Thanks. I will also remember these for the future. > > +# 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@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. I believe the situation you have in mind is already covered by a later test case, where the input does get changed: > # New test > $title = "fixChangeLogPatch: Leading context does not include first line."; > > $in = <<'END'; > @@ -2,6 +2,14 @@ > > Reviewed by Ray. > > + Changed some more code on 2009-12-22. > + > + * File: > + > +2009-12-22 Alice <alice@email.address> > + > + Reviewed by Ray. > + > Changed some code on 2009-12-22. > > * File: > END The only difference between these two cases is in the location in the chunk range description (line 11 vs. line 2): > > +@@ -11,6 +11,14 @@ and > @@ -2,6 +2,14 @@ The code uses the chunk range to check that the number of lines to insert at the beginning matches the number of lines skipped in the chunk range description. (In the latter case this matches since we're inserting one line -- the date line -- and the chunk range starts at line 2.) Leaving cases like the former unchanged is useful if we ever need to correct a ChangeLog file by inserting an entry into the middle that may have been mistakenly left out. This special case will never come up when using prepare-ChangeLog and svn-create-patch though. I will submit a revised patch with your suggestions shortly since I don't have commit access yet. Thanks!

Created attachment 45641 [details] Proposed patch 3 In addition to David's suggested changes, I also did the following: - Added my name to the top of VCSUtils.pm (I assume this is okay since the new code is about 20% of the file). - Added to the comments preceding fixChangeLogPatch why the function is needed (from the information that David provided in an earlier comment in response to my question). - Changed the name of the new file from run-webkit-unittests-perl to test-webkit-perl (as mentioned in my e-mail to webkit-dev: https://lists.webkit.org/pipermail/webkit-dev/2009-December/011104.html ). Also, I left out the VCSUtils:: prefix from the calls to fixChangeLogPatch in VCSUtils_unittest.pl since the unit tests run okay without it. Let me know if I should have included that for reasons of good practice.

style-queue ran check-webkit-style on attachment 45641 [details] without any errors.

Created attachment 45642 [details] Proposed patch 4 Overlooked changing $numSkippedFirstLines to $skippedFirstLineCount. Also updated the test-webkit-perl file name in ChangeLog.

style-queue ran check-webkit-style on attachment 45642 [details] without any errors.

Comment on attachment 45642 [details] Proposed patch 4 Clearing flags on attachment: 45642 Committed r52646: <http://trac.webkit.org/changeset/52646>

All reviewed patches have been landed. Closing bug.

(In reply to comment #23) > (From update of attachment 45642 [details]) > Clearing flags on attachment: 45642 > > Committed r52646: <http://trac.webkit.org/changeset/52646> It looks like the "execute" mode didn't get set on the following two files-- - Scripts/VCSUtils_unittest.pl - Scripts/test-webkit-perl Could someone correct this? It looks like this is a known issue with svn-apply: https://bugs.webkit.org/show_bug.cgi?id=27204#c4 Thanks.

Comment on attachment 45642 [details] Proposed patch 4 Rejecting patch 45642 from commit-queue. Failed to run "['/Users/eseidel/Projects/CommitQueueSVN/WebKitTools/Scripts/svn-apply', '--reviewer', 'David Kilzer', '--force']" exit_code: 1 Last 500 characters of output: geLog Hunk #1 succeeded at 1 with fuzz 3. patching file WebKitTools/Scripts/VCSUtils.pm Hunk #1 FAILED at 1. Hunk #2 FAILED at 357. 2 out of 2 hunks FAILED -- saving rejects to file WebKitTools/Scripts/VCSUtils.pm.rej patching file WebKitTools/Scripts/VCSUtils_unittest.pl svn: warning: 'WebKitTools/Scripts/VCSUtils_unittest.pl' is already under version control patching file WebKitTools/Scripts/test-webkit-perl svn: warning: 'WebKitTools/Scripts/test-webkit-perl' is already under version control Full output: http://webkit-commit-queue.appspot.com/results/152183

(In reply to comment #25) > (In reply to comment #23) > > (From update of attachment 45642 [details] [details]) > > Clearing flags on attachment: 45642 > > > > Committed r52646: <http://trac.webkit.org/changeset/52646> > > It looks like the "execute" mode didn't get set on the following two files-- > > - Scripts/VCSUtils_unittest.pl > - Scripts/test-webkit-perl > > Could someone correct this? Fixed in r52658: <http://trac.webkit.org/changeset/52658>

I think this failure might be another variant of bug 28603. I'm tempted to switch the commit-queue back to git until bug 28603 can be resolved. It affects SVN much worse than Git.

(In reply to comment #28) > I think this failure might be another variant of bug 28603. I'm tempted to > switch the commit-queue back to git until bug 28603 can be resolved. It > affects SVN much worse than Git. Just to be clear, I think the failure in comment 26 might be legitimate. It looks like Dave may have set cq+after the change was already committed. My e-mail shows the second cq+ happening at 11:22 PM which is after the change was committed at 11:19 PM (comment 24). Or is there something about how the failure was handled that isn't right?

(In reply to comment #28) > I think this failure might be another variant of bug 28603. I'm tempted to > switch the commit-queue back to git until bug 28603 can be resolved. It > affects SVN much worse than Git. No, this was an unfortunate sequence of events. The commit queue is clearing flags after landing patches (I thought that was fixed?!), so I added the flags back after the bug was closed. Then Chris reopened the bug to get the executable flag issue fixed, and the commit queue tried to land the patch again (and failed).

(In reply to comment #30) > No, this was an unfortunate sequence of events. The commit queue is clearing > flags after landing patches (I thought that was fixed?!), so I added the flags > back after the bug was closed. You and I seem to disagree on correct behavior here. Leaving r+ flags after landing a patch is bad, because then the patch shows up in http://webkit.org/pending-commit. If we happen to close the bug at the same time as landing then it's "OK", but only if that bug is never re-opened. So bugzilla-tool "correctly" (at least in my mind) clears flags when it lands a patch. If you disagree, we should hash it out in another bug or over webkit-dev. It's easy to see who set r+ and cq+ by looking at a bug's history. Every bug has a link in the upper-right hand corner (as I'm certain you're aware of). :) > Then Chris reopened the bug to get the > executable flag issue fixed, and the commit queue tried to land the patch again > (and failed). OK. The executable flag issue is bug 27204. :(

(In reply to comment #31) > (In reply to comment #30) > > No, this was an unfortunate sequence of events. The commit queue is clearing > > flags after landing patches (I thought that was fixed?!), so I added the flags > > back after the bug was closed. > > You and I seem to disagree on correct behavior here. Leaving r+ flags after > landing a patch is bad, because then the patch shows up in > http://webkit.org/pending-commit. If we happen to close the bug at the same > time as landing then it's "OK", but only if that bug is never re-opened. So > bugzilla-tool "correctly" (at least in my mind) clears flags when it lands a > patch. If you disagree, we should hash it out in another bug or over > webkit-dev. I'm pretty sure Darin Adler expressed concern about this in webkit-dev earlier. I don't feel strongly enough about it to bring it up again, though. > It's easy to see who set r+ and cq+ by looking at a bug's history. Every bug > has a link in the upper-right hand corner (as I'm certain you're aware of). :) Yep. The history doesn't have the patch title, though, so it's a bit difficult to map the attachment id on that page back to the attachment on the main page. (I realize you can click on the attachment from the history page, but you get the attachment instead of any description for patches.)