CQ to always post a message when Commit box is unchecked.

pending_manager.py

 # coding=utf8
 # Copyright (c) 2012 The Chromium Authors. All rights reserved.
 # Use of this source code is governed by a BSD-style license that can be
 # found in the LICENSE file.
 """Commit queue manager class.
 
 Security implications:
 
 The following hypothesis are made:
 - Commit queue:
@@ skipping 179 matching lines @@
 
       raise
 
     # If there is an issue in processed_issues that is not in new_issues,
     # discard it.
     for pending in self.queue.iterate():
       # Note that pending.issue is a int but self.queue.pending_commits keys
       # are str due to json support.
       if pending.issue not in new_issues:
         logging.info('Flushing issue %d' % pending.issue)
-        self.context.status.send(
-            pending,
-            { 'verification': 'abort',
-              'payload': {
-                'output': 'CQ bit was unchecked on CL. Ignoring.' }})
         pending.get_state = lambda: base.IGNORED
-        self._discard_pending(pending, None)
+        self._discard_pending(pending, 'CQ bit was unchecked on CL. Ignoring.')
 
     # Find new issues.
     for issue_id in new_issues:
       if str(issue_id) not in self.queue.pending_commits:
         try:
           issue_data = self.context.rietveld.get_issue_properties(
               issue_id, True)
         except urllib2.HTTPError as e:
           if e.code in (500, 502, 503):
             # Temporary AppEngine hiccup. Just log it and continue.
@@ skipping 47 matching lines @@
         # Take in account the case where a verifier was removed.
         done = set(pending.verifications.keys())
         missing = expected - done
         if (not missing or pending.get_state() != base.PROCESSING):
           continue
         logging.info(
             'Processing issue %s (%s, %d)' % (
                 pending.issue, missing, pending.get_state()))
         self._verify_pending(pending)
       except base.DiscardPending as e:
-        self._discard_pending(e.pending, e.status)
+        self._discard_pending(e.pending, e.status or

[Paweł Hajdan Jr., 2014/01/28 01:13:46]

nit: Indentation in these "or" expressions might be confusing.

How about just constructing the actual message first, and then just using
variable name in _discard_pending, e.g.

message = e.status or ...
self._discard_pending(... , message)

Applies to all lines where currently "or" is at the end of the line.

+                'process_new_pending_commit: ' + self.FAILED_NO_MESSAGE)
 
   def update_status(self):
     """Updates the status for each pending commit verifier."""
     why_nots = dict((p.issue, p.why_not()) for p in self.queue.iterate())
 
     for verifier in self.all_verifiers:
       try:
         verifier.update_status(self.queue.iterate())
       except base.DiscardPending as e:
         # It's not efficient since it takes a full loop for each pending
         # commit to discard.
-        self._discard_pending(e.pending, e.status)
+        self._discard_pending(e.pending, e.status or
+                              'update_status: ' + self.FAILED_NO_MESSAGE)
 
     for pending in self.queue.iterate():
       why_not = pending.why_not()
       if why_nots[pending.issue] != why_not:
         self.context.status.send(
             pending,
             {'verification': 'why not',
              'payload': {'message': why_not}})
 
 
   def scan_results(self):
     """Scans pending commits that can be committed or discarded."""
     for pending in self.queue.iterate():
       state = pending.get_state()
       if state == base.FAILED:
         self._discard_pending(
-            pending, pending.error_message() or self.FAILED_NO_MESSAGE)
+            pending, pending.error_message() or
+            'scan_results(FAILED): ' + self.FAILED_NO_MESSAGE)
       elif state == base.SUCCEEDED:
         if self._throttle(pending):
           continue
         try:
           # Runs checks. It's be nice to run the test before the postpone,
           # especially if the tree is closed for a long moment but at the same
           # time it would keep fetching the rietveld status constantly.
           self._last_minute_checks(pending)
           self.context.status.send(
               pending,
               {'verification': 'why not',
                'payload': {'message': ''}})
 
           self._commit_patch(pending)
         except base.DiscardPending as e:
-          self._discard_pending(e.pending, e.status)
+          self._discard_pending(e.pending, e.status or
+                  'scan_results(commit): ' + self.FAILED_NO_MESSAGE)
         except Exception as e:
           self._discard_pending(pending, self.INTERNAL_EXCEPTION)
           raise
       else:
         # When state is IGNORED, we need to keep this issue so it's not fetched
         # another time but we can't discard it since we don't want to remove the
         # commit bit for another project hosted on the same code review
         # instance.
         assert state in (base.PROCESSING, base.IGNORED)
 
@@ skipping 86 matching lines @@
           pending,
           'List of reviewers changed. %s did a drive-by without LGTM\'ing!' %
           ','.join(drivers_by))
     if pending.patchset != pending_data['patchsets'][-1]:
       raise base.DiscardPending(pending,
           'Commit queue failed due to new patchset.')
 
   def _discard_pending(self, pending, message):
     """Discards a pending commit. Attach an optional message to the review."""
     logging.debug('_discard_pending(%s, %s)', pending.issue, message)
+    msg = message or self.FAILED_NO_MESSAGE
     try:
       try:
         if pending.get_state() != base.IGNORED:
           self.context.rietveld.set_flag(
               pending.issue, pending.patchset, 'commit', False)
       except urllib2.HTTPError as e:
         logging.error(
             'Failed to set the flag to False for %s with message %s' % (
-              pending.pending_name(), message))
+              pending.pending_name(), msg))
         traceback.print_stack()
         logging.error(str(e))
         errors.send_stack(e)
-      if message:
-        try:
-          self.context.rietveld.add_comment(pending.issue, message)
-        except urllib2.HTTPError as e:
-          logging.error(
-              'Failed to add comment for %s with message %s' % (
-                pending.pending_name(), message))
-          traceback.print_stack()
-          errors.send_stack(e)
-        self.context.status.send(
-            pending,
-            { 'verification': 'abort',
-              'payload': {
-                'output': message }})
+      try:
+        self.context.rietveld.add_comment(pending.issue, msg)
+      except urllib2.HTTPError as e:
+        logging.error(
+            'Failed to add comment for %s with message %s' % (
+              pending.pending_name(), msg))
+        traceback.print_stack()
+        errors.send_stack(e)
+      self.context.status.send(
+          pending,
+          { 'verification': 'abort',
+            'payload': {
+              'output': msg }})
     finally:
       # Most importantly, remove the PendingCommit from the queue.
       self.queue.remove(pending.issue)
 
   def _commit_patch(self, pending):
     """Commits the pending patch to the repository.
 
     Do the checkout and applies the patch.
     """
     try:
@@ skipping 57 matching lines @@
         self.queue.remove(pending.issue)
       except (
           checkout.PatchApplicationFailed, patch.UnsupportedPatchFormat) as e:
         raise base.DiscardPending(pending, str(e))
       except subprocess2.CalledProcessError as e:
         stdout = getattr(e, 'stdout', None)
         out = 'Failed to apply the patch.'
         if stdout:
           out += '\n%s' % stdout
         raise base.DiscardPending(pending, out)
+      # Apply default message here

[Paweł Hajdan Jr., 2014/01/28 01:13:46]

nit: This comment line seems to be out of place - maybe just remove it.

     except base.DiscardPending as e:
-      self._discard_pending(e.pending, e.status)
+      self._discard_pending(e.pending, e.status or
+                            '_commit_patch: ' + self.FAILED_NO_MESSAGE)
 
   def _throttle(self, pending):
     """Returns True if a commit should be delayed."""
     if pending.postpone():
       self.context.status.send(
           pending,
           {'verification': 'why not',
            'payload': {
                'message': pending.why_not()}})
       return True
@@ skipping 19 matching lines @@
     """Loads the commit queue state from a JSON file."""
     self.queue = model.load_from_json_file(filename)
 
   def save(self, filename):
     """Save the commit queue state in a simple JSON file."""
     model.save_to_json_file(filename, self.queue)
 
   def close(self):
     """Close all the active pending manager items."""
     self.context.status.close()