Summary: | webkit-patch upload mistakenly picks up bug URLs in non-ChangeLog files | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Maciej Stachowiak <mjs> | ||||||||
Component: | Tools / Tests | Assignee: | 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
Maciej Stachowiak
2010-01-04 23:47:42 PST
Yeah, this is the same root cause as bug 31761. This is also related to (but not the same bug as) bug 28092. *** Bug 31761 has been marked as a duplicate of this bug. *** Created attachment 46245 [details]
Patch
Created attachment 46247 [details]
Patch
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. (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! Created attachment 46259 [details]
Patch
(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 on attachment 46259 [details]
Patch
Thanks for splitting out the rename changes into a different patch!
r=me
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 Committed r53079: <http://trac.webkit.org/changeset/53079> rollout bug filed: https://bugs.webkit.org/show_bug.cgi?id=33471 Comment on attachment 46259 [details]
Patch
I'm confused how this paricular patch got reviewd. It's missing the new class, no?
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. > 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.
> 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. OK. Will do. 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> I believe this is fixed now. http://trac.webkit.org/changeset/56964 |