main: Fix exitcode logging

Fixed a couple of bugs in ExitEvent logging:
- log exitcode 130 on KeyboardInterrupt
- log exitcode 1 on unhandled Exception
- log errorevent with specific reason for exit

Before this CL an exitcode of 0 would be logged, and it would be
difficult to determine the cause of non-zero exit codes

Bug: b/287105597
Change-Id: I2d34f180581f9fbd77a1c78c966ebed065223af6
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/377834
Tested-by: Jason Chang <jasonnc@google.com>
Reviewed-by: Josip Sokcevic <sokcevic@google.com>

README.md

@@ -8,7 +8,7 @@ that you can put anywhere in your path.
 
 * Homepage: <https://gerrit.googlesource.com/git-repo/>
 * Mailing list: [repo-discuss on Google Groups][repo-discuss]
-* Bug reports: <https://bugs.chromium.org/p/gerrit/issues/list?q=component:Applications%3Erepo>
+* Bug reports: <https://issues.gerritcodereview.com/issues?q=is:open%20componentid:1370071>
 * Source: <https://gerrit.googlesource.com/git-repo/>
 * Overview: <https://source.android.com/source/developing.html>
 * Docs: <https://source.android.com/source/using-repo.html>
@@ -50,6 +50,6 @@ $ chmod a+rx ~/.bin/repo
 ```
 
 
-[new-bug]: https://bugs.chromium.org/p/gerrit/issues/entry?template=Repo+tool+issue
-[issue tracker]: https://bugs.chromium.org/p/gerrit/issues/list?q=component:Applications%3Erepo
+[new-bug]: https://issues.gerritcodereview.com/issues/new?component=1370071
+[issue tracker]: https://issues.gerritcodereview.com/issues?q=is:open%20componentid:1370071
 [repo-discuss]: https://groups.google.com/forum/#!forum/repo-discuss

docs/manifest-format.md

@@ -109,7 +109,8 @@ following DTD:
   <!ATTLIST extend-project upstream CDATA #IMPLIED>
 
   <!ELEMENT remove-project EMPTY>
-  <!ATTLIST remove-project name  CDATA #REQUIRED>
+  <!ATTLIST remove-project name     CDATA #IMPLIED>
+  <!ATTLIST remove-project path     CDATA #IMPLIED>
   <!ATTLIST remove-project optional CDATA #IMPLIED>
 
   <!ELEMENT repo-hooks EMPTY>
@@ -473,7 +474,7 @@ of the repo client.
 
 ### Element remove-project
 
-Deletes the named project from the internal manifest table, possibly
+Deletes a project from the internal manifest table, possibly
 allowing a subsequent project element in the same manifest file to
 replace the project with a different source.
 
@@ -481,6 +482,17 @@ 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.
 
+The project `name` or project `path` can be used to specify the remove target
+meaning one of them is required. If only name is specified, all
+projects with that name are removed.
+
+If both name and path are specified, only projects with the same name and
+path are removed, meaning projects with the same name but in other
+locations are kept.
+
+If only path is specified, a matching project is removed regardless of its
+name. Logic otherwise behaves like both are specified.
+
 Attribute `optional`: Set to true to ignore remove-project elements with no
 matching `project` element.
 

main.py

@@ -25,6 +25,7 @@ import netrc
 import optparse
 import os
 import shlex
+import signal
 import sys
 import textwrap
 import time
@@ -95,6 +96,7 @@ else:
             file=sys.stderr,
         )
 
+KEYBOARD_INTERRUPT_EXIT = 128 + signal.SIGINT
 
 global_options = optparse.OptionParser(
     usage="repo [-p|--paginate|--no-pager] COMMAND [ARGS]",
@@ -374,7 +376,11 @@ class _Repo(object):
         git_trace2_event_log.StartEvent()
         git_trace2_event_log.CommandEvent(name="repo", subcommands=[name])
 
-        try:
+        def execute_command_helper():
+            """
+            Execute the subcommand.
+            """
+            nonlocal result
             cmd.CommonValidateOptions(copts, cargs)
             cmd.ValidateOptions(copts, cargs)
 
@@ -409,6 +415,23 @@ class _Repo(object):
                     if hasattr(copts, "manifest_branch"):
                         child_argv.extend(["--manifest-branch", spec.revision])
                     result = self._Run(name, gopts, child_argv) or result
+
+        def execute_command():
+            """
+            Execute the command and log uncaught exceptions.
+            """
+            try:
+                execute_command_helper()
+            except (KeyboardInterrupt, SystemExit, Exception) as e:
+                ok = isinstance(e, SystemExit) and not e.code
+                if not ok:
+                    exception_name = type(e).__name__
+                    git_trace2_event_log.ErrorEvent(
+                        f"RepoExitError:{exception_name}")
+                raise
+
+        try:
+            execute_command()
         except (
             DownloadError,
             ManifestInvalidRevisionError,
@@ -448,6 +471,12 @@ class _Repo(object):
             if e.code:
                 result = e.code
             raise
+        except KeyboardInterrupt:
+            result = KEYBOARD_INTERRUPT_EXIT
+            raise
+        except Exception:
+            result = 1
+            raise
         finally:
             finish = time.time()
             elapsed = finish - start
@@ -813,7 +842,7 @@ def _Main(argv):
         result = repo._Run(name, gopts, argv) or 0
     except KeyboardInterrupt:
         print("aborted by user", file=sys.stderr)
-        result = 1
+        result = KEYBOARD_INTERRUPT_EXIT
     except ManifestParseError as mpe:
         print("fatal: %s" % mpe, file=sys.stderr)
         result = 1

man/repo.1

@@ -1,5 +1,5 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man.
-.TH REPO "1" "November 2022" "repo" "Repo Manual"
+.TH REPO "1" "June 2023" "repo" "Repo Manual"
 .SH NAME
 repo \- repository management tool built on top of git
 .SH SYNOPSIS
@@ -137,4 +137,4 @@ version
 Display the version of repo
 .PP
 See 'repo help <command>' for more information on a specific command.
-Bug reports: https://bugs.chromium.org/p/gerrit/issues/entry?template=Repo+tool+issue
+Bug reports: https://issues.gerritcodereview.com/issues/new?component=1370071

manifest_xml.py

@@ -981,6 +981,12 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
             return self.manifestProject.clone_filter
         return None
 
+    @property
+    def CloneFilterForDepth(self):
+        if self.manifestProject.clone_filter_for_depth:
+            return self.manifestProject.clone_filter_for_depth
+        return None
+
     @property
     def PartialCloneExclude(self):
         exclude = self.manifest.manifestProject.partial_clone_exclude or ""
@@ -1529,22 +1535,45 @@ https://gerrit.googlesource.com/git-repo/+/HEAD/docs/manifest-format.md
                 self._contactinfo = ContactInfo(bugurl)
 
             if node.nodeName == "remove-project":
-                name = self._reqatt(node, "name")
+                name = node.getAttribute("name")
+                path = node.getAttribute("path")
 
-                if name in self._projects:
-                    for p in self._projects[name]:
+                # Name or path needed.
+                if not name and not path:
+                    raise ManifestParseError(
+                        "remove-project must have name and/or path"
+                    )
+
+                removed_project = ""
+
+                # Find and remove projects based on name and/or path.
+                for projname, projects in list(self._projects.items()):
+                    for p in projects:
+                        if name == projname and not path:
                             del self._paths[p.relpath]
+                            if not removed_project:
                                 del self._projects[name]
+                                removed_project = name
+                        elif path == p.relpath and (
+                            name == projname or not name
+                        ):
+                            self._projects[projname].remove(p)
+                            del self._paths[p.relpath]
+                            removed_project = p.name
 
                 # If the manifest removes the hooks project, treat it as if
-                    # it deleted
-                    # the repo-hooks element too.
-                    if repo_hooks_project == name:
+                # it deleted the repo-hooks element too.
+                if (
+                    removed_project
+                    and removed_project not in self._projects
+                    and repo_hooks_project == removed_project
+                ):
                     repo_hooks_project = None
-                elif not XmlBool(node, "optional", False):
+
+                if not removed_project and not XmlBool(node, "optional", False):
                     raise ManifestParseError(
                         "remove-project element specifies non-existent "
-                        "project: %s" % name
+                        "project: %s" % node.toxml()
                     )
 
         # Store repo hooks project information.

pager.py

@@ -118,7 +118,10 @@ def _BecomePager(pager):
     # available versions of 'less', a better 'more'.
     _a, _b, _c = select.select([0], [], [0])
 
-    os.environ["LESS"] = "FRSX"
+    # This matches the behavior of git, which sets $LESS to `FRX` if it is not
+    # set. See:
+    # https://git-scm.com/docs/git-config#Documentation/git-config.txt-corepager
+    os.environ.setdefault("LESS", "FRX")
 
     try:
         os.execvp(pager, [pager])

progress.py

@@ -23,7 +23,7 @@ except ImportError:
 
 from repo_trace import IsTraceToStderr
 
-_NOT_TTY = not os.isatty(2)
+_TTY = sys.stderr.isatty()
 
 # This will erase all content in the current line (wherever the cursor is).
 # It does not move the cursor, so this is usually followed by \r to move to
@@ -97,7 +97,8 @@ class Progress(object):
         self._start = time.time()
         self._show = not delay
         self._units = units
-        self._elide = elide
+        self._elide = elide and _TTY
+
         # Only show the active jobs section if we run more than one in parallel.
         self._show_jobs = False
         self._active = 0
@@ -129,7 +130,7 @@ class Progress(object):
     def _write(self, s):
         s = "\r" + s
         if self._elide:
-            col = os.get_terminal_size().columns
+            col = os.get_terminal_size(sys.stderr.fileno()).columns
             if len(s) > col:
                 s = s[: col - 1] + ".."
         sys.stderr.write(s)
@@ -157,7 +158,7 @@ class Progress(object):
             msg = self._last_msg
         self._last_msg = msg
 
-        if _NOT_TTY or IsTraceToStderr():
+        if not _TTY or IsTraceToStderr():
             return
 
         elapsed_sec = time.time() - self._start
@@ -199,7 +200,7 @@ class Progress(object):
 
     def end(self):
         self._update_event.set()
-        if _NOT_TTY or IsTraceToStderr() or not self._show:
+        if not _TTY or IsTraceToStderr() or not self._show:
             return
 
         duration = duration_str(time.time() - self._start)

project.py

@@ -1186,6 +1186,7 @@ class Project(object):
         ssh_proxy=None,
         clone_filter=None,
         partial_clone_exclude=set(),
+        clone_filter_for_depth=None,
     ):
         """Perform only the network IO portion of the sync process.
         Local working directory/branch state is not affected.
@@ -1295,6 +1296,10 @@ class Project(object):
         else:
             depth = self.manifest.manifestProject.depth
 
+        if depth and clone_filter_for_depth:
+            depth = None
+            clone_filter = clone_filter_for_depth
+
         # See if we can skip the network fetch entirely.
         remote_fetched = False
         if not (
@@ -3884,6 +3889,11 @@ class ManifestProject(MetaProject):
         """Partial clone exclude string"""
         return self.config.GetString("repo.partialcloneexclude")
 
+    @property
+    def clone_filter_for_depth(self):
+        """Replace shallow clone with partial clone."""
+        return self.config.GetString("repo.clonefilterfordepth")
+
     @property
     def manifest_platform(self):
         """The --platform argument from `repo init`."""
@@ -3961,6 +3971,7 @@ class ManifestProject(MetaProject):
             manifest_name=spec.manifestName,
             this_manifest_only=True,
             outer_manifest=False,
+            clone_filter_for_depth=mp.clone_filter_for_depth,
         )
 
     def Sync(
@@ -3991,6 +4002,7 @@ class ManifestProject(MetaProject):
         tags="",
         this_manifest_only=False,
         outer_manifest=True,
+        clone_filter_for_depth=None,
     ):
         """Sync the manifest and all submanifests.
 
@@ -4035,6 +4047,8 @@ class ManifestProject(MetaProject):
                 current sub manifest.
             outer_manifest: a boolean, whether to start at the outermost
                 manifest.
+            clone_filter_for_depth: a string, when specified replaces shallow
+                clones with partial.
 
         Returns:
             a boolean, whether the sync was successful.
@@ -4297,6 +4311,9 @@ class ManifestProject(MetaProject):
                     file=sys.stderr,
                 )
 
+        if clone_filter_for_depth is not None:
+            self.ConfigureCloneFilterForDepth(clone_filter_for_depth)
+
         if use_superproject is not None:
             self.config.SetBoolean("repo.superproject", use_superproject)
 
@@ -4311,6 +4328,7 @@ class ManifestProject(MetaProject):
                 submodules=submodules,
                 clone_filter=clone_filter,
                 partial_clone_exclude=self.manifest.PartialCloneExclude,
+                clone_filter_for_depth=self.manifest.CloneFilterForDepth,
             ).success
             if not success:
                 r = self.GetRemote()
@@ -4415,6 +4433,18 @@ class ManifestProject(MetaProject):
 
         return True
 
+    def ConfigureCloneFilterForDepth(self, clone_filter_for_depth):
+        """Configure clone filter to replace shallow clones.
+
+        Args:
+            clone_filter_for_depth: a string or None, e.g. 'blob:none' will
+            disable shallow clones and replace with partial clone. None will
+            enable shallow clones.
+        """
+        self.config.SetString(
+            "repo.clonefilterfordepth", clone_filter_for_depth
+        )
+
     def _ConfigureDepth(self, depth):
         """Configure the depth we'll sync down.
 

repo

@@ -146,10 +146,10 @@ REPO_REV = os.environ.get('REPO_REV')
 if not REPO_REV:
   REPO_REV = 'stable'
 # URL to file bug reports for repo tool issues.
-BUG_URL = 'https://bugs.chromium.org/p/gerrit/issues/entry?template=Repo+tool+issue'
+BUG_URL = 'https://issues.gerritcodereview.com/issues/new?component=1370071'
 
 # increment this whenever we make important changes to this script
-VERSION = (2, 32)
+VERSION = (2, 35)
 
 # increment this if the MAINTAINER_KEYS block is modified
 KEYRING_VERSION = (2, 3)

setup.py

@@ -40,7 +40,7 @@ setuptools.setup(
     long_description_content_type="text/plain",
     url="https://gerrit.googlesource.com/git-repo/",
     project_urls={
-        "Bug Tracker": "https://bugs.chromium.org/p/gerrit/issues/list?q=component:Applications%3Erepo",  # noqa: E501
+        "Bug Tracker": "https://issues.gerritcodereview.com/issues?q=is:open%20componentid:1370071",  # noqa: E501
     },
     # https://pypi.org/classifiers/
     classifiers=[

subcmds/init.py

@@ -20,6 +20,8 @@ from command import InteractiveCommand, MirrorSafeCommand
 from git_command import git_require, MIN_GIT_VERSION_SOFT, MIN_GIT_VERSION_HARD
 from wrapper import Wrapper
 
+_REPO_ALLOW_SHALLOW = os.environ.get("REPO_ALLOW_SHALLOW")
+
 
 class Init(InteractiveCommand, MirrorSafeCommand):
     COMMON = True
@@ -125,6 +127,9 @@ to update the working directory files.
         # manifest project is special and is created when instantiating the
         # manifest which happens before we parse options.
         self.manifest.manifestProject.clone_depth = opt.manifest_depth
+        clone_filter_for_depth = (
+            "blob:none" if (_REPO_ALLOW_SHALLOW == "0") else None
+        )
         if not self.manifest.manifestProject.Sync(
             manifest_url=opt.manifest_url,
             manifest_branch=opt.manifest_branch,
@@ -140,6 +145,7 @@ to update the working directory files.
             partial_clone=opt.partial_clone,
             clone_filter=opt.clone_filter,
             partial_clone_exclude=opt.partial_clone_exclude,
+            clone_filter_for_depth=clone_filter_for_depth,
             clone_bundle=opt.clone_bundle,
             git_lfs=opt.git_lfs,
             use_superproject=opt.use_superproject,

subcmds/sync.py

@@ -79,6 +79,8 @@ _ONE_DAY_S = 24 * 60 * 60
 _REPO_AUTO_GC = "REPO_AUTO_GC"
 _AUTO_GC = os.environ.get(_REPO_AUTO_GC) == "1"
 
+_REPO_ALLOW_SHALLOW = os.environ.get("REPO_ALLOW_SHALLOW")
+
 
 class _FetchOneResult(NamedTuple):
     """_FetchOne return value.
@@ -638,6 +640,7 @@ later is required to fix a server side protocol bug.
                 ssh_proxy=self.ssh_proxy,
                 clone_filter=project.manifest.CloneFilter,
                 partial_clone_exclude=project.manifest.PartialCloneExclude,
+                clone_filter_for_depth=project.manifest.CloneFilterForDepth,
             )
             success = sync_result.success
             remote_fetched = sync_result.remote_fetched
@@ -674,18 +677,22 @@ later is required to fix a server side protocol bug.
         cls.ssh_proxy = ssh_proxy
 
     def _GetSyncProgressMessage(self):
-        if len(self._sync_dict) == 0:
-            return None
-
         earliest_time = float("inf")
         earliest_proj = None
-        for project, t in self._sync_dict.items():
+        items = self._sync_dict.items()
+        for project, t in items:
             if t < earliest_time:
                 earliest_time = t
                 earliest_proj = project
 
+        if not earliest_proj:
+            # This function is called when sync is still running but in some
+            # cases (by chance), _sync_dict can contain no entries. Return some
+            # text to indicate that sync is still working.
+            return "..working.."
+
         elapsed = time.time() - earliest_time
-        jobs = jobs_str(len(self._sync_dict))
+        jobs = jobs_str(len(items))
         return f"{jobs} | {elapsed_str(elapsed)} {earliest_proj}"
 
     def _Fetch(self, projects, opt, err_event, ssh_proxy):
@@ -1440,6 +1447,7 @@ later is required to fix a server side protocol bug.
                 submodules=mp.manifest.HasSubmodules,
                 clone_filter=mp.manifest.CloneFilter,
                 partial_clone_exclude=mp.manifest.PartialCloneExclude,
+                clone_filter_for_depth=mp.manifest.CloneFilterForDepth,
             )
             finish = time.time()
             self.event_log.AddSync(
@@ -1589,6 +1597,15 @@ later is required to fix a server side protocol bug.
             _PostRepoUpgrade(manifest, quiet=opt.quiet)
 
         mp = manifest.manifestProject
+
+        if _REPO_ALLOW_SHALLOW is not None:
+            if _REPO_ALLOW_SHALLOW == "1":
+                mp.ConfigureCloneFilterForDepth(None)
+            elif (
+                _REPO_ALLOW_SHALLOW == "0" and mp.clone_filter_for_depth is None
+            ):
+                mp.ConfigureCloneFilterForDepth("blob:none")
+
         if opt.mp_update:
             self._UpdateAllManifestProjects(opt, mp, manifest_name)
         else:
@@ -1659,6 +1676,13 @@ later is required to fix a server side protocol bug.
         err_update_projects = False
         err_update_linkfiles = False
 
+        # Log the repo projects by existing and new.
+        existing = [x for x in all_projects if x.Exists]
+        mp.config.SetString("repo.existingprojectcount", str(len(existing)))
+        mp.config.SetString(
+            "repo.newprojectcount", str(len(all_projects) - len(existing))
+        )
+
         self._fetch_times = _FetchTimes(manifest)
         if not opt.local_only:
             with multiprocessing.Manager() as manager:

tests/test_manifest_xml.py

@@ -996,6 +996,44 @@ class RemoveProjectElementTests(ManifestParseTestCase):
         )
         self.assertEqual(manifest.projects, [])
 
+    def test_remove_using_path_attrib(self):
+        manifest = self.getXmlManifest(
+            """
+<manifest>
+  <remote name="default-remote" fetch="http://localhost" />
+  <default remote="default-remote" revision="refs/heads/main" />
+  <project name="project1" path="tests/path1" />
+  <project name="project1" path="tests/path2" />
+  <project name="project2" />
+  <project name="project3" />
+  <project name="project4" path="tests/path3" />
+  <project name="project4" path="tests/path4" />
+  <project name="project5" />
+  <project name="project6" path="tests/path6" />
+
+  <remove-project name="project1" path="tests/path2" />
+  <remove-project name="project3" />
+  <remove-project name="project4" />
+  <remove-project path="project5" />
+  <remove-project path="tests/path6" />
+</manifest>
+"""
+        )
+        found_proj1_path1 = False
+        found_proj2 = False
+        for proj in manifest.projects:
+            if proj.name == "project1":
+                found_proj1_path1 = True
+                self.assertEqual(proj.relpath, "tests/path1")
+            if proj.name == "project2":
+                found_proj2 = True
+            self.assertNotEqual(proj.name, "project3")
+            self.assertNotEqual(proj.name, "project4")
+            self.assertNotEqual(proj.name, "project5")
+            self.assertNotEqual(proj.name, "project6")
+        self.assertTrue(found_proj1_path1)
+        self.assertTrue(found_proj2)
+
 
 class ExtendProjectElementTests(ManifestParseTestCase):
     """Tests for <extend-project>."""