An interactive tool to help find owners covering current change list.

find_owners.py

Index: find_owners.py
diff --git a/find_owners.py b/find_owners.py
new file mode 100644
index 0000000000000000000000000000000000000000..5ba8af2c40d064477f1d6ac9b69bbeb9ac9223fe
--- /dev/null
+++ b/find_owners.py
@@ -0,0 +1,312 @@
+#!/usr/bin/python
+# Copyright (c) 2011 The Chromium Authors. All rights reserved.

[Dirk Pranke, 2013/04/03 00:54:16]

2013.

[Bei Zhang, 2013/04/08 17:33:38]

Done.

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+'''A tool to help picking owner_to_files for reviewing.'''
+
+import git_cl
+import os.path
+import sys
+import copy
+import glob
+import owners as owners_module
+
+
+def first(iterable):

[Dirk Pranke, 2013/04/03 00:54:16]

I wouldn't use this function, it's easier to just say iterable[0] down below.

[Bei Zhang, 2013/04/08 17:33:38]

It's for unordered set though. Maybe I should use iterable.__iter__().next()?

On 2013/04/03 00:54:16, Dirk Pranke wrote:

+  for element in iterable:
+    return element
+
+
+class FindOwners:

[Dirk Pranke, 2013/04/03 00:54:16]

class FindOwners(object):

[Bei Zhang, 2013/04/08 17:33:38]

Done.

+  COLOR_LINK = '\033[4m'
+  COLOR_BOLD = '\033[1;32m'
+  COLOR_GREY = '\033[0;37m'
+  COLOR_RESET = '\033[0m'
+  def __init__(self, files, local_root, disable_color=False):

[Dirk Pranke, 2013/04/03 00:54:16]

generally speaking, this method is too long, and it's bad still to do so much
work in a constructor. Can you split this up?

[Bei Zhang, 2013/04/08 17:33:38]

Done.

+    # Files in a change list
+    self.owner_to_files = owner_to_files = {}
+
+    self.file_to_owners = file_to_owners = {}
+
+    if os.name == "nt" or disable_color:
+      self.COLOR_LINK = ''
+      self.COLOR_BOLD = ''
+      self.COLOR_GREY = ''
+      self.COLOR_RESET = ''
+
+    db = owners_module.Database(local_root,
+        fopen=file, os_path=os.path, glob=glob.glob)
+    db.reviewers_for(files, None)
+
+    for file_name in files:
+      owners_set = set()
+      if file_name in db.owners_for:
+        owners_set = owners_set | db.owners_for[file_name]
+      else:
+        dir_name = file_name
+        while dir_name != '':
+          if dir_name in db.stop_looking:
+            break
+          dir_name = os.path.dirname(dir_name)
+          if dir_name in db.owners_for:
+            owners_set = owners_set | db.owners_for[dir_name]
+            if owners_module.EVERYONE in owners_set:
+              break

[Dirk Pranke, 2013/04/03 00:54:16]

can we just use db._all_possible_owners() here (you'd presumably through away
the cost values of the tuples)? If not, we should change _all_possible_owners()
to make this work, so that we're not duplicating this loop. 

[Bei Zhang, 2013/04/08 17:33:38]

I was a little bit scared of the leading underscore here. I'm not familiar with
the language though (as you may already notice), maybe it's okay to call them?
Also, to make _all_possible_owners work, we also need to call _check_paths,
_load_data_needed_for and _enclosing_dir_with_owners methods.

On 2013/04/03 00:54:16, Dirk Pranke wrote:

+
+      if len(owners_set) == 0:
+        raise Exception("File '%s' has no owner" % file_name)
+
+      # Eliminate files that EVERYONE can review
+      if owners_module.EVERYONE in owners_set:
+        continue
+      file_to_owners[file_name] = owners_set
+
+    self.original_files_to_owners = copy.deepcopy(file_to_owners)
+
+    for owner_name in db.owned_by:
+      if owner_name == owners_module.EVERYONE:
+        continue
+      files_set = set()
+      for file_name in file_to_owners:
+        if owner_name in file_to_owners[file_name]:
+          files_set.add(file_name)
+      if len(files_set) > 0:
+        owner_to_files[owner_name] = files_set

[Dirk Pranke, 2013/04/03 00:54:16]

it's unfortunate that we can't use just db.owned_by here. Can you pull this loop
into a separate function? (Maybe we can eventually move it into owners.py).

[Bei Zhang, 2013/04/24 00:13:08]

Done.

+
+    self.comments = db.comments
+    self.unreviewedFiles = set()
+    self.reviewedBy = {}
+    self.usedOwners = set()
+    self.unusedOwners = set()
+
+  def reset(self):
+    self.file_to_owners = copy.deepcopy(self.original_files_to_owners)
+    self.unreviewedFiles = set(self.file_to_owners.keys())
+    self.reviewedBy = {}
+    self.usedOwners = set()
+    self.unusedOwners = set()
+
+  def bold(self, text):
+    return (self.COLOR_BOLD + text + self.COLOR_RESET)
+
+  def bold_name(self, name):
+    return (self.COLOR_BOLD +
+            name.replace("@chromium.org", "") + self.COLOR_RESET)
+
+  def greyed(self, text):
+    return self.COLOR_GREY + text + self.COLOR_RESET
+
+  def unuseOwner(self, owner):
+    self.unusedOwners.add(owner)
+    for file_name in self.owner_to_files[owner]:
+      if file_name in self.unreviewedFiles:
+        self.file_to_owners[file_name].remove(owner)
+    self.findMustPickOwners()
+
+  def useOwner(self, owner):
+    self.usedOwners.add(owner)
+    for file_name in self.owner_to_files[owner]:
+      if file_name in self.unreviewedFiles:
+        self.unreviewedFiles.remove(file_name)
+        self.reviewedBy[file_name] = owner
+    self.findMustPickOwners()
+
+  def findMustPickOwners(self):

[Dirk Pranke, 2013/04/03 00:54:16]

so "findMustPickOwners" selects all the people that are the only option for a
particular file in the list?

I'd rename this to findMandatoryOwners and/or add a docstring.

[Bei Zhang, 2013/04/08 17:33:38]

Done. Sorry for my bad English.

On 2013/04/03 00:54:16, Dirk Pranke wrote:

+    continues = True
+    while continues:
+      continues = False
+      for file_name in self.unreviewedFiles:
+        if len(self.file_to_owners[file_name]) == 1:
+          self.useOwner(first(self.file_to_owners[file_name]))
+          continues = True
+          break

[Dirk Pranke, 2013/04/03 00:54:16]

Can you rewrite this as a loop over a filtered list of files, e.g.:

for filename in filter(lambda filename: len(self.file_to_owners[filename] == 1,
                                 self.unreviewedFiles):
  self.useOwner(self.file_to_owners[filename][0])

[Bei Zhang, 2013/04/08 17:33:38]

Done.

+
+  def printComments(self, owner):
+    if owner not in self.comments:
+      print self.bold_name(owner)
+    else:
+      print self.bold_name(owner) + " is commented as:"
+      for path in self.comments[owner]:
+        if len(self.comments[owner][path]) > 0:
+          print (self.greyed("  " + self.comments[owner][path]) + " (at " +
+            self.bold(path or "<root>") + ")")
+        else:
+          print (self.greyed("  [No comment] ") + " (at " +
+            self.bold(path or "<root>") + ")")
+
+  def printOwnedFiles(self, owner):
+    # Print owned files
+    self.printComments(owner)
+    print
+    print (self.bold_name(owner) + " owns " +
+           str(len(self.owner_to_files[owner])) + " file(s):")
+    for file_name in sorted(self.owner_to_files[owner]):
+      if file_name not in self.unreviewedFiles:
+        print "  " + self.greyed(file_name + ' (by ' +
+                                 self.bold_name(self.reviewedBy[file_name]) +
+                                 ')')
+      else:
+        if len(self.file_to_owners[file_name]) <= 3:
+          other_owners = []
+          for ow in self.file_to_owners[file_name]:
+            if ow != owner:
+              other_owners.append(self.bold_name(ow))
+          print ("  " + file_name +
+            " [" + (", ".join(other_owners)) + "]")
+        else:
+          print ("  " + file_name +
+           " [" + self.bold(str(len(self.file_to_owners[file_name]) - 1)) + "]")
+    print
+
+  def listOwners(self, owners_queue):
+    if (len(self.owner_to_files) - len(self.unusedOwners) -
+        len(self.usedOwners)) > 3:
+      for ow in owners_queue:
+        if ow not in self.unusedOwners and ow not in self.usedOwners:
+          self.printComments(ow)
+    else:
+      for ow in owners_queue:
+        if ow not in self.unusedOwners and ow not in self.usedOwners:
+          self.printOwnedFiles(ow)
+
+  def listFiles(self):
+    if len(self.unreviewedFiles) > 5:
+      for file_name in sorted(self.unreviewedFiles):
+        print (file_name + " [" +
+               self.bold(str(len(self.file_to_owners[file_name]))) + "]")
+    else:
+      for file_name in self.unreviewedFiles:
+        print file_name
+        for ow in sorted(self.file_to_owners[file_name]):
+          if ow in self.unusedOwners:
+            print "  " + self.bold_name(self.greyed(ow))
+          elif ow in self.usedOwners:
+            print "  " + self.bold_name(self.greyed(ow))
+          else:
+            print "  " + self.bold_name(ow)
+
+  def pickOwner(self, ow = False):
+    if not ow:
+      ow = raw_input("Pick an owner:")
+
+    # Allowing to omit domain suffixes
+    if ow not in self.owner_to_files:
+      if ow + "@chromium.org" in self.owner_to_files:
+        ow += "@chromium.org"
+
+    if ow not in self.owner_to_files:
+      print ("You cannot pick " + self.bold_name(ow) + " manually. " +
+            "It's an invalid name or not related to the change list.")
+      return False
+    elif ow in self.usedOwners:
+      print ("You cannot pick " + self.bold_name(ow) + " manually. " +
+            "It's already selected.")
+      return False
+    elif ow in self.unusedOwners:
+      print ("You cannot pick " + self.bold_name(ow) + " manually." +
+            "It's already unselected.")
+      return False
+    elif not any(f in self.unreviewedFiles for f in self.owner_to_files[ow]):
+      while True:
+        inp = raw_input("Picking " + self.bold_name(ow) +
+                        " does not cover any unreviewed file. "
+                        "Are you sure? [yes/No]").lower()
+        if inp == "" or inp == "n" or inp == "no":
+          return False
+        elif inp == "y" or inp == "yes":
+          break
+
+    self.useOwner(ow)
+    return True
+
+  def runOnce(self):

[Dirk Pranke, 2013/04/03 00:54:16]

Can you document what runOnce() and run() are doing? I'm guessing that runOnce
is handling one particular input from the user, but these names are not giving
me very many clues.

[Bei Zhang, 2013/04/08 17:33:38]

Done. It was for a hack to restart the run(). Removed.

On 2013/04/03 00:54:16, Dirk Pranke wrote:

+    self.reset()
+
+    # Initialize owners queue, sort it by the number of files
+    # each owns
+    owners_queue = list(sorted(self.owner_to_files.keys(),
+                        key=lambda owner: len(self.owner_to_files[owner]),
+                        reverse=True))
+
+    self.findMustPickOwners()
+    while len(owners_queue) > 0 and len(self.unreviewedFiles) > 0:
+      owner = owners_queue[0]
+      try:
+        if owner in self.usedOwners:
+          continue
+        if len(self.unreviewedFiles) == 0:
+          print "Finished.\n\n"
+          break
+        if owner in self.unusedOwners:
+          # If this owner is already deseleted.

[Dirk Pranke, 2013/04/03 00:54:16]

deselected?

[Bei Zhang, 2013/04/08 17:33:38]

That is, when you select "no" on this owner.

So every owner has 3 statuses: selected, deselected and deferred. You can defer
a decision and the owner will be shown again at the end of the queue.

On 2013/04/03 00:54:16, Dirk Pranke wrote:

+          continue
+        if not any((file_name in self.unreviewedFiles)
+                   for file_name in self.owner_to_files[owner]):
+          self.unuseOwner(owner)
+          continue
+        print "====================="
+        print self.bold(str(len(self.unreviewedFiles))) + " file(s) left."
+        self.printOwnedFiles(owner)
+        while True:
+          print "Add " + self.bold_name(owner) + " as your reviewer? "
+          inp = raw_input(
+            "[yes/no/Defer/pick/files/owners/quit/restart]: ").lower()
+          if inp == "y" or inp == "yes":
+            self.useOwner(owner)
+            break
+          elif inp == "n" or inp == "no":
+            self.unuseOwner(owner)
+            break
+          elif inp == "" or inp == "d" or inp == "defer":
+            owners_queue.append(owner)
+            break
+          elif inp == "f" or inp == "files":
+            self.listFiles()
+          elif inp == "o" or inp == "owners":
+            self.listOwners(owners_queue)
+          elif inp == "p" or inp == "pick":
+            self.pickOwner()
+            owners_queue.insert(0, owner)
+            break
+          elif inp.startswith("p ") or inp.startswith("pick "):
+            self.pickOwner(inp.split(' ', 2)[1])
+            owners_queue.insert(0, owner)
+            break
+          elif inp == 'r' or inp == 'restart':
+            return -1
+          elif inp == "q" or inp == "quit":
+            # Exit with error
+            return 1
+      finally:
+        if len(owners_queue):
+          owners_queue.pop(0)
+    # End of queue
+
+    # Print results
+    print "** You picked these owners **"
+    for owner in self.usedOwners:
+      print self.bold_name(owner) + ":"
+      for file_name in sorted(self.owner_to_files[owner]):
+        print "  " + file_name
+    return 0
+
+  def run(self):
+    result = -1
+    while result == -1:
+      result = self.runOnce()
+    return result
+
+
+def main():

[Dirk Pranke, 2013/04/03 00:54:16]

Do you actually imagine this being used as a separate standalone script as well
as part of git-cl ? I'd probably delete lines 300- just to simplify the number
of things you need to support.

[Bei Zhang, 2013/04/08 17:33:38]

Done.

+  cl = git_cl.Changelist()
+  local_root = os.path.abspath(
+      git_cl.RunGit(['rev-parse', '--show-cdup']).strip() or '.')
+  base_branch = git_cl.RunGit(
+      ['merge-base', cl.GetUpstreamBranch(), 'HEAD']).strip()
+  changes = cl.GetChange(base_branch, None)
+  files = [f.LocalPath() for f in changes.AffectedFiles()]
+  return FindOwners(files, local_root).run()
+
+
+if __name__ == "__main__":
+  sys.exit(main())