Bug 33197 – webkit-patch upload mistakenly picks up bug URLs in non-ChangeLog files

• Component: Tools / Tests
• Status: RESOLVED FIXED

[Maciej Stachowiak]

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

[Eric Seidel (no email)]

Yeah, this is the same root cause as bug 31761.

[Eric Seidel (no email)]

This is also related to (but not the same bug as) bug 28092.

[Adam Barth]

*** Bug 31761 has been marked as a duplicate of this bug. ***

[Adam Barth]

Created attachment 46245 [details]
Patch

[Adam Barth]

Created attachment 46247 [details]
Patch

[David Kilzer (:ddkilzer)]

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.

[Adam Barth]

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

[Adam Barth]

Created attachment 46259 [details]
Patch

[David Kilzer (:ddkilzer)]

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

[David Kilzer (:ddkilzer)]

Comment on attachment 46259 [details]
Patch

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

r=me

[WebKit Commit Bot]

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

[Adam Barth]

Committed r53079: <http://trac.webkit.org/changeset/53079>

[Adam Barth]

rollout bug filed:

https://bugs.webkit.org/show_bug.cgi?id=33471

[Eric Seidel (no email)]

Comment on attachment 46259 [details]
Patch

I'm confused how this paricular patch got reviewd.  It's missing the new class, no?

[Eric Seidel (no email)]

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.

[Adam Barth]

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

[Adam Barth]

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

[Eric Seidel (no email)]

OK.  Will do.

[Eric Seidel (no email)]

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>

[Eric Seidel (no email)]

I believe this is fixed now.  http://trac.webkit.org/changeset/56964