Bug 33197

Summary: webkit-patch upload mistakenly picks up bug URLs in non-ChangeLog files
Product: WebKit Reporter: Maciej Stachowiak <mjs>
Component: Tools / TestsAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, commit-queue, ddkilzer, eric
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on: 31761    
Bug Blocks:    
Attachments:
Description Flags
Patch
none
Patch
none
Patch none

Description Maciej Stachowiak 2010-01-04 23:47:42 PST
I tried to make a new bug and upload a patch with bugzilla-tool submit patch, containing the hunk below. Instead of creating a new bug, bugzilla-tool assumed this was a patch for 29268. There are two problems with this:

1) The bug URL was not in a ChangeLog.
2) The line containing the bug URL was not even added in the patch, it was just context.

I think bugzilla-tool should only look for bug URLs in ChangeLogs, and only in + lines.


Index: JavaScriptCore/wtf/Platform.h
===================================================================
--- JavaScriptCore/wtf/Platform.h       (revision 52782)
+++ JavaScriptCore/wtf/Platform.h       (working copy)
@@ -790,11 +804,11 @@
 #endif

 #if !defined(WTF_USE_JSVALUE64) && !defined(WTF_USE_JSVALUE32) && !defined(WTF_USE_JSVALUE32_64)
-#if (CPU(X86_64) && (PLATFORM(UNIX) || PLATFORM(WIN_OS))) || CPU(IA64) || CPU(ALPHA)
+#if (CPU(X86_64) && (OS(UNIX) || OS(WINDOWS))) || CPU(IA64) || CPU(ALPHA)
 #define WTF_USE_JSVALUE64 1
 #elif CPU(ARM) || CPU(PPC64)
 #define WTF_USE_JSVALUE32 1
-#elif PLATFORM(WIN_OS) && COMPILER(MINGW)
+#elif OS(WINDOWS) && COMPILER(MINGW)
 /* Using JSVALUE32_64 causes padding/alignement issues for JITStubArg
 on MinGW. See https://bugs.webkit.org/show_bug.cgi?id=29268 */
 #define WTF_USE_JSVALUE32 1
Comment 1 Eric Seidel (no email) 2010-01-05 00:05:07 PST
Yeah, this is the same root cause as bug 31761.
Comment 2 Eric Seidel (no email) 2010-01-05 00:06:23 PST
This is also related to (but not the same bug as) bug 28092.
Comment 3 Adam Barth 2010-01-10 15:16:50 PST
*** Bug 31761 has been marked as a duplicate of this bug. ***
Comment 4 Adam Barth 2010-01-10 15:26:13 PST
Created attachment 46245 [details]
Patch
Comment 5 Adam Barth 2010-01-10 15:58:45 PST
Created attachment 46247 [details]
Patch
Comment 6 David Kilzer (:ddkilzer) 2010-01-10 21:38:45 PST
Comment on attachment 46247 [details]
Patch

> +        Also, fixed a wide-spread typo.

This should be a separate patch.  rs=me to fix the typo separately.

> diff --git a/WebKitTools/Scripts/webkitpy/commands/download.py b/WebKitTools/Scripts/webkitpy/commands/download.py
> @@ -268,6 +269,7 @@ Commits the revert and updates the bug (including re-opening the bug if necessar
>      @staticmethod
>      def _parse_bug_id_from_revision_diff(tool, revision):
>          original_diff = tool.scm().diff_for_revision(revision)
> +        # FIXME: This is wrong because it picks up bug numbers outside of ChangeLogs
>          return parse_bug_id(original_diff)

Why wasn't this instance fixed at the same time?  At minimum, this should be explained in the ChangeLog.

> diff --git a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> index f8392aa..ee5a3c8 100644
> --- a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> +++ b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
> @@ -156,11 +156,14 @@ class MockSCM(Mock):
>      def commit_ids_from_commitish_arguments(self, args):
>          return ["Commitish1", "Commitish2"]
>  
> +    def commit_message_for_this_commit(self):
> +        return CommitMessage(["CommitMessage1", "https://bugs.webkit.org/show_bug.cgi?id=93"])
> +
>      def commit_message_for_local_commit(self, commit_id):
>          if commit_id == "Commitish1":
> -            return CommitMessage("CommitMessage1\nhttps://bugs.example.org/show_bug.cgi?id=42\n")
> +            return CommitMessage(["CommitMessage1", "https://bugs.webkit.org/show_bug.cgi?id=42"])
>          if commit_id == "Commitish2":
> -            return CommitMessage("CommitMessage2\nhttps://bugs.example.org/show_bug.cgi?id=75\n")
> +            return CommitMessage(["CommitMessage2", "https://bugs.webkit.org/show_bug.cgi?id=75"])
>          raise Exception("Bogus commit_id in commit_message_for_local_commit.")

Why didn't test results change in response to changing these text inputs?  Shouldn't there be tests for grabbing the bug id from a local commit?  (Or to reverse the question, why change the URLs if test results didn't change?)

r- to split the renames into a different patch and to answer the questions above.
Comment 7 Adam Barth 2010-01-10 21:53:23 PST
(In reply to comment #6)
> (From update of attachment 46247 [details])
> > +        Also, fixed a wide-spread typo.
> 
> This should be a separate patch.  rs=me to fix the typo separately.

Ok.

> >          original_diff = tool.scm().diff_for_revision(revision)
> > +        # FIXME: This is wrong because it picks up bug numbers outside of ChangeLogs
> >          return parse_bug_id(original_diff)
> 
> Why wasn't this instance fixed at the same time?  At minimum, this should be
> explained in the ChangeLog.

It's a different bug.  The problem is this code wants to grab the bug number for a specific revision, while the code I changed is grabbing it for the current diff.

> Why didn't test results change in response to changing these text inputs? 

Apparently none of the tests actually cared what the values of the messages were.

> Shouldn't there be tests for grabbing the bug id from a local commit? 

None of the sites I changed affected that.  I'll be happy to add that test once we fix that bug.  :)

> (Or to reverse the question, why change the URLs if test results didn't change?)

Because they were wrong.  The commit messages generated in those mocks were treated has having one character per line.  I just changed the mocks to be consistent and correct.

> r- to split the renames into a different patch and to answer the questions
> above.

Thank for the review!
Comment 8 Adam Barth 2010-01-10 22:23:31 PST
Created attachment 46259 [details]
Patch
Comment 9 David Kilzer (:ddkilzer) 2010-01-11 06:39:47 PST
(In reply to comment #7)
> (In reply to comment #6)
> > (From update of attachment 46247 [details] [details])
> > >          original_diff = tool.scm().diff_for_revision(revision)
> > > +        # FIXME: This is wrong because it picks up bug numbers outside of ChangeLogs
> > >          return parse_bug_id(original_diff)
> > 
> > Why wasn't this instance fixed at the same time?  At minimum, this should be
> > explained in the ChangeLog.
> 
> It's a different bug.  The problem is this code wants to grab the bug number
> for a specific revision, while the code I changed is grabbing it for the
> current diff.

That seems to me like it's the same bug on a different code path.  Since it's not being fixed by a patch for this bug, please file a new bug for it.
Comment 10 David Kilzer (:ddkilzer) 2010-01-11 06:40:31 PST
Comment on attachment 46259 [details]
Patch

Thanks for splitting out the rename changes into a different patch!

r=me
Comment 11 WebKit Commit Bot 2010-01-11 06:51:22 PST
Comment on attachment 46259 [details]
Patch

Rejecting patch 46259 from commit-queue.

Failed to run "['WebKitTools/Scripts/test-webkitpy']" exit_code: 1
Traceback (most recent call last):
  File "WebKitTools/Scripts/test-webkitpy", line 36, in <module>
    from webkitpy.commands.abstractdiffcommand_unittest import *
ImportError: No module named abstractdiffcommand_unittest

Full output: http://webkit-commit-queue.appspot.com/results/177819
Comment 12 Adam Barth 2010-01-11 08:50:16 PST
Committed r53079: <http://trac.webkit.org/changeset/53079>
Comment 13 Adam Barth 2010-01-11 08:54:35 PST
rollout bug filed:

https://bugs.webkit.org/show_bug.cgi?id=33471
Comment 14 Eric Seidel (no email) 2010-01-11 12:03:45 PST
Comment on attachment 46259 [details]
Patch

I'm confused how this paricular patch got reviewd.  It's missing the new class, no?
Comment 15 Eric Seidel (no email) 2010-01-11 12:04:51 PST
I'm not sure that the CommitMessage class is what we want to use long term for this.

 38             bug_id = parse_bug_id(tool.scm().commit_message_for_this_commit().message())

I'm glad it works for now, but that class is kinda a hack.  I think we should be dealing with ChangeLog entries instead of commit messages.
Comment 16 Adam Barth 2010-01-11 15:16:47 PST
> I'm confused how this paricular patch got reviewd.  It's missing the new class,
> no?

It was in the earlier version that included the typo fixes.
Comment 17 Adam Barth 2010-01-11 15:17:49 PST
> I'm glad it works for now, but that class is kinda a hack.  I think we should
> be dealing with ChangeLog entries instead of commit messages.

I don't think it actually works: https://bugs.webkit.org/show_bug.cgi?id=33488

Can you roll this out for me?  I'm at Berkeley today.
Comment 18 Eric Seidel (no email) 2010-01-11 15:27:11 PST
OK.  Will do.
Comment 19 Eric Seidel (no email) 2010-01-11 15:35:58 PST
Reverted r53079 for reason:

Adam doens't think this actually works, and believe it caused a regression https://bugs.webkit.org/show_bug.cgi?id=33488 so rolling this out.

Committed r53105: <http://trac.webkit.org/changeset/53105>
Comment 20 Eric Seidel (no email) 2010-04-01 23:25:01 PDT
I believe this is fixed now.  http://trac.webkit.org/changeset/56964