Teach 'repo upload --replace' how to add replacement patch sets

Users are prompted with the list of known changes we are about
to upload, and they can fill out the current change numbers for
any changes which already exist in the data store.  For each of
those changes the change number and commit id is sent as part of
the upload request, so Gerrit can insert the new commit as a new
patch set of the existing change, rather than make a new change.

This facility permits developers to replace a patch so they can
address comments made on a prior version of the same change.

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

codereview/__init__.py

@@ -1 +1 @@
-__version__ = 'v1.0-69-gd1f8508c'
+__version__ = 'v1.0-99-g9cd3ea2f'

codereview/upload_bundle_pb2.py

@@ -27,23 +27,31 @@ _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_PROJECT', index=5, number=2,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='NOT_BUNDLE_OWNER', index=6, number=6,
+      name='UNKNOWN_BRANCH', index=6, number=3,
       options=None,
       type=None),
     descriptor.EnumValueDescriptor(
-      name='BUNDLE_CLOSED', index=7, number=8,
+      name='UNKNOWN_BUNDLE', index=7, number=5,
+      options=None,
+      type=None),
+    descriptor.EnumValueDescriptor(
+      name='NOT_BUNDLE_OWNER', index=8, number=6,
+      options=None,
+      type=None),
+    descriptor.EnumValueDescriptor(
+      name='BUNDLE_CLOSED', index=9, number=8,
       options=None,
       type=None),
   ],
@@ -51,6 +59,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 +129,13 @@ _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),
   ],
   extensions=[
   ],
@@ -174,8 +218,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

@@ -20,12 +20,16 @@ A manifest XML file (e.g. 'default.xml') roughly conforms to the
 following DTD:
 
 <!DOCTYPE manifest [
-  <!ELEMENT manifest (remote*, default?, project*)>
+  <!ELEMENT manifest (remote*,
+                      default?,
+                      project*,
+                      add-remote*)>
 
   <!ELEMENT remote (EMPTY)>
-  <!ATTLIST remote name   ID    #REQUIRED>
+  <!ATTLIST remote name         ID    #REQUIRED>
-  <!ATTLIST remote fetch  CDATA #REQUIRED>
+  <!ATTLIST remote fetch        CDATA #REQUIRED>
-  <!ATTLIST remote review CDATA #IMPLIED>
+  <!ATTLIST remote review       CDATA #IMPLIED>
+  <!ATTLIST remote project-name CDATA #IMPLIED>
 
   <!ELEMENT default (EMPTY)>
   <!ATTLIST default remote   IDREF #IMPLIED>
@@ -36,6 +40,13 @@ following DTD:
   <!ATTLIST project path     CDATA #IMPLIED>
   <!ATTLIST project remote   IDREF #IMPLIED>
   <!ATTLIST project revision CDATA #IMPLIED>
+
+  <!ELEMENT add-remote (EMPTY)>
+  <!ATTLIST add-remote to-project   ID    #REQUIRED>
+  <!ATTLIST add-remote name         ID    #REQUIRED>
+  <!ATTLIST add-remote fetch        CDATA #REQUIRED>
+  <!ATTLIST add-remote review       CDATA #IMPLIED>
+  <!ATTLIST add-remote project-name CDATA #IMPLIED>
 ]>
 
 A description of the elements and their attributes follows.
@@ -67,6 +78,24 @@ Attribute `review`: Hostname of the Gerrit server where reviews
 are uploaded to by `repo upload`.  This attribute is optional;
 if not specified then `repo upload` will not function.
 
+Attribute `project-name`: Specifies the name of this project used
+by the review server given in the review attribute of this element.
+Only permitted when the remote element is nested inside of a project
+element (see below).  If not given, defaults to the name supplied
+in the project's name attribute.
+
+Element add-remote
+------------------
+
+Adds a remote to an existing project, whose name is given by the
+to-project attribute.  This is functionally equivalent to nesting
+a remote element under the project, but has the advantage that it
+can be specified in the uesr's `local_manifest.xml` to add a remote
+to a project declared by the normal manifest.
+
+The element can be used to add a fork of an existing project that
+the user needs to work with.
+
 
 Element default
 ---------------
@@ -124,3 +153,32 @@ but adds an additional remote to only this project.  These additional
 remotes are fetched from first on the initial `repo sync`, causing
 the majority of the project's object database to be obtained through
 these additional remotes.
+
+
+Local Manifest
+==============
+
+Additional remotes and projects may be added through a local
+manifest, stored in `$TOP_DIR/.repo/local_manifest.xml`.
+
+For example:
+
+----
+	$ cat .repo/local_manifest.xml
+	<?xml version="1.0" encoding="UTF-8"?>
+	<manifest>
+	  <project path="manifest"
+			   name="tools/manifest" />
+	  <project path="platform-manifest"
+			   name="platform/manifest" />
+	</manifest>
+----
+
+Users may add projects to the local manifest prior to a `repo sync`
+invocation, instructing repo to automatically download and manage
+these extra projects.
+
+Currently the only supported feature of a local manifest is to
+add new remotes and/or projects.  In the future a local manifest
+may support picking different revisions of a project, or deleting
+projects specified in the default manifest.

gerrit_upload.py

@@ -75,6 +75,7 @@ def UploadBundle(project,
                  dest_branch,
                  src_branch,
                  bases,
+                 replace_changes = None,
                  save_cookies=True):
 
   srv = _GetRpcServer(email, server, save_cookies)
@@ -113,6 +114,10 @@ def UploadBundle(project,
         req.dest_branch = str(dest_branch)
         for c in revlist:
           req.contained_object.append(c)
+        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 +153,10 @@ 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'
         else:
           reason = 'unknown error ' + str(rsp.status_code)
         raise UploadError(reason)

git_config.py

@@ -258,6 +258,7 @@ class Remote(object):
     self.name = name
     self.url = self._Get('url')
     self.review = self._Get('review')
+    self.projectname = self._Get('projectname')
     self.fetch = map(lambda x: RefSpec.FromString(x),
                      self._Get('fetch', all=True))
 
@@ -299,6 +300,7 @@ class Remote(object):
     """
     self._Set('url', self.url)
     self._Set('review', self.review)
+    self._Set('projectname', self.projectname)
     self._Set('fetch', map(lambda x: str(x), self.fetch))
 
   def _Set(self, key, value):

manifest.py

@@ -165,6 +165,16 @@ class Manifest(object):
                 (project.name, self.manifestFile)
         self._projects[project.name] = project
 
+    for node in config.childNodes:
+      if node.nodeName == 'add-remote':
+        pn = self._reqatt(node, 'to-project')
+        project = self._projects.get(pn)
+        if not project:
+          raise ManifestParseError, \
+                'project %s not defined in %s' % \
+                (pn, self.manifestFile)
+        self._ParseProjectExtraRemote(project, node)
+
   def _AddMetaProjectMirror(self, m):
     name = None
     m_url = m.GetRemote(m.remote.name).url
@@ -206,10 +216,17 @@ class Manifest(object):
     name = self._reqatt(node, 'name')
     fetch = self._reqatt(node, 'fetch')
     review = node.getAttribute('review')
+    if review == '':
+      review = None
+
+    projectName = node.getAttribute('project-name')
+    if projectName == '':
+      projectName = None
 
     r = Remote(name=name,
                fetch=fetch,
-               review=review)
+               review=review,
+               projectName=projectName)
 
     for n in node.childNodes:
       if n.nodeName == 'require':
@@ -224,6 +241,8 @@ class Manifest(object):
     d = _Default()
     d.remote = self._get_remote(node)
     d.revision = node.getAttribute('revision')
+    if d.revision == '':
+      d.revision = None
     return d
 
   def _ParseProject(self, node):
@@ -274,18 +293,21 @@ class Manifest(object):
 
     for n in node.childNodes:
       if n.nodeName == 'remote':
-        r = self._ParseRemote(n)
+        self._ParseProjectExtraRemote(project, n)
-        if project.extraRemotes.get(r.name) \
-           or project.remote.name == r.name:
-          raise ManifestParseError, \
-                'duplicate remote %s in project %s in %s' % \
-                (r.name, project.name, self.manifestFile)
-        project.extraRemotes[r.name] = r
       elif n.nodeName == 'copyfile':
         self._ParseCopyFile(project, n)
 
     return project
 
+  def _ParseProjectExtraRemote(self, project, n):
+    r = self._ParseRemote(n)
+    if project.extraRemotes.get(r.name) \
+       or project.remote.name == r.name:
+      raise ManifestParseError, \
+            'duplicate remote %s in project %s in %s' % \
+            (r.name, project.name, self.manifestFile)
+    project.extraRemotes[r.name] = r
+
   def _ParseCopyFile(self, project, node):
     src = self._reqatt(node, 'src')
     dest = self._reqatt(node, 'dest')

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(
@@ -132,7 +143,8 @@ class ReviewableBranch(object):
       '--')
 
   def UploadForReview(self):
-    self.project.UploadForReview(self.name)
+    self.project.UploadForReview(self.name,
+                                 self.replace_changes)
 
   @property
   def tip_url(self):
@@ -428,15 +440,23 @@ class Project(object):
       if branch in pubed and pubed[branch] == id:
         continue
 
-      branch = self.GetBranch(branch)
+      rb = self.GetUploadableBranch(branch)
-      base = branch.LocalMerge
+      if rb:
-      if branch.LocalMerge:
+        ready.append(rb)
-        rb = ReviewableBranch(self, branch, base)
-        if rb.commits:
-          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):
     """Uploads the named branch for code review.
     """
     if branch is None:
@@ -461,16 +481,21 @@ class Project(object):
     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, ''
     _info("Uploading %s to %s:", branch.name, self.name)
     try:
       UploadBundle(project = self,
                    server = branch.remote.review,
                    email = self.UserEmail,
-                   dest_project = self.name,
+                   dest_project = branch.remote.projectname,
                    dest_branch = dest_branch,
                    src_branch = R_HEADS + branch.name,
-                   bases = base_list)
+                   bases = base_list,
+                   replace_changes = replace_changes)
     except proto_client.ClientLoginError:
       raise UploadError('Login failure')
     except urllib2.HTTPError, e:
@@ -887,6 +912,8 @@ class Project(object):
       url += '/%s.git' % self.name
       remote.url = url
       remote.review = self.remote.reviewUrl
+      if remote.projectname is None:
+        remote.projectname = self.name
 
       if self.worktree:
         remote.ResetFetch(mirror=False)
@@ -898,6 +925,10 @@ class Project(object):
       remote = self.GetRemote(r.name)
       remote.url = r.fetchUrl
       remote.review = r.reviewUrl
+      if r.projectName:
+        remote.projectname = r.projectName
+      elif remote.projectname is None:
+        remote.projectname = self.name
       remote.ResetFetch()
       remote.Save()
 

remote.py

@@ -14,8 +14,12 @@
 # limitations under the License.
 
 class Remote(object):
-  def __init__(self, name, fetch=None, review=None):
+  def __init__(self, name,
+               fetch=None,
+               review=None,
+               projectName=None):
     self.name = name
     self.fetchUrl = fetch
     self.reviewUrl = review
+    self.projectName = projectName
     self.requiredCommits = []

subcmds/upload.py

@@ -29,7 +29,7 @@ class Upload(InteractiveCommand):
   common = True
   helpSummary = "Upload changes for code review"
   helpUsage="""
-%prog [<project>]...
+%prog {[<project>]... | --replace <project>}
 """
   helpDescription = """
 The '%prog' command is used to send changes to the Gerrit code
@@ -46,6 +46,11 @@ no projects are specified, '%prog' will search for uploadable
 changes in all projects listed in the manifest.
 """
 
+  def _Options(self, p):
+    p.add_option('--replace',
+                 dest='replace', action='store_true',
+                 help='Upload replacement patchesets from this branch')
+
   def _SingleBranch(self, branch):
     project = branch.project
     name = branch.name
@@ -129,6 +134,50 @@ changes in all projects listed in the manifest.
       _die("nothing uncommented for upload")
     self._UploadAndReport(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])
+
   def _UploadAndReport(self, todo):
     have_errors = False
     for branch in todo:
@@ -168,6 +217,14 @@ changes in all projects listed in the manifest.
     project_list = self.GetProjects(args)
     pending = []
 
+    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()
       if avail: