Add presubmit check for dcommitted / CQed CLs

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
+import re

[M-A Ruel, 2012/11/22 15:47:06]

why not input_api.re?

[Isaac (away), 2012/11/22 18:50:52]

I can do that -- is there a reason we're avoiding the python re class here?

[Dirk Pranke, 2012/11/22 18:54:07]

input_api.re would just be a pointer to the python module. You'll notice we
don't normally import anything directly into this file (I'm actually surprised
that "import os" has crept in). This makes it easier to mock out objects for
testing, and I seem to recall there's an actual reason to limit imports as well
(although that reason might've been obsolete).

[Isaac (away), 2012/11/25 02:12:44]

OK, fixed.

+
 _HERE = _os.path.dirname(_os.path.abspath(__file__))
 
 
 ### Description checks
 
 def CheckChangeHasTestField(input_api, output_api):
   """Requires that the changelist have a TEST= field."""
   if input_api.change.TEST:
     return []
   else:
@@ skipping 21 matching lines @@
 
 def CheckChangeHasQaField(input_api, output_api):
   """Requires that the changelist have a QA= field."""
   if input_api.change.QA:
     return []
   else:
     return [output_api.PresubmitError('Changelist must have a QA= field.')]
 
 
 def CheckDoNotSubmitInDescription(input_api, output_api):
-  """Checks that the user didn't add 'DO NOT ' + 'SUBMIT' to the CL description.
+  """Checks that the user didn't add 'DO NOT SUBMIT' to the CL description.

[M-A Ruel, 2012/11/22 15:47:06]

Why? This would block the CQ from processing this file.

[Isaac (away), 2012/11/22 18:50:52]

I didn't realize that was the reason for splitting this up.  OK.  We can use
pythons string concatenation and change to 'DO NOT ''SUBMIT', which is preferred
python style.

[Isaac (away), 2012/11/25 02:12:44]

Fixed.

   """
-  keyword = 'DO NOT ' + 'SUBMIT'
+  keyword = 'DO NOT SUBMIT'
   if keyword in input_api.change.DescriptionText():
     return [output_api.PresubmitError(
         keyword + ' is present in the changelist description.')]
   else:
     return []
 
 
 def CheckChangeHasDescription(input_api, output_api):
   """Checks the CL description is not empty."""
   text = input_api.change.DescriptionText()
   if text.strip() == '':
     if input_api.is_committing:
       return [output_api.PresubmitError('Add a description.')]
     else:
       return [output_api.PresubmitNotifyResult('Add a description.')]
   return []
 
 
+def CheckChangeDescriptionNotCommitted(input_api, output_api):
+  """Checks that the CL does not end with a Committed link."""
+  description = input_api.change.DescriptionText()
+  if re.match('\nCommitted: \S+$', description):

[M-A Ruel, 2012/11/22 15:47:06]

You could use ^ instead with re.search(.., re.MULTILINE)

Also, regexp string should be of r type.

[Isaac (away), 2012/11/22 18:50:52]

I only want to match Committed on the last line, so multiline wouldn't work.  A
revert which quotes the original commit might have a "Committed" line from the
original commit, but not as the last line.

+    return [output_api.PresubmitError(
+      'The CL description appears to end with a committed link. If this issue\n'
+      'has been previously used for a commit, please make a new issue number\n'
+      'by typing "git cl issue 0" and reuploading your CL.')]
+  return []
+
+
 def CheckChangeWasUploaded(input_api, output_api):
   """Checks that the issue was uploaded before committing."""
   if input_api.is_committing and not input_api.change.issue:
     return [output_api.PresubmitError(
       'Issue wasn\'t uploaded. Please upload first.')]
   return []
 
 
 ### Content checks
 
 def CheckDoNotSubmitInFiles(input_api, output_api):
-  """Checks that the user didn't add 'DO NOT ' + 'SUBMIT' to any files."""
+  """Checks that the user didn't add 'DO NOT SUBMIT' to any files."""
   # We want to check every text file, not just source files.
   file_filter = lambda x : x
-  keyword = 'DO NOT ' + 'SUBMIT'
+  keyword = 'DO NOT SUBMIT'
   errors = _FindNewViolationsOfRule(lambda _, line : keyword not in line,
                                     input_api, file_filter)
   text = '\n'.join('Found %s in %s' % (keyword, loc) for loc in errors)
   if text:
     return [output_api.PresubmitError(text)]
   return []
 
 
 def CheckChangeLintsClean(input_api, output_api, source_file_filter=None):
   """Checks that all '.cc' and '.h' files pass cpplint.py."""
@@ skipping 869 matching lines @@
         input_api, output_api, source_file_filter=text_files))
     snapshot("checking svn mime types")
     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("checking description")
+    results.extend(input_api.canned_checks.CheckChangeHasDescription(
+        input_api, output_api))
+    results.extend(input_api.canned_checks.CheckDoNotSubmitInDescription(
+        input_api, output_api))
+    results.extend(input_api.canned_checks.CheckChangeDescriptionNotCommitted(
+        input_api, output_api))
+    snapshot("checking do not submit in files")
+    results.extend(input_api.canned_checks.CheckDoNotSubmitInFiles(
+        input_api, output_api))
   snapshot("done")
   return results