From 46790229fcdb041c414a27035b72cbc0d2e78af6 Mon Sep 17 00:00:00 2001 From: Josip Sokcevic Date: Thu, 7 Mar 2024 22:18:58 +0000 Subject: [PATCH] sync: Fix sorting for nested projects The current logic to create checkout layers doesn't work in all cases. For example, let's assume there are three projects: "foo", "foo/bar" and "foo-bar". Sorting lexicographical order is incorrect as foo-bar would be placed between foo and foo/bar, breaking layering logic. Instead, we split filepaths based using path delimiter (always /) and then use lexicographical sort. BUG=b:325119758 TEST=./run_tests, manual sync on chromiumos repository Change-Id: I76924c3cc6ba2bb860d7a3e48406a6bba8f58c10 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/412338 Tested-by: Josip Sokcevic Commit-Queue: Josip Sokcevic Reviewed-by: George Engelbrecht --- subcmds/sync.py | 10 ++++++--- tests/test_subcmds_sync.py | 45 +++++++++++++++++++++++++++++--------- 2 files changed, 42 insertions(+), 13 deletions(-) diff --git a/subcmds/sync.py b/subcmds/sync.py index 7acb6e5b..113e7a67 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py @@ -102,9 +102,13 @@ def _SafeCheckoutOrder(checkouts: List[Project]) -> List[List[Project]]: # 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): + # Checkouts are iterated in the hierarchical order. That way, it can easily + # be determined if the previous checkout is parent of the current checkout. + # We are splitting by the path separator so the final result is + # hierarchical, and not just lexicographical. For example, if the projects + # are: foo, foo/bar, foo-bar, lexicographical order produces foo, foo-bar + # and foo/bar, which doesn't work. + for checkout in sorted(checkouts, key=lambda x: x.relpath.split("/")): checkout_path = Path(checkout.relpath) while depth_stack: try: diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index 13e23e34..8dde687c 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py @@ -304,29 +304,54 @@ class LocalSyncState(unittest.TestCase): self.assertEqual(self.state.GetFetchTime(projA), 5) +class FakeProject: + def __init__(self, relpath): + self.relpath = relpath + + def __str__(self): + return f"project: {self.relpath}" + + def __repr__(self): + return str(self) + + class SafeCheckoutOrder(unittest.TestCase): def test_no_nested(self): - p_f = mock.MagicMock(relpath="f") - p_foo = mock.MagicMock(relpath="foo") + p_f = FakeProject("f") + p_foo = FakeProject("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") + p_foo = p_foo = FakeProject("foo") + p_foo_bar = FakeProject("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") + p_foo = FakeProject("foo") + p_foobar = FakeProject("foobar") + p_foo_dash_bar = FakeProject("foo-bar") + p_foo_bar = FakeProject("foo/bar") + p_foo_bar_baz_baq = FakeProject("foo/bar/baz/baq") + p_bar = FakeProject("bar") out = sync._SafeCheckoutOrder( - [p_foo_bar_baz_baq, p_foo, p_foo_bar, p_bar] + [ + p_foo_bar_baz_baq, + p_foo, + p_foobar, + p_foo_dash_bar, + p_foo_bar, + p_bar, + ] ) self.assertEqual( - out, [[p_bar, p_foo], [p_foo_bar], [p_foo_bar_baz_baq]] + out, + [ + [p_bar, p_foo, p_foo_dash_bar, p_foobar], + [p_foo_bar], + [p_foo_bar_baz_baq], + ], )