project: fix variable typo

Bug: https://crbug.com/gerrit/11293
Reported-by: Daniel Kutik <daniel.kutik@lavawerk.com>
Change-Id: I37bac58aa1dc9ecc10e29253d14ff9e6fb42427c
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/298942
Reviewed-by: Ian Kasprzak <iankaz@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

command.py

@@ -23,6 +23,20 @@ from error import NoSuchProjectError
 from error import InvalidProjectGroupsError
 
 
+# Number of projects to submit to a single worker process at a time.
+# This number represents a tradeoff between the overhead of IPC and finer
+# grained opportunity for parallelism. This particular value was chosen by
+# iterating through powers of two until the overall performance no longer
+# improved. The performance of this batch size is not a function of the
+# number of cores on the system.
+WORKER_BATCH_SIZE = 32
+
+
+# How many jobs to run in parallel by default?  This assumes the jobs are
+# largely I/O bound and do not hit the network.
+DEFAULT_LOCAL_JOBS = min(os.cpu_count(), 8)
+
+
 class Command(object):
   """Base class for any command line action in repo.
   """
@@ -32,6 +46,10 @@ class Command(object):
   manifest = None
   _optparse = None
 
+  # Whether this command supports running in parallel.  If greater than 0,
+  # it is the number of parallel jobs to default to.
+  PARALLEL_JOBS = None
+
   def WantPager(self, _opt):
     return False
 
@@ -72,6 +90,11 @@ class Command(object):
   def _Options(self, p):
     """Initialize the option parser.
     """
+    if self.PARALLEL_JOBS is not None:
+      p.add_option(
+          '-j', '--jobs',
+          type=int, default=self.PARALLEL_JOBS,
+          help='number of jobs to run in parallel (default: %s)' % self.PARALLEL_JOBS)
 
   def _RegisteredEnvironmentOptions(self):
     """Get options that can be set from environment variables.

docs/internal-fs-layout.md

@@ -93,6 +93,23 @@ support, see the [manifest-format.md] file.
 
 ### Project objects
 
+*** note
+**Warning**: Please do not use repo's approach to projects/ & project-objects/
+layouts as a model for other tools to implement similar approaches.
+It has a number of known downsides like:
+*   [Symlinks do not work well under Windows](./windows.md).
+*   Git sometimes replaces symlinks under .git/ with real files (under unknown
+    circumstances), and then the internal state gets out of sync, and data loss
+    may ensue.
+*   When sharing project-objects between multiple project checkouts, Git might
+    automatically run `gc` or `prune` which may lead to data loss or corruption
+    (since those operate on leaf projects and miss refs in other leaves).  See
+    https://gerrit-review.googlesource.com/c/git-repo/+/254392 for more details.
+
+Instead, you should use standard Git workflows like [git worktree] or
+[gitsubmodules] with [superprojects].
+***
+
 *   `project.list`: Tracking file used by `repo sync` to determine when projects
     are added or removed and need corresponding updates in the checkout.
 *   `projects/`: Bare checkouts of every project synced by the manifest.  The
@@ -121,7 +138,7 @@ support, see the [manifest-format.md] file.
     (i.e. the path on the remote server) with a `.git` suffix.  This has the
     same advantages as the `project-objects/` layout above.
 
-    This is used when git worktrees are enabled.
+    This is used when [git worktree]'s are enabled.
 
 ### Global settings
 
@@ -143,7 +160,7 @@ User controlled settings are initialized when running `repo init`.
 | repo.reference    | `--reference`             | Reference repo client checkout |
 | repo.submodules   | `--submodules`            | Sync git submodules |
 | repo.superproject | `--use-superproject`      | Sync [superproject] |
-| repo.worktree     | `--worktree`              | Use `git worktree` for checkouts |
+| repo.worktree     | `--worktree`              | Use [git worktree] for checkouts |
 | user.email        | `--config-name`           | User's e-mail address; Copied into `.git/config` when checking out a new project |
 | user.name         | `--config-name`           | User's name; Copied into `.git/config` when checking out a new project |
 
@@ -228,7 +245,10 @@ Repo will create & maintain a few files in the user's home directory.
 
 
 [git-config]: https://git-scm.com/docs/git-config
+[git worktree]: https://git-scm.com/docs/git-worktree
+[gitsubmodules]: https://git-scm.com/docs/gitsubmodules
 [manifest-format.md]: ./manifest-format.md
 [local manifests]: ./manifest-format.md#Local-Manifests
+[superprojects]: https://en.wikibooks.org/wiki/Git/Submodules_and_Superprojects
 [topic]: https://gerrit-review.googlesource.com/Documentation/intro-user.html#topics
 [upload-notify]: https://gerrit-review.googlesource.com/Documentation/user-upload.html#notify

error.py

@@ -22,12 +22,12 @@ class ManifestParseError(Exception):
   """
 
 
-class ManifestInvalidRevisionError(Exception):
+class ManifestInvalidRevisionError(ManifestParseError):
   """The revision value in a project is incorrect.
   """
 
 
-class ManifestInvalidPathError(Exception):
+class ManifestInvalidPathError(ManifestParseError):
   """A path used in <copyfile> or <linkfile> is incorrect.
   """
 
@@ -37,7 +37,7 @@ class NoManifestException(Exception):
   """
 
   def __init__(self, path, reason):
-    super(NoManifestException, self).__init__()
+    super().__init__(path, reason)
     self.path = path
     self.reason = reason
 
@@ -50,7 +50,7 @@ class EditorError(Exception):
   """
 
   def __init__(self, reason):
-    super(EditorError, self).__init__()
+    super().__init__(reason)
     self.reason = reason
 
   def __str__(self):
@@ -62,7 +62,7 @@ class GitError(Exception):
   """
 
   def __init__(self, command):
-    super(GitError, self).__init__()
+    super().__init__(command)
     self.command = command
 
   def __str__(self):
@@ -74,7 +74,7 @@ class UploadError(Exception):
   """
 
   def __init__(self, reason):
-    super(UploadError, self).__init__()
+    super().__init__(reason)
     self.reason = reason
 
   def __str__(self):
@@ -86,7 +86,7 @@ class DownloadError(Exception):
   """
 
   def __init__(self, reason):
-    super(DownloadError, self).__init__()
+    super().__init__(reason)
     self.reason = reason
 
   def __str__(self):
@@ -98,7 +98,7 @@ class NoSuchProjectError(Exception):
   """
 
   def __init__(self, name=None):
-    super(NoSuchProjectError, self).__init__()
+    super().__init__(name)
     self.name = name
 
   def __str__(self):
@@ -112,7 +112,7 @@ class InvalidProjectGroupsError(Exception):
   """
 
   def __init__(self, name=None):
-    super(InvalidProjectGroupsError, self).__init__()
+    super().__init__(name)
     self.name = name
 
   def __str__(self):
@@ -128,7 +128,7 @@ class RepoChangedException(Exception):
   """
 
   def __init__(self, extra_args=None):
-    super(RepoChangedException, self).__init__()
+    super().__init__(extra_args)
     self.extra_args = extra_args or []
 
 

git_command.py

@@ -162,11 +162,10 @@ def RepoSourceVersion():
 
     proj = os.path.dirname(os.path.abspath(__file__))
     env[GIT_DIR] = os.path.join(proj, '.git')
-
+    result = subprocess.run([GIT, 'describe', HEAD], stdout=subprocess.PIPE,
-    p = subprocess.Popen([GIT, 'describe', HEAD], stdout=subprocess.PIPE,
+                            encoding='utf-8', env=env, check=False)
-                         env=env)
+    if result.returncode == 0:
-    if p.wait() == 0:
+      ver = result.stdout.strip()
-      ver = p.stdout.read().strip().decode('utf-8')
       if ver.startswith('v'):
         ver = ver[1:]
     else:
@@ -250,7 +249,7 @@ class GitCommand(object):
                project,
                cmdv,
                bare=False,
-               provide_stdin=False,
+               input=None,
                capture_stdout=False,
                capture_stderr=False,
                merge_output=False,
@@ -260,9 +259,6 @@ class GitCommand(object):
                gitdir=None):
     env = self._GetBasicEnv()
 
-    # If we are not capturing std* then need to print it.
-    self.tee = {'stdout': not capture_stdout, 'stderr': not capture_stderr}
-
     if disable_editor:
       env['GIT_EDITOR'] = ':'
     if ssh_proxy:
@@ -289,6 +285,9 @@ class GitCommand(object):
     command = [GIT]
     if bare:
       if gitdir:
+        # Git on Windows wants its paths only using / for reliability.
+        if platform_utils.isWindows():
+          gitdir = gitdir.replace('\\', '/')
         env[GIT_DIR] = gitdir
       cwd = None
     command.append(cmdv[0])
@@ -299,13 +298,10 @@ class GitCommand(object):
         command.append('--progress')
     command.extend(cmdv[1:])
 
-    if provide_stdin:
+    stdin = subprocess.PIPE if input else None
-      stdin = subprocess.PIPE
+    stdout = subprocess.PIPE if capture_stdout else None
-    else:
+    stderr = (subprocess.STDOUT if merge_output else
-      stdin = None
+              (subprocess.PIPE if capture_stderr else None))
-
-    stdout = subprocess.PIPE
-    stderr = subprocess.STDOUT if merge_output else subprocess.PIPE
 
     if IsTrace():
       global LAST_CWD
@@ -341,6 +337,8 @@ class GitCommand(object):
       p = subprocess.Popen(command,
                            cwd=cwd,
                            env=env,
+                           encoding='utf-8',
+                           errors='backslashreplace',
                            stdin=stdin,
                            stdout=stdout,
                            stderr=stderr)
@@ -351,7 +349,17 @@ class GitCommand(object):
       _add_ssh_client(p)
 
     self.process = p
-    self.stdin = p.stdin
+    if input:
+      if isinstance(input, str):
+        input = input.encode('utf-8')
+      p.stdin.write(input)
+      p.stdin.close()
+
+    try:
+      self.stdout, self.stderr = p.communicate()
+    finally:
+      _remove_ssh_client(p)
+    self.rc = p.wait()
 
   @staticmethod
   def _GetBasicEnv():
@@ -371,36 +379,4 @@ class GitCommand(object):
     return env
 
   def Wait(self):
-    try:
+    return self.rc
-      p = self.process
-      rc = self._CaptureOutput()
-    finally:
-      _remove_ssh_client(p)
-    return rc
-
-  def _CaptureOutput(self):
-    p = self.process
-    s_in = platform_utils.FileDescriptorStreams.create()
-    s_in.add(p.stdout, sys.stdout, 'stdout')
-    if p.stderr is not None:
-      s_in.add(p.stderr, sys.stderr, 'stderr')
-    self.stdout = ''
-    self.stderr = ''
-
-    while not s_in.is_done:
-      in_ready = s_in.select()
-      for s in in_ready:
-        buf = s.read()
-        if not buf:
-          s_in.remove(s)
-          continue
-        if not hasattr(buf, 'encode'):
-          buf = buf.decode('utf-8', 'backslashreplace')
-        if s.std_name == 'stdout':
-          self.stdout += buf
-        else:
-          self.stderr += buf
-        if self.tee[s.std_name]:
-          s.dest.write(buf)
-          s.dest.flush()
-    return p.wait()

git_refs.py

@@ -131,11 +131,14 @@ class GitRefs(object):
     base = os.path.join(self._gitdir, prefix)
     for name in platform_utils.listdir(base):
       p = os.path.join(base, name)
-      if platform_utils.isdir(p):
+      # We don't implement the full ref validation algorithm, just the simple
+      # rules that would show up in local filesystems.
+      # https://git-scm.com/docs/git-check-ref-format
+      if name.startswith('.') or name.endswith('.lock'):
+        pass
+      elif platform_utils.isdir(p):
         self._mtime[prefix] = os.path.getmtime(base)
         self._ReadLoose(prefix + name + '/')
-      elif name.endswith('.lock'):
-        pass
       else:
         self._ReadLoose1(p, prefix + name)
 
@@ -144,7 +147,7 @@ class GitRefs(object):
       with open(path) as fd:
         mtime = os.path.getmtime(path)
         ref_id = fd.readline()
-    except (IOError, OSError):
+    except (OSError, UnicodeError):
       return
 
     try:

git_superproject.py

@@ -22,13 +22,13 @@ Examples:
   project_commit_ids = superproject.UpdateProjectsRevisionId(projects)
 """
 
+import hashlib
 import os
 import sys
 
 from error import BUG_REPORT_URL
 from git_command import GitCommand
 from git_refs import R_HEADS
-import platform_utils
 
 _SUPERPROJECT_GIT_NAME = 'superproject.git'
 _SUPERPROJECT_MANIFEST_NAME = 'superproject_override.xml'
@@ -37,9 +37,9 @@ _SUPERPROJECT_MANIFEST_NAME = 'superproject_override.xml'
 class Superproject(object):
   """Get commit ids from superproject.
 
-  It does a 'git clone' of superproject and 'git ls-tree' to get list of commit ids
+  Initializes a local copy of a superproject for the manifest. This allows
-  for all projects. It contains project_commit_ids which is a dictionary with
+  lookup of commit ids for all projects. It contains _project_commit_ids which
-  project/commit id entries.
+  is a dictionary with project/commit id entries.
   """
   def __init__(self, manifest, repodir, superproject_dir='exp-superproject'):
     """Initializes superproject.
@@ -58,8 +58,12 @@ class Superproject(object):
     self._superproject_path = os.path.join(self._repodir, superproject_dir)
     self._manifest_path = os.path.join(self._superproject_path,
                                        _SUPERPROJECT_MANIFEST_NAME)
-    self._work_git = os.path.join(self._superproject_path,
+    git_name = ''
-                                  _SUPERPROJECT_GIT_NAME)
+    if self._manifest.superproject:
+      remote_name = self._manifest.superproject['remote'].name
+      git_name = hashlib.md5(remote_name.encode('utf8')).hexdigest() + '-'
+    self._work_git_name = git_name + _SUPERPROJECT_GIT_NAME
+    self._work_git = os.path.join(self._superproject_path, self._work_git_name)
 
   @property
   def project_commit_ids(self):
@@ -77,20 +81,15 @@ class Superproject(object):
       branch = branch[len(R_HEADS):]
     return branch
 
-  def _Clone(self, url):
+  def _Init(self):
-    """Do a 'git clone' for the given url.
+    """Sets up a local Git repository to get a copy of a superproject.
-
-    Args:
-      url: superproject's url to be passed to git clone.
 
     Returns:
-      True if git clone is successful, or False.
+      True if initialization is successful, or False.
     """
     if not os.path.exists(self._superproject_path):
       os.mkdir(self._superproject_path)
-    cmd = ['clone', url, '--filter', 'blob:none', '--bare']
+    cmd = ['init', '--bare', self._work_git_name]
-    if self._branch:
-      cmd += ['--branch', self._branch]
     p = GitCommand(None,
                    cmd,
                    cwd=self._superproject_path,
@@ -98,24 +97,27 @@ class Superproject(object):
                    capture_stderr=True)
     retval = p.Wait()
     if retval:
-      # `git clone` is documented to produce an exit status of `128` if
+      print('repo: error: git init call failed with return code: %r, stderr: %r' %
-      # the requested url or branch are not present in the configuration.
-      print('repo: error: git clone call failed with return code: %r, stderr: %r' %
             (retval, p.stderr), file=sys.stderr)
       return False
     return True
 
-  def _Fetch(self):
+  def _Fetch(self, url):
-    """Do a 'git fetch' to to fetch the latest content.
+    """Fetches a local copy of a superproject for the manifest based on url.
+
+    Args:
+      url: superproject's url.
 
     Returns:
-      True if 'git fetch' is successful, or False.
+      True if fetch is successful, or False.
     """
     if not os.path.exists(self._work_git):
       print('git fetch missing drectory: %s' % self._work_git,
             file=sys.stderr)
       return False
-    cmd = ['fetch', 'origin', '+refs/heads/*:refs/heads/*', '--prune']
+    cmd = ['fetch', url, '--force', '--no-tags', '--filter', 'blob:none']
+    if self._branch:
+      cmd += [self._branch + ':' + self._branch]
     p = GitCommand(None,
                    cmd,
                    cwd=self._work_git,
@@ -129,7 +131,7 @@ class Superproject(object):
     return True
 
   def _LsTree(self):
-    """Returns the data from 'git ls-tree ...'.
+    """Gets the commit ids for all projects.
 
     Works only in git repositories.
 
@@ -153,14 +155,12 @@ class Superproject(object):
     if retval == 0:
       data = p.stdout
     else:
-      # `git clone` is documented to produce an exit status of `128` if
-      # the requested url or branch are not present in the configuration.
       print('repo: error: git ls-tree call failed with return code: %r, stderr: %r' % (
           retval, p.stderr), file=sys.stderr)
     return data
 
   def Sync(self):
-    """Sync superproject either by git clone/fetch.
+    """Gets a local copy of a superproject for the manifest.
 
     Returns:
       True if sync of superproject is successful, or False.
@@ -179,16 +179,9 @@ class Superproject(object):
             file=sys.stderr)
       return False
 
-    do_clone = True
+    if not self._Init():
-    if os.path.exists(self._superproject_path):
+      return False
-      if not self._Fetch():
+    if not self._Fetch(url):
-        # If fetch fails due to a corrupted git directory, then do a git clone.
-        platform_utils.rmtree(self._superproject_path)
-      else:
-        do_clone = False
-    if do_clone:
-      if not self._Clone(url):
-        print('error: git clone failed for url: %s' % url, file=sys.stderr)
       return False
     return True
 
@@ -203,7 +196,8 @@ class Superproject(object):
 
     data = self._LsTree()
     if not data:
-      print('error: git ls-tree failed for superproject', file=sys.stderr)
+      print('error: git ls-tree failed to return data for superproject',
+            file=sys.stderr)
       return None
 
     # Parse lines like the following to select lines starting with '160000' and

gitc_utils.py

@@ -13,6 +13,7 @@
 # limitations under the License.
 
 import os
+import multiprocessing
 import platform
 import re
 import sys
@@ -35,6 +36,15 @@ def parse_clientdir(gitc_fs_path):
   return wrapper.Wrapper().gitc_parse_clientdir(gitc_fs_path)
 
 
+def _get_project_revision(args):
+  """Worker for _set_project_revisions to lookup one project remote."""
+  (i, url, expr) = args
+  gitcmd = git_command.GitCommand(
+      None, ['ls-remote', url, expr], capture_stdout=True, cwd='/tmp')
+  rc = gitcmd.Wait()
+  return (i, rc, gitcmd.stdout.split('\t', 1)[0])
+
+
 def _set_project_revisions(projects):
   """Sets the revisionExpr for a list of projects.
 
@@ -47,22 +57,24 @@ def _set_project_revisions(projects):
   """
   # Retrieve the commit id for each project based off of it's current
   # revisionExpr and it is not already a commit id.
-  project_gitcmds = [(
+  with multiprocessing.Pool(NUM_BATCH_RETRIEVE_REVISIONID) as pool:
-      project, git_command.GitCommand(None,
+    results_iter = pool.imap_unordered(
-                                      ['ls-remote',
+        _get_project_revision,
-                                       project.remote.url,
+        ((i, project.remote.url, project.revisionExpr)
-                                       project.revisionExpr],
+         for i, project in enumerate(projects)
-                                      capture_stdout=True, cwd='/tmp'))
+         if not git_config.IsId(project.revisionExpr)),
-                     for project in projects if not git_config.IsId(project.revisionExpr)]
+        chunksize=8)
-  for proj, gitcmd in project_gitcmds:
+    for (i, rc, revisionExpr) in results_iter:
-    if gitcmd.Wait():
+      project = projects[i]
-      print('FATAL: Failed to retrieve revisionExpr for %s' % proj)
+      if rc:
+        print('FATAL: Failed to retrieve revisionExpr for %s' % project.name)
+        pool.terminate()
         sys.exit(1)
-    revisionExpr = gitcmd.stdout.split('\t')[0]
       if not revisionExpr:
+        pool.terminate()
         raise ManifestParseError('Invalid SHA-1 revision project %s (%s)' %
-                               (proj.remote.url, proj.revisionExpr))
+                                 (project.remote.url, project.revisionExpr))
-    proj.revisionExpr = revisionExpr
+      project.revisionExpr = revisionExpr
 
 
 def _manifest_groups(manifest):
@@ -123,11 +135,7 @@ def generate_gitc_manifest(gitc_manifest, manifest, paths=None):
         else:
           proj.revisionExpr = gitc_proj.revisionExpr
 
-  index = 0
+  _set_project_revisions(projects)
-  while index < len(projects):
-    _set_project_revisions(
-        projects[index:(index + NUM_BATCH_RETRIEVE_REVISIONID)])
-    index += NUM_BATCH_RETRIEVE_REVISIONID
 
   if gitc_manifest is not None:
     for path, proj in gitc_manifest.paths.items():

manifest_xml.py

@@ -533,7 +533,6 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
   def _output_manifest_project_extras(self, p, e):
     """Manifests can modify e if they support extra project attributes."""
-    pass
 
   @property
   def paths(self):
@@ -625,16 +624,22 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
         b = b[len(R_HEADS):]
       self.branch = b
 
+      # The manifestFile was specified by the user which is why we allow include
+      # paths to point anywhere.
       nodes = []
-      nodes.append(self._ParseManifestXml(self.manifestFile,
+      nodes.append(self._ParseManifestXml(
-                                          self.manifestProject.worktree))
+          self.manifestFile, self.manifestProject.worktree,
+          restrict_includes=False))
 
       if self._load_local_manifests and self.local_manifests:
         try:
           for local_file in sorted(platform_utils.listdir(self.local_manifests)):
             if local_file.endswith('.xml'):
               local = os.path.join(self.local_manifests, local_file)
-              nodes.append(self._ParseManifestXml(local, self.repodir))
+              # Since local manifests are entirely managed by the user, allow
+              # them to point anywhere the user wants.
+              nodes.append(self._ParseManifestXml(
+                  local, self.repodir, restrict_includes=False))
         except OSError:
           pass
 
@@ -652,7 +657,19 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
       self._loaded = True
 
-  def _ParseManifestXml(self, path, include_root, parent_groups=''):
+  def _ParseManifestXml(self, path, include_root, parent_groups='',
+                        restrict_includes=True):
+    """Parse a manifest XML and return the computed nodes.
+
+    Args:
+      path: The XML file to read & parse.
+      include_root: The path to interpret include "name"s relative to.
+      parent_groups: The groups to apply to this projects.
+      restrict_includes: Whether to constrain the "name" attribute of includes.
+
+    Returns:
+      List of XML nodes.
+    """
     try:
       root = xml.dom.minidom.parse(path)
     except (OSError, xml.parsers.expat.ExpatError) as e:
@@ -671,6 +688,11 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     for node in manifest.childNodes:
       if node.nodeName == 'include':
         name = self._reqatt(node, 'name')
+        if restrict_includes:
+          msg = self._CheckLocalPath(name)
+          if msg:
+            raise ManifestInvalidPathError(
+                '<include> invalid "name": %s: %s' % (name, msg))
         include_groups = ''
         if parent_groups:
           include_groups = parent_groups
@@ -678,13 +700,13 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
           include_groups = node.getAttribute('groups') + ',' + include_groups
         fp = os.path.join(include_root, name)
         if not os.path.isfile(fp):
-          raise ManifestParseError("include %s doesn't exist or isn't a file"
+          raise ManifestParseError("include [%s/]%s doesn't exist or isn't a file"
-                                   % (name,))
+                                   % (include_root, name))
         try:
           nodes.extend(self._ParseManifestXml(fp, include_root, include_groups))
         # should isolate this to the exact exception, but that's
         # tricky.  actual parsing implementation may vary.
-        except (KeyboardInterrupt, RuntimeError, SystemExit):
+        except (KeyboardInterrupt, RuntimeError, SystemExit, ManifestParseError):
           raise
         except Exception as e:
           raise ManifestParseError(
@@ -980,6 +1002,10 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     reads a <project> element from the manifest file
     """
     name = self._reqatt(node, 'name')
+    msg = self._CheckLocalPath(name, dir_ok=True)
+    if msg:
+      raise ManifestInvalidPathError(
+          '<project> invalid "name": %s: %s' % (name, msg))
     if parent:
       name = self._JoinName(parent.name, name)
 
@@ -1000,9 +1026,11 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     path = node.getAttribute('path')
     if not path:
       path = name
-    if path.startswith('/'):
+    else:
-      raise ManifestParseError("project %s path cannot be absolute in %s" %
+      msg = self._CheckLocalPath(path, dir_ok=True)
-                               (name, self.manifestFile))
+      if msg:
+        raise ManifestInvalidPathError(
+            '<project> invalid "path": %s: %s' % (path, msg))
 
     rebase = XmlBool(node, 'rebase', True)
     sync_c = XmlBool(node, 'sync-c', False)
@@ -1122,8 +1150,33 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     return relpath, worktree, gitdir, objdir
 
   @staticmethod
-  def _CheckLocalPath(path, symlink=False):
+  def _CheckLocalPath(path, dir_ok=False, cwd_dot_ok=False):
-    """Verify |path| is reasonable for use in <copyfile> & <linkfile>."""
+    """Verify |path| is reasonable for use in filesystem paths.
+
+    Used with <copyfile> & <linkfile> & <project> elements.
+
+    This only validates the |path| in isolation: it does not check against the
+    current filesystem state.  Thus it is suitable as a first-past in a parser.
+
+    It enforces a number of constraints:
+    * No empty paths.
+    * No "~" in paths.
+    * No Unicode codepoints that filesystems might elide when normalizing.
+    * No relative path components like "." or "..".
+    * No absolute paths.
+    * No ".git" or ".repo*" path components.
+
+    Args:
+      path: The path name to validate.
+      dir_ok: Whether |path| may force a directory (e.g. end in a /).
+      cwd_dot_ok: Whether |path| may be just ".".
+
+    Returns:
+      None if |path| is OK, a failure message otherwise.
+    """
+    if not path:
+      return 'empty paths not allowed'
+
     if '~' in path:
       return '~ not allowed (due to 8.3 filenames on Windows filesystems)'
 
@@ -1166,12 +1219,12 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
     # Some people use src="." to create stable links to projects.  Lets allow
     # that but reject all other uses of "." to keep things simple.
-    if parts != ['.']:
+    if not cwd_dot_ok or parts != ['.']:
       for part in set(parts):
         if part in {'.', '..', '.git'} or part.startswith('.repo'):
           return 'bad component: %s' % (part,)
 
-    if not symlink and resep.match(path[-1]):
+    if not dir_ok and resep.match(path[-1]):
       return 'dirs not allowed'
 
     # NB: The two abspath checks here are to handle platforms with multiple
@@ -1203,7 +1256,8 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
 
     # |src| is the file we read from or path we point to for symlinks.
     # It is relative to the top of the git project checkout.
-    msg = cls._CheckLocalPath(src, symlink=element == 'linkfile')
+    is_linkfile = element == 'linkfile'
+    msg = cls._CheckLocalPath(src, dir_ok=is_linkfile, cwd_dot_ok=is_linkfile)
     if msg:
       raise ManifestInvalidPathError(
           '<%s> invalid "src": %s: %s' % (element, src, msg))
@@ -1302,7 +1356,7 @@ class GitcManifest(XmlManifest):
 
   def _ParseProject(self, node, parent=None):
     """Override _ParseProject and add support for GITC specific attributes."""
-    return super(GitcManifest, self)._ParseProject(
+    return super()._ParseProject(
         node, parent=parent, old_revision=node.getAttribute('old-revision'))
 
   def _output_manifest_project_extras(self, p, e):
@@ -1326,7 +1380,7 @@ class RepoClient(XmlManifest):
     if manifest_file is None:
       manifest_file = os.path.join(repodir, MANIFEST_FILE_NAME)
     local_manifests = os.path.abspath(os.path.join(repodir, LOCAL_MANIFESTS_DIR_NAME))
-    super(RepoClient, self).__init__(repodir, manifest_file, local_manifests)
+    super().__init__(repodir, manifest_file, local_manifests)
 
     # TODO: Completely separate manifest logic out of the client.
     self.manifest = self
@@ -1341,6 +1395,5 @@ class GitcClient(RepoClient, GitcManifest):
     self.gitc_client_dir = os.path.join(gitc_utils.get_gitc_manifest_dir(),
                                         gitc_client_name)
 
-    super(GitcManifest, self).__init__(
+    super().__init__(repodir, os.path.join(self.gitc_client_dir, '.manifest'))
-        repodir, os.path.join(self.gitc_client_dir, '.manifest'))
     self.isGitcClient = True

platform_utils.py

@@ -15,11 +15,8 @@
 import errno
 import os
 import platform
-from queue import Queue
-import select
 import shutil
 import stat
-from threading import Thread
 
 
 def isWindows():
@@ -31,161 +28,6 @@ def isWindows():
   return platform.system() == "Windows"
 
 
-class FileDescriptorStreams(object):
-  """ Platform agnostic abstraction enabling non-blocking I/O over a
-  collection of file descriptors. This abstraction is required because
-  fctnl(os.O_NONBLOCK) is not supported on Windows.
-  """
-  @classmethod
-  def create(cls):
-    """ Factory method: instantiates the concrete class according to the
-    current platform.
-    """
-    if isWindows():
-      return _FileDescriptorStreamsThreads()
-    else:
-      return _FileDescriptorStreamsNonBlocking()
-
-  def __init__(self):
-    self.streams = []
-
-  def add(self, fd, dest, std_name):
-    """ Wraps an existing file descriptor as a stream.
-    """
-    self.streams.append(self._create_stream(fd, dest, std_name))
-
-  def remove(self, stream):
-    """ Removes a stream, when done with it.
-    """
-    self.streams.remove(stream)
-
-  @property
-  def is_done(self):
-    """ Returns True when all streams have been processed.
-    """
-    return len(self.streams) == 0
-
-  def select(self):
-    """ Returns the set of streams that have data available to read.
-    The returned streams each expose a read() and a close() method.
-    When done with a stream, call the remove(stream) method.
-    """
-    raise NotImplementedError
-
-  def _create_stream(self, fd, dest, std_name):
-    """ Creates a new stream wrapping an existing file descriptor.
-    """
-    raise NotImplementedError
-
-
-class _FileDescriptorStreamsNonBlocking(FileDescriptorStreams):
-  """ Implementation of FileDescriptorStreams for platforms that support
-  non blocking I/O.
-  """
-  def __init__(self):
-    super(_FileDescriptorStreamsNonBlocking, self).__init__()
-    self._poll = select.poll()
-    self._fd_to_stream = {}
-
-  class Stream(object):
-    """ Encapsulates a file descriptor """
-
-    def __init__(self, fd, dest, std_name):
-      self.fd = fd
-      self.dest = dest
-      self.std_name = std_name
-      self.set_non_blocking()
-
-    def set_non_blocking(self):
-      import fcntl
-      flags = fcntl.fcntl(self.fd, fcntl.F_GETFL)
-      fcntl.fcntl(self.fd, fcntl.F_SETFL, flags | os.O_NONBLOCK)
-
-    def fileno(self):
-      return self.fd.fileno()
-
-    def read(self):
-      return self.fd.read(4096)
-
-    def close(self):
-      self.fd.close()
-
-  def _create_stream(self, fd, dest, std_name):
-    stream = self.Stream(fd, dest, std_name)
-    self._fd_to_stream[stream.fileno()] = stream
-    self._poll.register(stream, select.POLLIN)
-    return stream
-
-  def remove(self, stream):
-    self._poll.unregister(stream)
-    del self._fd_to_stream[stream.fileno()]
-    super(_FileDescriptorStreamsNonBlocking, self).remove(stream)
-
-  def select(self):
-    return [self._fd_to_stream[fd] for fd, _ in self._poll.poll()]
-
-
-class _FileDescriptorStreamsThreads(FileDescriptorStreams):
-  """ Implementation of FileDescriptorStreams for platforms that don't support
-  non blocking I/O. This implementation requires creating threads issuing
-  blocking read operations on file descriptors.
-  """
-
-  def __init__(self):
-    super(_FileDescriptorStreamsThreads, self).__init__()
-    # The queue is shared accross all threads so we can simulate the
-    # behavior of the select() function
-    self.queue = Queue(10)  # Limit incoming data from streams
-
-  def _create_stream(self, fd, dest, std_name):
-    return self.Stream(fd, dest, std_name, self.queue)
-
-  def select(self):
-    # Return only one stream at a time, as it is the most straighforward
-    # thing to do and it is compatible with the select() function.
-    item = self.queue.get()
-    stream = item.stream
-    stream.data = item.data
-    return [stream]
-
-  class QueueItem(object):
-    """ Item put in the shared queue """
-
-    def __init__(self, stream, data):
-      self.stream = stream
-      self.data = data
-
-  class Stream(object):
-    """ Encapsulates a file descriptor """
-
-    def __init__(self, fd, dest, std_name, queue):
-      self.fd = fd
-      self.dest = dest
-      self.std_name = std_name
-      self.queue = queue
-      self.data = None
-      self.thread = Thread(target=self.read_to_queue)
-      self.thread.daemon = True
-      self.thread.start()
-
-    def close(self):
-      self.fd.close()
-
-    def read(self):
-      data = self.data
-      self.data = None
-      return data
-
-    def read_to_queue(self):
-      """ The thread function: reads everything from the file descriptor into
-      the shared queue and terminates when reaching EOF.
-      """
-      for line in iter(self.fd.readline, b''):
-        self.queue.put(_FileDescriptorStreamsThreads.QueueItem(self, line))
-      self.fd.close()
-      self.queue.put(_FileDescriptorStreamsThreads.QueueItem(self, b''))
-
-
 def symlink(source, link_name):
   """Creates a symbolic link pointing to source named link_name.
   Note: On Windows, source must exist on disk, as the implementation needs

progress.py

@@ -25,18 +25,44 @@ _NOT_TTY = not os.isatty(2)
 CSI_ERASE_LINE = '\x1b[2K'
 
 
+def duration_str(total):
+  """A less noisy timedelta.__str__.
+
+  The default timedelta stringification contains a lot of leading zeros and
+  uses microsecond resolution.  This makes for noisy output.
+  """
+  hours, rem = divmod(total, 3600)
+  mins, secs = divmod(rem, 60)
+  ret = '%.3fs' % (secs,)
+  if mins:
+    ret = '%im%s' % (mins, ret)
+  if hours:
+    ret = '%ih%s' % (hours, ret)
+  return ret
+
+
 class Progress(object):
-  def __init__(self, title, total=0, units='', print_newline=False,
+  def __init__(self, title, total=0, units='', print_newline=False):
-               always_print_percentage=False):
     self._title = title
     self._total = total
     self._done = 0
-    self._lastp = -1
     self._start = time()
     self._show = False
     self._units = units
     self._print_newline = print_newline
-    self._always_print_percentage = always_print_percentage
+    # Only show the active jobs section if we run more than one in parallel.
+    self._show_jobs = False
+    self._active = 0
+
+  def start(self, name):
+    self._active += 1
+    if not self._show_jobs:
+      self._show_jobs = self._active > 1
+    self.update(inc=0, msg='started ' + name)
+
+  def finish(self, name):
+    self.update(msg='finished ' + name)
+    self._active -= 1
 
   def update(self, inc=1, msg=''):
     self._done += inc
@@ -58,35 +84,40 @@ class Progress(object):
       sys.stderr.flush()
     else:
       p = (100 * self._done) / self._total
-
+      if self._show_jobs:
-      if self._lastp != p or self._always_print_percentage:
+        jobs = '[%d job%s] ' % (self._active, 's' if self._active > 1 else '')
-        self._lastp = p
+      else:
-        sys.stderr.write('%s\r%s: %3d%% (%d%s/%d%s)%s%s%s' % (
+        jobs = ''
+      sys.stderr.write('%s\r%s: %2d%% %s(%d%s/%d%s)%s%s%s' % (
           CSI_ERASE_LINE,
           self._title,
           p,
+          jobs,
           self._done, self._units,
           self._total, self._units,
           ' ' if msg else '', msg,
-            "\n" if self._print_newline else ""))
+          '\n' if self._print_newline else ''))
       sys.stderr.flush()
 
   def end(self):
     if _NOT_TTY or IsTrace() or not self._show:
       return
 
+    duration = duration_str(time() - self._start)
     if self._total <= 0:
-      sys.stderr.write('%s\r%s: %d, done.\n' % (
+      sys.stderr.write('%s\r%s: %d, done in %s\n' % (
           CSI_ERASE_LINE,
           self._title,
-          self._done))
+          self._done,
+          duration))
       sys.stderr.flush()
     else:
       p = (100 * self._done) / self._total
-      sys.stderr.write('%s\r%s: %3d%% (%d%s/%d%s), done.\n' % (
+      sys.stderr.write('%s\r%s: %3d%% (%d%s/%d%s), done in %s\n' % (
           CSI_ERASE_LINE,
           self._title,
           p,
           self._done, self._units,
-          self._total, self._units))
+          self._total, self._units,
+          duration))
       sys.stderr.flush()

project.py

@@ -232,7 +232,7 @@ class ReviewableBranch(object):
 class StatusColoring(Coloring):
 
   def __init__(self, config):
-    Coloring.__init__(self, config, 'status')
+    super().__init__(config, 'status')
     self.project = self.printer('header', attr='bold')
     self.branch = self.printer('header', attr='bold')
     self.nobranch = self.printer('nobranch', fg='red')
@@ -246,7 +246,7 @@ class StatusColoring(Coloring):
 class DiffColoring(Coloring):
 
   def __init__(self, config):
-    Coloring.__init__(self, config, 'diff')
+    super().__init__(config, 'diff')
     self.project = self.printer('header', attr='bold')
     self.fail = self.printer('fail', fg='red')
 
@@ -832,10 +832,12 @@ class Project(object):
 
     return 'DIRTY'
 
-  def PrintWorkTreeDiff(self, absolute_paths=False):
+  def PrintWorkTreeDiff(self, absolute_paths=False, output_redir=None):
     """Prints the status of the repository to stdout.
     """
     out = DiffColoring(self.config)
+    if output_redir:
+      out.redirect(output_redir)
     cmd = ['diff']
     if out.is_on:
       cmd.append('--color')
@@ -849,6 +851,7 @@ class Project(object):
                      cmd,
                      capture_stdout=True,
                      capture_stderr=True)
+      p.Wait()
     except GitError as e:
       out.nl()
       out.project('project %s/' % self.relpath)
@@ -856,16 +859,11 @@ class Project(object):
       out.fail('%s', str(e))
       out.nl()
       return False
-    has_diff = False
+    if p.stdout:
-    for line in p.process.stdout:
-      if not hasattr(line, 'encode'):
-        line = line.decode()
-      if not has_diff:
       out.nl()
       out.project('project %s/' % self.relpath)
       out.nl()
-        has_diff = True
+      out.write(p.stdout)
-      print(line[:-1])
     return p.Wait() == 0
 
 # Publish / Upload ##
@@ -1041,6 +1039,7 @@ class Project(object):
   def Sync_NetworkHalf(self,
                        quiet=False,
                        verbose=False,
+                       output_redir=None,
                        is_new=None,
                        current_branch_only=False,
                        force_sync=False,
@@ -1082,6 +1081,12 @@ class Project(object):
         _warn("Cannot remove archive %s: %s", tarpath, str(e))
       self._CopyAndLinkFiles()
       return True
+
+    # If the shared object dir already exists, don't try to rebootstrap with a
+    # clone bundle download.  We should have the majority of objects already.
+    if clone_bundle and os.path.exists(self.objdir):
+      clone_bundle = False
+
     if is_new is None:
       is_new = not self.Exists
     if is_new:
@@ -1128,8 +1133,9 @@ class Project(object):
             (ID_RE.match(self.revisionExpr) and
              self._CheckForImmutableRevision())):
       if not self._RemoteFetch(
-              initial=is_new, quiet=quiet, verbose=verbose, alt_dir=alt_dir,
+              initial=is_new,
-              current_branch_only=current_branch_only,
+              quiet=quiet, verbose=verbose, output_redir=output_redir,
+              alt_dir=alt_dir, current_branch_only=current_branch_only,
               tags=tags, prune=prune, depth=depth,
               submodules=submodules, force_sync=force_sync,
               clone_filter=clone_filter, retry_fetches=retry_fetches):
@@ -1141,7 +1147,11 @@ class Project(object):
       alternates_file = os.path.join(self.gitdir, 'objects/info/alternates')
       if os.path.exists(alternates_file):
         cmd = ['repack', '-a', '-d']
-        if GitCommand(self, cmd, bare=True).Wait() != 0:
+        p = GitCommand(self, cmd, bare=True, capture_stdout=bool(output_redir),
+                       merge_output=bool(output_redir))
+        if p.stdout and output_redir:
+          output_redir.write(p.stdout)
+        if p.Wait() != 0:
           return False
         platform_utils.remove(alternates_file)
 
@@ -1953,6 +1963,7 @@ class Project(object):
                    initial=False,
                    quiet=False,
                    verbose=False,
+                   output_redir=None,
                    alt_dir=None,
                    tags=True,
                    prune=False,
@@ -2130,15 +2141,18 @@ class Project(object):
     ok = prune_tried = False
     for try_n in range(retry_fetches):
       gitcmd = GitCommand(self, cmd, bare=True, ssh_proxy=ssh_proxy,
-                          merge_output=True, capture_stdout=quiet)
+                          merge_output=True, capture_stdout=quiet or bool(output_redir))
+      if gitcmd.stdout and not quiet and output_redir:
+        output_redir.write(gitcmd.stdout)
       ret = gitcmd.Wait()
       if ret == 0:
         ok = True
         break
 
       # Retry later due to HTTP 429 Too Many Requests.
-      elif ('error:' in gitcmd.stderr and
+      elif (gitcmd.stdout and
-            'HTTP 429' in gitcmd.stderr):
+            'error:' in gitcmd.stdout and
+            'HTTP 429' in gitcmd.stdout):
         if not quiet:
           print('429 received, sleeping: %s sec' % retry_cur_sleep,
                 file=sys.stderr)
@@ -2151,8 +2165,9 @@ class Project(object):
 
       # If this is not last attempt, try 'git remote prune'.
       elif (try_n < retry_fetches - 1 and
-            'error:' in gitcmd.stderr and
+            gitcmd.stdout and
-            'git remote prune' in gitcmd.stderr and
+            'error:' in gitcmd.stdout and
+            'git remote prune' in gitcmd.stdout and
             not prune_tried):
         prune_tried = True
         prunecmd = GitCommand(self, ['remote', 'prune', name], bare=True,
@@ -2170,7 +2185,7 @@ class Project(object):
         # Git died with a signal, exit immediately
         break
       if not verbose:
-        print('%s:\n%s' % (self.name, gitcmd.stdout), file=sys.stderr)
+        print('\n%s:\n%s' % (self.name, gitcmd.stdout), file=sys.stderr)
       time.sleep(random.randint(30, 45))
 
     if initial:
@@ -2189,7 +2204,7 @@ class Project(object):
         # Sync the current branch only with depth set to None.
         # We always pass depth=None down to avoid infinite recursion.
         return self._RemoteFetch(
-            name=name, quiet=quiet, verbose=verbose,
+            name=name, quiet=quiet, verbose=verbose, output_redir=output_redir,
             current_branch_only=current_branch_only and depth,
             initial=False, alt_dir=alt_dir,
             depth=None, clone_filter=clone_filter)
@@ -2861,11 +2876,9 @@ class Project(object):
                      bare=False,
                      capture_stdout=True,
                      capture_stderr=True)
-      try:
+      p.Wait()
-        out = p.process.stdout.read()
-        if not hasattr(out, 'encode'):
-          out = out.decode()
       r = {}
+      out = p.stdout
       if out:
         out = iter(out[:-1].split('\0'))
         while out:
@@ -2901,8 +2914,6 @@ class Project(object):
             info.path = next(out)
           r[info.path] = info
       return r
-      finally:
-        p.Wait()
 
     def GetDotgitPath(self, subpath=None):
       """Return the full path to the .git dir.
@@ -3099,7 +3110,7 @@ class _Later(object):
 class _SyncColoring(Coloring):
 
   def __init__(self, config):
-    Coloring.__init__(self, config, 'reposync')
+    super().__init__(config, 'reposync')
     self.project = self.printer('header', attr='bold')
     self.info = self.printer('info')
     self.fail = self.printer('fail', fg='red')

repo

@@ -270,9 +270,9 @@ gpg_dir = os.path.join(home_dot_repo, 'gnupg')
 def GetParser(gitc_init=False):
   """Setup the CLI parser."""
   if gitc_init:
-    usage = 'repo gitc-init -u url -c client [options]'
+    usage = 'repo gitc-init -c client [options] [-u] url'
   else:
-    usage = 'repo init -u url [options]'
+    usage = 'repo init [options] [-u] url'
 
   parser = optparse.OptionParser(usage=usage)
 
@@ -289,8 +289,8 @@ def GetParser(gitc_init=False):
   group = parser.add_option_group('Manifest options')
   group.add_option('-u', '--manifest-url',
                    help='manifest repository location', metavar='URL')
-  group.add_option('-b', '--manifest-branch',
+  group.add_option('-b', '--manifest-branch', metavar='REVISION',
-                   help='manifest branch or revision', metavar='REVISION')
+                   help='manifest branch or revision (use HEAD for default)')
   group.add_option('-m', '--manifest-name',
                    help='initial manifest file', metavar='NAME.xml')
   cbr_opts = ['--current-branch']
@@ -521,6 +521,9 @@ def _Init(args, gitc_init=False):
   """
   parser = GetParser(gitc_init=gitc_init)
   opt, args = parser.parse_args(args)
+  if args:
+    if not opt.manifest_url:
+      opt.manifest_url = args.pop(0)
     if args:
       parser.print_usage()
       sys.exit(1)

subcmds/abandon.py

@@ -13,9 +13,12 @@
 # limitations under the License.
 
 from collections import defaultdict
+import functools
+import itertools
+import multiprocessing
 import sys
 
-from command import Command
+from command import Command, DEFAULT_LOCAL_JOBS, WORKER_BATCH_SIZE
 from git_command import git
 from progress import Progress
 
@@ -31,8 +34,10 @@ deleting it (and all its history) from your local repository.
 
 It is equivalent to "git branch -D <branchname>".
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
   def _Options(self, p):
+    super()._Options(p)
     p.add_option('-q', '--quiet',
                  action='store_true', default=False,
                  help='be quiet')
@@ -51,35 +56,49 @@ It is equivalent to "git branch -D <branchname>".
     else:
       args.insert(0, "'All local branches'")
 
+  def _ExecuteOne(self, opt, nb, project):
+    """Abandon one project."""
+    if opt.all:
+      branches = project.GetBranches()
+    else:
+      branches = [nb]
+
+    ret = {}
+    for name in branches:
+      status = project.AbandonBranch(name)
+      if status is not None:
+        ret[name] = status
+    return (ret, project)
+
   def Execute(self, opt, args):
     nb = args[0]
     err = defaultdict(list)
     success = defaultdict(list)
     all_projects = self.GetProjects(args[1:])
 
-    pm = Progress('Abandon %s' % nb, len(all_projects))
+    def _ProcessResults(states):
-    for project in all_projects:
+      for (results, project) in states:
+        for branch, status in results.items():
+          if status:
+            success[branch].append(project)
+          else:
+            err[branch].append(project)
         pm.update()
 
-      if opt.all:
+    pm = Progress('Abandon %s' % nb, len(all_projects))
-        branches = list(project.GetBranches().keys())
+    # NB: Multiprocessing is heavy, so don't spin it up for one job.
+    if len(all_projects) == 1 or opt.jobs == 1:
+      _ProcessResults(self._ExecuteOne(opt, nb, x) for x in all_projects)
     else:
-        branches = [nb]
+      with multiprocessing.Pool(opt.jobs) as pool:
-
+        states = pool.imap_unordered(
-      for name in branches:
+            functools.partial(self._ExecuteOne, opt, nb), all_projects,
-        status = project.AbandonBranch(name)
+            chunksize=WORKER_BATCH_SIZE)
-        if status is not None:
+        _ProcessResults(states)
-          if status:
-            success[name].append(project)
-          else:
-            err[name].append(project)
     pm.end()
 
-    width = 25
+    width = max(itertools.chain(
-    for name in branches:
+        [25], (len(x) for x in itertools.chain(success, err))))
-      if width < len(name):
-        width = len(name)
-
     if err:
       for br in err.keys():
         err_msg = "error: cannot abandon %s" % br

subcmds/branches.py

@@ -16,15 +16,7 @@ import itertools
 import multiprocessing
 import sys
 from color import Coloring
-from command import Command
+from command import Command, DEFAULT_LOCAL_JOBS, WORKER_BATCH_SIZE
-
-# Number of projects to submit to a single worker process at a time.
-# This number represents a tradeoff between the overhead of IPC and finer
-# grained opportunity for parallelism. This particular value was chosen by
-# iterating through powers of two until the overall performance no longer
-# improved. The performance of this batch size is not a function of the
-# number of cores on the system.
-WORKER_BATCH_SIZE = 32
 
 
 class BranchColoring(Coloring):
@@ -103,17 +95,7 @@ the branch appears in, or does not appear in.  If no project list
 is shown, then the branch appears in all projects.
 
 """
-
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
-  def _Options(self, p):
-    """Add flags to CLI parser for this subcommand."""
-    default_jobs = min(multiprocessing.cpu_count(), 8)
-    p.add_option(
-        '-j',
-        '--jobs',
-        type=int,
-        default=default_jobs,
-        help='Number of worker processes to spawn '
-        '(default: %s)' % default_jobs)
 
   def Execute(self, opt, args):
     projects = self.GetProjects(args)

subcmds/checkout.py

@@ -12,8 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import functools
+import multiprocessing
 import sys
-from command import Command
+
+from command import Command, DEFAULT_LOCAL_JOBS, WORKER_BATCH_SIZE
 from progress import Progress
 
 
@@ -31,27 +34,41 @@ The command is equivalent to:
 
   repo forall [<project>...] -c git checkout <branchname>
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
   def ValidateOptions(self, opt, args):
     if not args:
       self.Usage()
 
+  def _ExecuteOne(self, nb, project):
+    """Checkout one project."""
+    return (project.CheckoutBranch(nb), project)
+
   def Execute(self, opt, args):
     nb = args[0]
     err = []
     success = []
     all_projects = self.GetProjects(args[1:])
 
-    pm = Progress('Checkout %s' % nb, len(all_projects))
+    def _ProcessResults(results):
-    for project in all_projects:
+      for status, project in results:
-      pm.update()
-
-      status = project.CheckoutBranch(nb)
         if status is not None:
           if status:
             success.append(project)
           else:
             err.append(project)
+        pm.update()
+
+    pm = Progress('Checkout %s' % nb, len(all_projects))
+    # NB: Multiprocessing is heavy, so don't spin it up for one job.
+    if len(all_projects) == 1 or opt.jobs == 1:
+      _ProcessResults(self._ExecuteOne(nb, x) for x in all_projects)
+    else:
+      with multiprocessing.Pool(opt.jobs) as pool:
+        results = pool.imap_unordered(
+            functools.partial(self._ExecuteOne, nb), all_projects,
+            chunksize=WORKER_BATCH_SIZE)
+        _ProcessResults(results)
     pm.end()
 
     if err:

subcmds/cherry_pick.py

@@ -72,11 +72,9 @@ change id will be added.
       new_msg = self._Reformat(old_msg, sha1)
 
       p = GitCommand(None, ['commit', '--amend', '-F', '-'],
-                     provide_stdin=True,
+                     input=new_msg,
                      capture_stdout=True,
                      capture_stderr=True)
-      p.stdin.write(new_msg)
-      p.stdin.close()
       if p.Wait() != 0:
         print("error: Failed to update commit message", file=sys.stderr)
         sys.exit(1)

subcmds/diff.py

@@ -12,7 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-from command import PagedCommand
+import functools
+import io
+import multiprocessing
+
+from command import DEFAULT_LOCAL_JOBS, PagedCommand, WORKER_BATCH_SIZE
 
 
 class Diff(PagedCommand):
@@ -25,15 +29,45 @@ The -u option causes '%prog' to generate diff output with file paths
 relative to the repository root, so the output can be applied
 to the Unix 'patch' command.
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
   def _Options(self, p):
+    super()._Options(p)
     p.add_option('-u', '--absolute',
                  dest='absolute', action='store_true',
                  help='Paths are relative to the repository root')
 
+  def _DiffHelper(self, absolute, project):
+    """Obtains the diff for a specific project.
+
+    Args:
+      absolute: Paths are relative to the root.
+      project: Project to get status of.
+
+    Returns:
+      The status of the project.
+    """
+    buf = io.StringIO()
+    ret = project.PrintWorkTreeDiff(absolute, output_redir=buf)
+    return (ret, buf.getvalue())
+
   def Execute(self, opt, args):
     ret = 0
-    for project in self.GetProjects(args):
+    all_projects = self.GetProjects(args)
+
+    # NB: Multiprocessing is heavy, so don't spin it up for one job.
+    if len(all_projects) == 1 or opt.jobs == 1:
+      for project in all_projects:
         if not project.PrintWorkTreeDiff(opt.absolute):
           ret = 1
+    else:
+      with multiprocessing.Pool(opt.jobs) as pool:
+        states = pool.imap(functools.partial(self._DiffHelper, opt.absolute),
+                           all_projects, WORKER_BATCH_SIZE)
+        for (state, output) in states:
+          if output:
+            print(output, end='')
+          if not state:
+            ret = 1
+
     return ret

subcmds/download.py

@@ -16,7 +16,7 @@ import re
 import sys
 
 from command import Command
-from error import GitError
+from error import GitError, NoSuchProjectError
 
 CHANGE_RE = re.compile(r'^([1-9][0-9]*)(?:[/\.-]([1-9][0-9]*))?$')
 
@@ -60,6 +60,7 @@ If no project is specified try to use current directory as a project.
       if m:
         if not project:
           project = self.GetProjects(".")[0]
+          print('Defaulting to cwd project', project.name)
         chg_id = int(m.group(1))
         if m.group(2):
           ps_id = int(m.group(2))
@@ -76,7 +77,23 @@ If no project is specified try to use current directory as a project.
                 ps_id = max(int(match.group(1)), ps_id)
         to_get.append((project, chg_id, ps_id))
       else:
-        project = self.GetProjects([a])[0]
+        projects = self.GetProjects([a])
+        if len(projects) > 1:
+          # If the cwd is one of the projects, assume they want that.
+          try:
+            project = self.GetProjects('.')[0]
+          except NoSuchProjectError:
+            project = None
+          if project not in projects:
+            print('error: %s matches too many projects; please re-run inside '
+                  'the project checkout.' % (a,), file=sys.stderr)
+            for project in projects:
+              print('  %s/ @ %s' % (project.relpath, project.revisionExpr),
+                    file=sys.stderr)
+            sys.exit(1)
+        else:
+          project = projects[0]
+          print('Defaulting to cwd project', project.name)
     return to_get
 
   def ValidateOptions(self, opt, args):

subcmds/forall.py

@@ -13,6 +13,8 @@
 # limitations under the License.
 
 import errno
+import functools
+import io
 import multiprocessing
 import re
 import os
@@ -21,8 +23,7 @@ import sys
 import subprocess
 
 from color import Coloring
-from command import Command, MirrorSafeCommand
+from command import DEFAULT_LOCAL_JOBS, Command, MirrorSafeCommand, WORKER_BATCH_SIZE
-import platform_utils
 
 _CAN_COLOR = [
     'branch',
@@ -113,12 +114,17 @@ terminal and are not redirected.
 If -e is used, when a command exits unsuccessfully, '%prog' will abort
 without iterating through the remaining projects.
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
-  def _Options(self, p):
+  @staticmethod
-    def cmd(option, opt_str, value, parser):
+  def _cmd_option(option, _opt_str, _value, parser):
     setattr(parser.values, option.dest, list(parser.rargs))
     while parser.rargs:
       del parser.rargs[0]
+
+  def _Options(self, p):
+    super()._Options(p)
+
     p.add_option('-r', '--regex',
                  dest='regex', action='store_true',
                  help="Execute the command only on projects matching regex or wildcard expression")
@@ -133,7 +139,7 @@ without iterating through the remaining projects.
                  help='Command (and arguments) to execute',
                  dest='command',
                  action='callback',
-                 callback=cmd)
+                 callback=self._cmd_option)
     p.add_option('-e', '--abort-on-errors',
                  dest='abort_on_errors', action='store_true',
                  help='Abort if a command exits unsuccessfully')
@@ -148,38 +154,10 @@ without iterating through the remaining projects.
     g.add_option('-v', '--verbose',
                  dest='verbose', action='store_true',
                  help='Show command error messages')
-    g.add_option('-j', '--jobs',
-                 dest='jobs', action='store', type='int', default=1,
-                 help='number of commands to execute simultaneously')
 
   def WantPager(self, opt):
     return opt.project_header and opt.jobs == 1
 
-  def _SerializeProject(self, project):
-    """ Serialize a project._GitGetByExec instance.
-
-    project._GitGetByExec is not pickle-able. Instead of trying to pass it
-    around between processes, make a dict ourselves containing only the
-    attributes that we need.
-
-    """
-    if not self.manifest.IsMirror:
-      lrev = project.GetRevisionId()
-    else:
-      lrev = None
-    return {
-        'name': project.name,
-        'relpath': project.relpath,
-        'remote_name': project.remote.name,
-        'lrev': lrev,
-        'rrev': project.revisionExpr,
-        'annotations': dict((a.name, a.value) for a in project.annotations),
-        'gitdir': project.gitdir,
-        'worktree': project.worktree,
-        'upstream': project.upstream,
-        'dest_branch': project.dest_branch,
-    }
-
   def ValidateOptions(self, opt, args):
     if not opt.command:
       self.Usage()
@@ -234,60 +212,50 @@ without iterating through the remaining projects.
 
     os.environ['REPO_COUNT'] = str(len(projects))
 
-    pool = multiprocessing.Pool(opt.jobs, InitWorker)
     try:
       config = self.manifest.manifestProject.config
+      with multiprocessing.Pool(opt.jobs, InitWorker) as pool:
         results_it = pool.imap(
-          DoWorkWrapper,
+            functools.partial(DoWorkWrapper, mirror, opt, cmd, shell, config),
-          self.ProjectArgs(projects, mirror, opt, cmd, shell, config))
+            enumerate(projects),
-      pool.close()
+            chunksize=WORKER_BATCH_SIZE)
-      for r in results_it:
+        first = True
+        for (r, output) in results_it:
+          if output:
+            if first:
+              first = False
+            elif opt.project_header:
+              print()
+            # To simplify the DoWorkWrapper, take care of automatic newlines.
+            end = '\n'
+            if output[-1] == '\n':
+              end = ''
+            print(output, end=end)
           rc = rc or r
           if r != 0 and opt.abort_on_errors:
             raise Exception('Aborting due to previous error')
     except (KeyboardInterrupt, WorkerKeyboardInterrupt):
       # Catch KeyboardInterrupt raised inside and outside of workers
-      print('Interrupted - terminating the pool')
-      pool.terminate()
       rc = rc or errno.EINTR
     except Exception as e:
       # Catch any other exceptions raised
       print('Got an error, terminating the pool: %s: %s' %
             (type(e).__name__, e),
             file=sys.stderr)
-      pool.terminate()
       rc = rc or getattr(e, 'errno', 1)
-    finally:
-      pool.join()
     if rc != 0:
       sys.exit(rc)
 
-  def ProjectArgs(self, projects, mirror, opt, cmd, shell, config):
-    for cnt, p in enumerate(projects):
-      try:
-        project = self._SerializeProject(p)
-      except Exception as e:
-        print('Project list error on project %s: %s: %s' %
-              (p.name, type(e).__name__, e),
-              file=sys.stderr)
-        return
-      except KeyboardInterrupt:
-        print('Project list interrupted',
-              file=sys.stderr)
-        return
-      yield [mirror, opt, cmd, shell, cnt, config, project]
-
 
 class WorkerKeyboardInterrupt(Exception):
   """ Keyboard interrupt exception for worker processes. """
-  pass
 
 
 def InitWorker():
   signal.signal(signal.SIGINT, signal.SIG_IGN)
 
 
-def DoWorkWrapper(args):
+def DoWorkWrapper(mirror, opt, cmd, shell, config, args):
   """ A wrapper around the DoWork() method.
 
   Catch the KeyboardInterrupt exceptions here and re-raise them as a different,
@@ -295,11 +263,11 @@ def DoWorkWrapper(args):
   and making the parent hang indefinitely.
 
   """
-  project = args.pop()
+  cnt, project = args
   try:
-    return DoWork(project, *args)
+    return DoWork(project, mirror, opt, cmd, shell, cnt, config)
   except KeyboardInterrupt:
-    print('%s: Worker interrupted' % project['name'])
+    print('%s: Worker interrupted' % project.name)
     raise WorkerKeyboardInterrupt()
 
 
@@ -311,94 +279,57 @@ def DoWork(project, mirror, opt, cmd, shell, cnt, config):
       val = ''
     env[name] = val
 
-  setenv('REPO_PROJECT', project['name'])
+  setenv('REPO_PROJECT', project.name)
-  setenv('REPO_PATH', project['relpath'])
+  setenv('REPO_PATH', project.relpath)
-  setenv('REPO_REMOTE', project['remote_name'])
+  setenv('REPO_REMOTE', project.remote.name)
-  setenv('REPO_LREV', project['lrev'])
+  setenv('REPO_LREV', '' if mirror else project.GetRevisionId())
-  setenv('REPO_RREV', project['rrev'])
+  setenv('REPO_RREV', project.revisionExpr)
-  setenv('REPO_UPSTREAM', project['upstream'])
+  setenv('REPO_UPSTREAM', project.upstream)
-  setenv('REPO_DEST_BRANCH', project['dest_branch'])
+  setenv('REPO_DEST_BRANCH', project.dest_branch)
   setenv('REPO_I', str(cnt + 1))
-  for name in project['annotations']:
+  for annotation in project.annotations:
-    setenv("REPO__%s" % (name), project['annotations'][name])
+    setenv("REPO__%s" % (annotation.name), annotation.value)
 
   if mirror:
-    setenv('GIT_DIR', project['gitdir'])
+    setenv('GIT_DIR', project.gitdir)
-    cwd = project['gitdir']
+    cwd = project.gitdir
   else:
-    cwd = project['worktree']
+    cwd = project.worktree
 
   if not os.path.exists(cwd):
     # Allow the user to silently ignore missing checkouts so they can run on
     # partial checkouts (good for infra recovery tools).
     if opt.ignore_missing:
-      return 0
+      return (0, '')
+
+    output = ''
     if ((opt.project_header and opt.verbose)
             or not opt.project_header):
-      print('skipping %s/' % project['relpath'], file=sys.stderr)
+      output = 'skipping %s/' % project.relpath
-    return 1
+    return (1, output)
 
-  if opt.project_header:
+  if opt.verbose:
-    stdin = subprocess.PIPE
+    stderr = subprocess.STDOUT
-    stdout = subprocess.PIPE
-    stderr = subprocess.PIPE
   else:
-    stdin = None
+    stderr = subprocess.DEVNULL
-    stdout = None
-    stderr = None
 
-  p = subprocess.Popen(cmd,
+  result = subprocess.run(
-                       cwd=cwd,
+      cmd, cwd=cwd, shell=shell, env=env, check=False,
-                       shell=shell,
+      encoding='utf-8', errors='replace',
-                       env=env,
+      stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=stderr)
-                       stdin=stdin,
-                       stdout=stdout,
-                       stderr=stderr)
 
+  output = result.stdout
   if opt.project_header:
+    if output:
+      buf = io.StringIO()
       out = ForallColoring(config)
-    out.redirect(sys.stdout)
+      out.redirect(buf)
-    empty = True
-    errbuf = ''
-
-    p.stdin.close()
-    s_in = platform_utils.FileDescriptorStreams.create()
-    s_in.add(p.stdout, sys.stdout, 'stdout')
-    s_in.add(p.stderr, sys.stderr, 'stderr')
-
-    while not s_in.is_done:
-      in_ready = s_in.select()
-      for s in in_ready:
-        buf = s.read().decode()
-        if not buf:
-          s_in.remove(s)
-          s.close()
-          continue
-
-        if not opt.verbose:
-          if s.std_name == 'stderr':
-            errbuf += buf
-            continue
-
-        if empty and out:
-          if not cnt == 0:
-            out.nl()
-
       if mirror:
-            project_header_path = project['name']
+        project_header_path = project.name
       else:
-            project_header_path = project['relpath']
+        project_header_path = project.relpath
-          out.project('project %s/', project_header_path)
+      out.project('project %s/' % project_header_path)
       out.nl()
-          out.flush()
+      buf.write(output)
-          if errbuf:
+      output = buf.getvalue()
-            sys.stderr.write(errbuf)
+  return (result.returncode, output)
-            sys.stderr.flush()
-            errbuf = ''
-          empty = False
-
-        s.dest.write(buf)
-        s.dest.flush()
-
-  r = p.wait()
-  return r

subcmds/gitc_init.py

@@ -47,7 +47,7 @@ use for this GITC client.
 """
 
   def _Options(self, p):
-    super(GitcInit, self)._Options(p, gitc_init=True)
+    super()._Options(p, gitc_init=True)
     g = p.add_option_group('GITC options')
     g.add_option('-f', '--manifest-file',
                  dest='manifest_file',
@@ -64,7 +64,7 @@ use for this GITC client.
       sys.exit(1)
     self.client_dir = os.path.join(gitc_utils.get_gitc_manifest_dir(),
                                    gitc_client)
-    super(GitcInit, self).Execute(opt, args)
+    super().Execute(opt, args)
 
     manifest_file = self.manifest.manifestFile
     if opt.manifest_file:

subcmds/grep.py

@@ -62,11 +62,8 @@ contain a line that matches both expressions:
 
 """
 
-  def _Options(self, p):
+  @staticmethod
-    def carry(option,
+  def _carry_option(_option, opt_str, value, parser):
-              opt_str,
-              value,
-              parser):
     pt = getattr(parser.values, 'cmd_argv', None)
     if pt is None:
       pt = []
@@ -82,9 +79,10 @@ contain a line that matches both expressions:
     if value is not None:
       pt.append(value)
 
+  def _Options(self, p):
     g = p.add_option_group('Sources')
     g.add_option('--cached',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Search the index, instead of the work tree')
     g.add_option('-r', '--revision',
                  dest='revision', action='append', metavar='TREEish',
@@ -92,66 +90,66 @@ contain a line that matches both expressions:
 
     g = p.add_option_group('Pattern')
     g.add_option('-e',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  metavar='PATTERN', type='str',
                  help='Pattern to search for')
     g.add_option('-i', '--ignore-case',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Ignore case differences')
     g.add_option('-a', '--text',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help="Process binary files as if they were text")
     g.add_option('-I',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help="Don't match the pattern in binary files")
     g.add_option('-w', '--word-regexp',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Match the pattern only at word boundaries')
     g.add_option('-v', '--invert-match',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Select non-matching lines')
     g.add_option('-G', '--basic-regexp',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Use POSIX basic regexp for patterns (default)')
     g.add_option('-E', '--extended-regexp',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Use POSIX extended regexp for patterns')
     g.add_option('-F', '--fixed-strings',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Use fixed strings (not regexp) for pattern')
 
     g = p.add_option_group('Pattern Grouping')
     g.add_option('--all-match',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Limit match to lines that have all patterns')
     g.add_option('--and', '--or', '--not',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Boolean operators to combine patterns')
     g.add_option('-(', '-)',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Boolean operator grouping')
 
     g = p.add_option_group('Output')
     g.add_option('-n',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Prefix the line number to matching lines')
     g.add_option('-C',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  metavar='CONTEXT', type='str',
                  help='Show CONTEXT lines around match')
     g.add_option('-B',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  metavar='CONTEXT', type='str',
                  help='Show CONTEXT lines before match')
     g.add_option('-A',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  metavar='CONTEXT', type='str',
                  help='Show CONTEXT lines after match')
     g.add_option('-l', '--name-only', '--files-with-matches',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Show only file names containing matching lines')
     g.add_option('-L', '--files-without-match',
-                 action='callback', callback=carry,
+                 action='callback', callback=self._carry_option,
                  help='Show only file names not containing matching lines')
 
   def Execute(self, opt, args):

subcmds/init.py

@@ -32,9 +32,9 @@ from wrapper import Wrapper
 
 class Init(InteractiveCommand, MirrorSafeCommand):
   common = True
-  helpSummary = "Initialize repo in the current directory"
+  helpSummary = "Initialize a repo client checkout in the current directory"
   helpUsage = """
-%prog [options]
+%prog [options] [manifest url]
 """
   helpDescription = """
 The '%prog' command is run once to install and initialize repo.
@@ -42,9 +42,13 @@ The latest repo source code and manifest collection is downloaded
 from the server and is installed in the .repo/ directory in the
 current working directory.
 
+When creating a new checkout, the manifest URL is the only required setting.
+It may be specified using the --manifest-url option, or as the first optional
+argument.
+
 The optional -b argument can be used to select the manifest branch
 to checkout and use.  If no branch is specified, the remote's default
-branch is used.
+branch is used.  This is equivalent to using -b HEAD.
 
 The optional -m argument can be used to specify an alternate manifest
 to be used. If no manifest is specified, the manifest default.xml
@@ -90,9 +94,8 @@ to update the working directory files.
     g.add_option('-u', '--manifest-url',
                  dest='manifest_url',
                  help='manifest repository location', metavar='URL')
-    g.add_option('-b', '--manifest-branch',
+    g.add_option('-b', '--manifest-branch', metavar='REVISION',
-                 dest='manifest_branch',
+                 help='manifest branch or revision (use HEAD for default)')
-                 help='manifest branch or revision', metavar='REVISION')
     cbr_opts = ['--current-branch']
     # The gitc-init subcommand allocates -c itself, but a lot of init users
     # want -c, so try to satisfy both as best we can.
@@ -196,7 +199,7 @@ to update the working directory files.
 
     if is_new:
       if not opt.manifest_url:
-        print('fatal: manifest url (-u) is required.', file=sys.stderr)
+        print('fatal: manifest url is required.', file=sys.stderr)
         sys.exit(1)
 
       if not opt.quiet:
@@ -228,6 +231,11 @@ to update the working directory files.
       r.Save()
 
     if opt.manifest_branch:
+      if opt.manifest_branch == 'HEAD':
+        opt.manifest_branch = m.ResolveRemoteHead()
+        if opt.manifest_branch is None:
+          print('fatal: unable to resolve HEAD', file=sys.stderr)
+          sys.exit(1)
       m.revisionExpr = opt.manifest_branch
     else:
       if is_new:
@@ -498,7 +506,15 @@ to update the working directory files.
       self.OptionParser.error('--mirror and --archive cannot be used together.')
 
     if args:
-      self.OptionParser.error('init takes no arguments')
+      if opt.manifest_url:
+        self.OptionParser.error(
+            '--manifest-url option and URL argument both specified: only use '
+            'one to select the manifest URL.')
+
+      opt.manifest_url = args.pop(0)
+
+      if args:
+        self.OptionParser.error('too many arguments to init')
 
   def Execute(self, opt, args):
     git_require(MIN_GIT_VERSION_HARD, fail=True)

subcmds/start.py

@@ -12,10 +12,12 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import functools
+import multiprocessing
 import os
 import sys
 
-from command import Command
+from command import Command, DEFAULT_LOCAL_JOBS, WORKER_BATCH_SIZE
 from git_config import IsImmutable
 from git_command import git
 import gitc_utils
@@ -33,8 +35,10 @@ class Start(Command):
 '%prog' begins a new branch of development, starting from the
 revision specified in the manifest.
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
   def _Options(self, p):
+    super()._Options(p)
     p.add_option('--all',
                  dest='all', action='store_true',
                  help='begin branch in all projects')
@@ -51,6 +55,26 @@ revision specified in the manifest.
     if not git.check_ref_format('heads/%s' % nb):
       self.OptionParser.error("'%s' is not a valid name" % nb)
 
+  def _ExecuteOne(self, opt, nb, project):
+    """Start one project."""
+    # If the current revision is immutable, such as a SHA1, a tag or
+    # a change, then we can't push back to it. Substitute with
+    # dest_branch, if defined; or with manifest default revision instead.
+    branch_merge = ''
+    if IsImmutable(project.revisionExpr):
+      if project.dest_branch:
+        branch_merge = project.dest_branch
+      else:
+        branch_merge = self.manifest.default.revisionExpr
+
+    try:
+      ret = project.StartBranch(
+          nb, branch_merge=branch_merge, revision=opt.revision)
+    except Exception as e:
+      print('error: unable to checkout %s: %s' % (project.name, e), file=sys.stderr)
+      ret = False
+    return (ret, project)
+
   def Execute(self, opt, args):
     nb = args[0]
     err = []
@@ -82,11 +106,8 @@ revision specified in the manifest.
       if not os.path.exists(os.getcwd()):
         os.chdir(self.manifest.topdir)
 
-    pm = Progress('Starting %s' % nb, len(all_projects))
+      pm = Progress('Syncing %s' % nb, len(all_projects))
       for project in all_projects:
-      pm.update()
-
-      if self.gitc_manifest:
         gitc_project = self.gitc_manifest.paths[project.relpath]
         # Sync projects that have not been opened.
         if not gitc_project.already_synced:
@@ -99,20 +120,25 @@ revision specified in the manifest.
           sync_buf = SyncBuffer(self.manifest.manifestProject.config)
           project.Sync_LocalHalf(sync_buf)
           project.revisionId = gitc_project.old_revision
+        pm.update()
+      pm.end()
 
-      # If the current revision is immutable, such as a SHA1, a tag or
+    def _ProcessResults(results):
-      # a change, then we can't push back to it. Substitute with
+      for (result, project) in results:
-      # dest_branch, if defined; or with manifest default revision instead.
+        if not result:
-      branch_merge = ''
-      if IsImmutable(project.revisionExpr):
-        if project.dest_branch:
-          branch_merge = project.dest_branch
-        else:
-          branch_merge = self.manifest.default.revisionExpr
-
-      if not project.StartBranch(
-              nb, branch_merge=branch_merge, revision=opt.revision):
           err.append(project)
+        pm.update()
+
+    pm = Progress('Starting %s' % nb, len(all_projects))
+    # NB: Multiprocessing is heavy, so don't spin it up for one job.
+    if len(all_projects) == 1 or opt.jobs == 1:
+      _ProcessResults(self._ExecuteOne(opt, nb, x) for x in all_projects)
+    else:
+      with multiprocessing.Pool(opt.jobs) as pool:
+        results = pool.imap_unordered(
+            functools.partial(self._ExecuteOne, opt, nb), all_projects,
+            chunksize=WORKER_BATCH_SIZE)
+        _ProcessResults(results)
     pm.end()
 
     if err:

subcmds/status.py

@@ -14,10 +14,11 @@
 
 import functools
 import glob
+import io
 import multiprocessing
 import os
 
-from command import PagedCommand
+from command import DEFAULT_LOCAL_JOBS, PagedCommand, WORKER_BATCH_SIZE
 
 from color import Coloring
 import platform_utils
@@ -76,11 +77,10 @@ the following meanings:
  d:  deleted       (    in index, not in work tree                )
 
 """
+  PARALLEL_JOBS = DEFAULT_LOCAL_JOBS
 
   def _Options(self, p):
-    p.add_option('-j', '--jobs',
+    super()._Options(p)
-                 dest='jobs', action='store', type='int', default=2,
-                 help="number of projects to check simultaneously")
     p.add_option('-o', '--orphans',
                  dest='orphans', action='store_true',
                  help="include objects in working directory outside of repo projects")
@@ -100,7 +100,9 @@ the following meanings:
     Returns:
       The status of the project.
     """
-    return project.PrintWorkTreeStatus(quiet=quiet)
+    buf = io.StringIO()
+    ret = project.PrintWorkTreeStatus(quiet=quiet, output_redir=buf)
+    return (ret, buf.getvalue())
 
   def _FindOrphans(self, dirs, proj_dirs, proj_dirs_parents, outstring):
     """find 'dirs' that are present in 'proj_dirs_parents' but not in 'proj_dirs'"""
@@ -129,8 +131,13 @@ the following meanings:
           counter += 1
     else:
       with multiprocessing.Pool(opt.jobs) as pool:
-        states = pool.map(functools.partial(self._StatusHelper, opt.quiet), all_projects)
+        states = pool.imap(functools.partial(self._StatusHelper, opt.quiet),
-        counter += states.count('CLEAN')
+                           all_projects, chunksize=WORKER_BATCH_SIZE)
+        for (state, output) in states:
+          if output:
+            print(output, end='')
+          if state == 'CLEAN':
+            counter += 1
     if not opt.quiet and len(all_projects) == counter:
       print('nothing to commit (working directory clean)')
 

subcmds/sync.py

@@ -12,8 +12,11 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+import functools
 import http.cookiejar as cookielib
+import io
 import json
+import multiprocessing
 import netrc
 from optparse import SUPPRESS_HELP
 import os
@@ -55,7 +58,7 @@ import git_superproject
 import gitc_utils
 from project import Project
 from project import RemoteSpec
-from command import Command, MirrorSafeCommand
+from command import Command, MirrorSafeCommand, WORKER_BATCH_SIZE
 from error import RepoChangedException, GitError, ManifestParseError
 import platform_utils
 from project import SyncBuffer
@@ -68,11 +71,6 @@ _ONE_DAY_S = 24 * 60 * 60
 
 class _FetchError(Exception):
   """Internal error thrown in _FetchHelper() when we don't want stack trace."""
-  pass
-
-
-class _CheckoutError(Exception):
-  """Internal error thrown in _CheckoutOne() when we don't want stack trace."""
 
 
 class Sync(Command, MirrorSafeCommand):
@@ -178,12 +176,14 @@ If the remote SSH daemon is Gerrit Code Review, version 2.0.10 or
 later is required to fix a server side protocol bug.
 
 """
+  PARALLEL_JOBS = 1
 
   def _Options(self, p, show_smart=True):
     try:
-      self.jobs = self.manifest.default.sync_j
+      self.PARALLEL_JOBS = self.manifest.default.sync_j
     except ManifestParseError:
-      self.jobs = 1
+      pass
+    super()._Options(p)
 
     p.add_option('-f', '--force-broken',
                  dest='force_broken', action='store_true',
@@ -223,9 +223,6 @@ later is required to fix a server side protocol bug.
     p.add_option('-q', '--quiet',
                  dest='output_mode', action='store_false',
                  help='only show errors')
-    p.add_option('-j', '--jobs',
-                 dest='jobs', action='store', type='int',
-                 help="projects to fetch simultaneously (default %d)" % self.jobs)
     p.add_option('-m', '--manifest-name',
                  dest='manifest_name',
                  help='temporary manifest to use for this sync', metavar='NAME.xml')
@@ -356,11 +353,15 @@ later is required to fix a server side protocol bug.
     # - We always make sure we unlock the lock if we locked it.
     start = time.time()
     success = False
+    buf = io.StringIO()
+    with lock:
+      pm.start(project.name)
     try:
       try:
         success = project.Sync_NetworkHalf(
             quiet=opt.quiet,
             verbose=opt.verbose,
+            output_redir=buf,
             current_branch_only=opt.current_branch_only,
             force_sync=opt.force_sync,
             clone_bundle=opt.clone_bundle,
@@ -376,6 +377,10 @@ later is required to fix a server side protocol bug.
         lock.acquire()
         did_lock = True
 
+        output = buf.getvalue()
+        if opt.verbose and output:
+          pm.update(inc=0, msg=output.rstrip())
+
         if not success:
           err_event.set()
           print('error: Cannot fetch %s from %s'
@@ -385,7 +390,6 @@ later is required to fix a server side protocol bug.
             raise _FetchError()
 
         fetched.add(project.gitdir)
-        pm.update(msg=project.name)
       except _FetchError:
         pass
       except Exception as e:
@@ -394,7 +398,9 @@ later is required to fix a server side protocol bug.
         err_event.set()
         raise
     finally:
-      if did_lock:
+      if not did_lock:
+        lock.acquire()
+      pm.finish(project.name)
       lock.release()
       finish = time.time()
       self.event_log.AddSync(project, event_log.TASK_SYNC_NETWORK,
@@ -405,8 +411,7 @@ later is required to fix a server side protocol bug.
   def _Fetch(self, projects, opt, err_event):
     fetched = set()
     lock = _threading.Lock()
-    pm = Progress('Fetching projects', len(projects),
+    pm = Progress('Fetching', len(projects))
-                  always_print_percentage=opt.quiet)
 
     objdir_project_map = dict()
     for project in projects:
@@ -417,7 +422,7 @@ later is required to fix a server side protocol bug.
     for project_list in objdir_project_map.values():
       # Check for any errors before running any more tasks.
       # ...we'll let existing threads finish, though.
-      if err_event.isSet() and opt.fail_fast:
+      if err_event.is_set() and opt.fail_fast:
         break
 
       sem.acquire()
@@ -450,147 +455,80 @@ later is required to fix a server side protocol bug.
 
     return fetched
 
-  def _CheckoutWorker(self, opt, sem, project, *args, **kwargs):
+  def _CheckoutOne(self, opt, project):
-    """Main function of the fetch threads.
-
-    Delegates most of the work to _CheckoutOne.
-
-    Args:
-      opt: Program options returned from optparse.  See _Options().
-      projects: Projects to fetch.
-      sem: We'll release() this semaphore when we exit so that another thread
-          can be started up.
-      *args, **kwargs: Remaining arguments to pass to _CheckoutOne. See the
-          _CheckoutOne docstring for details.
-    """
-    try:
-      return self._CheckoutOne(opt, project, *args, **kwargs)
-    finally:
-      sem.release()
-
-  def _CheckoutOne(self, opt, project, lock, pm, err_event, err_results):
     """Checkout work tree for one project
 
     Args:
       opt: Program options returned from optparse.  See _Options().
       project: Project object for the project to checkout.
-      lock: Lock for accessing objects that are shared amongst multiple
-          _CheckoutWorker() threads.
-      pm: Instance of a Project object.  We will call pm.update() (with our
-          lock held).
-      err_event: We'll set this event in the case of an error (after printing
-          out info about the error).
-      err_results: A list of strings, paths to git repos where checkout
-          failed.
 
     Returns:
       Whether the fetch was successful.
     """
-    # We'll set to true once we've locked the lock.
-    did_lock = False
-
-    # Encapsulate everything in a try/except/finally so that:
-    # - We always set err_event in the case of an exception.
-    # - We always make sure we unlock the lock if we locked it.
     start = time.time()
     syncbuf = SyncBuffer(self.manifest.manifestProject.config,
                          detach_head=opt.detach_head)
     success = False
-    try:
     try:
       project.Sync_LocalHalf(syncbuf, force_sync=opt.force_sync)
-
-        # Lock around all the rest of the code, since printing, updating a set
-        # and Progress.update() are not thread safe.
-        lock.acquire()
       success = syncbuf.Finish()
-        did_lock = True
-
-        if not success:
-          err_event.set()
-          print('error: Cannot checkout %s' % (project.name),
-                file=sys.stderr)
-          raise _CheckoutError()
-
-        pm.update(msg=project.name)
-      except _CheckoutError:
-        pass
     except Exception as e:
       print('error: Cannot checkout %s: %s: %s' %
             (project.name, type(e).__name__, str(e)),
             file=sys.stderr)
-        err_event.set()
       raise
-    finally:
+
-      if did_lock:
     if not success:
-          err_results.append(project.relpath)
+      print('error: Cannot checkout %s' % (project.name), file=sys.stderr)
-        lock.release()
     finish = time.time()
-      self.event_log.AddSync(project, event_log.TASK_SYNC_LOCAL,
+    return (success, project, start, finish)
-                             start, finish, success)
 
-    return success
+  def _Checkout(self, all_projects, opt, err_results):
-
-  def _Checkout(self, all_projects, opt, err_event, err_results):
     """Checkout projects listed in all_projects
 
     Args:
       all_projects: List of all projects that should be checked out.
       opt: Program options returned from optparse.  See _Options().
-      err_event: We'll set this event in the case of an error (after printing
+      err_results: A list of strings, paths to git repos where checkout failed.
-          out info about the error).
-      err_results: A list of strings, paths to git repos where checkout
-          failed.
     """
+    ret = True
 
-    # Perform checkouts in multiple threads when we are using partial clone.
+    # Only checkout projects with worktrees.
-    # Without partial clone, all needed git objects are already downloaded,
+    all_projects = [x for x in all_projects if x.worktree]
-    # in this situation it's better to use only one process because the checkout
-    # would be mostly disk I/O; with partial clone, the objects are only
-    # downloaded when demanded (at checkout time), which is similar to the
-    # Sync_NetworkHalf case and parallelism would be helpful.
-    if self.manifest.CloneFilter:
-      syncjobs = self.jobs
-    else:
-      syncjobs = 1
 
-    lock = _threading.Lock()
+    pm = Progress('Checking out', len(all_projects))
-    pm = Progress('Checking out projects', len(all_projects))
 
-    threads = set()
+    def _ProcessResults(results):
-    sem = _threading.Semaphore(syncjobs)
+      for (success, project, start, finish) in results:
-
+        self.event_log.AddSync(project, event_log.TASK_SYNC_LOCAL,
-    for project in all_projects:
+                               start, finish, success)
         # Check for any errors before running any more tasks.
         # ...we'll let existing threads finish, though.
-      if err_event.isSet() and opt.fail_fast:
+        if not success:
-        break
+          err_results.append(project.relpath)
+          if opt.fail_fast:
+            return False
+        pm.update(msg=project.name)
+      return True
 
-      sem.acquire()
+    # NB: Multiprocessing is heavy, so don't spin it up for one job.
-      if project.worktree:
+    if len(all_projects) == 1 or opt.jobs == 1:
-        kwargs = dict(opt=opt,
+      if not _ProcessResults(self._CheckoutOne(opt, x) for x in all_projects):
-                      sem=sem,
+        ret = False
-                      project=project,
-                      lock=lock,
-                      pm=pm,
-                      err_event=err_event,
-                      err_results=err_results)
-        if syncjobs > 1:
-          t = _threading.Thread(target=self._CheckoutWorker,
-                                kwargs=kwargs)
-          # Ensure that Ctrl-C will not freeze the repo process.
-          t.daemon = True
-          threads.add(t)
-          t.start()
     else:
-          self._CheckoutWorker(**kwargs)
+      with multiprocessing.Pool(opt.jobs) as pool:
-
+        results = pool.imap_unordered(
-    for t in threads:
+            functools.partial(self._CheckoutOne, opt),
-      t.join()
+            all_projects,
+            chunksize=WORKER_BATCH_SIZE)
+        if not _ProcessResults(results):
+          ret = False
+          pool.close()
 
     pm.end()
 
+    return ret
+
   def _GCProjects(self, projects, opt, err_event):
     gc_gitdirs = {}
     for project in projects:
@@ -640,7 +578,7 @@ later is required to fix a server side protocol bug.
         sem.release()
 
     for bare_git in gc_gitdirs.values():
-      if err_event.isSet() and opt.fail_fast:
+      if err_event.is_set() and opt.fail_fast:
         break
       sem.acquire()
       t = _threading.Thread(target=GC, args=(bare_git,))
@@ -891,7 +829,9 @@ later is required to fix a server side protocol bug.
     else:
       self._UpdateManifestProject(opt, mp, manifest_name)
 
-    if opt.use_superproject:
+    if (opt.use_superproject or
+        self.manifest.manifestProject.config.GetBoolean(
+            'repo.superproject')):
       manifest_name = self._UpdateProjectsRevisionId(opt, args)
 
     if self.gitc_manifest:
@@ -935,7 +875,6 @@ later is required to fix a server side protocol bug.
 
     err_network_sync = False
     err_update_projects = False
-    err_checkout = False
 
     self._fetch_times = _FetchTimes(self.manifest)
     if not opt.local_only:
@@ -951,7 +890,7 @@ later is required to fix a server side protocol bug.
       _PostRepoFetch(rp, opt.repo_verify)
       if opt.network_only:
         # bail out now; the rest touches the working tree
-        if err_event.isSet():
+        if err_event.is_set():
           print('\nerror: Exited sync due to fetch errors.\n', file=sys.stderr)
           sys.exit(1)
         return
@@ -978,7 +917,7 @@ later is required to fix a server side protocol bug.
         fetched.update(self._Fetch(missing, opt, err_event))
 
       # If we saw an error, exit with code 1 so that other scripts can check.
-      if err_event.isSet():
+      if err_event.is_set():
         err_network_sync = True
         if opt.fail_fast:
           print('\nerror: Exited sync due to fetch errors.\n'
@@ -1000,10 +939,10 @@ later is required to fix a server side protocol bug.
         sys.exit(1)
 
     err_results = []
-    self._Checkout(all_projects, opt, err_event, err_results)
-    if err_event.isSet():
-      err_checkout = True
     # NB: We don't exit here because this is the last step.
+    err_checkout = not self._Checkout(all_projects, opt, err_results)
+    if err_checkout:
+      err_event.set()
 
     # If there's a notice that's supposed to print at the end of the sync, print
     # it now...
@@ -1011,7 +950,7 @@ later is required to fix a server side protocol bug.
       print(self.manifest.notice)
 
     # If we saw an error, exit with code 1 so that other scripts can check.
-    if err_event.isSet():
+    if err_event.is_set():
       print('\nerror: Unable to fully sync the tree.', file=sys.stderr)
       if err_network_sync:
         print('error: Downloading network changes failed.', file=sys.stderr)
@@ -1086,20 +1025,11 @@ def _VerifyTag(project):
   env['GNUPGHOME'] = gpg_dir
 
   cmd = [GIT, 'tag', '-v', cur]
-  proc = subprocess.Popen(cmd,
+  result = subprocess.run(cmd, stdout=subprocess.PIPE, stderr=subprocess.STDOUT,
-                          stdout=subprocess.PIPE,
+                          env=env, check=False)
-                          stderr=subprocess.PIPE,
+  if result.returncode:
-                          env=env)
-  out = proc.stdout.read()
-  proc.stdout.close()
-
-  err = proc.stderr.read()
-  proc.stderr.close()
-
-  if proc.wait() != 0:
     print(file=sys.stderr)
-    print(out, file=sys.stderr)
+    print(result.stdout, file=sys.stderr)
-    print(err, file=sys.stderr)
     print(file=sys.stderr)
     return False
   return True

tests/test_error.py

@@ -0,0 +1,53 @@
+# Copyright 2021 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the error.py module."""
+
+import inspect
+import pickle
+import unittest
+
+import error
+
+
+class PickleTests(unittest.TestCase):
+  """Make sure all our custom exceptions can be pickled."""
+
+  def getExceptions(self):
+    """Return all our custom exceptions."""
+    for name in dir(error):
+      cls = getattr(error, name)
+      if isinstance(cls, type) and issubclass(cls, Exception):
+        yield cls
+
+  def testExceptionLookup(self):
+    """Make sure our introspection logic works."""
+    classes = list(self.getExceptions())
+    self.assertIn(error.HookError, classes)
+    # Don't assert the exact number to avoid being a change-detector test.
+    self.assertGreater(len(classes), 10)
+
+  def testPickle(self):
+    """Try to pickle all the exceptions."""
+    for cls in self.getExceptions():
+      args = inspect.getfullargspec(cls.__init__).args[1:]
+      obj = cls(*args)
+      p = pickle.dumps(obj)
+      try:
+        newobj = pickle.loads(p)
+      except Exception as e:  # pylint: disable=broad-except
+        self.fail('Class %s is unable to be pickled: %s\n'
+                  'Incomplete super().__init__(...) call?' % (cls, e))
+      self.assertIsInstance(newobj, cls)
+      self.assertEqual(str(obj), str(newobj))

tests/test_git_superproject.py

@@ -97,17 +97,17 @@ class SuperprojectTestCase(unittest.TestCase):
     with mock.patch.object(self._superproject, '_GetBranch', return_value='junk'):
       self.assertFalse(superproject.Sync())
 
-  def test_superproject_get_superproject_mock_clone(self):
+  def test_superproject_get_superproject_mock_init(self):
-    """Test with _Clone failing."""
+    """Test with _Init failing."""
-    with mock.patch.object(self._superproject, '_Clone', return_value=False):
+    with mock.patch.object(self._superproject, '_Init', return_value=False):
       self.assertFalse(self._superproject.Sync())
 
   def test_superproject_get_superproject_mock_fetch(self):
-    """Test with _Fetch failing and _clone being called."""
+    """Test with _Fetch failing."""
-    with mock.patch.object(self._superproject, '_Clone', return_value=True):
+    with mock.patch.object(self._superproject, '_Init', return_value=True):
       os.mkdir(self._superproject._superproject_path)
       with mock.patch.object(self._superproject, '_Fetch', return_value=False):
-        self.assertTrue(self._superproject.Sync())
+        self.assertFalse(self._superproject.Sync())
 
   def test_superproject_get_all_project_commit_ids_mock_ls_tree(self):
     """Test with LsTree being a mock."""
@@ -116,7 +116,8 @@ class SuperprojectTestCase(unittest.TestCase):
             '160000 commit e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06\tbootable/recovery\x00'
             '120000 blob acc2cbdf438f9d2141f0ae424cec1d8fc4b5d97f\tbootstrap.bash\x00'
             '160000 commit ade9b7a0d874e25fff4bf2552488825c6f111928\tbuild/bazel\x00')
-    with mock.patch.object(self._superproject, '_Clone', return_value=True):
+    with mock.patch.object(self._superproject, '_Init', return_value=True):
+      with mock.patch.object(self._superproject, '_Fetch', return_value=True):
         with mock.patch.object(self._superproject, '_LsTree', return_value=data):
           commit_ids = self._superproject._GetAllProjectsCommitIds()
           self.assertEqual(commit_ids, {
@@ -151,7 +152,7 @@ class SuperprojectTestCase(unittest.TestCase):
     projects = self._superproject._manifest.projects
     data = ('160000 commit 2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea\tart\x00'
             '160000 commit e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06\tbootable/recovery\x00')
-    with mock.patch.object(self._superproject, '_Clone', return_value=True):
+    with mock.patch.object(self._superproject, '_Init', return_value=True):
       with mock.patch.object(self._superproject, '_Fetch', return_value=True):
         with mock.patch.object(self._superproject,
                                '_LsTree',

tests/test_manifest_xml.py

@@ -24,6 +24,72 @@ import error
 import manifest_xml
 
 
+# Invalid paths that we don't want in the filesystem.
+INVALID_FS_PATHS = (
+    '',
+    '.',
+    '..',
+    '../',
+    './',
+    'foo/',
+    './foo',
+    '../foo',
+    'foo/./bar',
+    'foo/../../bar',
+    '/foo',
+    './../foo',
+    '.git/foo',
+    # Check case folding.
+    '.GIT/foo',
+    'blah/.git/foo',
+    '.repo/foo',
+    '.repoconfig',
+    # Block ~ due to 8.3 filenames on Windows filesystems.
+    '~',
+    'foo~',
+    'blah/foo~',
+    # Block Unicode characters that get normalized out by filesystems.
+    u'foo\u200Cbar',
+)
+
+# Make sure platforms that use path separators (e.g. Windows) are also
+# rejected properly.
+if os.path.sep != '/':
+  INVALID_FS_PATHS += tuple(x.replace('/', os.path.sep) for x in INVALID_FS_PATHS)
+
+
+class ManifestParseTestCase(unittest.TestCase):
+  """TestCase for parsing manifests."""
+
+  def setUp(self):
+    self.tempdir = tempfile.mkdtemp(prefix='repo_tests')
+    self.repodir = os.path.join(self.tempdir, '.repo')
+    self.manifest_dir = os.path.join(self.repodir, 'manifests')
+    self.manifest_file = os.path.join(
+        self.repodir, manifest_xml.MANIFEST_FILE_NAME)
+    self.local_manifest_dir = os.path.join(
+        self.repodir, manifest_xml.LOCAL_MANIFESTS_DIR_NAME)
+    os.mkdir(self.repodir)
+    os.mkdir(self.manifest_dir)
+
+    # The manifest parsing really wants a git repo currently.
+    gitdir = os.path.join(self.repodir, 'manifests.git')
+    os.mkdir(gitdir)
+    with open(os.path.join(gitdir, 'config'), 'w') as fp:
+      fp.write("""[remote "origin"]
+        url = https://localhost:0/manifest
+""")
+
+  def tearDown(self):
+    shutil.rmtree(self.tempdir, ignore_errors=True)
+
+  def getXmlManifest(self, data):
+    """Helper to initialize a manifest for testing."""
+    with open(self.manifest_file, 'w') as fp:
+      fp.write(data)
+    return manifest_xml.XmlManifest(self.repodir, self.manifest_file)
+
+
 class ManifestValidateFilePaths(unittest.TestCase):
   """Check _ValidateFilePaths helper.
 
@@ -54,36 +120,7 @@ class ManifestValidateFilePaths(unittest.TestCase):
 
   def test_bad_paths(self):
     """Make sure bad paths (src & dest) are rejected."""
-    PATHS = (
+    for path in INVALID_FS_PATHS:
-        '..',
-        '../',
-        './',
-        'foo/',
-        './foo',
-        '../foo',
-        'foo/./bar',
-        'foo/../../bar',
-        '/foo',
-        './../foo',
-        '.git/foo',
-        # Check case folding.
-        '.GIT/foo',
-        'blah/.git/foo',
-        '.repo/foo',
-        '.repoconfig',
-        # Block ~ due to 8.3 filenames on Windows filesystems.
-        '~',
-        'foo~',
-        'blah/foo~',
-        # Block Unicode characters that get normalized out by filesystems.
-        u'foo\u200Cbar',
-    )
-    # Make sure platforms that use path separators (e.g. Windows) are also
-    # rejected properly.
-    if os.path.sep != '/':
-      PATHS += tuple(x.replace('/', os.path.sep) for x in PATHS)
-
-    for path in PATHS:
       self.assertRaises(
           error.ManifestInvalidPathError, self.check_both, path, 'a')
       self.assertRaises(
@@ -146,37 +183,9 @@ class ValueTests(unittest.TestCase):
       manifest_xml.XmlInt(node, 'a')
 
 
-class XmlManifestTests(unittest.TestCase):
+class XmlManifestTests(ManifestParseTestCase):
   """Check manifest processing."""
 
-  def setUp(self):
-    self.tempdir = tempfile.mkdtemp(prefix='repo_tests')
-    self.repodir = os.path.join(self.tempdir, '.repo')
-    self.manifest_dir = os.path.join(self.repodir, 'manifests')
-    self.manifest_file = os.path.join(
-        self.repodir, manifest_xml.MANIFEST_FILE_NAME)
-    self.local_manifest_dir = os.path.join(
-        self.repodir, manifest_xml.LOCAL_MANIFESTS_DIR_NAME)
-    os.mkdir(self.repodir)
-    os.mkdir(self.manifest_dir)
-
-    # The manifest parsing really wants a git repo currently.
-    gitdir = os.path.join(self.repodir, 'manifests.git')
-    os.mkdir(gitdir)
-    with open(os.path.join(gitdir, 'config'), 'w') as fp:
-      fp.write("""[remote "origin"]
-        url = https://localhost:0/manifest
-""")
-
-  def tearDown(self):
-    shutil.rmtree(self.tempdir, ignore_errors=True)
-
-  def getXmlManifest(self, data):
-    """Helper to initialize a manifest for testing."""
-    with open(self.manifest_file, 'w') as fp:
-      fp.write(data)
-    return manifest_xml.XmlManifest(self.repodir, self.manifest_file)
-
   def test_empty(self):
     """Parse an 'empty' manifest file."""
     manifest = self.getXmlManifest(
@@ -221,67 +230,6 @@ class XmlManifestTests(unittest.TestCase):
     self.assertEqual(manifest.repo_hooks_project.name, 'repohooks')
     self.assertEqual(manifest.repo_hooks_project.enabled_repo_hooks, ['a', 'b'])
 
-  def test_superproject(self):
-    """Check superproject settings."""
-    manifest = self.getXmlManifest("""
-<manifest>
-  <remote name="test-remote" fetch="http://localhost" />
-  <default remote="test-remote" revision="refs/heads/main" />
-  <superproject name="superproject"/>
-</manifest>
-""")
-    self.assertEqual(manifest.superproject['name'], 'superproject')
-    self.assertEqual(manifest.superproject['remote'].name, 'test-remote')
-    self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/superproject')
-    self.assertEqual(
-        manifest.ToXml().toxml(),
-        '<?xml version="1.0" ?><manifest>' +
-        '<remote name="test-remote" fetch="http://localhost"/>' +
-        '<default remote="test-remote" revision="refs/heads/main"/>' +
-        '<superproject name="superproject"/>' +
-        '</manifest>')
-
-  def test_superproject_with_remote(self):
-    """Check superproject settings."""
-    manifest = self.getXmlManifest("""
-<manifest>
-  <remote name="default-remote" fetch="http://localhost" />
-  <remote name="superproject-remote" fetch="http://localhost" />
-  <default remote="default-remote" revision="refs/heads/main" />
-  <superproject name="platform/superproject" remote="superproject-remote"/>
-</manifest>
-""")
-    self.assertEqual(manifest.superproject['name'], 'platform/superproject')
-    self.assertEqual(manifest.superproject['remote'].name, 'superproject-remote')
-    self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/platform/superproject')
-    self.assertEqual(
-        manifest.ToXml().toxml(),
-        '<?xml version="1.0" ?><manifest>' +
-        '<remote name="default-remote" fetch="http://localhost"/>' +
-        '<remote name="superproject-remote" fetch="http://localhost"/>' +
-        '<default remote="default-remote" revision="refs/heads/main"/>' +
-        '<superproject name="platform/superproject" remote="superproject-remote"/>' +
-        '</manifest>')
-
-  def test_superproject_with_defalut_remote(self):
-    """Check superproject settings."""
-    manifest = self.getXmlManifest("""
-<manifest>
-  <remote name="default-remote" fetch="http://localhost" />
-  <default remote="default-remote" revision="refs/heads/main" />
-  <superproject name="superproject" remote="default-remote"/>
-</manifest>
-""")
-    self.assertEqual(manifest.superproject['name'], 'superproject')
-    self.assertEqual(manifest.superproject['remote'].name, 'default-remote')
-    self.assertEqual(
-        manifest.ToXml().toxml(),
-        '<?xml version="1.0" ?><manifest>' +
-        '<remote name="default-remote" fetch="http://localhost"/>' +
-        '<default remote="default-remote" revision="refs/heads/main"/>' +
-        '<superproject name="superproject"/>' +
-        '</manifest>')
-
   def test_unknown_tags(self):
     """Check superproject settings."""
     manifest = self.getXmlManifest("""
@@ -303,51 +251,11 @@ class XmlManifestTests(unittest.TestCase):
         '<superproject name="superproject"/>' +
         '</manifest>')
 
-  def test_project_group(self):
-    """Check project group settings."""
-    manifest = self.getXmlManifest("""
-<manifest>
-  <remote name="test-remote" fetch="http://localhost" />
-  <default remote="test-remote" revision="refs/heads/main" />
-  <project name="test-name" path="test-path"/>
-  <project name="extras" path="path" groups="g1,g2,g1"/>
-</manifest>
-""")
-    self.assertEqual(len(manifest.projects), 2)
-    # Ordering isn't guaranteed.
-    result = {
-        manifest.projects[0].name: manifest.projects[0].groups,
-        manifest.projects[1].name: manifest.projects[1].groups,
-    }
-    project = manifest.projects[0]
-    self.assertCountEqual(
-        result['test-name'],
-        ['name:test-name', 'all', 'path:test-path'])
-    self.assertCountEqual(
-        result['extras'],
-        ['g1', 'g2', 'g1', 'name:extras', 'all', 'path:path'])
 
-  def test_project_set_revision_id(self):
+class IncludeElementTests(ManifestParseTestCase):
-    """Check setting of project's revisionId."""
+  """Tests for <include>."""
-    manifest = self.getXmlManifest("""
-<manifest>
-  <remote name="default-remote" fetch="http://localhost" />
-  <default remote="default-remote" revision="refs/heads/main" />
-  <project name="test-name"/>
-</manifest>
-""")
-    self.assertEqual(len(manifest.projects), 1)
-    project = manifest.projects[0]
-    project.SetRevisionId('ABCDEF')
-    self.assertEqual(
-        manifest.ToXml().toxml(),
-        '<?xml version="1.0" ?><manifest>' +
-        '<remote name="default-remote" fetch="http://localhost"/>' +
-        '<default remote="default-remote" revision="refs/heads/main"/>' +
-        '<project name="test-name" revision="ABCDEF"/>' +
-        '</manifest>')
 
-  def test_include_levels(self):
+  def test_group_levels(self):
     root_m = os.path.join(self.manifest_dir, 'root.xml')
     with open(root_m, 'w') as fp:
       fp.write("""
@@ -389,3 +297,221 @@ class XmlManifestTests(unittest.TestCase):
         self.assertIn('level2-group', proj.groups)
         # Check level2 proj group not removed.
         self.assertIn('l2g1', proj.groups)
+
+  def test_allow_bad_name_from_user(self):
+    """Check handling of bad name attribute from the user's input."""
+    def parse(name):
+      manifest = self.getXmlManifest(f"""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <include name="{name}" />
+</manifest>
+""")
+      # Force the manifest to be parsed.
+      manifest.ToXml()
+
+    # Setup target of the include.
+    target = os.path.join(self.tempdir, 'target.xml')
+    with open(target, 'w') as fp:
+      fp.write('<manifest></manifest>')
+
+    # Include with absolute path.
+    parse(os.path.abspath(target))
+
+    # Include with relative path.
+    parse(os.path.relpath(target, self.manifest_dir))
+
+  def test_bad_name_checks(self):
+    """Check handling of bad name attribute."""
+    def parse(name):
+      # Setup target of the include.
+      with open(os.path.join(self.manifest_dir, 'target.xml'), 'w') as fp:
+        fp.write(f'<manifest><include name="{name}"/></manifest>')
+
+      manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <include name="target.xml" />
+</manifest>
+""")
+      # Force the manifest to be parsed.
+      manifest.ToXml()
+
+    # Handle empty name explicitly because a different codepath rejects it.
+    with self.assertRaises(error.ManifestParseError):
+      parse('')
+
+    for path in INVALID_FS_PATHS:
+      if not path:
+        continue
+
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse(path)
+
+
+class ProjectElementTests(ManifestParseTestCase):
+  """Tests for <project>."""
+
+  def test_group(self):
+    """Check project group settings."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <project name="test-name" path="test-path"/>
+  <project name="extras" path="path" groups="g1,g2,g1"/>
+</manifest>
+""")
+    self.assertEqual(len(manifest.projects), 2)
+    # Ordering isn't guaranteed.
+    result = {
+        manifest.projects[0].name: manifest.projects[0].groups,
+        manifest.projects[1].name: manifest.projects[1].groups,
+    }
+    project = manifest.projects[0]
+    self.assertCountEqual(
+        result['test-name'],
+        ['name:test-name', 'all', 'path:test-path'])
+    self.assertCountEqual(
+        result['extras'],
+        ['g1', 'g2', 'g1', 'name:extras', 'all', 'path:path'])
+
+  def test_set_revision_id(self):
+    """Check setting of project's revisionId."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="test-name"/>
+</manifest>
+""")
+    self.assertEqual(len(manifest.projects), 1)
+    project = manifest.projects[0]
+    project.SetRevisionId('ABCDEF')
+    self.assertEqual(
+        manifest.ToXml().toxml(),
+        '<?xml version="1.0" ?><manifest>' +
+        '<remote name="default-remote" fetch="http://localhost"/>' +
+        '<default remote="default-remote" revision="refs/heads/main"/>' +
+        '<project name="test-name" revision="ABCDEF"/>' +
+        '</manifest>')
+
+  def test_trailing_slash(self):
+    """Check handling of trailing slashes in attributes."""
+    def parse(name, path):
+      return self.getXmlManifest(f"""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="{name}" path="{path}" />
+</manifest>
+""")
+
+    manifest = parse('a/path/', 'foo')
+    self.assertEqual(manifest.projects[0].gitdir,
+                     os.path.join(self.tempdir, '.repo/projects/foo.git'))
+    self.assertEqual(manifest.projects[0].objdir,
+                     os.path.join(self.tempdir, '.repo/project-objects/a/path.git'))
+
+    manifest = parse('a/path', 'foo/')
+    self.assertEqual(manifest.projects[0].gitdir,
+                     os.path.join(self.tempdir, '.repo/projects/foo.git'))
+    self.assertEqual(manifest.projects[0].objdir,
+                     os.path.join(self.tempdir, '.repo/project-objects/a/path.git'))
+
+  def test_bad_path_name_checks(self):
+    """Check handling of bad path & name attributes."""
+    def parse(name, path):
+      manifest = self.getXmlManifest(f"""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="{name}" path="{path}" />
+</manifest>
+""")
+      # Force the manifest to be parsed.
+      manifest.ToXml()
+
+    # Verify the parser is valid by default to avoid buggy tests below.
+    parse('ok', 'ok')
+
+    # Handle empty name explicitly because a different codepath rejects it.
+    # Empty path is OK because it defaults to the name field.
+    with self.assertRaises(error.ManifestParseError):
+      parse('', 'ok')
+
+    for path in INVALID_FS_PATHS:
+      if not path or path.endswith('/'):
+        continue
+
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse(path, 'ok')
+      with self.assertRaises(error.ManifestInvalidPathError):
+        parse('ok', path)
+
+
+class SuperProjectElementTests(ManifestParseTestCase):
+  """Tests for <superproject>."""
+
+  def test_superproject(self):
+    """Check superproject settings."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="test-remote" fetch="http://localhost" />
+  <default remote="test-remote" revision="refs/heads/main" />
+  <superproject name="superproject"/>
+</manifest>
+""")
+    self.assertEqual(manifest.superproject['name'], 'superproject')
+    self.assertEqual(manifest.superproject['remote'].name, 'test-remote')
+    self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/superproject')
+    self.assertEqual(
+        manifest.ToXml().toxml(),
+        '<?xml version="1.0" ?><manifest>' +
+        '<remote name="test-remote" fetch="http://localhost"/>' +
+        '<default remote="test-remote" revision="refs/heads/main"/>' +
+        '<superproject name="superproject"/>' +
+        '</manifest>')
+
+  def test_remote(self):
+    """Check superproject settings with a remote."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <remote name="superproject-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <superproject name="platform/superproject" remote="superproject-remote"/>
+</manifest>
+""")
+    self.assertEqual(manifest.superproject['name'], 'platform/superproject')
+    self.assertEqual(manifest.superproject['remote'].name, 'superproject-remote')
+    self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/platform/superproject')
+    self.assertEqual(
+        manifest.ToXml().toxml(),
+        '<?xml version="1.0" ?><manifest>' +
+        '<remote name="default-remote" fetch="http://localhost"/>' +
+        '<remote name="superproject-remote" fetch="http://localhost"/>' +
+        '<default remote="default-remote" revision="refs/heads/main"/>' +
+        '<superproject name="platform/superproject" remote="superproject-remote"/>' +
+        '</manifest>')
+
+  def test_defalut_remote(self):
+    """Check superproject settings with a default remote."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <superproject name="superproject" remote="default-remote"/>
+</manifest>
+""")
+    self.assertEqual(manifest.superproject['name'], 'superproject')
+    self.assertEqual(manifest.superproject['remote'].name, 'default-remote')
+    self.assertEqual(
+        manifest.ToXml().toxml(),
+        '<?xml version="1.0" ?><manifest>' +
+        '<remote name="default-remote" fetch="http://localhost"/>' +
+        '<default remote="default-remote" revision="refs/heads/main"/>' +
+        '<superproject name="superproject"/>' +
+        '</manifest>')

tests/test_subcmds_init.py

@@ -38,7 +38,7 @@ class InitCommand(unittest.TestCase):
     """Check invalid command line options."""
     ARGV = (
         # Too many arguments.
-        ['asdf'],
+        ['url', 'asdf'],
 
         # Conflicting options.
         ['--mirror', '--archive'],