Add presubmit check to verify issue is not closed.

presubmit_canned_checks.py

 # 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.
 
 """Generic presubmit checks that can be reused by other presubmit checks."""
 
 import os as _os
 _HERE = _os.path.dirname(_os.path.abspath(__file__))
 
 
@@ skipping 724 matching lines @@
       out.append('%s has %d build(s) pending' %
                   (builder_name, pending_builds_len))
   if out:
     return [output_api.PresubmitPromptWarning(
         'Build(s) pending. It is suggested to wait that no more than %d '
             'builds are pending.' % max_pendings,
         long_text='\n'.join(out))]
   return []
 
 
-def CheckOwners(input_api, output_api, source_file_filter=None):
+def CheckOwners(input_api, output_api, issue_props, source_file_filter=None):

[M-A Ruel, 2012/11/21 20:48:38]

https://code.google.com/p/chromium/source/search?q=CheckOwners

you'll break depot_tools and v8 presubmit checks.

[Isaac (away), 2012/11/22 06:53:15]

Changed to be an optional argument and moved to the last position to maintain
positional order.

   if input_api.is_committing:
     if input_api.tbr:
       return [output_api.PresubmitNotifyResult(
           '--tbr was specified, skipping OWNERS check')]
     if not input_api.change.issue:
       return [output_api.PresubmitError("OWNERS check failed: this change has "
           "no Rietveld issue number, so we can't check it for approvals.")]
     needed = 'LGTM from an OWNER'
     output = output_api.PresubmitError
   else:
     needed = 'OWNER reviewers'
     output = output_api.PresubmitNotifyResult
 
   affected_files = set([f.LocalPath() for f in
       input_api.change.AffectedFiles(file_filter=source_file_filter)])
 
   owners_db = input_api.owners_db
   owner_email, reviewers = _RietveldOwnerAndReviewers(
-      input_api,
+      issue_props,
       owners_db.email_regexp,
       approval_needed=input_api.is_committing)
 
   if owner_email:
     message = ''
     reviewers_plus_owner = reviewers.union(set([owner_email]))
   else:
     message = ('\nUntil the issue is uploaded, this list will include '
                'directories for which you \nare an OWNER.')
     owner_email = ''
     reviewers_plus_owner = set()
 
   missing_directories = owners_db.directories_not_covered_by(affected_files,
       reviewers_plus_owner)
   if missing_directories:
     output_list = [
         output('Missing %s for files in these directories:\n    %s%s' %
                (needed, '\n    '.join(missing_directories), message))]
     if not input_api.is_committing:
       suggested_owners = owners_db.reviewers_for(affected_files)
       output_list.append(output('Suggested OWNERS:\n    %s' %
                                 ('\n    '.join(suggested_owners))))
     return output_list
 
   if input_api.is_committing and not reviewers:
     return [output('Missing LGTM from someone other than %s' % owner_email)]
   return []
 
 
-def _RietveldOwnerAndReviewers(input_api, email_regexp, approval_needed=False):
+def _GetRietveldIssueProps(input_api, messages):
+  """Gets the issue properties from rietveld."""
+  issue = input_api.change.issue
+  if issue and input_api.rietveld:
+    return input_api.rietveld.get_issue_properties(

[M-A Ruel, 2012/11/21 20:48:38]

BTW, this could be cached in the interface instead. Roughyl, create a Rietveld
subclass that caches get_issue_properties return values.

[Isaac (away), 2012/11/22 06:53:15]

Good idea, but I'd like to punt on that.  Also it would require special
knowledge for this call because messages=True returns a super-set of the
messages=False info.

+        issue=issue, messages=messages)
+
+
+def CheckIssueNotClosed(issue_props, output_api):
+  """Verify issue is not closed.
+
+  We should not be working with a closed review. CQ and dcommit set this bit,
+  so it is a pretty good indicator of whether an issue has been committed.
+  """
+  if issue_props['closed']:
+    return [output_api.PresubmitError(
+        'Issue %s is closed.  If this issue was already used for a commit,\n'
+        'please reset the issue number associated with this branch with:\n'
+        'git cl issue 0\n' % issue_props['issue']
+        )]
+  return []
+
+
+def _RietveldOwnerAndReviewers(issue_props, email_regexp,
+                               approval_needed=False):
   """Return the owner and reviewers of a change, if any.
 
   If approval_needed is True, only reviewers who have approved the change
   will be returned.
   """
-  if not input_api.change.issue:
+  if not issue_props:
     return None, None
 
-  issue_props = input_api.rietveld.get_issue_properties(
-      int(input_api.change.issue), True)
   if not approval_needed:
     return issue_props['owner_email'], set(issue_props['reviewers'])
 
   owner_email = issue_props['owner_email']
 
   def match_reviewer(r):
     return email_regexp.match(r) and r != owner_email
 
   messages = issue_props.get('messages', [])
   approvers = set(
@@ skipping 113 matching lines @@
   snapshot_memory = []
   def snapshot(msg):
     """Measures & prints performance warning if a rule is running slow."""
     dt2 = input_api.time.clock()
     if snapshot_memory:
       delta_ms = int(1000*(dt2 - snapshot_memory[0]))
       if delta_ms > 500:
         print "  %s took a long time: %dms" % (snapshot_memory[1], delta_ms)
     snapshot_memory[:] = (dt2, msg)
 
+  issue_props = _GetRietveldIssueProps(input_api, messages=True)
   if owners_check:
     snapshot("checking owners")
     results.extend(input_api.canned_checks.CheckOwners(
-        input_api, output_api, source_file_filter=None))
+        input_api, output_api, issue_props, source_file_filter=None))
+
+  if issue_props:
+    snapshot("checking review not closed")
+    results.extend(input_api.canned_checks.CheckIssueNotClosed(
+        issue_props, output_api))
 
   snapshot("checking long lines")
   results.extend(input_api.canned_checks.CheckLongLines(
       input_api, output_api, source_file_filter=sources))
   snapshot( "checking tabs")
   results.extend(input_api.canned_checks.CheckChangeHasNoTabs(
       input_api, output_api, source_file_filter=sources))
   snapshot( "checking stray whitespace")
   results.extend(input_api.canned_checks.CheckChangeHasNoStrayWhitespace(
       input_api, output_api, source_file_filter=sources))
@@ skipping 14 matching lines @@
     results.extend(input_api.canned_checks.CheckSvnForCommonMimeTypes(
         input_api, output_api))
     snapshot("checking license")
     results.extend(input_api.canned_checks.CheckLicense(
         input_api, output_api, license_header, source_file_filter=sources))
     snapshot("checking was uploaded")
     results.extend(input_api.canned_checks.CheckChangeWasUploaded(
         input_api, output_api))
   snapshot("done")
   return results