Bug 38998

It is a regression. It is caused by the fact that the first diff (included below) is a ChangeLog and has leading junk containing the line "Reviewed by NOBODY (OOPS!)." The setChangeLogDateAndReviewer() subroutine of svn-apply substitutes the reviewer only for the first occurrence. We should probably make it so that the leading junk is not included in the patch contents sent to setChangeLogDateAndReviewer(). A quicker fix might be to have setChangeLogDateAndReviewer() replace all the occurrences and not just the first. -------------- From 25cc5eb85f766cafaa6ba715aa6cc62825ccdcc9 Mon Sep 17 00:00:00 2001 From: Rodrigo Belem <rodrigo.belem@openbossa.org> Date: Tue, 11 May 2010 09:13:02 -0400 Subject: [PATCH] [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter https://bugs.webkit.org/show_bug.cgi?id=26224 Reviewed by NOBODY (OOPS!). WebCore: This patch adds the ability, in the QtWebkit build system, to change the installation path. * WebCore.pro: WebKitTools: Added more parameters to build-webkit script, the --prefix for gkt and --install-libs, --install-headers for qt. Now it is possible to change the install prefix for gtk and install path for qt. * Scripts/build-webkit: * Scripts/webkitdirs.pm: --- WebCore/ChangeLog | 12 ++++++++++++ WebCore/WebCore.pro | 13 ++++++++++--- WebKitTools/ChangeLog | 14 ++++++++++++++ WebKitTools/Scripts/build-webkit | 15 ++++++++++++++- WebKitTools/Scripts/webkitdirs.pm | 19 +++++++++++++++---- 5 files changed, 65 insertions(+), 8 deletions(-) Index: WebCore/ChangeLog index eab24cc..b92c799 100644 --- WebCore/ChangeLog +++ WebCore/ChangeLog @@ -1,3 +1,15 @@ +2010-05-10 Rodrigo Belem <rodrigo.belem@openbossa.org> + + Reviewed by NOBODY (OOPS!). + + [Qt, Gtk] Allows build-webkit script to receive an install prefix as parameter + https://bugs.webkit.org/show_bug.cgi?id=26224 + + This patch adds the ability, in the QtWebkit build system, to change + the installation path.

Another possible quick fix would be to make the regular expression more specific. For example, it could look for something like-- "+ Reviewed by NOBODY (OOPS!)." (in particular, include the leading + in the reg ex).

We should probably also move setChangeLogDateAndReviewer() from svn-apply to VCSUtils.pm and create unit tests for it.

Either way. :) So long as we fix it and test it. I think the likelihood of multiple NOBODY (OOPS!) ever being an issue is low, so either solution sounds fine.

Bit us again: https://bugs.webkit.org/show_bug.cgi?id=38655#c7

Created attachment 55931 [details] Proposed patch

Comment on attachment 55931 [details] Proposed patch WebKitTools/Scripts/VCSUtils.pm:946 + my $savedTimeZone = $ENV{'TZ'}; I would have put the localTimeInProjectTimeZone code in its own function (for possible future unit testing) instead of lumping it in here. WebKitTools/Scripts/VCSUtils.pm:955 + $patch =~ s/(\n\+)\d{4}-[^-]{2}-[^-]{2}( )/$1$newDate$2/; Naming this regexp might help us understand what its' used for. :) I don't see any of the unit tests testing the case of Reviewed By in the leading junk, or?

(In reply to comment #7) > I don't see any of the unit tests testing the case of Reviewed By in the leading junk, or? Both unit tests test this case (one with the reviewer set and the other without setting the reviewer).

(In reply to comment #8) > (In reply to comment #7) > > I don't see any of the unit tests testing the case of Reviewed By in the leading junk, or? > > Both unit tests test this case (one with the reviewer set and the other without setting the reviewer). The name of the first test is, for example-- testName => "\"Reviewed by\" in leading junk with reviewer defined", (I should really name it "NOBODY (OOPS!)" in the leading junk though since that is what causes the issue -- not the Reviewed By.)

Comment on attachment 55931 [details] Proposed patch Obsoleting to incorporate Eric's comments.

Created attachment 55938 [details] Proposed patch 2

Comment on attachment 55938 [details] Proposed patch 2 Looks good to me. r=me.

The pre-commit hook is not letting me commit this patch. It looks like the pre-commit hook itself has a similar problem as what this patch was meant to address. Transmitting file data ....svn: Commit failed (details follow): svn: Commit blocked by pre-commit hook (exit code 1) with output: svnlook: Can't write to stream: Broken pipe The following ChangeLog files contain OOPS: trunk/WebKitTools/ChangeLog Please don't ever say "OOPS" in a ChangeLog file.

(In reply to comment #13) > The pre-commit hook is not letting me commit this patch. It looks like the pre-commit hook itself has a similar problem as what this patch was meant to address. I'll just misspell OOPS to let it go through.

(In reply to comment #13) > The pre-commit hook is not letting me commit this patch. It looks like the pre-commit hook itself has a similar problem as what this patch was meant to address. > > > Transmitting file data ....svn: Commit failed (details follow): > svn: Commit blocked by pre-commit hook (exit code 1) with output: > svnlook: Can't write to stream: Broken pipe > > The following ChangeLog files contain OOPS: > > trunk/WebKitTools/ChangeLog > > Please don't ever say "OOPS" in a ChangeLog file. CC'ing William Siegrist and Mark Rowe on this. It's probably a good thing that the pre-commit hook is overly sensitive when looking for "OOPS" in a change log file.

Committed: http://trac.webkit.org/changeset/59344