Bug 38998 – REGRESSION: svn-apply failed to set reviewer correctly for attachment 55695

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

svn-apply failed to set reviewer correctly for attachment 55695 [details]

https://bugs.webkit.org/show_bug.cgi?id=26224#c45

I'm not sure if this is a regression from recent svn-apply changes or what?

[Chris Jerdonek]

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 <[e-mail]>
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 @@
[phone]  Rodrigo Belem  <[e-mail]>
+
+        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.

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

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

[Chris Jerdonek]

Created attachment 55931 [details]
Proposed patch

[Eric Seidel (no email)]

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?

[Chris Jerdonek]

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

[Chris Jerdonek]

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

[Chris Jerdonek]

Comment on attachment 55931 [details]
Proposed patch

Obsoleting to incorporate Eric's comments.

[Chris Jerdonek]

Created attachment 55938 [details]
Proposed patch 2

[Daniel Bates]

Comment on attachment 55938 [details]
Proposed patch 2

Looks good to me.

r=me.

[Chris Jerdonek]

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.

[Chris Jerdonek]

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

[Daniel Bates]

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

[Chris Jerdonek]

Committed:

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