Clean up the parsing of approvals for OWNERS checks.

presubmit_canned_checks.py

Index: presubmit_canned_checks.py
diff --git a/presubmit_canned_checks.py b/presubmit_canned_checks.py
index 29ee9c0479c1de6d044735f51ac02f6ba94fe3a9..3d88edc12142f7ca5d6877e17768c669f6e5dc8e 100644
--- a/presubmit_canned_checks.py
+++ b/presubmit_canned_checks.py
@@ -626,35 +626,37 @@ def CheckBuildbotPendingBuilds(input_api, output_api, url, max_pendings,
   return []
 
 
-def CheckOwners(input_api, output_api, email_regexp=None,
-    source_file_filter=None):
+def CheckOwners(input_api, output_api, source_file_filter=None):
+  if not input_api.is_committing:
+    return []
+  if input_api.tbr:
+    return [output_api.PresubmitNotifyResult('--tbr was specified, '
+                                             'skipping OWNERS check')]
+  if not input_api.change.issue:
+    return [output_api.PresubmitError('Change not uploaded for review')]
+
   affected_files = set([f.LocalPath() for f in
       input_api.change.AffectedFiles(source_file_filter)])
-  owners_db = input_api.owners_db
-  if email_regexp:
-    owners_db.email_regexp = input_api.re.compile(email_regexp)
-
-  if input_api.is_committing and input_api.tbr:
-    return [output_api.PresubmitNotifyResult(
-        '--tbr was specified, skipping OWNERS check')]
-  elif input_api.is_committing:
-    approvers = _Approvers(input_api, owners_db.email_regexp)
-    missing_files = owners_db.files_not_covered_by(affected_files, approvers)
-    if missing_files:
-      return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' %
-              ','.join(missing_files))]
-    return []
-  elif input_api.change.tags.get('R'):
-    return []
 
-  suggested_reviewers = owners_db.reviewers_for(affected_files)
-  return [output_api.PresubmitAddReviewers(suggested_reviewers)]
+  owners_db = input_api.owners_db
+  owner, approvers = _RietveldOwnerAndApprovers(input_api,
+                                                owners_db.email_regexp)
+  approvers_plus_owner = approvers.union(set([owner]))
+
+  missing_files = owners_db.files_not_covered_by(affected_files,
+                                                 approvers_plus_owner)
+  if missing_files:
+    return [output_api.PresubmitError('Missing LGTM from an OWNER for: %s' %
+                                      ','.join(missing_files))]
+
+  if not approvers:
+    return [output_api.PresubmitError('Missing LGTM from someone other than '

[M-A Ruel, 2011/03/24 15:09:23]

nit: I prefer this style
return [output_api.PresubmitError(
    'Missing LGTM from someone other than %s' % owner)]

First, it doesn't throw is owner is a list, a None or Whatever else error could
happen.
Then it's easier to read the string if it's on one line.

+                                      + owner)]
+  return []
 
 
-def _Approvers(input_api, email_regexp):
-  if not input_api.change.issue:
-    return []
-
+def _RietveldOwnerAndApprovers(input_api, email_regexp):
+  """Return the owner and approvers of a change, if any."""
   # TODO(dpranke): Should figure out if input_api.host_url is supposed to
   # be a host or a scheme+host and normalize it there.
   host = input_api.host_url
@@ -664,18 +666,46 @@ def _Approvers(input_api, email_regexp):
 
   f = input_api.urllib2.urlopen(url)
   issue_props = input_api.json.load(f)
-  owner = input_api.re.escape(issue_props['owner'])
+  messages = issue_props.get('messages', [])
+  owner = issue_props['owner']
+  owner_regexp = input_api.re.compile(input_api.re.escape(owner))
+  approvers = _GetApprovers(messages, email_regexp, owner_regexp)
+
+  return (owner, set(approvers))
+
+
+def _IsApprovingComment(text):
+  """Implements the logic for parsing a change comment for approval."""
+
+  # Any comment that contains a line where the first non-quoted text matches
+  # one of the four strings below is an approval.
+  #
+  # TODO(dpranke): implement support for "LGTM++" and rescinding approval.
+  # Also, this check is simpler than the check used inside Google, which
+  # requires the first non-quoted line to match one of the phrases exactly.
+  for l in text.splitlines():
+    l = l.strip().lower()
+    if (l.startswith('lgtm') or l.startswith('lg') or

[M-A Ruel, 2011/03/24 15:09:23]

I disagree in two ways:

I prefer to accept lgtm anywhere in the sentence. The only exception would be to
filter out lines starting with '>'.

I prefer to not accept lg, looks good or whatever else.

Follow up comments already remove the commit bit, I'm not sure it's useful to
affect owners check in any way.

+        l.startswith('looks good') or l.startswith('looks good to me')):
+      return True
+  return False
+
+
+def _GetApprovers(messages, email_regexp, owner_regexp):
+  """Returns the set of approvers for a change given the owner and messages.
+
+  Messages should be a list of dicts containing 'sender' and 'text' keys."""
 
   # TODO(dpranke): This mimics the logic in
   # /tools/commit-queue/verifiers/reviewer_lgtm.py
   # We should share the code and/or remove the check there where it is
   # redundant (since the commit queue also enforces the presubmit checks).
   def match_reviewer(r):
-    return email_regexp.match(r) and not input_api.re.match(owner, r)
+    return email_regexp.match(r) and not owner_regexp.match(r)
 
   approvers = []
-  for m in issue_props.get('messages', []):
-    if 'lgtm' in m['text'].lower() and match_reviewer(m['sender']):
-      approvers.append(m['sender'])
+  for m in messages:
+    sender = m['sender']
+    if _IsApprovingComment(m['text']) and match_reviewer(sender):
+      approvers.append(sender)
   return set(approvers)
-