Support <remove-project name="X"> in manifest to remove/replace X

The manifest files now permit removing a project so the user can
either keep it out of their client, or replace it with a different
project using an entirely different configuration.

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

codereview/__init__.py

@@ -1 +1 @@
-__version__ = 'v1.0-69-gd1f8508c'
+__version__ = 'v1.0-112-gbcd4db5a'

codereview/upload_bundle_pb2.py

@@ -27,23 +27,35 @@ _UPLOADBUNDLERESPONSE_CODETYPE = descriptor.EnumDescriptor(
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_PROJECT', index=3, number=2,
+      name='UNKNOWN_CHANGE', index=3, number=9,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_BRANCH', index=4, number=3,
+      name='CHANGE_CLOSED', index=4, number=10,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='UNKNOWN_BUNDLE', index=5, number=5,
+      name='UNKNOWN_EMAIL', index=5, number=11,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='NOT_BUNDLE_OWNER', index=6, number=6,
+      name='UNKNOWN_PROJECT', index=6, number=2,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='BUNDLE_CLOSED', index=7, number=8,
+      name='UNKNOWN_BRANCH', index=7, number=3,
+      options=None,
+      type=None),
+    descriptor.EnumValueDescriptor(
+      name='UNKNOWN_BUNDLE', index=8, number=5,
+      options=None,
+      type=None),
+    descriptor.EnumValueDescriptor(
+      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),
   ],
@@ -51,6 +63,35 @@ _UPLOADBUNDLERESPONSE_CODETYPE = descriptor.EnumDescriptor(
 )
 
 
+_REPLACEPATCHSET = descriptor.Descriptor(
+  name='ReplacePatchSet',
+  full_name='codereview.ReplacePatchSet',
+  filename='upload_bundle.proto',
+  containing_type=None,
+  fields=[
+    descriptor.FieldDescriptor(
+      name='change_id', full_name='codereview.ReplacePatchSet.change_id', index=0,
+      number=1, type=9, cpp_type=9, label=2,
+      default_value=unicode("", "utf-8"),
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
+    descriptor.FieldDescriptor(
+      name='object_id', full_name='codereview.ReplacePatchSet.object_id', index=1,
+      number=2, type=9, cpp_type=9, label=2,
+      default_value=unicode("", "utf-8"),
+      message_type=None, enum_type=None, containing_type=None,
+      is_extension=False, extension_scope=None,
+      options=None),
+  ],
+  extensions=[
+  ],
+  nested_types=[],  # TODO(robinson): Implement.
+  enum_types=[
+  ],
+  options=None)
+
+
 _UPLOADBUNDLEREQUEST = descriptor.Descriptor(
   name='UploadBundleRequest',
   full_name='codereview.UploadBundleRequest',
@@ -92,6 +133,27 @@ _UPLOADBUNDLEREQUEST = descriptor.Descriptor(
       message_type=None, enum_type=None, containing_type=None,
       is_extension=False, extension_scope=None,
       options=None),
+    descriptor.FieldDescriptor(
+      name='replace', full_name='codereview.UploadBundleRequest.replace', index=5,
+      number=2, type=11, cpp_type=10, label=3,
+      default_value=[],
+      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=[
   ],
@@ -121,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=[
   ],
@@ -174,8 +250,13 @@ _UPLOADBUNDLECONTINUE = descriptor.Descriptor(
   options=None)
 
 
+_UPLOADBUNDLEREQUEST.fields_by_name['replace'].message_type = _REPLACEPATCHSET
 _UPLOADBUNDLERESPONSE.fields_by_name['status_code'].enum_type = _UPLOADBUNDLERESPONSE_CODETYPE
 
+class ReplacePatchSet(message.Message):
+  __metaclass__ = reflection.GeneratedProtocolMessageType
+  DESCRIPTOR = _REPLACEPATCHSET
+
 class UploadBundleRequest(message.Message):
   __metaclass__ = reflection.GeneratedProtocolMessageType
   DESCRIPTOR = _UPLOADBUNDLEREQUEST

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
 ==============
 

editor.py

@@ -69,14 +69,14 @@ least one of these before using this command."""
       Returns:
         new value of edited text; None if editing did not succeed
     """
-    editor = cls._GetEditor()
+    editor = cls._GetEditor().split()
     fd, path = tempfile.mkstemp()
     try:
       os.write(fd, data)
       os.close(fd)
       fd = None
 
-      if subprocess.Popen([editor, path]).wait() != 0:
+      if subprocess.Popen(editor + [path]).wait() != 0:
         raise EditorError()
       return open(path).read()
     finally:

gerrit_upload.py

@@ -75,6 +75,8 @@ def UploadBundle(project,
                  dest_branch,
                  src_branch,
                  bases,
+                 people,
+                 replace_changes = None,
                  save_cookies=True):
 
   srv = _GetRpcServer(email, server, save_cookies)
@@ -111,8 +113,17 @@ 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:
+          for change_id,commit_id in replace_changes.iteritems():
+            r = req.replace.add()
+            r.change_id = change_id
+            r.object_id = commit_id
       else:
         req = UploadBundleContinue()
         req.bundle_id = bundle_id
@@ -148,6 +159,14 @@ def UploadBundle(project,
         elif rsp.status_code == UploadBundleResponse.UNAUTHORIZED_USER:
           reason = ('Unauthorized user.  Visit http://%s/hello to sign up.'
                     % server)
+        elif rsp.status_code == UploadBundleResponse.UNKNOWN_CHANGE:
+          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)

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

@@ -104,6 +104,7 @@ class ReviewableBranch(object):
     self.project = project
     self.branch = branch
     self.base = base
+    self.replace_changes = None
 
   @property
   def name(self):
@@ -123,6 +124,16 @@ class ReviewableBranch(object):
         '--')
     return self._commit_cache
 
+  @property
+  def unabbrev_commits(self):
+    r = dict()
+    for commit in self.project.bare_git.rev_list(
+        not_rev(self.base),
+        R_HEADS + self.name,
+        '--'):
+      r[commit[0:8]] = commit
+    return r
+
   @property
   def date(self):
     return self.project.bare_git.log(
@@ -131,8 +142,10 @@ class ReviewableBranch(object):
       R_HEADS + self.name,
       '--')
 
-  def UploadForReview(self):
-    self.project.UploadForReview(self.name)
+  def UploadForReview(self, people):
+    self.project.UploadForReview(self.name,
+                                 self.replace_changes,
+                                 people)
 
   @property
   def tip_url(self):
@@ -428,15 +441,23 @@ class Project(object):
       if branch in pubed and pubed[branch] == id:
         continue
 
-      branch = self.GetBranch(branch)
-      base = branch.LocalMerge
-      if branch.LocalMerge:
-        rb = ReviewableBranch(self, branch, base)
-        if rb.commits:
-          ready.append(rb)
+      rb = self.GetUploadableBranch(branch)
+      if rb:
+        ready.append(rb)
     return ready
 
-  def UploadForReview(self, branch=None):
+  def GetUploadableBranch(self, branch_name):
+    """Get a single uploadable branch, or None.
+    """
+    branch = self.GetBranch(branch_name)
+    base = branch.LocalMerge
+    if branch.LocalMerge:
+      rb = ReviewableBranch(self, branch, base)
+      if rb.commits:
+        return rb
+    return None
+
+  def UploadForReview(self, branch=None, replace_changes=None, people=([],[])):
     """Uploads the named branch for code review.
     """
     if branch is None:
@@ -474,7 +495,9 @@ class Project(object):
                    dest_project = branch.remote.projectname,
                    dest_branch = dest_branch,
                    src_branch = R_HEADS + branch.name,
-                   bases = base_list)
+                   bases = base_list,
+                   people = people,
+                   replace_changes = replace_changes)
     except proto_client.ClientLoginError:
       raise UploadError('Login failure')
     except urllib2.HTTPError, e:

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>]...
+%prog [--re --cc] {[<project>]... | --replace <project>}
 """
   helpDescription = """
 The '%prog' command is used to send changes to the Gerrit code
@@ -44,9 +50,25 @@ 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.
 """
 
-  def _SingleBranch(self, branch):
+  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, people):
     project = branch.project
     name = branch.name
     date = branch.date
@@ -64,11 +86,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 = {}
 
@@ -127,13 +149,57 @@ 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 _UploadAndReport(self, todo):
+  def _ReplaceBranch(self, project):
+    branch = project.CurrentBranch
+    if not branch:
+      print >>sys.stdout, "no branches ready for upload"
+      return
+    branch = project.GetUploadableBranch(branch)
+    if not branch:
+      print >>sys.stdout, "no branches ready for upload"
+      return
+
+    script = []
+    script.append('# Replacing from branch %s' % branch.name)
+    for commit in branch.commits:
+      script.append('[      ] %s' % commit)
+    script.append('')
+    script.append('# Insert change numbers in the brackets to add a new patch set.')
+    script.append('# To create a new change record, leave the brackets empty.')
+
+    script = Editor.EditString("\n".join(script)).split("\n")
+
+    change_re = re.compile(r'^\[\s*(\d{1,})\s*\]\s*([0-9a-f]{1,}) .*$')
+    to_replace = dict()
+    full_hashes = branch.unabbrev_commits
+
+    for line in script:
+      m = change_re.match(line)
+      if m:
+        f = m.group(2)
+        try:
+          f = full_hashes[f]
+        except KeyError:
+          print 'fh = %s' % full_hashes
+          print >>sys.stderr, "error: commit %s not found" % f
+          sys.exit(1)
+        to_replace[m.group(1)] = f
+
+    if not to_replace:
+      print >>sys.stderr, "error: no replacements specified"
+      print >>sys.stderr, "       use 'repo upload' without --replace"
+      sys.exit(1)
+
+    branch.replace_changes = to_replace
+    self._UploadAndReport([branch], people)
+
+  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
@@ -167,6 +233,22 @@ 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])
+      return
 
     for project in project_list:
       avail = project.GetUploadableBranches()
@@ -176,6 +258,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)