Have drover ask for additional reviewers if its reverting a change made by the user

drover.py

 #!/usr/bin/env python
 # 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.
 
 import optparse
 import os
 import re
 import string
 import sys
 import urllib2
+import urlparse
 
 import breakpad  # pylint: disable=W0611
 
 import gclient_utils
+import rietveld
 import subprocess2
 
 USAGE = """
 WARNING: Please use this tool in an empty directory
 (or at least one that you don't mind clobbering.)
 
 REQUIRES: SVN 1.5+
 NOTE: NO NEED TO CHECKOUT ANYTHING IN ADVANCE OF USING THIS TOOL.
 Valid parameters:
 
@@ skipping 388 matching lines @@
   # Not all of the platforms have the same branch. Prompt the user and return
   # the greatest (by value) branch on success.
   if prompt("Not all platforms have the same branch number, "
             "continue with branch %s?" % branch):
     return branch
 
   # User cancelled.
   return None
 
 
+def getSVNAuthInfo(folder=None):
+  """Fetches SVN authorization information in the subversion auth folder and
+  returns it as a dictionary of dictionaries."""
+  if not folder:
+    if sys.platform == 'win32':
+      folder = '%%APPDATA%\\Subversion\\auth'
+    else:
+      folder = '$HOME/.subversion/auth'
+  folder = os.path.expandvars(folder)

[Isaac (away), 2012/11/22 01:36:05]

if you use ~ instead of $HOME, you can call expandvars for the windows path, and
use realpath for the != windows case

+  svn_simple_folder = os.path.join(folder, 'svn.simple')
+  svn_simple_files = (os.listdir(svn_simple_folder) if 
+                      os.path.exists(svn_simple_folder) else [])

[Isaac (away), 2012/11/22 01:36:05]

This is a race condition on the folder or svn files getting removed. 
Recommended style is to try to list the directory (i.e. directly in the for
loop) without calling exists and catch exceptions if they occur, as you can
never prevent all OS exceptions with if blocks.

+  results = {}
+  for auth_file in svn_simple_files:
+    # Read the SVN auth file, convert it into a dictionary, and store it.
+    results[auth_file] = dict(re.findall(r'K [0-9]+\n(.*)\nV [0-9]+\n(.*)\n', 
+        open(os.path.join(svn_simple_folder, auth_file)).read()))
+  return results
+
+
+def getCurrentSVNUsers(url):
+  """Tries to fetch the current SVN in the current checkout by scanning the
+  SVN authorization folder for a match with the current SVN URL."""
+  netloc = urlparse.urlparse(url)[1]
+  auth_infos = getSVNAuthInfo()
+  results = []
+  for _, auth_info in auth_infos.iteritems():
+    if ('svn:realmstring' in auth_info 
+        and netloc in auth_info['svn:realmstring']):

[Isaac (away), 2012/11/22 01:36:05]

better to parse the netloc from the svn url and compare for equality.

+      username = auth_info['username']
+      results.append(username)
+      if 'google.com' in username:
+        results.append(username.replace('google.com', 'chromium.org'))
+  return results
+
+
 def prompt(question):
   while True:
     print question + " [y|n]:",
     answer = sys.stdin.readline()
     if answer.lower().startswith('n'):
       return False
     elif answer.lower().startswith('y'):
       return True
 
 
@@ skipping 96 matching lines @@
     revertExportRevision(url, revision)
 
   # Check the base url so we actually find the author who made the change
   if options.auditor:
     author = options.auditor
   else:
     author = getAuthor(url, revision)
     if not author:
       author = getAuthor(TRUNK_URL, revision)
 
+  # Check that the author of the CL is different than the user making
+  # the revert.  If they're the same, then we'll want to prompt the user
+  # for a different reviewer to TBR.
+  current_users = getCurrentSVNUsers(BASE_URL)

[Isaac (away), 2012/11/22 01:36:05]

can't we just run svn info in the drover checkout to get the current svn user?

[Ryan Tseng, 2012/11/29 23:27:35]

AFAIK SVN user cache information isn't given in any svn commands, including svn
info
On 2012/11/22 01:36:05, Isaac wrote:

+  if options.revert and author in current_users:
+    # Fetch the reverting CL's authors.
+    revision_log = getRevisionLog(BASE_URL, revision)
+    review_url_match = re.search("Review URL:\s*(.*)", revision_log)

[Isaac (away), 2012/11/22 01:36:05]

the rietveld integration is nice, but it might be preferable to prompt.  And in
fact, it might be better to always prompt (and give a list of the original
reviewers), because sometimes the original committer is not a full committer,
and they should not be LGTMing the revert (they should be CCed)

Definitely if we depend on rietveld we need a timeout, and the behavior there
would be to prompt so we'll need to implement the prompt behavior either way.

[Isaac (away), 2012/11/22 01:43:24]

Oh, this comment is slightly stale.  I wrote it and then read the prompting code
below and forgot to modify the comment.

But still think we should change drover to always prompt (defaulting to TBRing
the original author in non-self-reverts, perhaps)

+    if review_url_match:
+      review_url = review_url_match.groups(1)[-1]
+      parsed_review_url = urlparse.urlparse(review_url)
+      rietveld_url = parsed_review_url[1]  # Index 1 for network location.
+      issue = int(re.search("\d+", parsed_review_url[2]).group())
+      issue_info = rietveld.Rietveld(rietveld_url, 
+          None, None).get_issue_properties(issue, None)
+      cl_reviewers = (",".join(issue_info["reviewers"]) if "reviewers" in 

[Isaac (away), 2012/11/22 01:36:05]

See the code for CheckOwners in the presubmit_canned_checks.  Maybe move the
LGTM checking code into rietveld.py, and then drover and the presubmit checks
can both depend on it.  Note that their original CL might not have been LGTMed
(as is disproportionately the case with reverted changes)

+                      issue_info else "")
+    else:
+      cl_reviewers = ""
+    additional_authors = text_prompt("You appear to be reverting "

[Isaac (away), 2012/11/22 01:36:05]

appear might be too weak, how about

"Is this a self-revert?  Please enter a TBR reviewer.

+        "your own change, please enter additional reviewer(s) to TBR", 
+        cl_reviewers)

[Isaac (away), 2012/11/22 01:36:05]

Reviewer lists can be quite long, are you sure we want this as a default?

+    if additional_authors:
+      author += "," + additional_authors

[Isaac (away), 2012/11/22 01:36:05]

author of a self-revert should not be in the TBR list

+
   filename = str(revision)+".txt"
   out = open(filename,"w")
   out.write(action +" " + str(revision) + " - ")
-  out.write(getRevisionLog(url, revision))
+  for line in getRevisionLog(url, revision).split('\n'):

[Isaac (away), 2012/11/22 01:36:05]

use splitlines()

+    out.write('> %s\n' % line)

[nsylvain, 2012/11/21 23:10:51]

we should probably just use spaces instead of ">".  

   if (author):
     out.write("\nTBR=" + author)
   out.close()
 
   change_cmd = 'change ' + str(revision) + " " + filename
   if options.revertbot:
     if sys.platform == 'win32':
       os.environ['SVN_EDITOR'] = 'cmd.exe /c exit'
     else:
       os.environ['SVN_EDITOR'] = 'true'
@@ skipping 74 matching lines @@
 
   if options.branch and options.milestone:
     option_parser.error("--branch cannot be used with --milestone")
     return 1
 
   return drover(options, args)
 
 
 if __name__ == "__main__":
   sys.exit(main())