[SCM] WebKit Debian packaging branch, webkit-1.2, updated. upstream/1.1.90-6072-g9a69373
eric at webkit.org
eric at webkit.org
Thu Apr 8 01:01:58 UTC 2010
The following commit has been merged in the webkit-1.2 branch:
commit 8a4021bacd423a5489b058026e6198fe2226bd82
Author: eric at webkit.org <eric at webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Date: Tue Jan 12 12:03:21 2010 +0000
2010-01-12 Eric Seidel <eric at webkit.org>
Reviewed by Adam Barth.
bugzilla.py should have an Attachment object instead of passing around dictionaries
https://bugs.webkit.org/show_bug.cgi?id=31594
* Scripts/webkitpy/bugzilla.py:
- Add a new Attachment class, with accessor methods for all the necessary properties.
- Update Bug to carry a pointer back to bugzilla (attachments need to access Bugzilla for committer validation and url())
- Move reviewed_patches and commit_queued_patches out of Bugzilla custom methods and onto Bug
- Move committer validation logic into its own class.
- Committer rejection is only used in one place. Make the new Bug reviewed_patches and commit_queued_patches
handle the common case (of returning "reviewer" or "committer" as None), and let CommitterValidation handle
the case where we want to reject patches in bugzilla.
- Simplify fetch_patches_from_commit_queue now that committer validation is simpler.
- Make all self.bugzilla.fetch_bug access go through BugzillaQueries._fetch_bug.
- Mark set_flag_on_attachment as non-private to denote that CommitterValidation depends on it.
- Move fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug logic onto the Bug class.
* Scripts/webkitpy/bugzilla_unittest.py:
- Move test_flag_permission_rejection_message into a new CommitterValidationTest class.
* Scripts/webkitpy/commands/download.py:
- Store "bug_id" in state instead of making a fake patch object.
- Update to use Attachment and Bug objects.
* Scripts/webkitpy/commands/download_unittest.py:
- Update expected results now that our testing framework covers more code.
* Scripts/webkitpy/commands/early_warning_system.py: Update to use new Attachment class.
* Scripts/webkitpy/commands/queries.py: Remove unused ReviewedPatches class.
* Scripts/webkitpy/commands/queries_unittest.py: ditto.
* Scripts/webkitpy/commands/queues.py: Update to use new Attachment and CommitterValidator classes.
* Scripts/webkitpy/commands/queuestest.py: ditto.
* Scripts/webkitpy/commands/upload.py: ditto.
* Scripts/webkitpy/mock_bugzillatool.py:
- Now that more logic has moved into Attachment and Bug, we have to actually
provide real reviewer emails as well as real reviewer flags.
- Update mock methods to return Attachment objects.
* Scripts/webkitpy/scm.py: Update to use Attachment class.
* Scripts/webkitpy/scm_unittest.py: Update to use Attachment class.
* Scripts/webkitpy/statusserver.py: ditto.
* Scripts/webkitpy/steps/applypatch.py: ditto.
* Scripts/webkitpy/steps/applypatchwithlocalcommit.py: ditto.
* Scripts/webkitpy/steps/closebug.py: ditto.
* Scripts/webkitpy/steps/closebugforlanddiff.py: Handle either state["bug_id"] or state["patch"].bug_id()
* Scripts/webkitpy/steps/closepatch.py: Update to use Attachment class.
* Scripts/webkitpy/steps/obsoletepatches.py: ditto.
* Scripts/webkitpy/steps/updatechangelogswithreviewer.py: ditto.
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@53133 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/WebKitTools/ChangeLog b/WebKitTools/ChangeLog
index b79fbb3..f20faab 100644
--- a/WebKitTools/ChangeLog
+++ b/WebKitTools/ChangeLog
@@ -1,3 +1,50 @@
+2010-01-12 Eric Seidel <eric at webkit.org>
+
+ Reviewed by Adam Barth.
+
+ bugzilla.py should have an Attachment object instead of passing around dictionaries
+ https://bugs.webkit.org/show_bug.cgi?id=31594
+
+ * Scripts/webkitpy/bugzilla.py:
+ - Add a new Attachment class, with accessor methods for all the necessary properties.
+ - Update Bug to carry a pointer back to bugzilla (attachments need to access Bugzilla for committer validation and url())
+ - Move reviewed_patches and commit_queued_patches out of Bugzilla custom methods and onto Bug
+ - Move committer validation logic into its own class.
+ - Committer rejection is only used in one place. Make the new Bug reviewed_patches and commit_queued_patches
+ handle the common case (of returning "reviewer" or "committer" as None), and let CommitterValidation handle
+ the case where we want to reject patches in bugzilla.
+ - Simplify fetch_patches_from_commit_queue now that committer validation is simpler.
+ - Make all self.bugzilla.fetch_bug access go through BugzillaQueries._fetch_bug.
+ - Mark set_flag_on_attachment as non-private to denote that CommitterValidation depends on it.
+ - Move fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug logic onto the Bug class.
+ * Scripts/webkitpy/bugzilla_unittest.py:
+ - Move test_flag_permission_rejection_message into a new CommitterValidationTest class.
+ * Scripts/webkitpy/commands/download.py:
+ - Store "bug_id" in state instead of making a fake patch object.
+ - Update to use Attachment and Bug objects.
+ * Scripts/webkitpy/commands/download_unittest.py:
+ - Update expected results now that our testing framework covers more code.
+ * Scripts/webkitpy/commands/early_warning_system.py: Update to use new Attachment class.
+ * Scripts/webkitpy/commands/queries.py: Remove unused ReviewedPatches class.
+ * Scripts/webkitpy/commands/queries_unittest.py: ditto.
+ * Scripts/webkitpy/commands/queues.py: Update to use new Attachment and CommitterValidator classes.
+ * Scripts/webkitpy/commands/queuestest.py: ditto.
+ * Scripts/webkitpy/commands/upload.py: ditto.
+ * Scripts/webkitpy/mock_bugzillatool.py:
+ - Now that more logic has moved into Attachment and Bug, we have to actually
+ provide real reviewer emails as well as real reviewer flags.
+ - Update mock methods to return Attachment objects.
+ * Scripts/webkitpy/scm.py: Update to use Attachment class.
+ * Scripts/webkitpy/scm_unittest.py: Update to use Attachment class.
+ * Scripts/webkitpy/statusserver.py: ditto.
+ * Scripts/webkitpy/steps/applypatch.py: ditto.
+ * Scripts/webkitpy/steps/applypatchwithlocalcommit.py: ditto.
+ * Scripts/webkitpy/steps/closebug.py: ditto.
+ * Scripts/webkitpy/steps/closebugforlanddiff.py: Handle either state["bug_id"] or state["patch"].bug_id()
+ * Scripts/webkitpy/steps/closepatch.py: Update to use Attachment class.
+ * Scripts/webkitpy/steps/obsoletepatches.py: ditto.
+ * Scripts/webkitpy/steps/updatechangelogswithreviewer.py: ditto.
+
2010-01-12 Adam Barth <abarth at webkit.org>
Unreviewed typo fix. :(
diff --git a/WebKitTools/Scripts/webkitpy/bugzilla.py b/WebKitTools/Scripts/webkitpy/bugzilla.py
index b911a04..c0bbb07 100644
--- a/WebKitTools/Scripts/webkitpy/bugzilla.py
+++ b/WebKitTools/Scripts/webkitpy/bugzilla.py
@@ -59,36 +59,115 @@ def timestamp():
return datetime.now().strftime("%Y%m%d%H%M%S")
+class Attachment(object):
+ def __init__(self, attachment_dictionary, bug):
+ self._attachment_dictionary = attachment_dictionary
+ self._bug = bug
+ self._reviewer = None
+ self._committer = None
+
+ def _bugzilla(self):
+ return self._bug._bugzilla
+
+ def id(self):
+ return int(self._attachment_dictionary.get("id"))
+
+ def attacher_is_committer(self):
+ return self._bugzilla.committers.committer_by_email(patch.attacher_email())
+
+ def attacher_email(self):
+ return self._attachment_dictionary.get("attacher_email")
+
+ def bug(self):
+ return self._bug
+
+ def bug_id(self):
+ return int(self._attachment_dictionary.get("bug_id"))
+
+ def is_patch(self):
+ return not not self._attachment_dictionary.get("is_patch")
+
+ def is_obsolete(self):
+ return not not self._attachment_dictionary.get("is_obsolete")
+
+ def name(self):
+ return self._attachment_dictionary.get("name")
+
+ def review(self):
+ return self._attachment_dictionary.get("review")
+
+ def commit_queue(self):
+ return self._attachment_dictionary.get("commit-queue")
+
+ def url(self):
+ # FIXME: This should just return self._bugzilla().attachment_url_for_id(self.id()). scm_unittest.py depends on the current behavior.
+ return self._attachment_dictionary.get("url")
+
+ def _validate_flag_value(self, flag):
+ email = self._attachment_dictionary.get("%s_email" % flag)
+ if not email:
+ return None
+ committer = getattr(self._bugzilla().committers, "%s_by_email" % flag)(email)
+ if committer:
+ return committer
+ log("Warning, attachment %s on bug %s has invalid %s (%s)" % (self._attachment_dictionary['id'], self._attachment_dictionary['bug_id'], flag, email))
+
+ def reviewer(self):
+ if not self._reviewer:
+ self._reviewer = self._validate_flag_value("reviewer")
+ return self._reviewer
+
+ def committer(self):
+ if not self._committer:
+ self._committer = self._validate_flag_value("committer")
+ return self._committer
+
+
# FIXME: This class is kinda a hack for now. It exists so we have one place
# to hold bug logic, even if much of the code deals with dictionaries still.
class Bug(object):
- def __init__(self, bug_dictionary):
+ def __init__(self, bug_dictionary, bugzilla):
self.bug_dictionary = bug_dictionary
+ self._bugzilla = bugzilla
def assigned_to_email(self):
return self.bug_dictionary["assigned_to_email"]
# Rarely do we actually want obsolete attachments
def attachments(self, include_obsolete=False):
- if include_obsolete:
- return self.bug_dictionary["attachments"][:] # Return a copy in either case.
- return [attachment for attachment in self.bug_dictionary["attachments"] if not attachment["is_obsolete"]]
+ attachments = self.bug_dictionary["attachments"]
+ if not include_obsolete:
+ attachments = filter(lambda attachment: not attachment["is_obsolete"], attachments)
+ return [Attachment(attachment, self) for attachment in attachments]
def patches(self, include_obsolete=False):
- return [patch for patch in self.attachments(include_obsolete) if patch["is_patch"]]
+ return [patch for patch in self.attachments(include_obsolete) if patch.is_patch()]
def unreviewed_patches(self):
- return [patch for patch in self.patches() if patch.get("review") == "?"]
+ return [patch for patch in self.patches() if patch.review() == "?"]
+
+ def reviewed_patches(self):
+ # Checking reviewer() ensures that it was both reviewed and has a valid reviewer.
+ return [patch for patch in self.patches() if patch.review() == "+" and patch.reviewer()]
+
+ def commit_queued_patches(self):
+ # Checking committer() ensures that it was both commit-queue+'d and has a valid committer.
+ return [patch for patch in self.patches() if patch.commit_queue() == "+" and patch.committer()]
# A container for all of the logic for making a parsing buzilla queries.
class BugzillaQueries(object):
def __init__(self, bugzilla):
- self.bugzilla = bugzilla
+ self._bugzilla = bugzilla
+ # Note: _load_query and _fetch_bug are the only two methods which access self._bugzilla.
def _load_query(self, query):
- full_url = "%s%s" % (self.bugzilla.bug_server_url, query)
- return self.bugzilla.browser.open(full_url)
+ full_url = "%s%s" % (self._bugzilla.bug_server_url, query)
+ return self._bugzilla.browser.open(full_url)
+
+ def _fetch_bug(self, bug_id):
+ return self._bugzilla.fetch_bug(bug_id)
+
def _fetch_bug_ids_advanced_query(self, query):
soup = BeautifulSoup(self._load_query(query))
@@ -110,29 +189,21 @@ class BugzillaQueries(object):
return self._fetch_bug_ids_advanced_query(needs_commit_query_url)
def fetch_patches_from_pending_commit_list(self):
- # FIXME: This should not have to go through self.bugzilla
- return sum([self.bugzilla.fetch_reviewed_patches_from_bug(bug_id) for bug_id in self.fetch_bug_ids_from_pending_commit_list()], [])
+ return sum([self._fetch_bug(bug_id).reviewed_patches() for bug_id in self.fetch_bug_ids_from_pending_commit_list()], [])
def fetch_bug_ids_from_commit_queue(self):
commit_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=commit-queue%2B"
return self._fetch_bug_ids_advanced_query(commit_queue_url)
- def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
- # FIXME: Once reject_invalid_patches is moved out of this function this becomes a simple list comprehension using fetch_bug_ids_from_commit_queue.
- patches_to_land = []
- for bug_id in self.fetch_bug_ids_from_commit_queue():
- # FIXME: This should not have to go through self.bugzilla
- patches = self.bugzilla.fetch_commit_queue_patches_from_bug(bug_id, reject_invalid_patches)
- patches_to_land += patches
- return patches_to_land
+ def fetch_patches_from_commit_queue(self):
+ return sum([self._fetch_bug(bug_id).commit_queued_patches() for bug_id in self.fetch_bug_ids_from_commit_queue()], [])
def _fetch_bug_ids_from_review_queue(self):
review_queue_url = "buglist.cgi?query_format=advanced&bug_status=UNCONFIRMED&bug_status=NEW&bug_status=ASSIGNED&bug_status=REOPENED&field0-0-0=flagtypes.name&type0-0-0=equals&value0-0-0=review?"
return self._fetch_bug_ids_advanced_query(review_queue_url)
def fetch_patches_from_review_queue(self, limit=None):
- # FIXME: We should probably have a self.fetch_bug to minimize the number of self.bugzilla calls.
- return sum([self.bugzilla.fetch_bug(bug_id).unreviewed_patches() for bug_id in self._fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
+ return sum([self._fetch_bug(bug_id).unreviewed_patches() for bug_id in self._fetch_bug_ids_from_review_queue()[:limit]], []) # [:None] returns the whole array.
# FIXME: Why do we have both fetch_patches_from_review_queue and fetch_attachment_ids_from_review_queue??
# NOTE: This is also the only client of _fetch_attachment_ids_request_query
@@ -141,17 +212,62 @@ class BugzillaQueries(object):
return self._fetch_attachment_ids_request_query(review_queue_url)
+class CommitterValidator(object):
+ def __init__(self, bugzilla):
+ self._bugzilla = bugzilla
+
+ # _view_source_link belongs in some sort of webkit_config.py module.
+ def _view_source_link(self, local_path):
+ return "http://trac.webkit.org/browser/trunk/%s" % local_path
+
+ def _flag_permission_rejection_message(self, setter_email, flag_name):
+ committer_list = "WebKitTools/Scripts/webkitpy/committers.py" # This could be computed from CommitterList.__file__
+ contribution_guidlines_url = "http://webkit.org/coding/contributing.html" # Should come from some webkit_config.py
+ queue_administrator = "eseidel at chromium.org" # This could be queried from the status_server.
+ queue_name = "commit-queue" # This could be queried from the tool.
+ rejection_message = "%s does not have %s permissions according to %s." % (setter_email, flag_name, self._view_source_link(committer_list))
+ rejection_message += "\n\n- If you do not have %s rights please read %s for instructions on how to use bugzilla flags." % (flag_name, contribution_guidlines_url)
+ rejection_message += "\n\n- If you have %s rights please correct the error in %s by adding yourself to the file (no review needed)." % (flag_name, committer_list)
+ rejection_message += " Due to bug 30084 the %s will require a restart after your change." % queue_name
+ rejection_message += " Please contact %s to request a %s restart." % (queue_administrator, queue_name)
+ rejection_message += " After restart the %s will correctly respect your %s rights." % (queue_name, flag_name)
+ return rejection_message
+
+ def _validate_setter_email(self, patch, result_key, rejection_function):
+ committer = getattr(patch, result_key)()
+ # If the flag is set, and we don't recognize the setter, reject the flag!
+ if patch._attachment_dictionary.get("%s_email" % result_key) and not committer:
+ rejection_function(patch.id(), self._flag_permission_rejection_message(setter_email, result_key))
+ return False
+ return True
+
+ def patches_after_rejecting_invalid_commiters_and_reviewers(self, patches):
+ validated_patches = []
+ for patch in patches:
+ if self._validate_setter_email(patch, "reviewer", self.reject_patch_from_review_queue) and self._validate_setter_email(patch, "committer", self.reject_patch_from_commit_queue):
+ validated_patches.append(patch)
+ return validated_patches
+
+ def reject_patch_from_commit_queue(self, attachment_id, additional_comment_text=None):
+ comment_text = "Rejecting patch %s from commit-queue." % attachment_id
+ self._bugzilla.set_flag_on_attachment(attachment_id, 'commit-queue', '-', comment_text, additional_comment_text)
+
+ def reject_patch_from_review_queue(self, attachment_id, additional_comment_text=None):
+ comment_text = "Rejecting patch %s from review queue." % attachment_id
+ self._bugzilla.set_flag_on_attachment(attachment_id, 'review', '-', comment_text, additional_comment_text)
+
+
class Bugzilla(object):
def __init__(self, dryrun=False, committers=CommitterList()):
self.dryrun = dryrun
self.authenticated = False
self.queries = BugzillaQueries(self)
+ self.committers = committers
# FIXME: We should use some sort of Browser mock object when in dryrun mode (to prevent any mistakes).
self.browser = Browser()
# Ignore bugs.webkit.org/robots.txt until we fix it to allow this script
self.browser.set_handle_robots(False)
- self.committers = committers
# FIXME: Much of this should go into some sort of config module:
bug_server_host = "bugs.webkit.org"
@@ -185,6 +301,7 @@ class Bugzilla(object):
attachment['is_obsolete'] = (element.has_key('isobsolete') and element['isobsolete'] == "1")
attachment['is_patch'] = (element.has_key('ispatch') and element['ispatch'] == "1")
attachment['id'] = int(element.find('attachid').string)
+ # FIXME: No need to parse out the url here.
attachment['url'] = self.attachment_url_for_id(attachment['id'])
attachment['name'] = unicode(element.find('desc').string)
attachment['attacher_email'] = str(element.find('attacher').string)
@@ -215,7 +332,7 @@ class Bugzilla(object):
# FIXME: A BugzillaCache object should provide all these fetch_ methods.
def fetch_bug(self, bug_id):
- return Bug(self.fetch_bug_dictionary(bug_id))
+ return Bug(self.fetch_bug_dictionary(bug_id), self)
def _parse_bug_id_from_attachment_page(self, page):
up_link = BeautifulSoup(page).find('link', rel='Up') # The "Up" relation happens to point to the bug.
@@ -230,8 +347,7 @@ class Bugzilla(object):
page = self.browser.open(attachment_url)
return self._parse_bug_id_from_attachment_page(page)
- # This should really return an Attachment object
- # which can lazily fetch any missing data.
+ # FIXME: This should just return Attachment(id), which should be able to lazily fetch needed data.
def fetch_attachment(self, attachment_id):
# We could grab all the attachment details off of the attachment edit page
# but we already have working code to do so off of the bugs page, so re-use that.
@@ -239,79 +355,10 @@ class Bugzilla(object):
if not bug_id:
return None
for attachment in self.fetch_bug(bug_id).attachments(include_obsolete=True):
- # FIXME: Once we have a real Attachment class we shouldn't paper over this possible comparison failure
- # and we should remove the int() == int() hacks and leave it just ==.
- if int(attachment['id']) == int(attachment_id):
- self._validate_committer_and_reviewer(attachment)
+ if attachment.id() == int(attachment_id):
return attachment
return None # This should never be hit.
- # fetch_patches_from_bug exists until we expose a Bug class outside of bugzilla.py
- def fetch_patches_from_bug(self, bug_id):
- return self.fetch_bug(bug_id).patches()
-
- # _view_source_link belongs in some sort of webkit_config.py module.
- def _view_source_link(self, local_path):
- return "http://trac.webkit.org/browser/trunk/%s" % local_path
-
- def _flag_permission_rejection_message(self, setter_email, flag_name):
- committer_list = "WebKitTools/Scripts/webkitpy/committers.py" # This could be computed from CommitterList.__file__
- contribution_guidlines_url = "http://webkit.org/coding/contributing.html" # Should come from some webkit_config.py
- queue_administrator = "eseidel at chromium.org" # This could be queried from the status_server.
- queue_name = "commit-queue" # This could be queried from the tool.
- rejection_message = "%s does not have %s permissions according to %s." % (setter_email, flag_name, self._view_source_link(committer_list))
- rejection_message += "\n\n- If you do not have %s rights please read %s for instructions on how to use bugzilla flags." % (flag_name, contribution_guidlines_url)
- rejection_message += "\n\n- If you have %s rights please correct the error in %s by adding yourself to the file (no review needed)." % (flag_name, committer_list)
- rejection_message += " Due to bug 30084 the %s will require a restart after your change." % queue_name
- rejection_message += " Please contact %s to request a %s restart." % (queue_administrator, queue_name)
- rejection_message += " After restart the %s will correctly respect your %s rights." % (queue_name, flag_name)
- return rejection_message
-
- def _validate_setter_email(self, patch, result_key, lookup_function, rejection_function, reject_invalid_patches):
- setter_email = patch.get(result_key + '_email')
- if not setter_email:
- return None
-
- committer = lookup_function(setter_email)
- if committer:
- patch[result_key] = committer.full_name
- return patch[result_key]
-
- if reject_invalid_patches:
- rejection_function(patch['id'], self._flag_permission_rejection_message(setter_email, result_key))
- else:
- log("Warning, attachment %s on bug %s has invalid %s (%s)" % (patch['id'], patch['bug_id'], result_key, setter_email))
- return None
-
- def _validate_reviewer(self, patch, reject_invalid_patches):
- return self._validate_setter_email(patch, 'reviewer', self.committers.reviewer_by_email, self.reject_patch_from_review_queue, reject_invalid_patches)
-
- def _validate_committer(self, patch, reject_invalid_patches):
- return self._validate_setter_email(patch, 'committer', self.committers.committer_by_email, self.reject_patch_from_commit_queue, reject_invalid_patches)
-
- # FIXME: This is a hack until we have a real Attachment object.
- # _validate_committer and _validate_reviewer fill in the 'reviewer' and 'committer'
- # keys which other parts of the code expect to be filled in.
- def _validate_committer_and_reviewer(self, patch):
- self._validate_reviewer(patch, reject_invalid_patches=False)
- self._validate_committer(patch, reject_invalid_patches=False)
-
- # FIXME: fetch_reviewed_patches_from_bug and fetch_commit_queue_patches_from_bug
- # should share more code and use list comprehensions.
- def fetch_reviewed_patches_from_bug(self, bug_id, reject_invalid_patches=False):
- reviewed_patches = []
- for attachment in self.fetch_bug(bug_id).attachments():
- if self._validate_reviewer(attachment, reject_invalid_patches):
- reviewed_patches.append(attachment)
- return reviewed_patches
-
- def fetch_commit_queue_patches_from_bug(self, bug_id, reject_invalid_patches=False):
- commit_queue_patches = []
- for attachment in self.fetch_reviewed_patches_from_bug(bug_id, reject_invalid_patches):
- if self._validate_committer(attachment, reject_invalid_patches):
- commit_queue_patches.append(attachment)
- return commit_queue_patches
-
def authenticate(self):
if self.authenticated:
return
@@ -446,7 +493,7 @@ class Bugzilla(object):
self.browser.submit()
# FIXME: We need a way to test this on a live bugzilla instance.
- def _set_flag_on_attachment(self, attachment_id, flag_name, flag_value, comment_text, additional_comment_text):
+ def set_flag_on_attachment(self, attachment_id, flag_name, flag_value, comment_text, additional_comment_text):
self.authenticate()
if additional_comment_text:
@@ -462,14 +509,6 @@ class Bugzilla(object):
self._find_select_element_for_flag(flag_name).value = (flag_value,)
self.browser.submit()
- def reject_patch_from_commit_queue(self, attachment_id, additional_comment_text=None):
- comment_text = "Rejecting patch %s from commit-queue." % attachment_id
- self._set_flag_on_attachment(attachment_id, 'commit-queue', '-', comment_text, additional_comment_text)
-
- def reject_patch_from_review_queue(self, attachment_id, additional_comment_text=None):
- comment_text = "Rejecting patch %s from review queue." % attachment_id
- self._set_flag_on_attachment(attachment_id, 'review', '-', comment_text, additional_comment_text)
-
# FIXME: All of these bug editing methods have a ridiculous amount of copy/paste code.
def obsolete_attachment(self, attachment_id, comment_text = None):
self.authenticate()
diff --git a/WebKitTools/Scripts/webkitpy/bugzilla_unittest.py b/WebKitTools/Scripts/webkitpy/bugzilla_unittest.py
index 0d78602..d555f78 100644
--- a/WebKitTools/Scripts/webkitpy/bugzilla_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/bugzilla_unittest.py
@@ -29,7 +29,7 @@
import unittest
from webkitpy.committers import CommitterList, Reviewer, Committer
-from webkitpy.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id
+from webkitpy.bugzilla import Bugzilla, BugzillaQueries, parse_bug_id, CommitterValidator
from webkitpy.outputcapture import OutputCapture
from webkitpy.mock import Mock
@@ -49,6 +49,16 @@ class MockBrowser(object):
def submit(self):
pass
+class CommitterValidatorTest(unittest.TestCase):
+ def test_flag_permission_rejection_message(self):
+ validator = CommitterValidator(bugzilla=None)
+ expected_messsage="""foo at foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.
+
+- If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
+
+- If you have review rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel at chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your review rights."""
+ self.assertEqual(validator._flag_permission_rejection_message("foo at foo.com", "review"), expected_messsage)
+
class BugzillaTest(unittest.TestCase):
_example_attachment = '''
@@ -178,7 +188,7 @@ ZEZpbmlzaExvYWRXaXRoUmVhc29uOnJlYXNvbl07Cit9CisKIEBlbmQKIAogI2VuZGlmCg==
"assigned_to_email" : "webkit-unassigned at lists.webkit.org",
"attachments" : [{
'name': u'Patch',
- 'url': 'https://bugs.webkit.org/attachment.cgi?id=45548',
+ 'url' : "https://bugs.webkit.org/attachment.cgi?id=45548",
'is_obsolete': False,
'review': '?',
'is_patch': True,
@@ -231,15 +241,6 @@ ZEZpbmlzaExvYWRXaXRoUmVhc29uOnJlYXNvbl07Cit9CisKIEBlbmQKIAogI2VuZGlmCg==
expected_stderr = "Adding ['adam at example.com'] to the CC list for bug 42\n"
OutputCapture().assert_outputs(self, bugzilla.add_cc_to_bug, [42, ["adam at example.com"]], expected_stderr=expected_stderr)
- def test_flag_permission_rejection_message(self):
- bugzilla = Bugzilla()
- expected_messsage="""foo at foo.com does not have review permissions according to http://trac.webkit.org/browser/trunk/WebKitTools/Scripts/webkitpy/committers.py.
-
-- If you do not have review rights please read http://webkit.org/coding/contributing.html for instructions on how to use bugzilla flags.
-
-- If you have review rights please correct the error in WebKitTools/Scripts/webkitpy/committers.py by adding yourself to the file (no review needed). Due to bug 30084 the commit-queue will require a restart after your change. Please contact eseidel at chromium.org to request a commit-queue restart. After restart the commit-queue will correctly respect your review rights."""
- self.assertEqual(bugzilla._flag_permission_rejection_message("foo at foo.com", "review"), expected_messsage)
-
class BugzillaQueriesTest(unittest.TestCase):
_sample_request_page = """
diff --git a/WebKitTools/Scripts/webkitpy/commands/download.py b/WebKitTools/Scripts/webkitpy/commands/download.py
index bb8c01a..49a6862 100644
--- a/WebKitTools/Scripts/webkitpy/commands/download.py
+++ b/WebKitTools/Scripts/webkitpy/commands/download.py
@@ -86,10 +86,7 @@ If a bug id is provided, or one can be found in the ChangeLog land will update t
def _prepare_state(self, options, args, tool):
return {
- "patch" : {
- "id" : None,
- "bug_id" : (args and args[0]) or parse_bug_id(tool.scm().create_patch()),
- }
+ "bug_id" : (args and args[0]) or parse_bug_id(tool.scm().create_patch()),
}
@@ -104,8 +101,7 @@ class AbstractPatchProcessingCommand(AbstractDeclarativeCommand):
def _collect_patches_by_bug(patches):
bugs_to_patches = {}
for patch in patches:
- bug_id = patch["bug_id"]
- bugs_to_patches[bug_id] = bugs_to_patches.get(bug_id, []) + [patch]
+ bugs_to_patches[patch.bug_id()] = bugs_to_patches.get(patch.bug_id(), []) + [patch]
return bugs_to_patches
def execute(self, options, args, tool):
@@ -148,7 +144,7 @@ class ProcessBugsMixin(object):
def _fetch_list_of_patches_to_process(self, options, args, tool):
all_patches = []
for bug_id in args:
- patches = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
+ patches = tool.bugs.fetch_bug(bug_id).reviewed_patches()
log("%s found on bug %s." % (pluralize("reviewed patch", len(patches)), bug_id))
all_patches += patches
return all_patches
diff --git a/WebKitTools/Scripts/webkitpy/commands/download_unittest.py b/WebKitTools/Scripts/webkitpy/commands/download_unittest.py
index 81b9da5..9959b60 100644
--- a/WebKitTools/Scripts/webkitpy/commands/download_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/download_unittest.py
@@ -82,10 +82,21 @@ class DownloadCommandsTest(CommandsTest):
self.assert_execute_outputs(BuildAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr)
def test_land_attachment(self):
- expected_stderr = "Processing 1 patch from 1 bug.\nUpdating working directory\nProcessing patch 197 from bug 42.\nBuilding WebKit\nRunning Python unit tests\nRunning Perl unit tests\nRunning JavaScriptCore tests\nRunning run-webkit-tests\n"
+ # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
+ expected_stderr = """Processing 1 patch from 1 bug.
+Updating working directory
+Processing patch 197 from bug 42.
+Building WebKit
+Running Python unit tests
+Running Perl unit tests
+Running JavaScriptCore tests
+Running run-webkit-tests
+Not closing bug 42 as attachment 197 has review=+. Assuming there are more patches to land from this bug.
+"""
self.assert_execute_outputs(LandAttachment(), [197], options=self._default_options(), expected_stderr=expected_stderr)
def test_land_patches(self):
+ # FIXME: This expected result is imperfect, notice how it's seeing the same patch as still there after it thought it would have cleared the flags.
expected_stderr = """2 reviewed patches found on bug 42.
Processing 2 patches from 1 bug.
Updating working directory
@@ -95,6 +106,7 @@ Running Python unit tests
Running Perl unit tests
Running JavaScriptCore tests
Running run-webkit-tests
+Not closing bug 42 as attachment 197 has review=+. Assuming there are more patches to land from this bug.
Updating working directory
Processing patch 128 from bug 42.
Building WebKit
@@ -102,6 +114,7 @@ Running Python unit tests
Running Perl unit tests
Running JavaScriptCore tests
Running run-webkit-tests
+Not closing bug 42 as attachment 197 has review=+. Assuming there are more patches to land from this bug.
"""
self.assert_execute_outputs(LandFromBug(), [42], options=self._default_options(), expected_stderr=expected_stderr)
diff --git a/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py b/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
index 287db57..2abcc42 100644
--- a/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
+++ b/WebKitTools/Scripts/webkitpy/commands/early_warning_system.py
@@ -59,7 +59,7 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue):
"--non-interactive",
"--parent-command=%s" % self.name,
"--no-update",
- patch["id"]])
+ patch.id()])
@classmethod
def handle_script_error(cls, tool, state, script_error):
@@ -68,8 +68,8 @@ class AbstractEarlyWarningSystem(AbstractReviewQueue):
if is_svn_apply:
QueueEngine.exit_after_handled_error(script_error)
results_link = tool.status_server.results_url_for_status(status_id)
- message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"]["id"], cls.port_name, results_link)
- tool.bugs.post_comment_to_bug(state["patch"]["bug_id"], message, cc=cls.watchers)
+ message = "Attachment %s did not build on %s:\nBuild output: %s" % (state["patch"].id(), cls.port_name, results_link)
+ tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
exit(1)
@@ -103,7 +103,7 @@ class AbstractCommitterOnlyEWS(AbstractEarlyWarningSystem):
self._committers = committers
def process_work_item(self, patch):
- if not self._committers.committer_by_email(patch["attacher_email"]):
+ if not self._committers.committer_by_email(patch.attacher_email()):
self._did_error(patch, "%s cannot process patches from non-committers :(" % self.name)
return
AbstractEarlyWarningSystem.process_work_item(self, patch)
diff --git a/WebKitTools/Scripts/webkitpy/commands/queries.py b/WebKitTools/Scripts/webkitpy/commands/queries.py
index 18108a0..c11fb85 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queries.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queries.py
@@ -55,7 +55,7 @@ class PatchesToCommit(AbstractDeclarativeCommand):
patches = tool.bugs.queries.fetch_patches_from_commit_queue()
log("Patches in commit queue:")
for patch in patches:
- print "%s" % patch["url"]
+ print patch.url()
class PatchesToCommitQueue(AbstractDeclarativeCommand):
@@ -69,28 +69,27 @@ class PatchesToCommitQueue(AbstractDeclarativeCommand):
@staticmethod
def _needs_commit_queue(patch):
- commit_queue_flag = patch.get("commit-queue")
- if (commit_queue_flag and commit_queue_flag == '+'): # If it's already cq+, ignore the patch.
- log("%s already has cq=%s" % (patch["id"], commit_queue_flag))
+ if patch.commit_queue() == "+": # If it's already cq+, ignore the patch.
+ log("%s already has cq=%s" % (patch.id(), patch.commit_queue()))
return False
# We only need to worry about patches from contributers who are not yet committers.
- committer_record = CommitterList().committer_by_email(patch["attacher_email"])
+ committer_record = CommitterList().committer_by_email(patch.attacher_email())
if committer_record:
- log("%s committer = %s" % (patch["id"], committer_record))
+ log("%s committer = %s" % (patch.id(), committer_record))
return not committer_record
def execute(self, options, args, tool):
patches = tool.bugs.queries.fetch_patches_from_pending_commit_list()
patches_needing_cq = filter(self._needs_commit_queue, patches)
if options.bugs:
- bugs_needing_cq = map(lambda patch: patch['bug_id'], patches_needing_cq)
+ bugs_needing_cq = map(lambda patch: patch.bug_id(), patches_needing_cq)
bugs_needing_cq = sorted(set(bugs_needing_cq))
for bug_id in bugs_needing_cq:
print "%s" % tool.bugs.bug_url_for_bug_id(bug_id)
else:
for patch in patches_needing_cq:
- print "%s" % tool.bugs.attachment_url_for_id(patch["id"], action="edit")
+ print "%s" % tool.bugs.attachment_url_for_id(patch.id(), action="edit")
class PatchesToReview(AbstractDeclarativeCommand):
@@ -104,18 +103,6 @@ class PatchesToReview(AbstractDeclarativeCommand):
print patch_id
-class ReviewedPatches(AbstractDeclarativeCommand):
- name = "reviewed-patches"
- help_text = "List r+'d patches on a bug"
- argument_names = "BUGID"
-
- def execute(self, options, args, tool):
- bug_id = args[0]
- patches_to_land = tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
- for patch in patches_to_land:
- print "%s" % patch["url"]
-
-
class TreeStatus(AbstractDeclarativeCommand):
name = "tree-status"
help_text = "Print the status of the %s buildbots" % BuildBot.default_host
diff --git a/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py b/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
index c7fd7a8..da77be1 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queries_unittest.py
@@ -59,10 +59,6 @@ class QueryCommandsTest(CommandsTest):
expected_stderr = "Patches pending review:\n"
self.assert_execute_outputs(PatchesToReview(), None, expected_stdout, expected_stderr)
- def test_reviewed_patches(self):
- expected_stdout = "http://example.com/197\nhttp://example.com/128\n"
- self.assert_execute_outputs(ReviewedPatches(), [42], expected_stdout)
-
def test_tree_status(self):
expected_stdout = "ok : Builder1\nok : Builder2\n"
self.assert_execute_outputs(TreeStatus(), None, expected_stdout)
diff --git a/WebKitTools/Scripts/webkitpy/commands/queues.py b/WebKitTools/Scripts/webkitpy/commands/queues.py
index 105d5ce..4133e34 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queues.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queues.py
@@ -35,6 +35,7 @@ from datetime import datetime
from optparse import make_option
from StringIO import StringIO
+from webkitpy.bugzilla import CommitterValidator
from webkitpy.executive import ScriptError
from webkitpy.grammar import pluralize
from webkitpy.webkit_logging import error, log
@@ -83,7 +84,7 @@ class AbstractQueue(Command, QueueEngineDelegate):
return "%s.log" % self.name
def work_item_log_path(self, patch):
- return os.path.join("%s-logs" % self.name, "%s.log" % patch["bug_id"])
+ return os.path.join("%s-logs" % self.name, "%s.log" % patch.bug_id())
def begin_work_queue(self):
log("CAUTION: %s will discard all local changes in \"%s\"" % (self.name, self.tool.scm().checkout_root))
@@ -140,14 +141,16 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
def begin_work_queue(self):
AbstractQueue.begin_work_queue(self)
+ self.committer_validator = CommitterValidator(self.tool.bugs)
def next_work_item(self):
- patches = self.tool.bugs.queries.fetch_patches_from_commit_queue(reject_invalid_patches=True)
+ patches = self.tool.bugs.queries.fetch_patches_from_commit_queue()
+ patches = self.committer_validator.patches_after_rejecting_invalid_commiters_and_reviewers(patches)
if not patches:
self._update_status("Empty queue")
return None
# Only bother logging if we have patches in the queue.
- self.log_progress([patch['id'] for patch in patches])
+ self.log_progress([patch.id() for patch in patches])
return patches[0]
def _can_build_and_test(self):
@@ -178,18 +181,18 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
def process_work_item(self, patch):
try:
- self._cc_watchers(patch["bug_id"])
+ self._cc_watchers(patch.bug_id())
# We pass --no-update here because we've already validated
# that the current revision actually builds and passes the tests.
# If we update, we risk moving to a revision that doesn't!
- self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch["id"]])
+ self.run_webkit_patch(["land-attachment", "--force-clean", "--non-interactive", "--no-update", "--parent-command=commit-queue", "--build-style=both", "--quiet", patch.id()])
self._did_pass(patch)
except ScriptError, e:
self._did_fail(patch)
raise e
def handle_unexpected_error(self, patch, message):
- self.tool.bugs.reject_patch_from_commit_queue(patch["id"], message)
+ self.committer_validator.reject_patch_from_commit_queue(patch.id(), message)
# StepSequenceErrorHandler methods
@@ -203,7 +206,8 @@ class CommitQueue(AbstractQueue, StepSequenceErrorHandler):
@classmethod
def handle_script_error(cls, tool, state, script_error):
status_id = cls._update_status_for_script_error(tool, state, script_error)
- tool.bugs.reject_patch_from_commit_queue(state["patch"]["id"], cls._error_message_for_bug(tool, status_id, script_error))
+ validator = CommitterValidator(tool.bugs)
+ validator.reject_patch_from_commit_queue(state["patch"].id(), cls._error_message_for_bug(tool, status_id, script_error))
class AbstractReviewQueue(AbstractQueue, PersistentPatchCollectionDelegate, StepSequenceErrorHandler):
@@ -268,7 +272,7 @@ class StyleQueue(AbstractReviewQueue):
return True
def _review_patch(self, patch):
- self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch["id"]])
+ self.run_webkit_patch(["check-style", "--force-clean", "--non-interactive", "--parent-command=style-queue", patch.id()])
@classmethod
def handle_script_error(cls, tool, state, script_error):
@@ -276,6 +280,6 @@ class StyleQueue(AbstractReviewQueue):
status_id = cls._update_status_for_script_error(tool, state, script_error, is_error=is_svn_apply)
if is_svn_apply:
QueueEngine.exit_after_handled_error(script_error)
- message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"]["id"], cls.name, script_error.message_with_output(output_limit=3*1024))
- tool.bugs.post_comment_to_bug(state["patch"]["bug_id"], message, cc=cls.watchers)
+ message = "Attachment %s did not pass %s:\n\n%s" % (state["patch"].id(), cls.name, script_error.message_with_output(output_limit=3*1024))
+ tool.bugs.post_comment_to_bug(state["patch"].bug_id(), message, cc=cls.watchers)
exit(1)
diff --git a/WebKitTools/Scripts/webkitpy/commands/queuestest.py b/WebKitTools/Scripts/webkitpy/commands/queuestest.py
index 71096b6..09d1c26 100644
--- a/WebKitTools/Scripts/webkitpy/commands/queuestest.py
+++ b/WebKitTools/Scripts/webkitpy/commands/queuestest.py
@@ -28,6 +28,7 @@
import unittest
+from webkitpy.bugzilla import Attachment
from webkitpy.mock import Mock
from webkitpy.mock_bugzillatool import MockBugzillaTool
from webkitpy.outputcapture import OutputCapture
@@ -42,11 +43,11 @@ class MockQueueEngine(object):
class QueuesTest(unittest.TestCase):
- mock_work_item = {
+ mock_work_item = Attachment({
"id" : 1234,
"bug_id" : 345,
"attacher_email": "adam at example.com",
- }
+ }, None)
def assert_queue_outputs(self, queue, args=None, work_item=None, expected_stdout=None, expected_stderr=None, options=Mock(), tool=MockBugzillaTool()):
if not expected_stdout:
diff --git a/WebKitTools/Scripts/webkitpy/commands/upload.py b/WebKitTools/Scripts/webkitpy/commands/upload.py
index 58f5899..870c4b1 100644
--- a/WebKitTools/Scripts/webkitpy/commands/upload.py
+++ b/WebKitTools/Scripts/webkitpy/commands/upload.py
@@ -68,18 +68,18 @@ class AssignToCommitter(AbstractDeclarativeCommand):
return
# FIXME: This should call a reviewed_patches() method on bug instead of re-fetching.
- reviewed_patches = self.tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
+ reviewed_patches = self.tool.bugs.fetch_bug(bug_id).reviewed_patches()
if not reviewed_patches:
log("Bug %s has no non-obsolete patches, ignoring." % bug_id)
return
latest_patch = reviewed_patches[-1]
- attacher_email = latest_patch["attacher_email"]
+ attacher_email = latest_patch.attacher_email()
committer = committers.committer_by_email(attacher_email)
if not committer:
log("Attacher %s is not a committer. Bug %s likely needs commit-queue+." % (attacher_email, bug_id))
return
- reassign_message = "Attachment %s was posted by a committer and has review+, assigning to %s for commit." % (latest_patch["id"], committer.full_name)
+ reassign_message = "Attachment %s was posted by a committer and has review+, assigning to %s for commit." % (latest_patch.id(), committer.full_name)
self.tool.bugs.reassign_bug(bug_id, committer.bugzilla_email(), reassign_message)
def execute(self, options, args, tool):
diff --git a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
index f8392aa..146e40b 100644
--- a/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
+++ b/WebKitTools/Scripts/webkitpy/mock_bugzillatool.py
@@ -30,7 +30,8 @@ import os
from webkitpy.mock import Mock
from webkitpy.scm import CommitMessage
-from webkitpy.bugzilla import Bug
+from webkitpy.committers import CommitterList, Reviewer
+from webkitpy.bugzilla import Bug, Attachment
def _id_to_object_dictionary(*objects):
dictionary = {}
@@ -45,7 +46,8 @@ _patch1 = {
"url" : "http://example.com/197",
"is_obsolete" : False,
"is_patch" : True,
- "reviewer" : "Reviewer1",
+ "review" : "+",
+ "reviewer_email" : "foo at bar.com",
"attacher_email" : "Contributer1",
}
_patch2 = {
@@ -54,7 +56,8 @@ _patch2 = {
"url" : "http://example.com/128",
"is_obsolete" : False,
"is_patch" : True,
- "reviewer" : "Reviewer2",
+ "review" : "+",
+ "reviewer_email" : "foo at bar.com",
"attacher_email" : "eric at webkit.org",
}
@@ -82,6 +85,10 @@ _bug3 = {
}
class MockBugzillaQueries(Mock):
+ def __init__(self, bugzilla):
+ Mock.__init__(self)
+ self._bugzilla = bugzilla
+
def fetch_bug_ids_from_commit_queue(self):
return [42, 75]
@@ -89,42 +96,45 @@ class MockBugzillaQueries(Mock):
return [197, 128]
def fetch_patches_from_commit_queue(self, reject_invalid_patches=False):
- return [_patch1, _patch2]
+ return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
def fetch_bug_ids_from_pending_commit_list(self):
return [42, 75, 76]
def fetch_patches_from_pending_commit_list(self):
- return [_patch1, _patch2]
+ return [self._bugzilla.fetch_attachment(_patch1["id"]), self._bugzilla.fetch_attachment(_patch2["id"])]
+# FIXME: Bugzilla is the wrong Mock-point. Once we have a BugzillaNetwork class we should mock that instead.
+# Most of this class is just copy/paste from Bugzilla.
class MockBugzilla(Mock):
bug_server_url = "http://example.com"
unassigned_email = _unassigned_email
bug_cache = _id_to_object_dictionary(_bug1, _bug2, _bug3)
attachment_cache = _id_to_object_dictionary(_patch1, _patch2)
- queries = MockBugzillaQueries()
- def fetch_bug(self, bug_id):
- return Bug(self.bug_cache.get(bug_id))
+ def __init__(self):
+ Mock.__init__(self)
+ self.queries = MockBugzillaQueries(self)
+ self.committers = CommitterList(reviewers=[Reviewer("Foo Bar", "foo at bar.com")])
- def fetch_reviewed_patches_from_bug(self, bug_id):
- return self.fetch_patches_from_bug(bug_id) # Return them all for now.
+ def fetch_bug(self, bug_id):
+ return Bug(self.bug_cache.get(bug_id), self)
def fetch_attachment(self, attachment_id):
- return self.attachment_cache[attachment_id] # This could be changed to .get() if we wish to allow failed lookups.
+ attachment_dictionary = self.attachment_cache[attachment_id] # This could be changed to .get() if we wish to allow failed lookups.
+ bug = self.fetch_bug(attachment_dictionary["bug_id"])
+ for attachment in bug.attachments(include_obsolete=True):
+ if attachment.id() == int(attachment_id):
+ return attachment
- # NOTE: Functions below this are direct copies from bugzilla.py
- def fetch_patches_from_bug(self, bug_id):
- return self.fetch_bug(bug_id).patches()
-
def bug_url_for_bug_id(self, bug_id):
return "%s/%s" % (self.bug_server_url, bug_id)
def fetch_bug_dictionary(self, bug_id):
return self.bug_cache.get(bug_id)
- def attachment_url_for_id(self, attachment_id, action):
+ def attachment_url_for_id(self, attachment_id, action="view"):
action_param = ""
if action and action != "view":
action_param = "&action=%s" % action
diff --git a/WebKitTools/Scripts/webkitpy/scm.py b/WebKitTools/Scripts/webkitpy/scm.py
index 9a2fc4a..743f3fe 100644
--- a/WebKitTools/Scripts/webkitpy/scm.py
+++ b/WebKitTools/Scripts/webkitpy/scm.py
@@ -123,10 +123,11 @@ class SCM:
def apply_patch(self, patch, force=False):
# It's possible that the patch was not made from the root directory.
# We should detect and handle that case.
- curl_process = subprocess.Popen(['curl', '--location', '--silent', '--show-error', patch['url']], stdout=subprocess.PIPE)
+ # FIXME: scm.py should not deal with fetching Attachment data. Attachment should just have a .data() accessor.
+ curl_process = subprocess.Popen(['curl', '--location', '--silent', '--show-error', patch.url()], stdout=subprocess.PIPE)
args = [self.script_path('svn-apply')]
- if patch.get('reviewer'):
- args += ['--reviewer', patch['reviewer']]
+ if patch.reviewer():
+ args += ['--reviewer', patch.reviewer().full_name]
if force:
args.append('--force')
diff --git a/WebKitTools/Scripts/webkitpy/scm_unittest.py b/WebKitTools/Scripts/webkitpy/scm_unittest.py
index c68d367..73faf40 100644
--- a/WebKitTools/Scripts/webkitpy/scm_unittest.py
+++ b/WebKitTools/Scripts/webkitpy/scm_unittest.py
@@ -40,6 +40,7 @@ import urllib
from datetime import date
from webkitpy.executive import Executive, run_command, ScriptError
from webkitpy.scm import detect_scm_system, SCM, CheckoutNeedsUpdate, commit_error_handler
+from webkitpy.bugzilla import Attachment # FIXME: This should not be needed
# Eventually we will want to write tests which work for both scms. (like update_webkit, changed_files, etc.)
# Perhaps through some SCMTest base-class which both SVNTest and GitTest inherit from.
@@ -168,7 +169,7 @@ class SCMTest(unittest.TestCase):
patch['reviewer'] = 'Joe Cool'
patch['bug_id'] = '12345'
patch['url'] = 'file://%s' % urllib.pathname2url(patch_path)
- return patch
+ return Attachment(patch, None) # FIXME: This is a hack, scm.py shouldn't be fetching attachment data.
def _setup_webkittools_scripts_symlink(self, local_scm):
webkit_scm = detect_scm_system(os.path.dirname(os.path.abspath(__file__)))
diff --git a/WebKitTools/Scripts/webkitpy/statusserver.py b/WebKitTools/Scripts/webkitpy/statusserver.py
index cc2aeae..c17b162 100644
--- a/WebKitTools/Scripts/webkitpy/statusserver.py
+++ b/WebKitTools/Scripts/webkitpy/statusserver.py
@@ -54,10 +54,10 @@ class StatusServer:
def _add_patch(self, patch):
if not patch:
return
- if patch.get('bug_id'):
- self.browser['bug_id'] = str(patch['bug_id'])
- if patch.get('id'):
- self.browser['patch_id'] = str(patch['id'])
+ if patch.bug_id():
+ self.browser["bug_id"] = str(patch.bug_id())
+ if patch.id():
+ self.browser["patch_id"] = str(patch.id())
def _add_results_file(self, results_file):
if not results_file:
diff --git a/WebKitTools/Scripts/webkitpy/steps/applypatch.py b/WebKitTools/Scripts/webkitpy/steps/applypatch.py
index e1a1551..aba81ae 100644
--- a/WebKitTools/Scripts/webkitpy/steps/applypatch.py
+++ b/WebKitTools/Scripts/webkitpy/steps/applypatch.py
@@ -38,5 +38,5 @@ class ApplyPatch(AbstractStep):
]
def run(self, state):
- log("Processing patch %s from bug %s." % (state["patch"]["id"], state["patch"]["bug_id"]))
+ log("Processing patch %s from bug %s." % (state["patch"].id(), state["patch"].bug_id()))
self._tool.scm().apply_patch(state["patch"], force=self._options.non_interactive)
diff --git a/WebKitTools/Scripts/webkitpy/steps/applypatchwithlocalcommit.py b/WebKitTools/Scripts/webkitpy/steps/applypatchwithlocalcommit.py
index 01fab1a..bfaf52a 100644
--- a/WebKitTools/Scripts/webkitpy/steps/applypatchwithlocalcommit.py
+++ b/WebKitTools/Scripts/webkitpy/steps/applypatchwithlocalcommit.py
@@ -40,4 +40,4 @@ class ApplyPatchWithLocalCommit(ApplyPatch):
ApplyPatch.run(self, state)
if self._options.local_commit:
commit_message = self._tool.scm().commit_message_for_this_commit()
- self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"]["name"])
+ self._tool.scm().commit_locally_with_message(commit_message.message() or state["patch"].name())
diff --git a/WebKitTools/Scripts/webkitpy/steps/closebug.py b/WebKitTools/Scripts/webkitpy/steps/closebug.py
index aa4b213..2640ee3 100644
--- a/WebKitTools/Scripts/webkitpy/steps/closebug.py
+++ b/WebKitTools/Scripts/webkitpy/steps/closebug.py
@@ -43,10 +43,9 @@ class CloseBug(AbstractStep):
return
# Check to make sure there are no r? or r+ patches on the bug before closing.
# Assume that r- patches are just previous patches someone forgot to obsolete.
- patches = self._tool.bugs.fetch_patches_from_bug(state["patch"]["bug_id"])
+ patches = self._tool.bugs.fetch_bug(state["patch"].bug_id()).patches()
for patch in patches:
- review_flag = patch.get("review")
- if review_flag == "?" or review_flag == "+":
- log("Not closing bug %s as attachment %s has review=%s. Assuming there are more patches to land from this bug." % (patch["bug_id"], patch["id"], review_flag))
+ if patch.review() == "?" or patch.review() == "+":
+ log("Not closing bug %s as attachment %s has review=%s. Assuming there are more patches to land from this bug." % (patch.bug_id(), patch.id(), patch.review()))
return
- self._tool.bugs.close_bug_as_fixed(state["patch"]["bug_id"], "All reviewed patches have been landed. Closing bug.")
+ self._tool.bugs.close_bug_as_fixed(state["patch"].bug_id(), "All reviewed patches have been landed. Closing bug.")
diff --git a/WebKitTools/Scripts/webkitpy/steps/closebugforlanddiff.py b/WebKitTools/Scripts/webkitpy/steps/closebugforlanddiff.py
index 0d52fe2..25e7848 100644
--- a/WebKitTools/Scripts/webkitpy/steps/closebugforlanddiff.py
+++ b/WebKitTools/Scripts/webkitpy/steps/closebugforlanddiff.py
@@ -41,7 +41,8 @@ class CloseBugForLandDiff(AbstractStep):
def run(self, state):
comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
- bug_id = state["patch"]["bug_id"]
+ bug_id = state.get("bug_id") or state["patch"].bug_id()
+
if bug_id:
log("Updating bug %s" % bug_id)
if self._options.close_bug:
diff --git a/WebKitTools/Scripts/webkitpy/steps/closepatch.py b/WebKitTools/Scripts/webkitpy/steps/closepatch.py
index de1a563..f20fe7e 100644
--- a/WebKitTools/Scripts/webkitpy/steps/closepatch.py
+++ b/WebKitTools/Scripts/webkitpy/steps/closepatch.py
@@ -33,4 +33,4 @@ from webkitpy.steps.abstractstep import AbstractStep
class ClosePatch(AbstractStep):
def run(self, state):
comment_text = bug_comment_from_commit_text(self._tool.scm(), state["commit_text"])
- self._tool.bugs.clear_attachment_flags(state["patch"]["id"], comment_text)
+ self._tool.bugs.clear_attachment_flags(state["patch"].id(), comment_text)
diff --git a/WebKitTools/Scripts/webkitpy/steps/obsoletepatches.py b/WebKitTools/Scripts/webkitpy/steps/obsoletepatches.py
index d9c6431..dbdbabd 100644
--- a/WebKitTools/Scripts/webkitpy/steps/obsoletepatches.py
+++ b/WebKitTools/Scripts/webkitpy/steps/obsoletepatches.py
@@ -43,9 +43,9 @@ class ObsoletePatches(AbstractStep):
if not self._options.obsolete_patches:
return
bug_id = state["bug_id"]
- patches = self._tool.bugs.fetch_patches_from_bug(bug_id)
+ patches = self._tool.bugs.fetch_bug(bug_id).patches()
if not patches:
return
log("Obsoleting %s on bug %s" % (pluralize("old patch", len(patches)), bug_id))
for patch in patches:
- self._tool.bugs.obsolete_attachment(patch["id"])
+ self._tool.bugs.obsolete_attachment(patch.id())
diff --git a/WebKitTools/Scripts/webkitpy/steps/updatechangelogswithreviewer.py b/WebKitTools/Scripts/webkitpy/steps/updatechangelogswithreviewer.py
index fcb57e1..3c8797a 100644
--- a/WebKitTools/Scripts/webkitpy/steps/updatechangelogswithreviewer.py
+++ b/WebKitTools/Scripts/webkitpy/steps/updatechangelogswithreviewer.py
@@ -42,17 +42,17 @@ class UpdateChangeLogsWithReviewer(AbstractStep):
]
def _guess_reviewer_from_bug(self, bug_id):
- patches = self._tool.bugs.fetch_reviewed_patches_from_bug(bug_id)
+ patches = self._tool.bugs.fetch_bug(bug_id).reviewed_patches()
if len(patches) != 1:
log("%s on bug %s, cannot infer reviewer." % (pluralize("reviewed patch", len(patches)), bug_id))
return None
patch = patches[0]
- reviewer = patch["reviewer"]
- log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (reviewer, patch["id"], bug_id))
- return reviewer
+ log("Guessing \"%s\" as reviewer from attachment %s on bug %s." % (patch.reviewer().full_name, patch.id(), bug_id))
+ return patch.reviewer().full_name
def run(self, state):
- bug_id = state.get("bug_id") or state["patch"]["bug_id"]
+ bug_id = state.get("bug_id") or state["patch"].bug_id()
+
reviewer = self._options.reviewer
if not reviewer:
if not bug_id:
--
WebKit Debian packaging
More information about the Pkg-webkit-commits
mailing list