Use CLs more consistently instead of branch names

git_cl.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.
 
 # Copyright (C) 2008 Evan Martin <martine@danga.com>
 
 """A git-command for integrating reviews on Rietveld and Gerrit."""
 
 from distutils.version import LooseVersion
 from multiprocessing.pool import ThreadPool
 import base64
 import collections
 import glob
 import httplib
 import json
 import logging
 import optparse
 import os
-import Queue
 import re
 import stat
 import sys
-import tempfile
 import textwrap
 import time
 import traceback
 import urllib
 import urllib2
 import urlparse
 import uuid
 import webbrowser
 import zlib
 
@@ skipping 2854 matching lines @@
   return {
     'unsent': Fore.RED,
     'waiting': Fore.BLUE,
     'reply': Fore.YELLOW,
     'lgtm': Fore.GREEN,
     'commit': Fore.MAGENTA,
     'closed': Fore.CYAN,
     'error': Fore.WHITE,
   }.get(status, Fore.WHITE)
 
-def fetch_cl_status(branch, auth_config=None):
+def fetch_cl_status(cl, auth_config=None):

[tandrii(chromium), 2016/04/15 15:38:35]

auth_config isn't needed any more.

   """Fetches information for an issue and returns (branch, issue, status)."""
-  cl = Changelist(branchref=branch, auth_config=auth_config)
   url = cl.GetIssueURL()
   status = cl.GetStatus()
 
   if url and (not status or status == 'error'):
     # The issue probably doesn't exist anymore.
     url += ' (broken)'

[tandrii(chromium), 2016/04/15 15:38:35]

i think with your awesome refactoring, we can go further and remove this
function.

cl.GetIssueUrl() doesn't block, and can be computed on the fly in status_str
(line 3117). The logic for " (broken)" can be moved there too.

then the only remaining bit here is just calling cl.GetStatus(), which can be
used directly in line 2926.

[Clemens Hammacher, 2016/04/21 12:54:46]

Removing this function would conflict with this other CL:
https://codereview.chromium.org/1895463002
I will address the other comments though.

 
-  return (branch, url, status)
+  return (cl, url, status)
 
 def get_cl_statuses(
-    branches, fine_grained, max_processes=None, auth_config=None):
+    changes, fine_grained, max_processes=None, auth_config=None):
   """Returns a blocking iterable of (branch, issue, color) for given branches.
 
   If fine_grained is true, this will fetch CL statuses from the server.
   Otherwise, simply indicate if there's a matching url for the given branches.
 
   If max_processes is specified, it is used as the maximum number of processes
   to spawn to fetch CL status from the server. Otherwise 1 process per branch is
   spawned.
   """
   # Silence upload.py otherwise it becomes unwieldly.
   upload.verbosity = 0
 
   if fine_grained:
     # Process one branch synchronously to work through authentication, then
     # spawn processes to process all the other branches in parallel.
-    if branches:
-      fetch = lambda branch: fetch_cl_status(branch, auth_config=auth_config)
-      yield fetch(branches[0])
+    if changes:
+      fetch = lambda cl: fetch_cl_status(cl, auth_config=auth_config)

[tandrii(chromium), 2016/04/15 15:38:35]

so based on above comment, i propose to change this to:

fetch = lambda cl: (cl, cl.GetStatus())

+      yield fetch(changes[0])
 
-      branches_to_fetch = branches[1:]
+      changes_to_fetch = changes[1:]
       pool = ThreadPool(
-          min(max_processes, len(branches_to_fetch))
+          min(max_processes, len(changes_to_fetch))
               if max_processes is not None
-              else len(branches_to_fetch))
-      for x in pool.imap_unordered(fetch, branches_to_fetch):
+              else len(changes_to_fetch))
+      for x in pool.imap_unordered(fetch, changes_to_fetch):
         yield x
   else:
     # Do not use GetApprovingReviewers(), since it requires an HTTP request.
-    for b in branches:
-      cl = Changelist(branchref=b, auth_config=auth_config)
+    for c in changes:
+      cl = Changelist(branchref=c.GetBranch(), auth_config=auth_config)

[tandrii(chromium), 2016/04/15 15:38:35]

I think you can just use one from changes list, and if so, nit: s/c/cl

       url = cl.GetIssueURL()
-      yield (b, url, 'waiting' if url else 'error')
+      yield (c, url, 'waiting' if url else 'error')

[tandrii(chromium), 2016/04/15 15:38:35]

and based on refactoring suggestion for line 2926, this can become just:

yield (cl, 'waiting' if cl.GetIssueURL() else 'error')

 
 
 def upload_branch_deps(cl, args):
   """Uploads CLs of local branches that are dependents of the current branch.
 
   If the local branch dependency tree looks like:
   test1 -> test2.1 -> test3.1
                    -> test3.2
         -> test2.2 -> test3.3
 
@@ skipping 132 matching lines @@
       url = cl.GetIssueURL()
       if url:
         print url
     return 0
 
   branches = RunGit(['for-each-ref', '--format=%(refname)', 'refs/heads'])
   if not branches:
     print('No local branch found.')
     return 0
 
-  changes = (
+  changes = [
       Changelist(branchref=b, auth_config=auth_config)
-      for b in branches.splitlines())
-  # TODO(tandrii): refactor to use CLs list instead of branches list.
-  branches = [c.GetBranch() for c in changes]
-  alignment = max(5, max(len(b) for b in branches))
+      for b in branches.splitlines()]
   print 'Branches associated with reviews:'
-  output = get_cl_statuses(branches,
+  output = get_cl_statuses(changes,
                            fine_grained=not options.fast,
                            max_processes=options.maxjobs,
                            auth_config=auth_config)
 
   branch_statuses = {}
-  alignment = max(5, max(len(ShortBranchName(b)) for b in branches))
-  for branch in sorted(branches):
+  alignment = max(5, max(len(ShortBranchName(c.GetBranch())) for c in changes))
+  for cl in sorted(changes, key = lambda c : c.GetBranch()):

[tandrii(chromium), 2016/04/15 15:38:35]

nits: no space before ":" and no space around "=" for key-value arguments.

=>
for cl in sorted(changes, key=lambda c: c.GetBranch()):

+    branch = cl.GetBranch()
     while branch not in branch_statuses:
-      b, i, status = output.next()
-      branch_statuses[b] = (i, status)
+      c, i, status = output.next()
+      branch_statuses[c.GetBranch()] = (i, status)
     issue_url, status = branch_statuses.pop(branch)
     color = color_for_status(status)
     reset = Fore.RESET
     if not setup_color.IS_TTY:
       color = ''
       reset = ''
     status_str = '(%s)' % status if status else ''
     print '  %*s : %s%s %s%s' % (
-          alignment, ShortBranchName(branch), color, issue_url, status_str,
-          reset)
+          alignment, ShortBranchName(branch), color, issue_url,
+          status_str, reset)
 
   cl = Changelist(auth_config=auth_config)
   print
   print 'Current branch:',
   print cl.GetBranch()
   if not cl.GetIssue():
     print 'No issue assigned.'
     return 0
   print 'Issue number: %s (%s)' % (cl.GetIssue(), cl.GetIssueURL())
   if not options.fast:
@@ skipping 1690 matching lines @@
 if __name__ == '__main__':
   # These affect sys.stdout so do it outside of main() to simplify mocks in
   # unit testing.
   fix_encoding.fix_encoding()
   setup_color.init()
   try:
     sys.exit(main(sys.argv[1:]))
   except KeyboardInterrupt:
     sys.stderr.write('interrupted\n')
     sys.exit(1)