Update manifest_name with the returned manifest name for superproject.

This is similar to smart sync, allowing sync to benefit from the patched
manifest.

Bug: [google internal] b/190688390
Change-Id: I158a80afceca606dcd81ec76b2caede369f7ed03
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/312142
Reviewed-by: Raman Tenneti <rtenneti@google.com>
Tested-by: Xin Li <delphij@google.com>

.github/workflows/test-ci.yml

@@ -14,7 +14,7 @@ jobs:
       fail-fast: false
       matrix:
         os: [ubuntu-latest, macos-latest, windows-latest]
-        python-version: [3.5, 3.6, 3.7, 3.8, 3.9]
+        python-version: [3.6, 3.7, 3.8, 3.9]
     runs-on: ${{ matrix.os }}
 
     steps:

command.py

@@ -15,7 +15,6 @@
 import multiprocessing
 import os
 import optparse
-import platform
 import re
 import sys
 
@@ -43,15 +42,32 @@ class Command(object):
   """Base class for any command line action in repo.
   """
 
-  common = False
+  # Singleton for all commands to track overall repo command execution and
+  # provide event summary to callers.  Only used by sync subcommand currently.
+  #
+  # NB: This is being replaced by git trace2 events.  See git_trace2_event_log.
   event_log = EventLog()
-  manifest = None
-  _optparse = None
+
+  # Whether this command is a "common" one, i.e. whether the user would commonly
+  # use it or it's a more uncommon command.  This is used by the help command to
+  # show short-vs-full summaries.
+  COMMON = False
 
   # 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 __init__(self, repodir=None, client=None, manifest=None, gitc_manifest=None,
+               git_event_log=None):
+    self.repodir = repodir
+    self.client = client
+    self.manifest = manifest
+    self.gitc_manifest = gitc_manifest
+    self.git_event_log = git_event_log
+
+    # Cache for the OptionParser property.
+    self._optparse = None
+
   def WantPager(self, _opt):
     return False
 

docs/manifest-format.md

@@ -96,6 +96,7 @@ following DTD:
 
   <!ELEMENT remove-project EMPTY>
   <!ATTLIST remove-project name  CDATA #REQUIRED>
+  <!ATTLIST remove-project optional  CDATA #IMPLIED>
 
   <!ELEMENT repo-hooks EMPTY>
   <!ATTLIST repo-hooks in-project CDATA #REQUIRED>
@@ -393,6 +394,9 @@ This element is mostly useful in a local manifest file, where
 the user can remove a project, and possibly replace it with their
 own definition.
 
+Attribute `optional`: Set to true to ignore remove-project elements with no
+matching `project` element.
+
 ### Element repo-hooks
 
 NB: See the [practical documentation](./repo-hooks.md) for using repo hooks.
@@ -485,6 +489,9 @@ these extra projects.
 Manifest files stored in `$TOP_DIR/.repo/local_manifests/*.xml` will
 be loaded in alphabetical order.
 
+Projects from local manifest files are added into
+local::<local manifest filename> group.
+
 The legacy `$TOP_DIR/.repo/local_manifest.xml` path is no longer supported.
 
 

git_command.py

@@ -75,7 +75,8 @@ 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,
-                            encoding='utf-8', env=env, check=False)
+                            stderr=subprocess.DEVNULL, encoding='utf-8',
+                            env=env, check=False)
     if result.returncode == 0:
       ver = result.stdout.strip()
       if ver.startswith('v'):

git_config.py

@@ -65,6 +65,15 @@ class GitConfig(object):
 
   _USER_CONFIG = '~/.gitconfig'
 
+  _ForSystem = None
+  _SYSTEM_CONFIG = '/etc/gitconfig'
+
+  @classmethod
+  def ForSystem(cls):
+    if cls._ForSystem is None:
+      cls._ForSystem = cls(configfile=cls._SYSTEM_CONFIG)
+    return cls._ForSystem
+
   @classmethod
   def ForUser(cls):
     if cls._ForUser is None:
@@ -356,7 +365,10 @@ class GitConfig(object):
     return c
 
   def _do(self, *args):
-    command = ['config', '--file', self.file, '--includes']
+    if self.file == self._SYSTEM_CONFIG:
+      command = ['config', '--system', '--includes']
+    else:
+      command = ['config', '--file', self.file, '--includes']
     command.extend(args)
 
     p = GitCommand(None,

git_superproject.py

@@ -19,20 +19,52 @@ https://en.wikibooks.org/wiki/Git/Submodules_and_Superprojects
 
 Examples:
   superproject = Superproject()
-  project_commit_ids = superproject.UpdateProjectsRevisionId(projects)
+  UpdateProjectsResult = superproject.UpdateProjectsRevisionId(projects)
 """
 
 import hashlib
+import functools
 import os
 import sys
+import time
+from typing import NamedTuple
 
-from git_command import GitCommand
+from git_command import git_require, GitCommand
+from git_config import RepoConfig
 from git_refs import R_HEADS
+from manifest_xml import LOCAL_MANIFEST_GROUP_PREFIX
 
 _SUPERPROJECT_GIT_NAME = 'superproject.git'
 _SUPERPROJECT_MANIFEST_NAME = 'superproject_override.xml'
 
 
+class SyncResult(NamedTuple):
+  """Return the status of sync and whether caller should exit."""
+
+  # Whether the superproject sync was successful.
+  success: bool
+  # Whether the caller should exit.
+  fatal: bool
+
+
+class CommitIdsResult(NamedTuple):
+  """Return the commit ids and whether caller should exit."""
+
+  # A dictionary with the projects/commit ids on success, otherwise None.
+  commit_ids: dict
+  # Whether the caller should exit.
+  fatal: bool
+
+
+class UpdateProjectsResult(NamedTuple):
+  """Return the overriding manifest file and whether caller should exit."""
+
+  # Path name of the overriding manfiest file if successful, otherwise None.
+  manifest_path: str
+  # Whether the caller should exit.
+  fatal: bool
+
+
 class Superproject(object):
   """Get commit ids from superproject.
 
@@ -40,19 +72,21 @@ class Superproject(object):
   lookup of commit ids for all projects. It contains _project_commit_ids which
   is a dictionary with project/commit id entries.
   """
-  def __init__(self, manifest, repodir, superproject_dir='exp-superproject',
-               quiet=False):
+  def __init__(self, manifest, repodir, git_event_log,
+               superproject_dir='exp-superproject', quiet=False):
     """Initializes superproject.
 
     Args:
       manifest: A Manifest object that is to be written to a file.
       repodir: Path to the .repo/ dir for holding all internal checkout state.
           It must be in the top directory of the repo client checkout.
+      git_event_log: A git trace2 event log to log events.
       superproject_dir: Relative path under |repodir| to checkout superproject.
       quiet:  If True then only print the progress messages.
     """
     self._project_commit_ids = None
     self._manifest = manifest
+    self._git_event_log = git_event_log
     self._quiet = quiet
     self._branch = self._GetBranch()
     self._repodir = os.path.abspath(repodir)
@@ -83,6 +117,11 @@ class Superproject(object):
       branch = branch[len(R_HEADS):]
     return branch
 
+  def _LogError(self, message):
+    """Logs message to stderr and _git_event_log."""
+    print(message, file=sys.stderr)
+    self._git_event_log.ErrorEvent(message, '')
+
   def _Init(self):
     """Sets up a local Git repository to get a copy of a superproject.
 
@@ -102,8 +141,8 @@ class Superproject(object):
                    capture_stderr=True)
     retval = p.Wait()
     if retval:
-      print('repo: error: git init call failed with return code: %r, stderr: %r' %
-            (retval, p.stderr), file=sys.stderr)
+      self._LogError(f'repo: error: git init call failed, command: git {cmd}, '
+                     f'return code: {retval}, stderr: {p.stderr}')
       return False
     return True
 
@@ -117,8 +156,10 @@ class Superproject(object):
       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)
+      self._LogError(f'git fetch missing directory: {self._work_git}')
+      return False
+    if not git_require((2, 28, 0)):
+      print('superproject requires a git version 2.28 or later', file=sys.stderr)
       return False
     cmd = ['fetch', url, '--depth', '1', '--force', '--no-tags', '--filter', 'blob:none']
     if self._branch:
@@ -130,8 +171,8 @@ class Superproject(object):
                    capture_stderr=True)
     retval = p.Wait()
     if retval:
-      print('repo: error: git fetch call failed with return code: %r, stderr: %r' %
-            (retval, p.stderr), file=sys.stderr)
+      self._LogError(f'repo: error: git fetch call failed, command: git {cmd}, '
+                     f'return code: {retval}, stderr: {p.stderr}')
       return False
     return True
 
@@ -144,8 +185,7 @@ class Superproject(object):
       data: data returned from 'git ls-tree ...' instead of None.
     """
     if not os.path.exists(self._work_git):
-      print('git ls-tree missing drectory: %s' % self._work_git,
-            file=sys.stderr)
+      self._LogError(f'git ls-tree missing directory: {self._work_git}')
       return None
     data = None
     branch = 'HEAD' if not self._branch else self._branch
@@ -160,52 +200,54 @@ class Superproject(object):
     if retval == 0:
       data = p.stdout
     else:
-      print('repo: error: git ls-tree call failed with return code: %r, stderr: %r' % (
-          retval, p.stderr), file=sys.stderr)
+      self._LogError(f'repo: error: git ls-tree call failed, command: git {cmd}, '
+                     f'return code: {retval}, stderr: {p.stderr}')
     return data
 
   def Sync(self):
     """Gets a local copy of a superproject for the manifest.
 
     Returns:
-      True if sync of superproject is successful, or False.
+      SyncResult
     """
-    print('WARNING: --use-superproject is experimental and not '
-          'for general use', file=sys.stderr)
+    print('NOTICE: --use-superproject is in beta; report any issues to the '
+          'address described in `repo version`', file=sys.stderr)
 
     if not self._manifest.superproject:
-      print('error: superproject tag is not defined in manifest',
-            file=sys.stderr)
-      return False
+      self._LogError(f'repo error: superproject tag is not defined in manifest: '
+                     f'{self._manifest.manifestFile}')
+      return SyncResult(False, False)
 
+    should_exit = True
     url = self._manifest.superproject['remote'].url
     if not url:
-      print('error: superproject URL is not defined in manifest',
-            file=sys.stderr)
-      return False
+      self._LogError(f'repo error: superproject URL is not defined in manifest: '
+                     f'{self._manifest.manifestFile}')
+      return SyncResult(False, should_exit)
 
     if not self._Init():
-      return False
+      return SyncResult(False, should_exit)
     if not self._Fetch(url):
-      return False
+      return SyncResult(False, should_exit)
     if not self._quiet:
       print('%s: Initial setup for superproject completed.' % self._work_git)
-    return True
+    return SyncResult(True, False)
 
   def _GetAllProjectsCommitIds(self):
     """Get commit ids for all projects from superproject and save them in _project_commit_ids.
 
     Returns:
-      A dictionary with the projects/commit ids on success, otherwise None.
+      CommitIdsResult
     """
-    if not self.Sync():
-      return None
+    sync_result = self.Sync()
+    if not sync_result.success:
+      return CommitIdsResult(None, sync_result.fatal)
 
     data = self._LsTree()
     if not data:
-      print('error: git ls-tree failed to return data for superproject',
+      print('warning: git ls-tree failed to return data for superproject',
             file=sys.stderr)
-      return None
+      return CommitIdsResult(None, True)
 
     # Parse lines like the following to select lines starting with '160000' and
     # build a dictionary with project path (last element) and its commit id (3rd element).
@@ -221,7 +263,7 @@ class Superproject(object):
         commit_ids[ls_data[3]] = ls_data[2]
 
     self._project_commit_ids = commit_ids
-    return commit_ids
+    return CommitIdsResult(commit_ids, False)
 
   def _WriteManfiestFile(self):
     """Writes manifest to a file.
@@ -230,9 +272,7 @@ class Superproject(object):
       manifest_path: Path name of the file into which manifest is written instead of None.
     """
     if not os.path.exists(self._superproject_path):
-      print('error: missing superproject directory %s' %
-            self._superproject_path,
-            file=sys.stderr)
+      self._LogError(f'error: missing superproject directory: {self._superproject_path}')
       return None
     manifest_str = self._manifest.ToXml(groups=self._manifest.GetGroupsStr()).toxml()
     manifest_path = self._manifest_path
@@ -240,12 +280,30 @@ class Superproject(object):
       with open(manifest_path, 'w', encoding='utf-8') as fp:
         fp.write(manifest_str)
     except IOError as e:
-      print('error: cannot write manifest to %s:\n%s'
-            % (manifest_path, e),
-            file=sys.stderr)
+      self._LogError(f'error: cannot write manifest to : {manifest_path} {e}')
       return None
     return manifest_path
 
+  def _SkipUpdatingProjectRevisionId(self, project):
+    """Checks if a project's revision id needs to be updated or not.
+
+    Revision id for projects from local manifest will not be updated.
+
+    Args:
+      project: project whose revision id is being updated.
+
+    Returns:
+      True if a project's revision id should not be updated, or False,
+    """
+    path = project.relpath
+    if not path:
+      return True
+    # Skip the project with revisionId.
+    if project.revisionId:
+      return True
+    # Skip the project if it comes from the local manifest.
+    return any(s.startswith(LOCAL_MANIFEST_GROUP_PREFIX) for s in project.groups)
+
   def UpdateProjectsRevisionId(self, projects):
     """Update revisionId of every project in projects with the commit id.
 
@@ -253,37 +311,111 @@ class Superproject(object):
       projects: List of projects whose revisionId needs to be updated.
 
     Returns:
-      manifest_path: Path name of the overriding manfiest file instead of None.
+      UpdateProjectsResult
     """
-    commit_ids = self._GetAllProjectsCommitIds()
+    commit_ids_result = self._GetAllProjectsCommitIds()
+    commit_ids = commit_ids_result.commit_ids
     if not commit_ids:
-      print('error: Cannot get project commit ids from manifest', file=sys.stderr)
-      return None
+      print('warning: Cannot get project commit ids from manifest', file=sys.stderr)
+      return UpdateProjectsResult(None, commit_ids_result.fatal)
 
     projects_missing_commit_ids = []
-    superproject_fetchUrl = self._manifest.superproject['remote'].fetchUrl
     for project in projects:
+      if self._SkipUpdatingProjectRevisionId(project):
+        continue
       path = project.relpath
-      if not path:
-        continue
-      # Some manifests that pull projects from the "chromium" GoB
-      # (remote="chromium"), and have a private manifest that pulls projects
-      # from both the chromium GoB and "chrome-internal" GoB (remote="chrome").
-      # For such projects, one of the remotes will be different from
-      # superproject's remote. Until superproject, supports multiple remotes,
-      # don't update the commit ids of remotes that don't match superproject's
-      # remote.
-      if project.remote.fetchUrl != superproject_fetchUrl:
-        continue
       commit_id = commit_ids.get(path)
-      if commit_id:
-        project.SetRevisionId(commit_id)
-      else:
+      if not commit_id:
         projects_missing_commit_ids.append(path)
+
+    # If superproject doesn't have a commit id for a project, then report an
+    # error event and continue as if do not use superproject is specified.
     if projects_missing_commit_ids:
-      print('error: please file a bug using %s to report missing commit_ids for: %s' %
-            (self._manifest.contactinfo.bugurl, projects_missing_commit_ids), file=sys.stderr)
-      return None
+      self._LogError(f'error: please file a bug using {self._manifest.contactinfo.bugurl} '
+                     f'to report missing commit_ids for: {projects_missing_commit_ids}')
+      return UpdateProjectsResult(None, False)
+
+    for project in projects:
+      if not self._SkipUpdatingProjectRevisionId(project):
+        project.SetRevisionId(commit_ids.get(project.relpath))
 
     manifest_path = self._WriteManfiestFile()
-    return manifest_path
+    return UpdateProjectsResult(manifest_path, False)
+
+
+@functools.lru_cache(maxsize=None)
+def _UseSuperprojectFromConfiguration():
+  """Returns the user choice of whether to use superproject."""
+  user_cfg = RepoConfig.ForUser()
+  system_cfg = RepoConfig.ForSystem()
+  time_now = int(time.time())
+
+  user_value = user_cfg.GetBoolean('repo.superprojectChoice')
+  if user_value is not None:
+    user_expiration = user_cfg.GetInt('repo.superprojectChoiceExpire')
+    if user_expiration is not None and (user_expiration <= 0 or user_expiration >= time_now):
+      # TODO(b/190688390) - Remove prompt when we are comfortable with the new
+      # default value.
+      print(('You are currently enrolled in Git submodules experiment '
+             '(go/android-submodules-quickstart).  Use --no-use-superproject '
+             'to override.\n'), file=sys.stderr)
+    return user_value
+
+  # We don't have an unexpired choice, ask for one.
+  system_value = system_cfg.GetBoolean('repo.superprojectChoice')
+  if system_value:
+    # The system configuration is proposing that we should enable the
+    # use of superproject. Present this to user for confirmation if we
+    # are on a TTY, or, when we are not on a TTY, accept the system
+    # default for this time only.
+    #
+    # TODO(b/190688390) - Remove prompt when we are comfortable with the new
+    # default value.
+    prompt = ('Repo can now use Git submodules (go/android-submodules-quickstart) '
+              'instead of manifests to represent the state of the Android '
+              'superproject, which results in faster syncs and better atomicity.\n\n')
+    if sys.stdout.isatty():
+      prompt += 'Would you like to opt in for two weeks (y/N)? '
+      response = input(prompt).lower()
+      time_choiceexpire = time_now + (86400 * 14)
+      if response in ('y', 'yes'):
+        userchoice = True
+      elif response in ('a', 'always'):
+        userchoice = True
+        time_choiceexpire = 0
+      elif response == 'never':
+        userchoice = False
+        time_choiceexpire = 0
+      elif response in ('n', 'no'):
+        userchoice = False
+      else:
+        # Unrecognized user response, assume the intention was no, but
+        # only for 2 hours instead of 2 weeks to balance between not
+        # being overly pushy while still retain the opportunity to
+        # enroll.
+        userchoice = False
+        time_choiceexpire = time_now + 7200
+
+      user_cfg.SetString('repo.superprojectChoiceExpire', str(time_choiceexpire))
+      user_cfg.SetBoolean('repo.superprojectChoice', userchoice)
+
+      return userchoice
+    else:
+      print('Accepting once since we are not on a TTY', file=sys.stderr)
+      return True
+
+  # For all other cases, we would not use superproject by default.
+  return False
+
+
+def UseSuperproject(opt, manifest):
+  """Returns a boolean if use-superproject option is enabled."""
+
+  if opt.use_superproject is not None:
+    return opt.use_superproject
+  else:
+    client_value = manifest.manifestProject.config.GetBoolean('repo.superproject')
+    if client_value is not None:
+      return client_value
+    else:
+      return _UseSuperprojectFromConfiguration()

git_trace2_event_log.py

@@ -159,6 +159,13 @@ class EventLog(object):
       def_param_event['value'] = value
       self._log.append(def_param_event)
 
+  def ErrorEvent(self, msg, fmt):
+    """Append a 'error' event to the current log."""
+    error_event = self._CreateEventDict('error')
+    error_event['msg'] = msg
+    error_event['fmt'] = fmt
+    self._log.append(error_event)
+
   def _GetEventTargetPath(self):
     """Get the 'trace2.eventtarget' path from git configuration.
 

main.py

@@ -71,7 +71,7 @@ from subcmds import all_commands
 #
 # python-3.6 is in Ubuntu Bionic.
 MIN_PYTHON_VERSION_SOFT = (3, 6)
-MIN_PYTHON_VERSION_HARD = (3, 5)
+MIN_PYTHON_VERSION_HARD = (3, 6)
 
 if sys.version_info.major < 3:
   print('repo: error: Python 2 is no longer supported; '
@@ -195,23 +195,26 @@ class _Repo(object):
 
     SetDefaultColoring(gopts.color)
 
+    git_trace2_event_log = EventLog()
+    repo_client = RepoClient(self.repodir)
+    gitc_manifest = None
+    gitc_client_name = gitc_utils.parse_clientdir(os.getcwd())
+    if gitc_client_name:
+      gitc_manifest = GitcClient(self.repodir, gitc_client_name)
+      repo_client.isGitcClient = True
+
     try:
-      cmd = self.commands[name]()
+      cmd = self.commands[name](
+          repodir=self.repodir,
+          client=repo_client,
+          manifest=repo_client.manifest,
+          gitc_manifest=gitc_manifest,
+          git_event_log=git_trace2_event_log)
     except KeyError:
       print("repo: '%s' is not a repo command.  See 'repo help'." % name,
             file=sys.stderr)
       return 1
 
-    git_trace2_event_log = EventLog()
-    cmd.repodir = self.repodir
-    cmd.client = RepoClient(cmd.repodir)
-    cmd.manifest = cmd.client.manifest
-    cmd.gitc_manifest = None
-    gitc_client_name = gitc_utils.parse_clientdir(os.getcwd())
-    if gitc_client_name:
-      cmd.gitc_manifest = GitcClient(cmd.repodir, gitc_client_name)
-      cmd.client.isGitcClient = True
-
     Editor.globalConfig = cmd.client.globalConfig
 
     if not isinstance(cmd, MirrorSafeCommand) and cmd.manifest.IsMirror:

manifest_xml.py

@@ -34,6 +34,9 @@ MANIFEST_FILE_NAME = 'manifest.xml'
 LOCAL_MANIFEST_NAME = 'local_manifest.xml'
 LOCAL_MANIFESTS_DIR_NAME = 'local_manifests'
 
+# Add all projects from local manifest into a group.
+LOCAL_MANIFEST_GROUP_PREFIX = 'local:'
+
 # ContactInfo has the self-registered bug url, supplied by the manifest authors.
 ContactInfo = collections.namedtuple('ContactInfo', 'bugurl')
 
@@ -119,9 +122,13 @@ class _Default(object):
   sync_tags = True
 
   def __eq__(self, other):
+    if not isinstance(other, _Default):
+      return False
     return self.__dict__ == other.__dict__
 
   def __ne__(self, other):
+    if not isinstance(other, _Default):
+      return True
     return self.__dict__ != other.__dict__
 
 
@@ -144,12 +151,18 @@ class _XmlRemote(object):
     self.resolvedFetchUrl = self._resolveFetchUrl()
 
   def __eq__(self, other):
+    if not isinstance(other, _XmlRemote):
+      return False
     return self.__dict__ == other.__dict__
 
   def __ne__(self, other):
+    if not isinstance(other, _XmlRemote):
+      return True
     return self.__dict__ != other.__dict__
 
   def _resolveFetchUrl(self):
+    if self.fetchUrl is None:
+      return ''
     url = self.fetchUrl.rstrip('/')
     manifestUrl = self.manifestUrl.rstrip('/')
     # urljoin will gets confused over quite a few things.  The ones we care
@@ -679,7 +692,9 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
               # 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))
+                  local, self.repodir,
+                  parent_groups=f'{LOCAL_MANIFEST_GROUP_PREFIX}:{local_file[:-4]}',
+                  restrict_includes=False))
         except OSError:
           pass
 
@@ -776,9 +791,10 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     for node in itertools.chain(*node_list):
       if node.nodeName == 'default':
         new_default = self._ParseDefault(node)
+        emptyDefault = not node.hasAttributes() and not node.hasChildNodes()
         if self._default is None:
           self._default = new_default
-        elif new_default != self._default:
+        elif not emptyDefault and new_default != self._default:
           raise ManifestParseError('duplicate default in %s' %
                                    (self.manifestFile))
 
@@ -902,19 +918,19 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
       if node.nodeName == 'remove-project':
         name = self._reqatt(node, 'name')
 
-        if name not in self._projects:
+        if name in self._projects:
+          for p in self._projects[name]:
+            del self._paths[p.relpath]
+          del self._projects[name]
+
+          # If the manifest removes the hooks project, treat it as if it deleted
+          # the repo-hooks element too.
+          if self._repo_hooks_project and (self._repo_hooks_project.name == name):
+            self._repo_hooks_project = None
+        elif not XmlBool(node, 'optional', False):
           raise ManifestParseError('remove-project element specifies non-existent '
                                    'project: %s' % name)
 
-        for p in self._projects[name]:
-          del self._paths[p.relpath]
-        del self._projects[name]
-
-        # If the manifest removes the hooks project, treat it as if it deleted
-        # the repo-hooks element too.
-        if self._repo_hooks_project and (self._repo_hooks_project.name == name):
-          self._repo_hooks_project = None
-
   def _AddMetaProjectMirror(self, m):
     name = None
     m_url = m.GetRemote(m.remote.name).url

project.py

@@ -1216,7 +1216,7 @@ class Project(object):
                                          (self.revisionExpr, self.name))
 
   def SetRevisionId(self, revisionId):
-    if self.clone_depth or self.manifest.manifestProject.config.GetString('repo.depth'):
+    if self.revisionExpr:
       self.upstream = self.revisionExpr
 
     self.revisionId = revisionId
@@ -1967,6 +1967,10 @@ class Project(object):
       # throws an error.
       self.bare_git.rev_list('-1', '--missing=allow-any',
                              '%s^0' % self.revisionExpr, '--')
+      if self.upstream:
+        rev = self.GetRemote(self.remote.name).ToLocal(self.upstream)
+        self.bare_git.rev_list('-1', '--missing=allow-any',
+                               '%s^0' % rev, '--')
       return True
     except GitError:
       # There is no such persistent revision. We have to fetch it.

requirements.json

@@ -38,9 +38,9 @@
   # Supported Python versions.
   #
   # python-3.6 is in Ubuntu Bionic.
-  # python-3.5 is in Debian Stretch.
+  # python-3.7 is in Debian Buster.
   "python": {
-    "hard": [3, 5],
+    "hard": [3, 6],
     "soft": [3, 6]
   },
 

run_tests

@@ -24,6 +24,10 @@ import sys
 
 def find_pytest():
   """Try to locate a good version of pytest."""
+  # If we're in a virtualenv, assume that it's provided the right pytest.
+  if 'VIRTUAL_ENV' in os.environ:
+    return 'pytest'
+
   # Use the Python 3 version if available.
   ret = shutil.which('pytest-3')
   if ret:

setup.py

@@ -56,6 +56,6 @@ setuptools.setup(
         'Programming Language :: Python :: 3 :: Only',
         'Topic :: Software Development :: Version Control :: Git',
     ],
-    python_requires='>=3.5',
+    python_requires='>=3.6',
     packages=['subcmds'],
 )

subcmds/abandon.py

@@ -23,7 +23,7 @@ from progress import Progress
 
 
 class Abandon(Command):
-  common = True
+  COMMON = True
   helpSummary = "Permanently abandon a development branch"
   helpUsage = """
 %prog [--all | <branchname>] [<project>...]

subcmds/branches.py

@@ -62,7 +62,7 @@ class BranchInfo(object):
 
 
 class Branches(Command):
-  common = True
+  COMMON = True
   helpSummary = "View current topic branches"
   helpUsage = """
 %prog [<project>...]

subcmds/checkout.py

@@ -20,7 +20,7 @@ from progress import Progress
 
 
 class Checkout(Command):
-  common = True
+  COMMON = True
   helpSummary = "Checkout a branch for development"
   helpUsage = """
 %prog <branchname> [<project>...]

subcmds/cherry_pick.py

@@ -21,7 +21,7 @@ CHANGE_ID_RE = re.compile(r'^\s*Change-Id: I([0-9a-f]{40})\s*$')
 
 
 class CherryPick(Command):
-  common = True
+  COMMON = True
   helpSummary = "Cherry-pick a change."
   helpUsage = """
 %prog <sha1>

subcmds/diff.py

@@ -19,7 +19,7 @@ from command import DEFAULT_LOCAL_JOBS, PagedCommand
 
 
 class Diff(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Show changes between commit and working tree"
   helpUsage = """
 %prog [<project>...]

subcmds/diffmanifests.py

@@ -31,7 +31,7 @@ class Diffmanifests(PagedCommand):
   deeper level.
   """
 
-  common = True
+  COMMON = True
   helpSummary = "Manifest diff utility"
   helpUsage = """%prog manifest1.xml [manifest2.xml] [options]"""
 

subcmds/download.py

@@ -22,7 +22,7 @@ CHANGE_RE = re.compile(r'^([1-9][0-9]*)(?:[/\.-]([1-9][0-9]*))?$')
 
 
 class Download(Command):
-  common = True
+  COMMON = True
   helpSummary = "Download and checkout a change"
   helpUsage = """
 %prog {[project] change[/patchset]}...

subcmds/forall.py

@@ -41,7 +41,7 @@ class ForallColoring(Coloring):
 
 
 class Forall(Command, MirrorSafeCommand):
-  common = False
+  COMMON = False
   helpSummary = "Run a shell command in each project"
   helpUsage = """
 %prog [<project>...] -c <command> [<arg>...]

subcmds/gitc_delete.py

@@ -19,7 +19,7 @@ import platform_utils
 
 
 class GitcDelete(Command, GitcClientCommand):
-  common = True
+  COMMON = True
   visible_everywhere = False
   helpSummary = "Delete a GITC Client."
   helpUsage = """

subcmds/gitc_init.py

@@ -23,7 +23,7 @@ import wrapper
 
 
 class GitcInit(init.Init, GitcAvailableCommand):
-  common = True
+  COMMON = True
   helpSummary = "Initialize a GITC Client."
   helpUsage = """
 %prog [options] [client name]

subcmds/grep.py

@@ -29,7 +29,7 @@ class GrepColoring(Coloring):
 
 
 class Grep(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Print lines matching a pattern"
   helpUsage = """
 %prog {pattern | -e pattern} [<project>...]

subcmds/help.py

@@ -24,7 +24,7 @@ from wrapper import Wrapper
 
 
 class Help(PagedCommand, MirrorSafeCommand):
-  common = False
+  COMMON = False
   helpSummary = "Display detailed help on a command"
   helpUsage = """
 %prog [--all|command]
@@ -73,7 +73,7 @@ Displays detailed usage information about a command.
 
     commandNames = list(sorted([name
                                 for name, command in all_commands.items()
-                                if command.common and gitc_supported(command)]))
+                                if command.COMMON and gitc_supported(command)]))
     self._PrintCommands(commandNames)
 
     print(
@@ -138,8 +138,7 @@ Displays detailed usage information about a command.
 
   def _PrintAllCommandHelp(self):
     for name in sorted(all_commands):
-      cmd = all_commands[name]()
-      cmd.manifest = self.manifest
+      cmd = all_commands[name](manifest=self.manifest)
       self._PrintCommandHelp(cmd, header_prefix='[%s] ' % (name,))
 
   def _Options(self, p):
@@ -163,12 +162,11 @@ Displays detailed usage information about a command.
       name = args[0]
 
       try:
-        cmd = all_commands[name]()
+        cmd = all_commands[name](manifest=self.manifest)
       except KeyError:
         print("repo: '%s' is not a repo command." % name, file=sys.stderr)
         sys.exit(1)
 
-      cmd.manifest = self.manifest
       self._PrintCommandHelp(cmd)
 
     else:

subcmds/info.py

@@ -25,7 +25,7 @@ class _Coloring(Coloring):
 
 
 class Info(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Get info on the manifest branch, current branch or unmerged branches"
   helpUsage = "%prog [-dl] [-o [-c]] [<project>...]"
 

subcmds/init.py

@@ -12,7 +12,6 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
-import optparse
 import os
 import platform
 import re
@@ -31,7 +30,7 @@ from wrapper import Wrapper
 
 
 class Init(InteractiveCommand, MirrorSafeCommand):
-  common = True
+  COMMON = True
   helpSummary = "Initialize a repo client checkout in the current directory"
   helpUsage = """
 %prog [options] [manifest url]
@@ -97,10 +96,17 @@ to update the working directory files.
     """
     superproject = git_superproject.Superproject(self.manifest,
                                                  self.repodir,
+                                                 self.git_event_log,
                                                  quiet=opt.quiet)
-    if not superproject.Sync():
-      print('error: git update of superproject failed', file=sys.stderr)
-      sys.exit(1)
+    sync_result = superproject.Sync()
+    if not sync_result.success:
+      print('warning: git update of superproject failed, repo sync will not '
+            'use superproject to fetch source; while this error is not fatal, '
+            'and you can continue to run repo sync, please run repo init with '
+            'the --no-use-superproject option to stop seeing this warning',
+            file=sys.stderr)
+      if sync_result.fatal and opt.use_superproject is not None:
+        sys.exit(1)
 
   def _SyncManifest(self, opt):
     m = self.manifest.manifestProject

subcmds/list.py

@@ -16,7 +16,7 @@ from command import Command, MirrorSafeCommand
 
 
 class List(Command, MirrorSafeCommand):
-  common = True
+  COMMON = True
   helpSummary = "List projects and their associated directories"
   helpUsage = """
 %prog [-f] [<project>...]

subcmds/manifest.py

@@ -20,7 +20,7 @@ from command import PagedCommand
 
 
 class Manifest(PagedCommand):
-  common = False
+  COMMON = False
   helpSummary = "Manifest inspection utility"
   helpUsage = """
 %prog [-o {-|NAME.xml}] [-m MANIFEST.xml] [-r]

subcmds/overview.py

@@ -19,7 +19,7 @@ from command import PagedCommand
 
 
 class Overview(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Display overview of unmerged project branches"
   helpUsage = """
 %prog [--current-branch] [<project>...]

subcmds/prune.py

@@ -19,7 +19,7 @@ from command import DEFAULT_LOCAL_JOBS, PagedCommand
 
 
 class Prune(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Prune (delete) already merged topics"
   helpUsage = """
 %prog [<project>...]

subcmds/rebase.py

@@ -27,7 +27,7 @@ class RebaseColoring(Coloring):
 
 
 class Rebase(Command):
-  common = True
+  COMMON = True
   helpSummary = "Rebase local branches on upstream branch"
   helpUsage = """
 %prog {[<project>...] | -i <project>...}

subcmds/selfupdate.py

@@ -21,7 +21,7 @@ from subcmds.sync import _PostRepoFetch
 
 
 class Selfupdate(Command, MirrorSafeCommand):
-  common = False
+  COMMON = False
   helpSummary = "Update repo to the latest version"
   helpUsage = """
 %prog

subcmds/smartsync.py

@@ -16,7 +16,7 @@ from subcmds.sync import Sync
 
 
 class Smartsync(Sync):
-  common = True
+  COMMON = True
   helpSummary = "Update working tree to the latest known good revision"
   helpUsage = """
 %prog [<project>...]

subcmds/stage.py

@@ -28,7 +28,7 @@ class _ProjectList(Coloring):
 
 
 class Stage(InteractiveCommand):
-  common = True
+  COMMON = True
   helpSummary = "Stage file(s) for commit"
   helpUsage = """
 %prog -i [<project>...]

subcmds/start.py

@@ -25,7 +25,7 @@ from project import SyncBuffer
 
 
 class Start(Command):
-  common = True
+  COMMON = True
   helpSummary = "Start a new branch for development"
   helpUsage = """
 %prog <newbranchname> [--all | <project>...]

subcmds/status.py

@@ -24,7 +24,7 @@ import platform_utils
 
 
 class Status(PagedCommand):
-  common = True
+  COMMON = True
   helpSummary = "Show the working tree status"
   helpUsage = """
 %prog [<project>...]

subcmds/sync.py

@@ -66,7 +66,7 @@ _ONE_DAY_S = 24 * 60 * 60
 
 class Sync(Command, MirrorSafeCommand):
   jobs = 1
-  common = True
+  COMMON = True
   helpSummary = "Update working tree to the latest revision"
   helpUsage = """
 %prog [<project>...]
@@ -278,16 +278,9 @@ later is required to fix a server side protocol bug.
       branch = branch[len(R_HEADS):]
     return branch
 
-  def _UseSuperproject(self, opt):
-    """Returns True if use-superproject option is enabled"""
-    if opt.use_superproject is not None:
-      return opt.use_superproject
-    else:
-      return self.manifest.manifestProject.config.GetBoolean('repo.superproject')
-
   def _GetCurrentBranchOnly(self, opt):
     """Returns True if current-branch or use-superproject options are enabled."""
-    return opt.current_branch_only or self._UseSuperproject(opt)
+    return opt.current_branch_only or git_superproject.UseSuperproject(opt, self.manifest)
 
   def _UpdateProjectsRevisionId(self, opt, args, load_local_manifests):
     """Update revisionId of every project with the SHA from superproject.
@@ -302,21 +295,26 @@ later is required to fix a server side protocol bug.
       load_local_manifests: Whether to load local manifests.
 
     Returns:
-      Returns path to the overriding manifest file.
+      Returns path to the overriding manifest file instead of None.
     """
     superproject = git_superproject.Superproject(self.manifest,
                                                  self.repodir,
+                                                 self.git_event_log,
                                                  quiet=opt.quiet)
     all_projects = self.GetProjects(args,
                                     missing_ok=True,
                                     submodules_ok=opt.fetch_submodules)
-    manifest_path = superproject.UpdateProjectsRevisionId(all_projects)
-    if not manifest_path:
-      print('error: Update of revsionId from superproject has failed. '
-            'Please resync with --no-use-superproject option',
+    update_result = superproject.UpdateProjectsRevisionId(all_projects)
+    manifest_path = update_result.manifest_path
+    if manifest_path:
+      self._ReloadManifest(manifest_path, load_local_manifests)
+    else:
+      print('warning: Update of revisionId from superproject has failed, '
+            'repo sync will not use superproject to fetch the source. ',
+            'Please resync with the --no-use-superproject option to avoid this repo warning.',
             file=sys.stderr)
-      sys.exit(1)
-    self._ReloadManifest(manifest_path, load_local_manifests)
+      if update_result.fatal and opt.use_superproject is not None:
+        sys.exit(1)
     return manifest_path
 
   def _FetchProjectList(self, opt, projects):
@@ -363,7 +361,7 @@ later is required to fix a server side protocol bug.
           partial_clone_exclude=self.manifest.PartialCloneExclude)
 
       output = buf.getvalue()
-      if opt.verbose and output:
+      if (opt.verbose or not success) and output:
         print('\n' + output.rstrip())
 
       if not success:
@@ -469,11 +467,14 @@ later is required to fix a server side protocol bug.
     Args:
       opt: Program options returned from optparse.  See _Options().
       args: Command line args used to filter out projects.
-      all_projects: List of all projects that should be checked out.
+      all_projects: List of all projects that should be fetched.
       err_event: Whether an error was hit while processing.
       manifest_name: Manifest file to be reloaded.
       load_local_manifests: Whether to load local manifests.
       ssh_proxy: SSH manager for clients & masters.
+
+    Returns:
+      List of all projects that should be checked out.
     """
     rp = self.manifest.repoProject
 
@@ -520,6 +521,8 @@ later is required to fix a server side protocol bug.
         err_event.set()
       fetched.update(new_fetched)
 
+    return all_projects
+
   def _CheckoutOne(self, detach_head, force_sync, project):
     """Checkout work tree for one project
 
@@ -955,8 +958,8 @@ later is required to fix a server side protocol bug.
       self._UpdateManifestProject(opt, mp, manifest_name)
 
     load_local_manifests = not self.manifest.HasLocalManifests
-    if self._UseSuperproject(opt):
-      manifest_name = self._UpdateProjectsRevisionId(opt, args, load_local_manifests)
+    if git_superproject.UseSuperproject(opt, self.manifest):
+      manifest_name = self._UpdateProjectsRevisionId(opt, args, load_local_manifests) or opt.manifest_name
 
     if self.gitc_manifest:
       gitc_manifest_projects = self.GetProjects(args,
@@ -1006,8 +1009,9 @@ later is required to fix a server side protocol bug.
         with ssh.ProxyManager(manager) as ssh_proxy:
           # Initialize the socket dir once in the parent.
           ssh_proxy.sock()
-          self._FetchMain(opt, args, all_projects, err_event, manifest_name,
-                          load_local_manifests, ssh_proxy)
+          all_projects = self._FetchMain(opt, args, all_projects, err_event,
+                                         manifest_name, load_local_manifests,
+                                         ssh_proxy)
 
       if opt.network_only:
         return

subcmds/upload.py

@@ -55,7 +55,7 @@ def _SplitEmails(values):
 
 
 class Upload(InteractiveCommand):
-  common = True
+  COMMON = True
   helpSummary = "Upload changes for code review"
   helpUsage = """
 %prog [--re --cc] [<project>]...

subcmds/version.py

@@ -25,7 +25,7 @@ class Version(Command, MirrorSafeCommand):
   wrapper_version = None
   wrapper_path = None
 
-  common = False
+  COMMON = False
   helpSummary = "Display the version of repo"
   helpUsage = """
 %prog

tests/test_git_superproject.py

@@ -14,6 +14,7 @@
 
 """Unittests for the git_superproject.py module."""
 
+import json
 import os
 import platform
 import tempfile
@@ -21,13 +22,20 @@ import unittest
 from unittest import mock
 
 import git_superproject
+import git_trace2_event_log
 import manifest_xml
 import platform_utils
+from test_manifest_xml import sort_attributes
 
 
 class SuperprojectTestCase(unittest.TestCase):
   """TestCase for the Superproject module."""
 
+  PARENT_SID_KEY = 'GIT_TRACE2_PARENT_SID'
+  PARENT_SID_VALUE = 'parent_sid'
+  SELF_SID_REGEX = r'repo-\d+T\d+Z-.*'
+  FULL_SID_REGEX = r'^%s/%s' % (PARENT_SID_VALUE, SELF_SID_REGEX)
+
   def setUp(self):
     """Set up superproject every time."""
     self.tempdir = tempfile.mkdtemp(prefix='repo_tests')
@@ -37,6 +45,13 @@ class SuperprojectTestCase(unittest.TestCase):
     os.mkdir(self.repodir)
     self.platform = platform.system().lower()
 
+    # By default we initialize with the expected case where
+    # repo launches us (so GIT_TRACE2_PARENT_SID is set).
+    env = {
+        self.PARENT_SID_KEY: self.PARENT_SID_VALUE,
+    }
+    self.git_event_log = git_trace2_event_log.EventLog(env=env)
+
     # The manifest parsing really wants a git repo currently.
     gitdir = os.path.join(self.repodir, 'manifests.git')
     os.mkdir(gitdir)
@@ -53,7 +68,8 @@ class SuperprojectTestCase(unittest.TestCase):
   <project path="art" name="platform/art" groups="notdefault,platform-""" + self.platform + """
   " /></manifest>
 """)
-    self._superproject = git_superproject.Superproject(manifest, self.repodir)
+    self._superproject = git_superproject.Superproject(manifest, self.repodir,
+                                                       self.git_event_log)
 
   def tearDown(self):
     """Tear down superproject every time."""
@@ -65,14 +81,56 @@ class SuperprojectTestCase(unittest.TestCase):
       fp.write(data)
     return manifest_xml.XmlManifest(self.repodir, self.manifest_file)
 
+  def verifyCommonKeys(self, log_entry, expected_event_name, full_sid=True):
+    """Helper function to verify common event log keys."""
+    self.assertIn('event', log_entry)
+    self.assertIn('sid', log_entry)
+    self.assertIn('thread', log_entry)
+    self.assertIn('time', log_entry)
+
+    # Do basic data format validation.
+    self.assertEqual(expected_event_name, log_entry['event'])
+    if full_sid:
+      self.assertRegex(log_entry['sid'], self.FULL_SID_REGEX)
+    else:
+      self.assertRegex(log_entry['sid'], self.SELF_SID_REGEX)
+    self.assertRegex(log_entry['time'], r'^\d+-\d+-\d+T\d+:\d+:\d+\.\d+Z$')
+
+  def readLog(self, log_path):
+    """Helper function to read log data into a list."""
+    log_data = []
+    with open(log_path, mode='rb') as f:
+      for line in f:
+        log_data.append(json.loads(line))
+    return log_data
+
+  def verifyErrorEvent(self):
+    """Helper to verify that error event is written."""
+
+    with tempfile.TemporaryDirectory(prefix='event_log_tests') as tempdir:
+      log_path = self.git_event_log.Write(path=tempdir)
+      self.log_data = self.readLog(log_path)
+
+    self.assertEqual(len(self.log_data), 2)
+    error_event = self.log_data[1]
+    self.verifyCommonKeys(self.log_data[0], expected_event_name='version')
+    self.verifyCommonKeys(error_event, expected_event_name='error')
+    # Check for 'error' event specific fields.
+    self.assertIn('msg', error_event)
+    self.assertIn('fmt', error_event)
+
   def test_superproject_get_superproject_no_superproject(self):
     """Test with no url."""
     manifest = self.getXmlManifest("""
 <manifest>
 </manifest>
 """)
-    superproject = git_superproject.Superproject(manifest, self.repodir)
-    self.assertFalse(superproject.Sync())
+    superproject = git_superproject.Superproject(manifest, self.repodir, self.git_event_log)
+    # Test that exit condition is false when there is no superproject tag.
+    sync_result = superproject.Sync()
+    self.assertFalse(sync_result.success)
+    self.assertFalse(sync_result.fatal)
+    self.verifyErrorEvent()
 
   def test_superproject_get_superproject_invalid_url(self):
     """Test with an invalid url."""
@@ -83,8 +141,10 @@ class SuperprojectTestCase(unittest.TestCase):
   <superproject name="superproject"/>
 </manifest>
 """)
-    superproject = git_superproject.Superproject(manifest, self.repodir)
-    self.assertFalse(superproject.Sync())
+    superproject = git_superproject.Superproject(manifest, self.repodir, self.git_event_log)
+    sync_result = superproject.Sync()
+    self.assertFalse(sync_result.success)
+    self.assertTrue(sync_result.fatal)
 
   def test_superproject_get_superproject_invalid_branch(self):
     """Test with an invalid branch."""
@@ -95,21 +155,28 @@ class SuperprojectTestCase(unittest.TestCase):
   <superproject name="superproject"/>
 </manifest>
 """)
-    superproject = git_superproject.Superproject(manifest, self.repodir)
+    self._superproject = git_superproject.Superproject(manifest, self.repodir,
+                                                       self.git_event_log)
     with mock.patch.object(self._superproject, '_GetBranch', return_value='junk'):
-      self.assertFalse(superproject.Sync())
+      sync_result = self._superproject.Sync()
+      self.assertFalse(sync_result.success)
+      self.assertTrue(sync_result.fatal)
 
   def test_superproject_get_superproject_mock_init(self):
     """Test with _Init failing."""
     with mock.patch.object(self._superproject, '_Init', return_value=False):
-      self.assertFalse(self._superproject.Sync())
+      sync_result = self._superproject.Sync()
+      self.assertFalse(sync_result.success)
+      self.assertTrue(sync_result.fatal)
 
   def test_superproject_get_superproject_mock_fetch(self):
     """Test with _Fetch failing."""
     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.assertFalse(self._superproject.Sync())
+        sync_result = self._superproject.Sync()
+        self.assertFalse(sync_result.success)
+        self.assertTrue(sync_result.fatal)
 
   def test_superproject_get_all_project_commit_ids_mock_ls_tree(self):
     """Test with LsTree being a mock."""
@@ -121,12 +188,13 @@ class SuperprojectTestCase(unittest.TestCase):
     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, {
+          commit_ids_result = self._superproject._GetAllProjectsCommitIds()
+          self.assertEqual(commit_ids_result.commit_ids, {
               'art': '2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea',
               'bootable/recovery': 'e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06',
               'build/bazel': 'ade9b7a0d874e25fff4bf2552488825c6f111928'
           })
+          self.assertFalse(commit_ids_result.fatal)
 
   def test_superproject_write_manifest_file(self):
     """Test with writing manifest to a file after setting revisionId."""
@@ -138,14 +206,14 @@ class SuperprojectTestCase(unittest.TestCase):
     manifest_path = self._superproject._WriteManfiestFile()
     self.assertIsNotNone(manifest_path)
     with open(manifest_path, 'r') as fp:
-      manifest_xml = fp.read()
+      manifest_xml_data = fp.read()
     self.assertEqual(
-        manifest_xml,
+        sort_attributes(manifest_xml_data),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="default-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="default-remote"/>'
         '<default remote="default-remote" revision="refs/heads/main"/>'
-        '<project name="platform/art" path="art" revision="ABCDEF" '
-        'groups="notdefault,platform-' + self.platform + '"/>'
+        '<project groups="notdefault,platform-' + self.platform + '" '
+        'name="platform/art" path="art" revision="ABCDEF" upstream="refs/heads/main"/>'
         '<superproject name="superproject"/>'
         '</manifest>')
 
@@ -162,40 +230,71 @@ class SuperprojectTestCase(unittest.TestCase):
                                return_value=data):
           # Create temporary directory so that it can write the file.
           os.mkdir(self._superproject._superproject_path)
-          manifest_path = self._superproject.UpdateProjectsRevisionId(projects)
-          self.assertIsNotNone(manifest_path)
-          with open(manifest_path, 'r') as fp:
-            manifest_xml = fp.read()
+          update_result = self._superproject.UpdateProjectsRevisionId(projects)
+          self.assertIsNotNone(update_result.manifest_path)
+          self.assertFalse(update_result.fatal)
+          with open(update_result.manifest_path, 'r') as fp:
+            manifest_xml_data = fp.read()
           self.assertEqual(
-              manifest_xml,
+              sort_attributes(manifest_xml_data),
               '<?xml version="1.0" ?><manifest>'
-              '<remote name="default-remote" fetch="http://localhost"/>'
+              '<remote fetch="http://localhost" name="default-remote"/>'
               '<default remote="default-remote" revision="refs/heads/main"/>'
-              '<project name="platform/art" path="art" '
-              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" '
-              'groups="notdefault,platform-' + self.platform + '"/>'
+              '<project groups="notdefault,platform-' + self.platform + '" '
+              'name="platform/art" path="art" '
+              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" upstream="refs/heads/main"/>'
               '<superproject name="superproject"/>'
               '</manifest>')
 
-  def test_superproject_update_project_revision_id_with_different_remotes(self):
-    """Test update of commit ids of a manifest with mutiple remotes."""
+  def test_superproject_update_project_revision_id_no_superproject_tag(self):
+    """Test update of commit ids of a manifest without superproject tag."""
+    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.maxDiff = None
+    self._superproject = git_superproject.Superproject(manifest, self.repodir,
+                                                       self.git_event_log)
+    self.assertEqual(len(self._superproject._manifest.projects), 1)
+    projects = self._superproject._manifest.projects
+    project = projects[0]
+    project.SetRevisionId('ABCDEF')
+    update_result = self._superproject.UpdateProjectsRevisionId(projects)
+    self.assertIsNone(update_result.manifest_path)
+    self.assertFalse(update_result.fatal)
+    self.verifyErrorEvent()
+    self.assertEqual(
+        sort_attributes(manifest.ToXml().toxml()),
+        '<?xml version="1.0" ?><manifest>'
+        '<remote fetch="http://localhost" name="default-remote"/>'
+        '<default remote="default-remote" revision="refs/heads/main"/>'
+        '<project name="test-name" revision="ABCDEF" upstream="refs/heads/main"/>'
+        '</manifest>')
+
+  def test_superproject_update_project_revision_id_from_local_manifest_group(self):
+    """Test update of commit ids of a manifest that have local manifest no superproject group."""
+    local_group = manifest_xml.LOCAL_MANIFEST_GROUP_PREFIX + ':local'
     manifest = self.getXmlManifest("""
 <manifest>
   <remote name="default-remote" fetch="http://localhost" />
   <remote name="goog" fetch="http://localhost2" />
   <default remote="default-remote" revision="refs/heads/main" />
   <superproject name="superproject"/>
-  <project path="vendor/x" name="platform/vendor/x" remote="goog" groups="vendor"
-           revision="master-with-vendor" clone-depth="1" />
+  <project path="vendor/x" name="platform/vendor/x" remote="goog"
+           groups=\"""" + local_group + """
+         " revision="master-with-vendor" clone-depth="1" />
   <project path="art" name="platform/art" groups="notdefault,platform-""" + self.platform + """
   " /></manifest>
 """)
     self.maxDiff = None
-    self._superproject = git_superproject.Superproject(manifest, self.repodir)
+    self._superproject = git_superproject.Superproject(manifest, self.repodir,
+                                                       self.git_event_log)
     self.assertEqual(len(self._superproject._manifest.projects), 2)
     projects = self._superproject._manifest.projects
-    data = ('160000 commit 2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea\tart\x00'
-            '160000 commit e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06\tbootable/recovery\x00')
+    data = ('160000 commit 2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea\tart\x00')
     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,
@@ -203,21 +302,72 @@ class SuperprojectTestCase(unittest.TestCase):
                                return_value=data):
           # Create temporary directory so that it can write the file.
           os.mkdir(self._superproject._superproject_path)
-          manifest_path = self._superproject.UpdateProjectsRevisionId(projects)
-          self.assertIsNotNone(manifest_path)
-          with open(manifest_path, 'r') as fp:
-            manifest_xml = fp.read()
+          update_result = self._superproject.UpdateProjectsRevisionId(projects)
+          self.assertIsNotNone(update_result.manifest_path)
+          self.assertFalse(update_result.fatal)
+          with open(update_result.manifest_path, 'r') as fp:
+            manifest_xml_data = fp.read()
+          # Verify platform/vendor/x's project revision hasn't changed.
           self.assertEqual(
-              manifest_xml,
+              sort_attributes(manifest_xml_data),
               '<?xml version="1.0" ?><manifest>'
-              '<remote name="default-remote" fetch="http://localhost"/>'
-              '<remote name="goog" fetch="http://localhost2"/>'
+              '<remote fetch="http://localhost" name="default-remote"/>'
+              '<remote fetch="http://localhost2" name="goog"/>'
               '<default remote="default-remote" revision="refs/heads/main"/>'
-              '<project name="platform/art" path="art" '
-              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" '
-              'groups="notdefault,platform-' + self.platform + '"/>'
-              '<project name="platform/vendor/x" path="vendor/x" remote="goog" '
-              'revision="master-with-vendor" groups="vendor" clone-depth="1"/>'
+              '<project groups="notdefault,platform-' + self.platform + '" '
+              'name="platform/art" path="art" '
+              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" upstream="refs/heads/main"/>'
+              '<project clone-depth="1" groups="' + local_group + '" '
+              'name="platform/vendor/x" path="vendor/x" remote="goog" '
+              'revision="master-with-vendor"/>'
+              '<superproject name="superproject"/>'
+              '</manifest>')
+
+  def test_superproject_update_project_revision_id_with_pinned_manifest(self):
+    """Test update of commit ids of a pinned manifest."""
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <superproject name="superproject"/>
+  <project path="vendor/x" name="platform/vendor/x" revision="" />
+  <project path="vendor/y" name="platform/vendor/y"
+           revision="52d3c9f7c107839ece2319d077de0cd922aa9d8f" />
+  <project path="art" name="platform/art" groups="notdefault,platform-""" + self.platform + """
+  " /></manifest>
+""")
+    self.maxDiff = None
+    self._superproject = git_superproject.Superproject(manifest, self.repodir,
+                                                       self.git_event_log)
+    self.assertEqual(len(self._superproject._manifest.projects), 3)
+    projects = self._superproject._manifest.projects
+    data = ('160000 commit 2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea\tart\x00'
+            '160000 commit e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06\tvendor/x\x00')
+    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):
+          # Create temporary directory so that it can write the file.
+          os.mkdir(self._superproject._superproject_path)
+          update_result = self._superproject.UpdateProjectsRevisionId(projects)
+          self.assertIsNotNone(update_result.manifest_path)
+          self.assertFalse(update_result.fatal)
+          with open(update_result.manifest_path, 'r') as fp:
+            manifest_xml_data = fp.read()
+          # Verify platform/vendor/x's project revision hasn't changed.
+          self.assertEqual(
+              sort_attributes(manifest_xml_data),
+              '<?xml version="1.0" ?><manifest>'
+              '<remote fetch="http://localhost" name="default-remote"/>'
+              '<default remote="default-remote" revision="refs/heads/main"/>'
+              '<project groups="notdefault,platform-' + self.platform + '" '
+              'name="platform/art" path="art" '
+              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" upstream="refs/heads/main"/>'
+              '<project name="platform/vendor/x" path="vendor/x" '
+              'revision="e9d25da64d8d365dbba7c8ee00fe8c4473fe9a06" upstream="refs/heads/main"/>'
+              '<project name="platform/vendor/y" path="vendor/y" '
+              'revision="52d3c9f7c107839ece2319d077de0cd922aa9d8f"/>'
               '<superproject name="superproject"/>'
               '</manifest>')
 

tests/test_git_trace2_event_log.py

@@ -234,6 +234,30 @@ class EventLogTestCase(unittest.TestCase):
     self.assertEqual(len(self._log_data), 1)
     self.verifyCommonKeys(self._log_data[0], expected_event_name='version')
 
+  def test_error_event(self):
+    """Test and validate 'error' event data is valid.
+
+    Expected event log:
+    <version event>
+    <error event>
+    """
+    msg = 'invalid option: --cahced'
+    fmt = 'invalid option: %s'
+    self._event_log_module.ErrorEvent(msg, fmt)
+    with tempfile.TemporaryDirectory(prefix='event_log_tests') as tempdir:
+      log_path = self._event_log_module.Write(path=tempdir)
+      self._log_data = self.readLog(log_path)
+
+    self.assertEqual(len(self._log_data), 2)
+    error_event = self._log_data[1]
+    self.verifyCommonKeys(self._log_data[0], expected_event_name='version')
+    self.verifyCommonKeys(error_event, expected_event_name='error')
+    # Check for 'error' event specific fields.
+    self.assertIn('msg', error_event)
+    self.assertIn('fmt', error_event)
+    self.assertEqual(error_event['msg'], msg)
+    self.assertEqual(error_event['fmt'], fmt)
+
   def test_write_with_filename(self):
     """Test Write() with a path to a file exits with None."""
     self.assertIsNone(self._event_log_module.Write(path='path/to/file'))

tests/test_manifest_xml.py

@@ -16,6 +16,7 @@
 
 import os
 import platform
+import re
 import shutil
 import tempfile
 import unittest
@@ -63,6 +64,30 @@ if os.path.sep != '/':
   INVALID_FS_PATHS += tuple(x.replace('/', os.path.sep) for x in INVALID_FS_PATHS)
 
 
+def sort_attributes(manifest):
+  """Sort the attributes of all elements alphabetically.
+
+  This is needed because different versions of the toxml() function from
+  xml.dom.minidom outputs the attributes of elements in different orders.
+  Before Python 3.8 they were output alphabetically, later versions preserve
+  the order specified by the user.
+
+  Args:
+    manifest: String containing an XML manifest.
+
+  Returns:
+    The XML manifest with the attributes of all elements sorted alphabetically.
+  """
+  new_manifest = ''
+  # This will find every element in the XML manifest, whether they have
+  # attributes or not. This simplifies recreating the manifest below.
+  matches = re.findall(r'(<[/?]?[a-z-]+\s*)((?:\S+?="[^"]+"\s*?)*)(\s*[/?]?>)', manifest)
+  for head, attrs, tail in matches:
+    m = re.findall(r'\S+?="[^"]+"', attrs)
+    new_manifest += head + ' '.join(sorted(m)) + tail
+  return new_manifest
+
+
 class ManifestParseTestCase(unittest.TestCase):
   """TestCase for parsing manifests."""
 
@@ -254,9 +279,9 @@ class XmlManifestTests(ManifestParseTestCase):
     self.assertEqual(manifest.superproject['name'], 'superproject')
     self.assertEqual(manifest.superproject['remote'].name, 'test-remote')
     self.assertEqual(
-        manifest.ToXml().toxml(),
+        sort_attributes(manifest.ToXml().toxml()),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="test-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="test-remote"/>'
         '<default remote="test-remote" revision="refs/heads/main"/>'
         '<superproject name="superproject"/>'
         '</manifest>')
@@ -408,11 +433,11 @@ class ProjectElementTests(ManifestParseTestCase):
     project = manifest.projects[0]
     project.SetRevisionId('ABCDEF')
     self.assertEqual(
-        manifest.ToXml().toxml(),
+        sort_attributes(manifest.ToXml().toxml()),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="default-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="default-remote"/>'
         '<default remote="default-remote" revision="refs/heads/main"/>'
-        '<project name="test-name" revision="ABCDEF"/>'
+        '<project name="test-name" revision="ABCDEF" upstream="refs/heads/main"/>'
         '</manifest>')
 
   def test_trailing_slash(self):
@@ -516,9 +541,9 @@ class SuperProjectElementTests(ManifestParseTestCase):
     self.assertEqual(manifest.superproject['remote'].name, 'test-remote')
     self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/superproject')
     self.assertEqual(
-        manifest.ToXml().toxml(),
+        sort_attributes(manifest.ToXml().toxml()),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="test-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="test-remote"/>'
         '<default remote="test-remote" revision="refs/heads/main"/>'
         '<superproject name="superproject"/>'
         '</manifest>')
@@ -537,10 +562,10 @@ class SuperProjectElementTests(ManifestParseTestCase):
     self.assertEqual(manifest.superproject['remote'].name, 'superproject-remote')
     self.assertEqual(manifest.superproject['remote'].url, 'http://localhost/platform/superproject')
     self.assertEqual(
-        manifest.ToXml().toxml(),
+        sort_attributes(manifest.ToXml().toxml()),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="default-remote" fetch="http://localhost"/>'
-        '<remote name="superproject-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="default-remote"/>'
+        '<remote fetch="http://localhost" name="superproject-remote"/>'
         '<default remote="default-remote" revision="refs/heads/main"/>'
         '<superproject name="platform/superproject" remote="superproject-remote"/>'
         '</manifest>')
@@ -557,9 +582,9 @@ class SuperProjectElementTests(ManifestParseTestCase):
     self.assertEqual(manifest.superproject['name'], 'superproject')
     self.assertEqual(manifest.superproject['remote'].name, 'default-remote')
     self.assertEqual(
-        manifest.ToXml().toxml(),
+        sort_attributes(manifest.ToXml().toxml()),
         '<?xml version="1.0" ?><manifest>'
-        '<remote name="default-remote" fetch="http://localhost"/>'
+        '<remote fetch="http://localhost" name="default-remote"/>'
         '<default remote="default-remote" revision="refs/heads/main"/>'
         '<superproject name="superproject"/>'
         '</manifest>')
@@ -582,3 +607,84 @@ class ContactinfoElementTests(ManifestParseTestCase):
         '<?xml version="1.0" ?><manifest>'
         f'<contactinfo bugurl="{bugurl}"/>'
         '</manifest>')
+
+
+class DefaultElementTests(ManifestParseTestCase):
+  """Tests for <default>."""
+
+  def test_default(self):
+    """Check default settings."""
+    a = manifest_xml._Default()
+    a.revisionExpr = 'foo'
+    a.remote = manifest_xml._XmlRemote(name='remote')
+    b = manifest_xml._Default()
+    b.revisionExpr = 'bar'
+    self.assertEqual(a, a)
+    self.assertNotEqual(a, b)
+    self.assertNotEqual(b, a.remote)
+    self.assertNotEqual(a, 123)
+    self.assertNotEqual(a, None)
+
+
+class RemoteElementTests(ManifestParseTestCase):
+  """Tests for <remote>."""
+
+  def test_remote(self):
+    """Check remote settings."""
+    a = manifest_xml._XmlRemote(name='foo')
+    b = manifest_xml._XmlRemote(name='bar')
+    self.assertEqual(a, a)
+    self.assertNotEqual(a, b)
+    self.assertNotEqual(a, manifest_xml._Default())
+    self.assertNotEqual(a, 123)
+    self.assertNotEqual(a, None)
+
+
+class RemoveProjectElementTests(ManifestParseTestCase):
+  """Tests for <remove-project>."""
+
+  def test_remove_one_project(self):
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="myproject" />
+  <remove-project name="myproject" />
+</manifest>
+""")
+    self.assertEqual(manifest.projects, [])
+
+  def test_remove_one_project_one_remains(self):
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="myproject" />
+  <project name="yourproject" />
+  <remove-project name="myproject" />
+</manifest>
+""")
+
+    self.assertEqual(len(manifest.projects), 1)
+    self.assertEqual(manifest.projects[0].name, 'yourproject')
+
+  def test_remove_one_project_doesnt_exist(self):
+    with self.assertRaises(manifest_xml.ManifestParseError):
+      manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <remove-project name="myproject" />
+</manifest>
+""")
+      manifest.projects
+
+  def test_remove_one_optional_project_doesnt_exist(self):
+    manifest = self.getXmlManifest("""
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <remove-project name="myproject" optional="true" />
+</manifest>
+""")
+    self.assertEqual(manifest.projects, [])

tox.ini

@@ -15,11 +15,10 @@
 # https://tox.readthedocs.io/
 
 [tox]
-envlist = py35, py36, py37, py38, py39
+envlist = py36, py37, py38, py39
 
 [gh-actions]
 python =
-    3.5: py35
     3.6: py36
     3.7: py37
     3.8: py38