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

drover.py

Index: drover.py
diff --git a/drover.py b/drover.py
index bb4de0f01a9a71c01b2a256461267c3779ad261b..6b97fd5a6cdbd661bf656c7a58f0f1c1d314c1d1 100755
--- a/drover.py
+++ b/drover.py
@@ -9,10 +9,12 @@ import re
 import string
 import sys
 import urllib2
+import urlparse
 
 import breakpad  # pylint: disable=W0611
 
 import gclient_utils
+import rietveld
 import subprocess2
 
 USAGE = """
@@ -421,6 +423,42 @@ def getBranchForMilestone(milestone):
   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]:",
@@ -537,10 +575,36 @@ def drover(options, args):
     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()