From 5554572f02537b8646139d59ab520e59e1d5f7b3 Mon Sep 17 00:00:00 2001
From: Josip Sokcevic <sokcevic@google.com>
Date: Thu, 22 Feb 2024 16:38:00 -0800
Subject: [PATCH] sync: Introduce git checkout levels

If a repo manifest is updated so that project B is placed within a
project A, and if project A had content in new B's location in the old
checkout, then repo sync could break depending on checkout order, since
B can't be checked out before A.

This change introduces checkout levels which enforces right sequence of
checkouts while still allowing for parallel checkout. In an example
above, A will always be checked out first before B.

BUG=b:325119758
TEST=./run_tests, manual sync on ChromeOS repository

Change-Id: Ib3b5e4d2639ca56620a1e4c6bf76d7b1ab805250
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/410421
Tested-by: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Greg Edelston <gredelston@google.com>
Commit-Queue: Josip Sokcevic <sokcevic@google.com>
Reviewed-by: Gavin Mak <gavinmak@google.com>
---
 subcmds/sync.py            | 64 ++++++++++++++++++++++++++++++++------
 tests/test_subcmds_sync.py | 26 ++++++++++++++++
 2 files changed, 81 insertions(+), 9 deletions(-)

diff --git a/subcmds/sync.py b/subcmds/sync.py
index bf1171dd..c6682a5b 100644
--- a/subcmds/sync.py
+++ b/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
@@ -87,6 +88,45 @@ _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.
 
@@ -1035,15 +1075,21 @@ 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.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
diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py
index af6bbef7..13e23e34 100644
--- a/tests/test_subcmds_sync.py
+++ b/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."""