Fix color parsing to not crash when user defined colors are set

We didn't use the right Python string methods to parse colors.

  $ git config --global color.status.added yellow

managed to cause a stack trace due to undefined methods trim()
and lowercase().  Instead use strip() and lower().

Signed-off-by: Shawn O. Pearce <sop@google.com>

codereview/__init__.py

@@ -1 +1 @@
-__version__ = 'v1.0-99-g9cd3ea2f'
+__version__ = 'v1.0-112-gbcd4db5a'

codereview/upload_bundle_pb2.py

@@ -35,23 +35,27 @@ _UPLOADBUNDLERESPONSE_CODETYPE = descriptor.EnumDescriptor(
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_PROJECT', index=5, number=2,
+      name='UNKNOWN_EMAIL', index=5, number=11,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_BRANCH', index=6, number=3,
+      name='UNKNOWN_PROJECT', index=6, number=2,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_BUNDLE', index=7, number=5,
+      name='UNKNOWN_BRANCH', index=7, number=3,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='NOT_BUNDLE_OWNER', index=8, number=6,
+      name='UNKNOWN_BUNDLE', index=8, number=5,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='BUNDLE_CLOSED', index=9, number=8,
+      name='NOT_BUNDLE_OWNER', index=9, number=6,
+      options=None,
+      type=None),
+    descriptor.EnumValueDescriptor(
+      name='BUNDLE_CLOSED', index=10, number=8,
       options=None,
       type=None),
   ],
@@ -136,6 +140,20 @@ _UPLOADBUNDLEREQUEST = descriptor.Descriptor(
       message_type=None, enum_type=None, containing_type=None,
       is_extension=False, extension_scope=None,
       options=None),
+    descriptor.FieldDescriptor(
+      name='reviewers', full_name='codereview.UploadBundleRequest.reviewers', index=6,
+      number=3, type=9, cpp_type=9, label=3,
+      default_value=[],
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
+    descriptor.FieldDescriptor(
+      name='cc', full_name='codereview.UploadBundleRequest.cc', index=7,
+      number=4, type=9, cpp_type=9, label=3,
+      default_value=[],
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
   ],
   extensions=[
   ],
@@ -165,6 +183,20 @@ _UPLOADBUNDLERESPONSE = descriptor.Descriptor(
       message_type=None, enum_type=None, containing_type=None,
       is_extension=False, extension_scope=None,
       options=None),
+    descriptor.FieldDescriptor(
+      name='invalid_reviewers', full_name='codereview.UploadBundleResponse.invalid_reviewers', index=2,
+      number=12, type=9, cpp_type=9, label=3,
+      default_value=[],
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
+    descriptor.FieldDescriptor(
+      name='invalid_cc', full_name='codereview.UploadBundleResponse.invalid_cc', index=3,
+      number=13, type=9, cpp_type=9, label=3,
+      default_value=[],
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
   ],
   extensions=[
   ],

color.py

@@ -137,7 +137,7 @@ class Coloring(object):
     if v is None:
       return _Color(fg, bg, attr)
 
-    v = v.trim().lowercase()
+    v = v.strip().lower()
     if v == "reset":
       return RESET
     elif v == '':

docs/manifest-format.txt

@@ -22,6 +22,7 @@ following DTD:
 <!DOCTYPE manifest [
   <!ELEMENT manifest (remote*,
                       default?,
+                      remove-project*,
                       project*,
                       add-remote*)>
 
@@ -47,6 +48,9 @@ following DTD:
   <!ATTLIST add-remote fetch        CDATA #REQUIRED>
   <!ATTLIST add-remote review       CDATA #IMPLIED>
   <!ATTLIST add-remote project-name CDATA #IMPLIED>
+
+  <!ELEMENT remove-project (EMPTY)>
+  <!ATTLIST remove-project name  CDATA #REQUIRED>
 ]>
 
 A description of the elements and their attributes follows.
@@ -155,6 +159,18 @@ the majority of the project's object database to be obtained through
 these additional remotes.
 
 
+Element remove-project
+----------------------
+
+Deletes the named project from the internal manifest table, possibly
+allowing a subsequent project element in the same manifest file to
+replace the project with a different source.
+
+This element is mostly useful in the local_manifest.xml, where
+the user can remove a project, and possibly replace it with their
+own definition.
+
+
 Local Manifest
 ==============
 

gerrit_upload.py

@@ -75,6 +75,7 @@ def UploadBundle(project,
                  dest_branch,
                  src_branch,
                  bases,
+                 people,
                  replace_changes = None,
                  save_cookies=True):
 
@@ -112,6 +113,10 @@ def UploadBundle(project,
         req = UploadBundleRequest()
         req.dest_project = str(dest_project)
         req.dest_branch = str(dest_branch)
+        for e in people[0]:
+          req.reviewers.append(e)
+        for e in people[1]:
+          req.cc.append(e)
         for c in revlist:
           req.contained_object.append(c)
         if replace_changes:
@@ -158,6 +163,10 @@ def UploadBundle(project,
           reason = 'invalid change id'
         elif rsp.status_code == UploadBundleResponse.CHANGE_CLOSED:
           reason = 'one or more changes are closed'
+        elif rsp.status_code == UploadBundleResponse.UNKNOWN_EMAIL:
+          emails = [x for x in rsp.invalid_reviewers] + [
+                    x for x in rsp.invalid_cc]
+          reason = 'invalid email addresses: %s' % ", ".join(emails)
         else:
           reason = 'unknown error ' + str(rsp.status_code)
         raise UploadError(reason)

git_config.py

@@ -16,7 +16,8 @@
 import os
 import re
 import sys
-from error import GitError
+from urllib2 import urlopen, HTTPError
+from error import GitError, UploadError
 from git_command import GitCommand
 
 R_HEADS = 'refs/heads/'
@@ -261,6 +262,45 @@ class Remote(object):
     self.projectname = self._Get('projectname')
     self.fetch = map(lambda x: RefSpec.FromString(x),
                      self._Get('fetch', all=True))
+    self._review_protocol = None
+
+  @property
+  def ReviewProtocol(self):
+    if self._review_protocol is None:
+      if self.review is None:
+        return None
+
+      u = self.review
+      if not u.startswith('http:') and not u.startswith('https:'):
+        u = 'http://%s' % u
+      if not u.endswith('/'):
+        u += '/'
+      u += 'ssh_info'
+
+      try:
+        info = urlopen(u).read()
+        if info == 'NOT_AVAILABLE':
+          raise UploadError('Upload over ssh unavailable')
+
+        self._review_protocol = 'ssh'
+        self._review_host = info.split(" ")[0]
+        self._review_port = info.split(" ")[1]
+
+      except HTTPError, e:
+        if e.code == 404:
+          self._review_protocol = 'http-post'
+        else:
+          raise UploadError('Cannot guess Gerrit version')
+    return self._review_protocol
+
+  def SshReviewUrl(self, userEmail):
+    if self.ReviewProtocol != 'ssh':
+      return None
+    return 'ssh://%s@%s:%s/%s' % (
+      userEmail.split("@")[0],
+      self._review_host,
+      self._review_port,
+      self.projectname)
 
   def ToLocal(self, rev):
     """Convert a remote revision string to something we have locally.

manifest.py

@@ -137,6 +137,16 @@ class Manifest(object):
             "no <manifest> in %s" % \
             self.manifestFile
 
+    for node in config.childNodes:
+      if node.nodeName == 'remove-project':
+        name = self._reqatt(node, 'name')
+        try:
+          del self._projects[name]
+        except KeyError:
+          raise ManifestParseError, \
+                'project %s not found' % \
+                (name)
+
     for node in config.childNodes:
       if node.nodeName == 'remote':
         remote = self._ParseRemote(node)

project.py

@@ -46,6 +46,8 @@ def _info(fmt, *args):
 def not_rev(r):
   return '^' + r
 
+def sq(r):
+  return "'" + r.replace("'", "'\''") + "'"
 
 hook_list = None
 def repo_hooks():
@@ -142,9 +144,10 @@ class ReviewableBranch(object):
       R_HEADS + self.name,
       '--')
 
-  def UploadForReview(self):
+  def UploadForReview(self, people):
     self.project.UploadForReview(self.name,
-                                 self.replace_changes)
+                                 self.replace_changes,
+                                 people)
 
   @property
   def tip_url(self):
@@ -456,7 +459,7 @@ class Project(object):
         return rb
     return None
 
-  def UploadForReview(self, branch=None, replace_changes=None):
+  def UploadForReview(self, branch=None, replace_changes=None, people=([],[])):
     """Uploads the named branch for code review.
     """
     if branch is None:
@@ -474,32 +477,58 @@ class Project(object):
     if not dest_branch.startswith(R_HEADS):
       dest_branch = R_HEADS + dest_branch
 
-    base_list = []
-    for name, id in self._allrefs.iteritems():
-      if branch.remote.WritesTo(name):
-        base_list.append(not_rev(name))
-    if not base_list:
-      raise GitError('no base refs, cannot upload %s' % branch.name)
-
     if not branch.remote.projectname:
       branch.remote.projectname = self.name
       branch.remote.Save()
 
-    print >>sys.stderr, ''
+    if branch.remote.ReviewProtocol == 'http-post':
-    _info("Uploading %s to %s:", branch.name, self.name)
+      base_list = []
-    try:
+      for name, id in self._allrefs.iteritems():
-      UploadBundle(project = self,
+        if branch.remote.WritesTo(name):
-                   server = branch.remote.review,
+          base_list.append(not_rev(name))
-                   email = self.UserEmail,
+      if not base_list:
-                   dest_project = branch.remote.projectname,
+        raise GitError('no base refs, cannot upload %s' % branch.name)
-                   dest_branch = dest_branch,
+
-                   src_branch = R_HEADS + branch.name,
+      print >>sys.stderr, ''
-                   bases = base_list,
+      _info("Uploading %s to %s:", branch.name, self.name)
-                   replace_changes = replace_changes)
+      try:
-    except proto_client.ClientLoginError:
+        UploadBundle(project = self,
-      raise UploadError('Login failure')
+                     server = branch.remote.review,
-    except urllib2.HTTPError, e:
+                     email = self.UserEmail,
-      raise UploadError('HTTP error %d' % e.code)
+                     dest_project = branch.remote.projectname,
+                     dest_branch = dest_branch,
+                     src_branch = R_HEADS + branch.name,
+                     bases = base_list,
+                     people = people,
+                     replace_changes = replace_changes)
+      except proto_client.ClientLoginError:
+        raise UploadError('Login failure')
+      except urllib2.HTTPError, e:
+        raise UploadError('HTTP error %d' % e.code)
+
+    elif branch.remote.ReviewProtocol == 'ssh':
+      if dest_branch.startswith(R_HEADS):
+        dest_branch = dest_branch[len(R_HEADS):]
+
+      rp = ['gerrit receive-pack']
+      for e in people[0]:
+        rp.append('--reviewer=%s' % sq(e))
+      for e in people[1]:
+        rp.append('--cc=%s' % sq(e))
+
+      cmd = ['push']
+      cmd.append('--receive-pack=%s' % " ".join(rp))
+      cmd.append(branch.remote.SshReviewUrl(self.UserEmail))
+      cmd.append('%s:refs/for/%s' % (R_HEADS + branch.name, dest_branch))
+      if replace_changes:
+        for change_id,commit_id in replace_changes.iteritems():
+          cmd.append('%s:refs/changes/%s/new' % (commit_id, change_id))
+      if GitCommand(self, cmd, bare = True).Wait() != 0:
+        raise UploadError('Upload failed')
+
+    else:
+        raise UploadError('Unsupported protocol %s' \
+          % branch.remote.review)
 
     msg = "posted to %s for %s" % (branch.remote.review, dest_branch)
     self.bare_git.UpdateRef(R_PUB + branch.name,
@@ -938,7 +967,7 @@ class Project(object):
       ref = R_M + self.manifest.branch
 
       if IsId(self.revision):
-        dst = self.revision + '^0',
+        dst = self.revision + '^0'
         self.bare_git.UpdateRef(ref, dst, message = msg, detach = True)
       else:
         remote = self.GetRemote(self.remote.name)

repo

@@ -2,7 +2,7 @@
 
 ## repo default configuration
 ##
-REPO_URL='git://android.kernel.org/tools/repo.git'
+REPO_URL='git://android.git.kernel.org/tools/repo.git'
 REPO_REV='stable'
 
 # Copyright (C) 2008 Google Inc.
@@ -28,7 +28,7 @@ if __name__ == '__main__':
 del magic
 
 # increment this whenever we make important changes to this script
-VERSION = (1, 7)
+VERSION = (1, 8)
 
 # increment this if the MAINTAINER_KEYS block is modified
 KEYRING_VERSION = (1,0)

subcmds/upload.py

@@ -25,11 +25,17 @@ def _die(fmt, *args):
   print >>sys.stderr, 'error: %s' % msg
   sys.exit(1)
 
+def _SplitEmails(values):
+  result = []
+  for str in values:
+    result.extend([s.strip() for s in str.split(',')])
+  return result
+
 class Upload(InteractiveCommand):
   common = True
   helpSummary = "Upload changes for code review"
   helpUsage="""
-%prog {[<project>]... | --replace <project>}
+%prog [--re --cc] {[<project>]... | --replace <project>}
 """
   helpDescription = """
 The '%prog' command is used to send changes to the Gerrit code
@@ -44,14 +50,31 @@ at the command line.  Projects can be specified either by name, or
 by a relative or absolute path to the project's local directory. If
 no projects are specified, '%prog' will search for uploadable
 changes in all projects listed in the manifest.
+
+If the --reviewers or --cc options are passed, those emails are
+added to the respective list of users, and emails are sent to any
+new users.  Users passed to --reviewers must be already registered
+with the code review system, or the upload will fail.
+
+If the --replace option is passed the user can designate which
+existing change(s) in Gerrit match up to the commits in the branch
+being uploaded.  For each matched pair of change,commit the commit
+will be added as a new patch set, completely replacing the set of
+files and description associated with the change in Gerrit.
 """
 
   def _Options(self, p):
     p.add_option('--replace',
                  dest='replace', action='store_true',
                  help='Upload replacement patchesets from this branch')
+    p.add_option('--re', '--reviewers',
+                 type='string',  action='append', dest='reviewers',
+                 help='Request reviews from these people.')
+    p.add_option('--cc',
+                 type='string',  action='append', dest='cc',
+                 help='Also send email to these email addresses.')
 
-  def _SingleBranch(self, branch):
+  def _SingleBranch(self, branch, people):
     project = branch.project
     name = branch.name
     date = branch.date
@@ -69,11 +92,11 @@ changes in all projects listed in the manifest.
     sys.stdout.write('(y/n)? ')
     answer = sys.stdin.readline().strip()
     if answer in ('y', 'Y', 'yes', '1', 'true', 't'):
-      self._UploadAndReport([branch])
+      self._UploadAndReport([branch], people)
     else:
       _die("upload aborted by user")
 
-  def _MultipleBranches(self, pending):
+  def _MultipleBranches(self, pending, people):
     projects = {}
     branches = {}
 
@@ -132,9 +155,9 @@ changes in all projects listed in the manifest.
         todo.append(branch)
     if not todo:
       _die("nothing uncommented for upload")
-    self._UploadAndReport(todo)
+    self._UploadAndReport(todo, people)
 
-  def _ReplaceBranch(self, project):
+  def _ReplaceBranch(self, project, people):
     branch = project.CurrentBranch
     if not branch:
       print >>sys.stdout, "no branches ready for upload"
@@ -161,6 +184,7 @@ changes in all projects listed in the manifest.
     for line in script:
       m = change_re.match(line)
       if m:
+        c = m.group(1)
         f = m.group(2)
         try:
           f = full_hashes[f]
@@ -168,7 +192,11 @@ changes in all projects listed in the manifest.
           print 'fh = %s' % full_hashes
           print >>sys.stderr, "error: commit %s not found" % f
           sys.exit(1)
-        to_replace[m.group(1)] = f
+        if c in to_replace:
+          print >>sys.stderr,\
+            "error: change %s cannot accept multiple commits" % c
+          sys.exit(1)
+        to_replace[c] = f
 
     if not to_replace:
       print >>sys.stderr, "error: no replacements specified"
@@ -176,13 +204,13 @@ changes in all projects listed in the manifest.
       sys.exit(1)
 
     branch.replace_changes = to_replace
-    self._UploadAndReport([branch])
+    self._UploadAndReport([branch], people)
 
-  def _UploadAndReport(self, todo):
+  def _UploadAndReport(self, todo, people):
     have_errors = False
     for branch in todo:
       try:
-        branch.UploadForReview()
+        branch.UploadForReview(people)
         branch.uploaded = True
       except UploadError, e:
         branch.error = e
@@ -216,13 +244,21 @@ changes in all projects listed in the manifest.
   def Execute(self, opt, args):
     project_list = self.GetProjects(args)
     pending = []
+    reviewers = []
+    cc = []
+
+    if opt.reviewers:
+      reviewers = _SplitEmails(opt.reviewers)
+    if opt.cc:
+      cc = _SplitEmails(opt.cc)
+    people = (reviewers,cc)
 
     if opt.replace:
       if len(project_list) != 1:
         print >>sys.stderr, \
               'error: --replace requires exactly one project'
         sys.exit(1)
-      self._ReplaceBranch(project_list[0])
+      self._ReplaceBranch(project_list[0], people)
       return
 
     for project in project_list:
@@ -233,6 +269,6 @@ changes in all projects listed in the manifest.
     if not pending:
       print >>sys.stdout, "no branches ready for upload"
     elif len(pending) == 1 and len(pending[0][1]) == 1:
-      self._SingleBranch(pending[0][1][0])
+      self._SingleBranch(pending[0][1][0], people)
     else:
-      self._MultipleBranches(pending)
+      self._MultipleBranches(pending, people)