sync: Always use WORKER_BATCH_SIZE

With 551285fa35, the comment about number
of workers no longer stands - dict is shared among multiprocesses and
real time information is available.

Using 2.7k projects as the baseline, using chunk size of 4 takes close
to 5 minutes. A chunk size of 32 takes this down to 40s - a reduction of
rougly 8 times which matches the increase.

R=gavinmak@google.com

Bug: b/371638995
Change-Id: Ida5fd8f7abc44b3b82c02aa0f7f7ae01dff5eb07
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/438523
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Tested-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>

color.py

@@ -210,6 +210,7 @@ class Coloring:
                 if have_fg:
                     bg = a
                 else:
+                    have_fg = True
                     fg = a
             elif is_attr(a):
                 attr = a

constraints.txt

@@ -0,0 +1 @@
+black<24

error.py

@@ -107,6 +107,10 @@ class GitError(RepoError):
         return self.message
 
 
+class GitAuthError(RepoExitError):
+    """Cannot talk to remote due to auth issue."""
+
+
 class GitcUnsupportedError(RepoExitError):
     """Gitc no longer supported."""
 

git_command.py

@@ -313,12 +313,15 @@ class GitCommand:
             cwd = None
         command_name = cmdv[0]
         command.append(command_name)
-        # Need to use the --progress flag for fetch/clone so output will be
-        # displayed as by default git only does progress output if stderr is a
-        # TTY.
-        if sys.stderr.isatty() and command_name in ("fetch", "clone"):
-            if "--progress" not in cmdv and "--quiet" not in cmdv:
-                command.append("--progress")
+
+        if command_name in ("fetch", "clone"):
+            env["GIT_TERMINAL_PROMPT"] = "0"
+            # Need to use the --progress flag for fetch/clone so output will be
+            # displayed as by default git only does progress output if stderr is
+            # a TTY.
+            if sys.stderr.isatty():
+                if "--progress" not in cmdv and "--quiet" not in cmdv:
+                    command.append("--progress")
         command.extend(cmdv[1:])
 
         event_log = (

git_superproject.py

@@ -307,8 +307,6 @@ class Superproject:
             )
             return SyncResult(False, False)
 
-        _PrintBetaNotice()
-
         should_exit = True
         if not self._remote_url:
             self._LogWarning(
@@ -452,16 +450,6 @@ class Superproject:
         return UpdateProjectsResult(manifest_path, False)
 
 
-@functools.lru_cache(maxsize=10)
-def _PrintBetaNotice():
-    """Print the notice of beta status."""
-    print(
-        "NOTICE: --use-superproject is in beta; report any issues to the "
-        "address described in `repo version`",
-        file=sys.stderr,
-    )
-
-
 @functools.lru_cache(maxsize=None)
 def _UseSuperprojectFromConfiguration():
     """Returns the user choice of whether to use superproject."""

man/repo-init.1

@@ -1,5 +1,5 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man.
-.TH REPO "1" "October 2022" "repo init" "Repo Manual"
+.TH REPO "1" "September 2024" "repo init" "Repo Manual"
 .SH NAME
 repo \- repo init - manual page for repo init
 .SH SYNOPSIS
@@ -28,6 +28,11 @@ manifest repository location
 \fB\-b\fR REVISION, \fB\-\-manifest\-branch\fR=\fI\,REVISION\/\fR
 manifest branch or revision (use HEAD for default)
 .TP
+\fB\-\-manifest\-upstream\-branch\fR=\fI\,BRANCH\/\fR
+when a commit is provided to \fB\-\-manifest\-branch\fR, this
+is the name of the git ref in which the commit can be
+found
+.TP
 \fB\-m\fR NAME.xml, \fB\-\-manifest\-name\fR=\fI\,NAME\/\fR.xml
 initial manifest file
 .TP
@@ -163,6 +168,10 @@ The optional \fB\-b\fR argument can be used to select the manifest branch to che
 and use. If no branch is specified, the remote's default branch is used. This is
 equivalent to using \fB\-b\fR HEAD.
 .PP
+The optional \fB\-\-manifest\-upstream\-branch\fR argument can be used when a commit is
+provided to \fB\-\-manifest\-branch\fR (or \fB\-b\fR), to specify the name of the git ref in
+which the commit can be found.
+.PP
 The optional \fB\-m\fR argument can be used to specify an alternate manifest to be
 used. If no manifest is specified, the manifest default.xml will be used.
 .PP

man/repo-smartsync.1

@@ -1,5 +1,5 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man.
-.TH REPO "1" "April 2024" "repo smartsync" "Repo Manual"
+.TH REPO "1" "September 2024" "repo smartsync" "Repo Manual"
 .SH NAME
 repo \- repo smartsync - manual page for repo smartsync
 .SH SYNOPSIS
@@ -47,6 +47,10 @@ force remove projects with uncommitted modifications
 if projects no longer exist in the manifest. WARNING:
 this may cause loss of data
 .TP
+\fB\-\-rebase\fR
+rebase local commits regardless of whether they are
+published
+.TP
 \fB\-l\fR, \fB\-\-local\-only\fR
 only update working tree, don't fetch
 .TP

man/repo-sync.1

@@ -1,5 +1,5 @@
 .\" DO NOT MODIFY THIS FILE!  It was generated by help2man.
-.TH REPO "1" "April 2024" "repo sync" "Repo Manual"
+.TH REPO "1" "September 2024" "repo sync" "Repo Manual"
 .SH NAME
 repo \- repo sync - manual page for repo sync
 .SH SYNOPSIS
@@ -47,6 +47,10 @@ force remove projects with uncommitted modifications
 if projects no longer exist in the manifest. WARNING:
 this may cause loss of data
 .TP
+\fB\-\-rebase\fR
+rebase local commits regardless of whether they are
+published
+.TP
 \fB\-l\fR, \fB\-\-local\-only\fR
 only update working tree, don't fetch
 .TP

project.py

@@ -32,6 +32,7 @@ import urllib.parse
 
 from color import Coloring
 from error import DownloadError
+from error import GitAuthError
 from error import GitError
 from error import ManifestInvalidPathError
 from error import ManifestInvalidRevisionError
@@ -749,7 +750,7 @@ class Project:
 
         def _git(*args):
             # Ignore return code, in case there was no rebase in progress.
-            GitCommand(self, *args, log_as_error=False).Wait()
+            GitCommand(self, args, log_as_error=False).Wait()
 
         _git("cherry-pick", "--abort")
         _git("rebase", "--abort")
@@ -1535,6 +1536,7 @@ class Project:
         syncbuf,
         force_sync=False,
         force_checkout=False,
+        force_rebase=False,
         submodules=False,
         errors=None,
         verbose=False,
@@ -1680,18 +1682,21 @@ class Project:
         if pub:
             not_merged = self._revlist(not_rev(revid), pub)
             if not_merged:
-                if upstream_gain:
+                if upstream_gain and not force_rebase:
                     # The user has published this branch and some of those
                     # commits are not yet merged upstream.  We do not want
                     # to rewrite the published commits so we punt.
                     fail(
                         LocalSyncFail(
                             "branch %s is published (but not merged) and is "
-                            "now %d commits behind"
+                            "now %d commits behind. Fix this manually or rerun "
+                            "with the --rebase option to force a rebase."
                             % (branch.name, len(upstream_gain)),
                             project=self.name,
                         )
                     )
+                    return
+                syncbuf.later1(self, _doff, not verbose)
                 return
             elif pub == head:
                 # All published commits are merged, and thus we are a
@@ -2391,26 +2396,25 @@ class Project:
         try:
             # if revision (sha or tag) is not present then following function
             # throws an error.
+            revs = [f"{self.revisionExpr}^0"]
+            upstream_rev = None
+            if self.upstream:
+                upstream_rev = self.GetRemote().ToLocal(self.upstream)
+                revs.append(upstream_rev)
+
             self.bare_git.rev_list(
                 "-1",
                 "--missing=allow-any",
-                "%s^0" % self.revisionExpr,
+                *revs,
                 "--",
                 log_as_error=False,
             )
+
             if self.upstream:
-                rev = self.GetRemote().ToLocal(self.upstream)
-                self.bare_git.rev_list(
-                    "-1",
-                    "--missing=allow-any",
-                    "%s^0" % rev,
-                    "--",
-                    log_as_error=False,
-                )
                 self.bare_git.merge_base(
                     "--is-ancestor",
                     self.revisionExpr,
-                    rev,
+                    upstream_rev,
                     log_as_error=False,
                 )
             return True
@@ -2657,6 +2661,20 @@ class Project:
                 # Fallthru to sleep+retry logic at the bottom.
                 pass
 
+            # TODO(b/360889369#comment24): git may gc commits incorrectly.
+            # Until the root cause is fixed, retry fetch with --refetch which
+            # will bring the repository into a good state.
+            elif gitcmd.stdout and (
+                "could not parse commit" in gitcmd.stdout
+                or "unable to parse commit" in gitcmd.stdout
+            ):
+                cmd.insert(1, "--refetch")
+                print(
+                    "could not parse commit error, retrying with refetch",
+                    file=output_redir,
+                )
+                continue
+
             # Try to prune remote branches once in case there are conflicts.
             # For example, if the remote had refs/heads/upstream, but deleted
             # that and now has refs/heads/upstream/foo.
@@ -2682,6 +2700,33 @@ class Project:
                 )
                 # Continue right away so we don't sleep as we shouldn't need to.
                 continue
+            elif (
+                ret == 128
+                and gitcmd.stdout
+                and "fatal: could not read Username" in gitcmd.stdout
+            ):
+                # User needs to be authenticated, and Git wants to prompt for
+                # username and password.
+                print(
+                    "git requires authentication, but repo cannot perform "
+                    "interactive authentication. Check git credentials.",
+                    file=output_redir,
+                )
+                break
+            elif (
+                ret == 128
+                and gitcmd.stdout
+                and "remote helper 'sso' aborted session" in gitcmd.stdout
+            ):
+                # User needs to be authenticated, and Git wants to prompt for
+                # username and password.
+                print(
+                    "git requires authentication, but repo cannot perform "
+                    "interactive authentication.",
+                    file=output_redir,
+                )
+                raise GitAuthError(gitcmd.stdout)
+                break
             elif current_branch_only and is_sha1 and ret == 128:
                 # Exit code 128 means "couldn't find the ref you asked for"; if
                 # we're in sha1 mode, we just tried sync'ing from the upstream

repo

@@ -124,7 +124,7 @@ if not REPO_REV:
 BUG_URL = "https://issues.gerritcodereview.com/issues/new?component=1370071"
 
 # increment this whenever we make important changes to this script
-VERSION = (2, 45)
+VERSION = (2, 48)
 
 # increment this if the MAINTAINER_KEYS block is modified
 KEYRING_VERSION = (2, 3)
@@ -282,6 +282,12 @@ def InitParser(parser):
         metavar="REVISION",
         help="manifest branch or revision (use HEAD for default)",
     )
+    group.add_option(
+        "--manifest-upstream-branch",
+        help="when a commit is provided to --manifest-branch, this "
+        "is the name of the git ref in which the commit can be found",
+        metavar="BRANCH",
+    )
     group.add_option(
         "-m",
         "--manifest-name",

subcmds/init.py

@@ -52,6 +52,10 @@ The optional -b argument can be used to select the manifest branch
 to checkout and use.  If no branch is specified, the remote's default
 branch is used.  This is equivalent to using -b HEAD.
 
+The optional --manifest-upstream-branch argument can be used when a commit is
+provided to --manifest-branch (or -b), to specify the name of the git ref in
+which the commit can be found.
+
 The optional -m argument can be used to specify an alternate manifest
 to be used. If no manifest is specified, the manifest default.xml
 will be used.
@@ -135,6 +139,7 @@ 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
+        self.manifest.manifestProject.upstream = opt.manifest_upstream_branch
         clone_filter_for_depth = (
             "blob:none" if (_REPO_ALLOW_SHALLOW == "0") else None
         )
@@ -317,6 +322,12 @@ to update the working directory files.
                 " be used with --standalone-manifest."
             )
 
+        if opt.manifest_upstream_branch and opt.manifest_branch is None:
+            self.OptionParser.error(
+                "--manifest-upstream-branch cannot be used without "
+                "--manifest-branch."
+            )
+
         if args:
             if opt.manifest_url:
                 self.OptionParser.error(

subcmds/sync.py

@@ -131,6 +131,11 @@ def _SafeCheckoutOrder(checkouts: List[Project]) -> List[List[Project]]:
     return res
 
 
+def _chunksize(projects: int, jobs: int) -> int:
+    """Calculate chunk size for the given number of projects and jobs."""
+    return min(max(1, projects // jobs), WORKER_BATCH_SIZE)
+
+
 class _FetchOneResult(NamedTuple):
     """_FetchOne return value.
 
@@ -400,6 +405,13 @@ later is required to fix a server side protocol bug.
             "projects no longer exist in the manifest. "
             "WARNING: this may cause loss of data",
         )
+        p.add_option(
+            "--rebase",
+            dest="rebase",
+            action="store_true",
+            help="rebase local commits regardless of whether they are "
+            "published",
+        )
         p.add_option(
             "-l",
             "--local-only",
@@ -812,7 +824,6 @@ later is required to fix a server side protocol bug.
     def _Fetch(self, projects, opt, err_event, ssh_proxy, errors):
         ret = True
 
-        jobs = opt.jobs_network
         fetched = set()
         remote_fetched = set()
         pm = Progress(
@@ -842,6 +853,8 @@ later is required to fix a server side protocol bug.
             objdir_project_map.setdefault(project.objdir, []).append(project)
         projects_list = list(objdir_project_map.values())
 
+        jobs = min(opt.jobs_network, len(projects_list))
+
         def _ProcessResults(results_sets):
             ret = True
             for results in results_sets:
@@ -881,35 +894,22 @@ later is required to fix a server side protocol bug.
         Sync.ssh_proxy = None
 
         # NB: Multiprocessing is heavy, so don't spin it up for one job.
-        if len(projects_list) == 1 or jobs == 1:
+        if jobs == 1:
             self._FetchInitChild(ssh_proxy)
             if not _ProcessResults(
                 self._FetchProjectList(opt, x) for x in projects_list
             ):
                 ret = False
         else:
-            # Favor throughput over responsiveness when quiet.  It seems that
-            # imap() will yield results in batches relative to chunksize, so
-            # even as the children finish a sync, we won't see the result until
-            # one child finishes ~chunksize jobs.  When using a large --jobs
-            # with large chunksize, this can be jarring as there will be a large
-            # initial delay where repo looks like it isn't doing anything and
-            # sits at 0%, but then suddenly completes a lot of jobs all at once.
-            # Since this code is more network bound, we can accept a bit more
-            # CPU overhead with a smaller chunksize so that the user sees more
-            # immediate & continuous feedback.
-            if opt.quiet:
-                chunksize = WORKER_BATCH_SIZE
-            else:
+            if not opt.quiet:
                 pm.update(inc=0, msg="warming up")
-                chunksize = 4
             with multiprocessing.Pool(
                 jobs, initializer=self._FetchInitChild, initargs=(ssh_proxy,)
             ) as pool:
                 results = pool.imap_unordered(
                     functools.partial(self._FetchProjectList, opt),
                     projects_list,
-                    chunksize=chunksize,
+                    chunksize=_chunksize(len(projects_list), jobs),
                 )
                 if not _ProcessResults(results):
                     ret = False
@@ -1009,7 +1009,13 @@ later is required to fix a server side protocol bug.
         return _FetchMainResult(all_projects)
 
     def _CheckoutOne(
-        self, detach_head, force_sync, force_checkout, verbose, project
+        self,
+        detach_head,
+        force_sync,
+        force_checkout,
+        force_rebase,
+        verbose,
+        project,
     ):
         """Checkout work tree for one project
 
@@ -1019,6 +1025,7 @@ later is required to fix a server side protocol bug.
             existing git directory that was previously linked to a different
             object directory).
             force_checkout: Force checking out of the repo content.
+            force_rebase: Force rebase.
             verbose: Whether to show verbose messages.
             project: Project object for the project to checkout.
 
@@ -1036,6 +1043,7 @@ later is required to fix a server side protocol bug.
                 syncbuf,
                 force_sync=force_sync,
                 force_checkout=force_checkout,
+                force_rebase=force_rebase,
                 errors=errors,
                 verbose=verbose,
             )
@@ -1109,6 +1117,7 @@ later is required to fix a server side protocol bug.
                     opt.detach_head,
                     opt.force_sync,
                     opt.force_checkout,
+                    opt.rebase,
                     opt.verbose,
                 ),
                 projects,
@@ -1480,6 +1489,19 @@ later is required to fix a server side protocol bug.
                     [success, manifest_str] = server.GetApprovedManifest(
                         branch, target
                     )
+                elif (
+                    "TARGET_PRODUCT" in os.environ
+                    and "TARGET_BUILD_VARIANT" in os.environ
+                    and "TARGET_RELEASE" in os.environ
+                ):
+                    target = "%s-%s-%s" % (
+                        os.environ["TARGET_PRODUCT"],
+                        os.environ["TARGET_RELEASE"],
+                        os.environ["TARGET_BUILD_VARIANT"],
+                    )
+                    [success, manifest_str] = server.GetApprovedManifest(
+                        branch, target
+                    )
                 elif (
                     "TARGET_PRODUCT" in os.environ
                     and "TARGET_BUILD_VARIANT" in os.environ

tests/fixtures/test.gitconfig

@@ -11,3 +11,11 @@
 	intk = 10k
 	intm = 10m
 	intg = 10g
+
+[color "status"]
+	one = yellow
+	two = magenta cyan
+	three = black red ul
+	reset = reset
+	none
+	empty =

tests/test_color.py

@@ -0,0 +1,74 @@
+# Copyright (C) 2024 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the color.py module."""
+
+import os
+import unittest
+
+import color
+import git_config
+
+
+def fixture(*paths):
+    """Return a path relative to test/fixtures."""
+    return os.path.join(os.path.dirname(__file__), "fixtures", *paths)
+
+
+class ColoringTests(unittest.TestCase):
+    """tests of the Coloring class."""
+
+    def setUp(self):
+        """Create a GitConfig object using the test.gitconfig fixture."""
+        config_fixture = fixture("test.gitconfig")
+        self.config = git_config.GitConfig(config_fixture)
+        color.SetDefaultColoring("true")
+        self.color = color.Coloring(self.config, "status")
+
+    def test_Color_Parse_all_params_none(self):
+        """all params are None"""
+        val = self.color._parse(None, None, None, None)
+        self.assertEqual("", val)
+
+    def test_Color_Parse_first_parameter_none(self):
+        """check fg & bg & attr"""
+        val = self.color._parse(None, "black", "red", "ul")
+        self.assertEqual("\x1b[4;30;41m", val)
+
+    def test_Color_Parse_one_entry(self):
+        """check fg"""
+        val = self.color._parse("one", None, None, None)
+        self.assertEqual("\033[33m", val)
+
+    def test_Color_Parse_two_entry(self):
+        """check fg & bg"""
+        val = self.color._parse("two", None, None, None)
+        self.assertEqual("\033[35;46m", val)
+
+    def test_Color_Parse_three_entry(self):
+        """check fg & bg & attr"""
+        val = self.color._parse("three", None, None, None)
+        self.assertEqual("\033[4;30;41m", val)
+
+    def test_Color_Parse_reset_entry(self):
+        """check reset entry"""
+        val = self.color._parse("reset", None, None, None)
+        self.assertEqual("\033[m", val)
+
+    def test_Color_Parse_empty_entry(self):
+        """check empty entry"""
+        val = self.color._parse("none", "blue", "white", "dim")
+        self.assertEqual("\033[2;34;47m", val)
+        val = self.color._parse("empty", "green", "white", "bold")
+        self.assertEqual("\033[1;32;47m", val)

tests/test_subcmds_sync.py

@@ -355,6 +355,30 @@ class SafeCheckoutOrder(unittest.TestCase):
         )
 
 
+class Chunksize(unittest.TestCase):
+    """Tests for _chunksize."""
+
+    def test_single_project(self):
+        """Single project."""
+        self.assertEqual(sync._chunksize(1, 1), 1)
+
+    def test_low_project_count(self):
+        """Multiple projects, low number of projects to sync."""
+        self.assertEqual(sync._chunksize(10, 1), 10)
+        self.assertEqual(sync._chunksize(10, 2), 5)
+        self.assertEqual(sync._chunksize(10, 4), 2)
+        self.assertEqual(sync._chunksize(10, 8), 1)
+        self.assertEqual(sync._chunksize(10, 16), 1)
+
+    def test_high_project_count(self):
+        """Multiple projects, high number of projects to sync."""
+        self.assertEqual(sync._chunksize(2800, 1), 32)
+        self.assertEqual(sync._chunksize(2800, 16), 32)
+        self.assertEqual(sync._chunksize(2800, 32), 32)
+        self.assertEqual(sync._chunksize(2800, 64), 32)
+        self.assertEqual(sync._chunksize(2800, 128), 21)
+
+
 class GetPreciousObjectsState(unittest.TestCase):
     """Tests for _GetPreciousObjectsState."""
 

tox.ini

@@ -30,6 +30,7 @@ python =
 
 [testenv]
 deps =
+    -c constraints.txt
     black
     flake8
     isort
@@ -44,17 +45,19 @@ setenv =
 [testenv:lint]
 skip_install = true
 deps =
+    -c constraints.txt
     black
     flake8
 commands =
-    black --check {posargs:.}
+    black --check {posargs:. repo run_tests release/update-hooks release/update-manpages}
     flake8
 
 [testenv:format]
 skip_install = true
 deps =
+    -c constraints.txt
     black
     flake8
 commands =
-    black {posargs:.}
+    black {posargs:. repo run_tests release/update-hooks release/update-manpages}
     flake8