forall: allow interactive commands with -j1

Historically forall has been interactive since it ran in serial.
Recent rework in here dropped that to enable parallel processing.
Restore support for interactive commands when running -j1 or with
an explicit --interactive option.

Bug: https://crbug.com/gerrit/14256
Change-Id: I502007186f771914cfd7830846a4e1938b5e1f38
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/300722
Reviewed-by: Michael Mortensen <mmortensen@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

command.py

@@ -178,9 +178,7 @@ class Command(object):
     mp = manifest.manifestProject
 
     if not groups:
-      groups = mp.config.GetString('manifest.groups')
-    if not groups:
-      groups = 'default,platform-' + platform.system().lower()
+      groups = manifest.GetGroupsStr()
     groups = [x for x in re.split(r'[,\s]+', groups) if x]
 
     if not args:

completion.bash

@@ -0,0 +1,121 @@
+# 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.
+
+# Programmable bash completion.  https://github.com/scop/bash-completion
+
+# Complete the list of repo subcommands.
+__complete_repo_list_commands() {
+  local repo=${COMP_WORDS[0]}
+  (
+  # Handle completions if running outside of a checkout.
+  if ! "${repo}" help --all 2>/dev/null; then
+    repo help 2>/dev/null
+  fi
+  ) | sed -n '/^  /{s/  \([^ ]\+\) .\+/\1/;p}'
+}
+
+# Complete list of all branches available in all projects in the repo client
+# checkout.
+__complete_repo_list_branches() {
+  local repo=${COMP_WORDS[0]}
+  "${repo}" branches 2>/dev/null | \
+    sed -n '/|/{s/[ *][Pp ] *\([^ ]\+\) .*/\1/;p}'
+}
+
+# Complete list of all projects available in the repo client checkout.
+__complete_repo_list_projects() {
+  local repo=${COMP_WORDS[0]}
+  "${repo}" list -n 2>/dev/null
+}
+
+# Complete the repo <command> argument.
+__complete_repo_command() {
+  if [[ ${COMP_CWORD} -ne 1 ]]; then
+    return 1
+  fi
+
+  local command=${COMP_WORDS[1]}
+  COMPREPLY=($(compgen -W "$(__complete_repo_list_commands)" -- "${command}"))
+  return 0
+}
+
+# Complete repo subcommands that take <branch> <projects>.
+__complete_repo_command_branch_projects() {
+  local current=$1
+  if [[ ${COMP_CWORD} -eq 2 ]]; then
+    COMPREPLY=($(compgen -W "$(__complete_repo_list_branches)" -- "${current}"))
+  else
+    COMPREPLY=($(compgen -W "$(__complete_repo_list_projects)" -- "${current}"))
+  fi
+}
+
+# Complete repo subcommands that take only <projects>.
+__complete_repo_command_projects() {
+  local current=$1
+  COMPREPLY=($(compgen -W "$(__complete_repo_list_projects)" -- "${current}"))
+}
+
+# Complete the repo subcommand arguments.
+__complete_repo_arg() {
+  if [[ ${COMP_CWORD} -le 1 ]]; then
+    return 1
+  fi
+
+  local command=${COMP_WORDS[1]}
+  local current=${COMP_WORDS[COMP_CWORD]}
+  case ${command} in
+  abandon|checkout)
+    __complete_repo_command_branch_projects "${current}"
+    return 0
+    ;;
+
+  branch|branches|diff|info|list|overview|prune|rebase|smartsync|stage|status|\
+  sync|upload)
+    __complete_repo_command_projects "${current}"
+    return 0
+    ;;
+
+  help)
+    if [[ ${COMP_CWORD} -eq 2 ]]; then
+      COMPREPLY=(
+        $(compgen -W "$(__complete_repo_list_commands)" -- "${current}")
+      )
+    fi
+    return 0
+    ;;
+
+  start)
+    if [[ ${COMP_CWORD} -gt 2 ]]; then
+      COMPREPLY=(
+        $(compgen -W "$(__complete_repo_list_projects)" -- "${current}")
+      )
+    fi
+    return 0
+    ;;
+
+  *)
+    return 1
+    ;;
+  esac
+}
+
+# Complete the repo arguments.
+__complete_repo() {
+  COMPREPLY=()
+  __complete_repo_command && return 0
+  __complete_repo_arg && return 0
+  return 0
+}
+
+complete -F __complete_repo repo

docs/manifest-format.md

@@ -252,12 +252,25 @@ name will be prefixed by the parent's.
 The project name must match the name Gerrit knows, if Gerrit is
 being used for code reviews.
 
+"name" must not be empty, and may not be an absolute path or use "." or ".."
+path components.  It is always interpreted relative to the remote's fetch
+settings, so if a different base path is needed, declare a different remote
+with the new settings needed.
+These restrictions are not enforced for [Local Manifests].
+
 Attribute `path`: An optional path relative to the top directory
 of the repo client where the Git working directory for this project
-should be placed.  If not supplied the project name is used.
+should be placed.  If not supplied the project "name" is used.
 If the project has a parent element, its path will be prefixed
 by the parent's.
 
+"path" may not be an absolute path or use "." or ".." path components.
+These restrictions are not enforced for [Local Manifests].
+
+If you want to place files into the root of the checkout (e.g. a README or
+Makefile or another build script), use the [copyfile] or [linkfile] elements
+instead.
+
 Attribute `remote`: Name of a previously defined remote element.
 If not supplied the remote given by the default element is used.
 
@@ -419,12 +432,15 @@ target manifest to include - it must be a usable manifest on its own.
 Attribute `name`: the manifest to include, specified relative to
 the manifest repository's root.
 
+"name" may not be an absolute path or use "." or ".." path components.
+These restrictions are not enforced for [Local Manifests].
+
 Attribute `groups`: List of additional groups to which all projects
 in the included manifest belong. This appends and recurses, meaning
 all projects in sub-manifests carry all parent include groups.
 Same syntax as the corresponding element of `project`.
 
-## Local Manifests
+## Local Manifests {#local-manifests}
 
 Additional remotes and projects may be added through local manifest
 files stored in `$TOP_DIR/.repo/local_manifests/*.xml`.
@@ -452,3 +468,8 @@ Manifest files stored in `$TOP_DIR/.repo/local_manifests/*.xml` will
 be loaded in alphabetical order.
 
 The legacy `$TOP_DIR/.repo/local_manifest.xml` path is no longer supported.
+
+
+[copyfile]: #Element-copyfile
+[linkfile]: #Element-linkfile
+[Local Manifests]: #local-manifests

git_superproject.py

@@ -235,7 +235,7 @@ class Superproject(object):
             self._superproject_path,
             file=sys.stderr)
       return None
-    manifest_str = self._manifest.ToXml().toxml()
+    manifest_str = self._manifest.ToXml(groups=self._manifest.GetGroupsStr()).toxml()
     manifest_path = self._manifest_path
     try:
       with open(manifest_path, 'w', encoding='utf-8') as fp:

git_trace2_event_log.py

@@ -132,6 +132,18 @@ class EventLog(object):
     exit_event['code'] = result
     self._log.append(exit_event)
 
+  def CommandEvent(self, name, subcommands):
+    """Append a 'command' event to the current log.
+
+    Args:
+      name: Name of the primary command (ex: repo, git)
+      subcommands: List of the sub-commands (ex: version, init, sync)
+    """
+    command_event = self._CreateEventDict('command')
+    command_event['name'] = name
+    command_event['subcommands'] = subcommands
+    self._log.append(command_event)
+
   def DefParamRepoEvents(self, config):
     """Append a 'def_param' event for each repo.* config key to the current log.
 

gitc_utils.py

@@ -77,22 +77,6 @@ def _set_project_revisions(projects):
       project.revisionExpr = revisionExpr
 
 
-def _manifest_groups(manifest):
-  """Returns the manifest group string that should be synced
-
-  This is the same logic used by Command.GetProjects(), which is used during
-  repo sync
-
-  Args:
-    manifest: The XmlManifest object
-  """
-  mp = manifest.manifestProject
-  groups = mp.config.GetString('manifest.groups')
-  if not groups:
-    groups = 'default,platform-' + platform.system().lower()
-  return groups
-
-
 def generate_gitc_manifest(gitc_manifest, manifest, paths=None):
   """Generate a manifest for shafsd to use for this GITC client.
 
@@ -107,7 +91,7 @@ def generate_gitc_manifest(gitc_manifest, manifest, paths=None):
   if paths is None:
     paths = list(manifest.paths.keys())
 
-  groups = [x for x in re.split(r'[,\s]+', _manifest_groups(manifest)) if x]
+  groups = [x for x in re.split(r'[,\s]+', manifest.GetGroupsStr()) if x]
 
   # Convert the paths to projects, and filter them to the matched groups.
   projects = [manifest.paths[p] for p in paths]
@@ -166,7 +150,7 @@ def save_manifest(manifest, client_dir=None):
   else:
     manifest_file = os.path.join(client_dir, '.manifest')
   with open(manifest_file, 'w') as f:
-    manifest.Save(f, groups=_manifest_groups(manifest))
+    manifest.Save(f, groups=manifest.GetGroupsStr())
   # TODO(sbasi/jorg): Come up with a solution to remove the sleep below.
   # Give the GITC filesystem time to register the manifest changes.
   time.sleep(3)

main.py

@@ -254,6 +254,7 @@ class _Repo(object):
     cmd_event = cmd.event_log.Add(name, event_log.TASK_COMMAND, start)
     cmd.event_log.SetParent(cmd_event)
     git_trace2_event_log.StartEvent()
+    git_trace2_event_log.CommandEvent(name='repo', subcommands=[name])
 
     try:
       cmd.ValidateOptions(copts, cargs)

manifest_xml.py

@@ -14,6 +14,7 @@
 
 import itertools
 import os
+import platform
 import re
 import sys
 import xml.dom.minidom
@@ -604,6 +605,17 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
   def HasSubmodules(self):
     return self.manifestProject.config.GetBoolean('repo.submodules')
 
+  def GetDefaultGroupsStr(self):
+    """Returns the default group string for the platform."""
+    return 'default,platform-' + platform.system().lower()
+
+  def GetGroupsStr(self):
+    """Returns the manifest group string that should be synced."""
+    groups = self.manifestProject.config.GetString('manifest.groups')
+    if not groups:
+      groups = self.GetDefaultGroupsStr()
+    return groups
+
   def _Unload(self):
     self._loaded = False
     self._projects = {}
@@ -1027,7 +1039,8 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     if not path:
       path = name
     else:
-      msg = self._CheckLocalPath(path, dir_ok=True)
+      # NB: The "." project is handled specially in Project.Sync_LocalHalf.
+      msg = self._CheckLocalPath(path, dir_ok=True, cwd_dot_ok=True)
       if msg:
         raise ManifestInvalidPathError(
             '<project> invalid "path": %s: %s' % (path, msg))
@@ -1215,7 +1228,9 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
     # our constructed logic here.  Especially since manifest authors only use
     # / in their paths.
     resep = re.compile(r'[/%s]' % re.escape(os.path.sep))
-    parts = resep.split(path)
+    # Strip off trailing slashes as those only produce '' elements, and we use
+    # parts to look for individual bad components.
+    parts = resep.split(path.rstrip('/'))
 
     # Some people use src="." to create stable links to projects.  Lets allow
     # that but reject all other uses of "." to keep things simple.

project.py

@@ -1227,6 +1227,18 @@ class Project(object):
     self.CleanPublishedCache(all_refs)
     revid = self.GetRevisionId(all_refs)
 
+    # Special case the root of the repo client checkout.  Make sure it doesn't
+    # contain files being checked out to dirs we don't allow.
+    if self.relpath == '.':
+      PROTECTED_PATHS = {'.repo'}
+      paths = set(self.work_git.ls_tree('-z', '--name-only', '--', revid).split('\0'))
+      bad_paths = paths & PROTECTED_PATHS
+      if bad_paths:
+        syncbuf.fail(self,
+                     'Refusing to checkout project that writes to protected '
+                     'paths: %s' % (', '.join(bad_paths),))
+        return
+
     def _doff():
       self._FastForward(revid)
       self._CopyAndLinkFiles()
@@ -1698,6 +1710,11 @@ class Project(object):
         if cb is None or name != cb:
           kill.append(name)
 
+    # Minor optimization: If there's nothing to prune, then don't try to read
+    # any project state.
+    if not kill and not cb:
+      return []
+
     rev = self.GetRevisionId(left)
     if cb is not None \
        and not self._revlist(HEAD + '...' + rev) \

subcmds/forall.py

@@ -52,6 +52,11 @@ Executes the same shell command in each project.
 The -r option allows running the command only on projects matching
 regex or wildcard expression.
 
+By default, projects are processed non-interactively in parallel.  If you want
+to run interactive commands, make sure to pass --interactive to force --jobs 1.
+While the processing order of projects is not guaranteed, the order of project
+output is stable.
+
 # Output Formatting
 
 The -p option causes '%prog' to bind pipes to the command's stdin,
@@ -154,6 +159,9 @@ without iterating through the remaining projects.
     g.add_option('-v', '--verbose',
                  dest='verbose', action='store_true',
                  help='Show command error messages')
+    p.add_option('--interactive',
+                 action='store_true',
+                 help='force interactive usage')
 
   def WantPager(self, opt):
     return opt.project_header and opt.jobs == 1
@@ -173,6 +181,11 @@ without iterating through the remaining projects.
       cmd.append(cmd[0])
     cmd.extend(opt.command[1:])
 
+    # Historically, forall operated interactively, and in serial.  If the user
+    # has selected 1 job, then default to interacive mode.
+    if opt.jobs == 1:
+      opt.interactive = True
+
     if opt.project_header \
             and not shell \
             and cmd[0] == 'git':
@@ -313,10 +326,12 @@ def DoWork(project, mirror, opt, cmd, shell, cnt, config):
   else:
     stderr = subprocess.DEVNULL
 
+  stdin = None if opt.interactive else subprocess.DEVNULL
+
   result = subprocess.run(
       cmd, cwd=cwd, shell=shell, env=env, check=False,
       encoding='utf-8', errors='replace',
-      stdin=subprocess.DEVNULL, stdout=subprocess.PIPE, stderr=stderr)
+      stdin=stdin, stdout=subprocess.PIPE, stderr=stderr)
 
   output = result.stdout
   if opt.project_header:

subcmds/init.py

@@ -267,7 +267,7 @@ to update the working directory files.
 
     groups = [x for x in groups if x]
     groupstr = ','.join(groups)
-    if opt.platform == 'auto' and groupstr == 'default,platform-' + platform.system().lower():
+    if opt.platform == 'auto' and groupstr == self.manifest.GetDefaultGroupsStr():
       groupstr = None
     m.config.SetString('manifest.groups', groupstr)
 

subcmds/sync.py

@@ -528,7 +528,7 @@ later is required to fix a server side protocol bug.
 
     pm.end()
 
-    return ret
+    return ret and not err_results
 
   def _GCProjects(self, projects, opt, err_event):
     gc_gitdirs = {}

tests/test_git_superproject.py

@@ -15,6 +15,7 @@
 """Unittests for the git_superproject.py module."""
 
 import os
+import platform
 import tempfile
 import unittest
 from unittest import mock
@@ -34,6 +35,7 @@ class SuperprojectTestCase(unittest.TestCase):
     self.manifest_file = os.path.join(
         self.repodir, manifest_xml.MANIFEST_FILE_NAME)
     os.mkdir(self.repodir)
+    self.platform = platform.system().lower()
 
     # The manifest parsing really wants a git repo currently.
     gitdir = os.path.join(self.repodir, 'manifests.git')
@@ -48,8 +50,8 @@ class SuperprojectTestCase(unittest.TestCase):
   <remote name="default-remote" fetch="http://localhost" />
   <default remote="default-remote" revision="refs/heads/main" />
   <superproject name="superproject"/>
-  <project path="art" name="platform/art" />
-</manifest>
+  <project path="art" name="platform/art" groups="notdefault,platform-""" + self.platform + """
+  " /></manifest>
 """)
     self._superproject = git_superproject.Superproject(manifest, self.repodir)
 
@@ -142,7 +144,8 @@ class SuperprojectTestCase(unittest.TestCase):
         '<?xml version="1.0" ?><manifest>' +
         '<remote name="default-remote" fetch="http://localhost"/>' +
         '<default remote="default-remote" revision="refs/heads/main"/>' +
-        '<project name="platform/art" path="art" revision="ABCDEF"/>' +
+        '<project name="platform/art" path="art" revision="ABCDEF" ' +
+        'groups="notdefault,platform-' + self.platform + '"/>' +
         '<superproject name="superproject"/>' +
         '</manifest>')
 
@@ -169,7 +172,8 @@ class SuperprojectTestCase(unittest.TestCase):
               '<remote name="default-remote" fetch="http://localhost"/>' +
               '<default remote="default-remote" revision="refs/heads/main"/>' +
               '<project name="platform/art" path="art" ' +
-              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea"/>' +
+              'revision="2c2724cb36cd5a9cec6c852c681efc3b7c6b86ea" ' +
+              'groups="notdefault,platform-' + self.platform + '"/>' +
               '<superproject name="superproject"/>' +
               '</manifest>')
 

tests/test_git_trace2_event_log.py

@@ -161,6 +161,30 @@ class EventLogTestCase(unittest.TestCase):
     self.assertIn('code', exit_event)
     self.assertEqual(exit_event['code'], 2)
 
+  def test_command_event(self):
+    """Test and validate 'command' event data is valid.
+
+    Expected event log:
+    <version event>
+    <command event>
+    """
+    name = 'repo'
+    subcommands = ['init' 'this']
+    self._event_log_module.CommandEvent(name='repo', subcommands=subcommands)
+    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)
+    command_event = self._log_data[1]
+    self.verifyCommonKeys(self._log_data[0], expected_event_name='version')
+    self.verifyCommonKeys(command_event, expected_event_name='command')
+    # Check for 'command' event specific fields.
+    self.assertIn('name', command_event)
+    self.assertIn('subcommands', command_event)
+    self.assertEqual(command_event['name'], name)
+    self.assertEqual(command_event['subcommands'], subcommands)
+
   def test_def_params_event_repo_config(self):
     """Test 'def_params' event data outputs only repo config keys.
 

tests/test_manifest_xml.py

@@ -15,6 +15,7 @@
 """Unittests for the manifest_xml.py module."""
 
 import os
+import platform
 import shutil
 import tempfile
 import unittest
@@ -31,6 +32,7 @@ INVALID_FS_PATHS = (
     '..',
     '../',
     './',
+    './/',
     'foo/',
     './foo',
     '../foo',
@@ -377,6 +379,11 @@ class ProjectElementTests(ManifestParseTestCase):
     self.assertCountEqual(
         result['extras'],
         ['g1', 'g2', 'g1', 'name:extras', 'all', 'path:path'])
+    groupstr = 'default,platform-' + platform.system().lower()
+    self.assertEqual(groupstr, manifest.GetGroupsStr())
+    groupstr = 'g1,g2,g1'
+    manifest.manifestProject.config.SetString('manifest.groups', groupstr)
+    self.assertEqual(groupstr, manifest.GetGroupsStr())
 
   def test_set_revision_id(self):
     """Check setting of project's revisionId."""
@@ -421,6 +428,28 @@ class ProjectElementTests(ManifestParseTestCase):
     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_toplevel_path(self):
+    """Check handling of path=. specially."""
+    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>
+""")
+
+    for path in ('.', './', './/', './//'):
+      manifest = parse('server/path', path)
+      self.assertEqual(manifest.projects[0].gitdir,
+                       os.path.join(self.tempdir, '.repo/projects/..git'))
+
   def test_bad_path_name_checks(self):
     """Check handling of bad path & name attributes."""
     def parse(name, path):
@@ -448,8 +477,11 @@ class ProjectElementTests(ManifestParseTestCase):
 
       with self.assertRaises(error.ManifestInvalidPathError):
         parse(path, 'ok')
-      with self.assertRaises(error.ManifestInvalidPathError):
-        parse('ok', path)
+
+      # We have a dedicated test for path=".".
+      if path not in {'.'}:
+        with self.assertRaises(error.ManifestInvalidPathError):
+          parse('ok', path)
 
 
 class SuperProjectElementTests(ManifestParseTestCase):