sync: introduce --force-checkout

In some cases (e.g. in a CI system), it's desirable to be able to
instruct repo to force checkout. This flag passes --force flag to `git
checkout` operations.

Bug: b/327624021
Change-Id: I579edda546fb8147c4e1a267e2605fcf6e597421
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/411518
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
Reviewed-by: George Engelbrecht <engeg@google.com>
Tested-by: Josip Sokcevic <sokcevic@google.com>

git_command.py

@@ -135,6 +135,8 @@ def GetEventTargetPath():
     if retval == 0:
         # Strip trailing carriage-return in path.
         path = p.stdout.rstrip("\n")
+        if path == "":
+            return None
     elif retval != 1:
         # `git config --get` is documented to produce an exit status of `1`
         # if the requested variable is not present in the configuration.

main.py

@@ -270,10 +270,14 @@ class _Repo:
             self._PrintHelp(short=True)
             return 1
 
-        run = lambda: self._RunLong(name, gopts, argv) or 0
+        git_trace2_event_log = EventLog()
+        run = (
+            lambda: self._RunLong(name, gopts, argv, git_trace2_event_log) or 0
+        )
         with Trace(
-            "starting new command: %s",
+            "starting new command: %s [sid=%s]",
             ", ".join([name] + argv),
+            git_trace2_event_log.full_sid,
             first_trace=True,
         ):
             if gopts.trace_python:
@@ -290,12 +294,11 @@ class _Repo:
                 result = run()
         return result
 
-    def _RunLong(self, name, gopts, argv):
+    def _RunLong(self, name, gopts, argv, git_trace2_event_log):
         """Execute the (longer running) requested subcommand."""
         result = 0
         SetDefaultColoring(gopts.color)
 
-        git_trace2_event_log = EventLog()
         outer_client = RepoClient(self.repodir)
         repo_client = outer_client
         if gopts.submanifest_path:

project.py

@@ -21,6 +21,7 @@ import random
 import re
 import shutil
 import stat
+import string
 import subprocess
 import sys
 import tarfile
@@ -266,6 +267,7 @@ class ReviewableBranch:
         dest_branch=None,
         validate_certs=True,
         push_options=None,
+        patchset_description=None,
     ):
         self.project.UploadForReview(
             branch=self.name,
@@ -281,6 +283,7 @@ class ReviewableBranch:
             dest_branch=dest_branch,
             validate_certs=validate_certs,
             push_options=push_options,
+            patchset_description=patchset_description,
         )
 
     def GetPublishedRefs(self):
@@ -1089,6 +1092,7 @@ class Project:
         dest_branch=None,
         validate_certs=True,
         push_options=None,
+        patchset_description=None,
     ):
         """Uploads the named branch for code review."""
         if branch is None:
@@ -1171,6 +1175,10 @@ class Project:
             opts += ["wip"]
         if ready:
             opts += ["ready"]
+        if patchset_description:
+            opts += [
+                f"m={self._encode_patchset_description(patchset_description)}"
+            ]
         if opts:
             ref_spec = ref_spec + "%" + ",".join(opts)
         cmd.append(ref_spec)
@@ -1183,6 +1191,30 @@ class Project:
                 R_PUB + branch.name, R_HEADS + branch.name, message=msg
             )
 
+    @staticmethod
+    def _encode_patchset_description(original):
+        """Applies percent-encoding for strings sent as patchset description.
+
+        The encoding used is based on but stricter than URL encoding (Section
+        2.1 of RFC 3986). The only non-escaped characters are alphanumerics, and
+        'SPACE' (U+0020) can be represented as 'LOW LINE' (U+005F) or
+        'PLUS SIGN' (U+002B).
+
+        For more information, see the Gerrit docs here:
+        https://gerrit-review.googlesource.com/Documentation/user-upload.html#patch_set_description
+        """
+        SAFE = {ord(x) for x in string.ascii_letters + string.digits}
+
+        def _enc(b):
+            if b in SAFE:
+                return chr(b)
+            elif b == ord(" "):
+                return "_"
+            else:
+                return f"%{b:02x}"
+
+        return "".join(_enc(x) for x in original.encode("utf-8"))
+
     def _ExtractArchive(self, tarpath, path=None):
         """Extract the given tar on its current location
 
@@ -1483,6 +1515,7 @@ class Project:
         self,
         syncbuf,
         force_sync=False,
+        force_checkout=False,
         submodules=False,
         errors=None,
         verbose=False,
@@ -1570,7 +1603,7 @@ class Project:
                     syncbuf.info(self, "discarding %d commits", len(lost))
 
             try:
-                self._Checkout(revid, quiet=True)
+                self._Checkout(revid, force_checkout=force_checkout, quiet=True)
                 if submodules:
                     self._SyncSubmodules(quiet=True)
             except GitError as e:
@@ -2825,10 +2858,12 @@ class Project:
         except OSError:
             return False
 
-    def _Checkout(self, rev, quiet=False):
+    def _Checkout(self, rev, force_checkout=False, quiet=False):
         cmd = ["checkout"]
         if quiet:
             cmd.append("-q")
+        if force_checkout:
+            cmd.append("-f")
         cmd.append(rev)
         cmd.append("--")
         if GitCommand(self, cmd).Wait() != 0:

repo

@@ -1,4 +1,4 @@
-#!/usr/bin/env python
+#!/usr/bin/env python3
 #
 # Copyright (C) 2008 The Android Open Source Project
 #
@@ -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, 40)
+VERSION = (2, 42)
 
 # increment this if the MAINTAINER_KEYS block is modified
 KEYRING_VERSION = (2, 3)

subcmds/sync.py

@@ -21,6 +21,7 @@ import multiprocessing
 import netrc
 import optparse
 import os
+from pathlib import Path
 import sys
 import tempfile
 import time
@@ -82,16 +83,50 @@ from wrapper import Wrapper
 
 _ONE_DAY_S = 24 * 60 * 60
 
-# Env var to implicitly turn auto-gc back on.  This was added to allow a user to
-# revert a change in default behavior in v2.29.9.  Remove after 2023-04-01.
-_REPO_AUTO_GC = "REPO_AUTO_GC"
-_AUTO_GC = os.environ.get(_REPO_AUTO_GC) == "1"
-
 _REPO_ALLOW_SHALLOW = os.environ.get("REPO_ALLOW_SHALLOW")
 
 logger = RepoLogger(__file__)
 
 
+def _SafeCheckoutOrder(checkouts: List[Project]) -> List[List[Project]]:
+    """Generate a sequence of checkouts that is safe to perform. The client
+    should checkout everything from n-th index before moving to n+1.
+
+    This is only useful if manifest contains nested projects.
+
+    E.g. if foo, foo/bar and foo/bar/baz are project paths, then foo needs to
+    finish before foo/bar can proceed, and foo/bar needs to finish before
+    foo/bar/baz."""
+    res = [[]]
+    current = res[0]
+
+    # depth_stack contains a current stack of parent paths.
+    depth_stack = []
+    # checkouts are iterated in asc order by relpath. That way, it can easily be
+    # determined if the previous checkout is parent of the current checkout.
+    for checkout in sorted(checkouts, key=lambda x: x.relpath):
+        checkout_path = Path(checkout.relpath)
+        while depth_stack:
+            try:
+                checkout_path.relative_to(depth_stack[-1])
+            except ValueError:
+                # Path.relative_to returns ValueError if paths are not relative.
+                # TODO(sokcevic): Switch to is_relative_to once min supported
+                # version is py3.9.
+                depth_stack.pop()
+            else:
+                if len(depth_stack) >= len(res):
+                    # Another depth created.
+                    res.append([])
+                break
+
+        current = res[len(depth_stack)]
+        current.append(checkout)
+        depth_stack.append(checkout_path)
+
+    return res
+
+
 class _FetchOneResult(NamedTuple):
     """_FetchOne return value.
 
@@ -243,6 +278,11 @@ directories if they have previously been linked to a different
 object directory. WARNING: This may cause data to be lost since
 refs may be removed when overwriting.
 
+The --force-checkout option can be used to force git to switch revs even if the
+index or the working tree differs from HEAD, and if there are untracked files.
+WARNING: This may cause data to be lost since uncommitted changes may be
+removed.
+
 The --force-remove-dirty option can be used to remove previously used
 projects with uncommitted changes. WARNING: This may cause data to be
 lost since uncommitted changes may be removed with projects that no longer
@@ -340,6 +380,14 @@ later is required to fix a server side protocol bug.
             "point to a different object directory. WARNING: this "
             "may cause loss of data",
         )
+        p.add_option(
+            "--force-checkout",
+            dest="force_checkout",
+            action="store_true",
+            help="force checkout even if it results in throwing away "
+            "uncommitted modifications. "
+            "WARNING: this may cause loss of data",
+        )
         p.add_option(
             "--force-remove-dirty",
             dest="force_remove_dirty",
@@ -956,12 +1004,17 @@ later is required to fix a server side protocol bug.
 
         return _FetchMainResult(all_projects)
 
-    def _CheckoutOne(self, detach_head, force_sync, verbose, project):
+    def _CheckoutOne(
+        self, detach_head, force_sync, force_checkout, verbose, project
+    ):
         """Checkout work tree for one project
 
         Args:
             detach_head: Whether to leave a detached HEAD.
-            force_sync: Force checking out of the repo.
+            force_sync: Force checking out of .git directory (e.g. overwrite
+            existing git directory that was previously linked to a different
+            object directory).
+            force_checkout: Force checking out of the repo content.
             verbose: Whether to show verbose messages.
             project: Project object for the project to checkout.
 
@@ -976,7 +1029,11 @@ later is required to fix a server side protocol bug.
         errors = []
         try:
             project.Sync_LocalHalf(
-                syncbuf, force_sync=force_sync, errors=errors, verbose=verbose
+                syncbuf,
+                force_sync=force_sync,
+                force_checkout=force_checkout,
+                errors=errors,
+                verbose=verbose,
             )
             success = syncbuf.Finish()
         except GitError as e:
@@ -1040,15 +1097,22 @@ later is required to fix a server side protocol bug.
                 pm.update(msg=project.name)
             return ret
 
-        proc_res = self.ExecuteInParallel(
-            opt.jobs_checkout,
-            functools.partial(
-                self._CheckoutOne, opt.detach_head, opt.force_sync, opt.verbose
-            ),
-            all_projects,
-            callback=_ProcessResults,
-            output=Progress("Checking out", len(all_projects), quiet=opt.quiet),
-        )
+        for projects in _SafeCheckoutOrder(all_projects):
+            proc_res = self.ExecuteInParallel(
+                opt.jobs_checkout,
+                functools.partial(
+                    self._CheckoutOne,
+                    opt.detach_head,
+                    opt.force_sync,
+                    opt.force_checkout,
+                    opt.verbose,
+                ),
+                projects,
+                callback=_ProcessResults,
+                output=Progress(
+                    "Checking out", len(all_projects), quiet=opt.quiet
+                ),
+            )
 
         self._local_sync_state.Save()
         return proc_res and not err_results
@@ -1544,9 +1608,7 @@ later is required to fix a server side protocol bug.
                 mp, event_log.TASK_SYNC_LOCAL, start, time.time(), clean
             )
             if not clean:
-                raise UpdateManifestError(
-                    aggregate_errors=errors, project=mp.name
-                )
+                raise UpdateManifestError(aggregate_errors=errors)
             self._ReloadManifest(manifest_name, mp.manifest)
 
     def ValidateOptions(self, opt, args):
@@ -1577,16 +1639,6 @@ later is required to fix a server side protocol bug.
         if opt.prune is None:
             opt.prune = True
 
-        if opt.auto_gc is None and _AUTO_GC:
-            logger.error(
-                "Will run `git gc --auto` because %s is set. %s is deprecated "
-                "and will be removed in a future release.  Use `--auto-gc` "
-                "instead.",
-                _REPO_AUTO_GC,
-                _REPO_AUTO_GC,
-            )
-            opt.auto_gc = True
-
     def _ValidateOptionsWithManifest(self, opt, mp):
         """Like ValidateOptions, but after we've updated the manifest.
 
@@ -1630,7 +1682,7 @@ later is required to fix a server side protocol bug.
         errors = []
         try:
             self._ExecuteHelper(opt, args, errors)
-        except RepoExitError:
+        except (RepoExitError, RepoChangedException):
             raise
         except (KeyboardInterrupt, Exception) as e:
             raise RepoUnhandledExceptionError(e, aggregate_errors=errors)

subcmds/upload.py

@@ -244,6 +244,11 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             default=[],
             help="add a label when uploading",
         )
+        p.add_option(
+            "--pd",
+            "--patchset-description",
+            help="description for patchset",
+        )
         p.add_option(
             "--re",
             "--reviewers",
@@ -655,6 +660,7 @@ Gerrit Code Review:  https://www.gerritcodereview.com/
             dest_branch=destination,
             validate_certs=opt.validate_certs,
             push_options=opt.push_options,
+            patchset_description=opt.patchset_description,
         )
 
         branch.uploaded = True

tests/test_project.py

@@ -107,6 +107,16 @@ class ReviewableBranchTests(unittest.TestCase):
             self.assertTrue(rb.date)
 
 
+class ProjectTests(unittest.TestCase):
+    """Check Project behavior."""
+
+    def test_encode_patchset_description(self):
+        self.assertEqual(
+            project.Project._encode_patchset_description("abcd00!! +"),
+            "abcd00%21%21_%2b",
+        )
+
+
 class CopyLinkTestCase(unittest.TestCase):
     """TestCase for stub repo client checkouts.
 

tests/test_subcmds_sync.py

@@ -304,6 +304,32 @@ class LocalSyncState(unittest.TestCase):
         self.assertEqual(self.state.GetFetchTime(projA), 5)
 
 
+class SafeCheckoutOrder(unittest.TestCase):
+    def test_no_nested(self):
+        p_f = mock.MagicMock(relpath="f")
+        p_foo = mock.MagicMock(relpath="foo")
+        out = sync._SafeCheckoutOrder([p_f, p_foo])
+        self.assertEqual(out, [[p_f, p_foo]])
+
+    def test_basic_nested(self):
+        p_foo = p_foo = mock.MagicMock(relpath="foo")
+        p_foo_bar = mock.MagicMock(relpath="foo/bar")
+        out = sync._SafeCheckoutOrder([p_foo, p_foo_bar])
+        self.assertEqual(out, [[p_foo], [p_foo_bar]])
+
+    def test_complex_nested(self):
+        p_foo = mock.MagicMock(relpath="foo")
+        p_foo_bar = mock.MagicMock(relpath="foo/bar")
+        p_foo_bar_baz_baq = mock.MagicMock(relpath="foo/bar/baz/baq")
+        p_bar = mock.MagicMock(relpath="bar")
+        out = sync._SafeCheckoutOrder(
+            [p_foo_bar_baz_baq, p_foo, p_foo_bar, p_bar]
+        )
+        self.assertEqual(
+            out, [[p_bar, p_foo], [p_foo_bar], [p_foo_bar_baz_baq]]
+        )
+
+
 class GetPreciousObjectsState(unittest.TestCase):
     """Tests for _GetPreciousObjectsState."""