diff --git a/platform_utils.py b/platform_utils.py index 45ffec78c..58df6a9e2 100644 --- a/platform_utils.py +++ b/platform_utils.py @@ -222,6 +222,43 @@ def rmdir(path): os.rmdir(_makelongpath(path)) +def removedirs(path: str) -> None: + """Remove a directory tree of symlinks and empty directories. + + Walks |path| bottom-up without following symlinks. Removes symlinks + and empty directories. Stops at (and preserves) regular files and + non-empty directories. Silently succeeds if |path| does not exist. + + Availability: Unix, Windows. + """ + if islink(path): + remove(path) + return + + if not isdir(path): + return + + for dirpath, dirnames, filenames in walk(path, topdown=False): + for name in dirnames: + entry = os.path.join(dirpath, name) + if islink(entry): + remove(entry) + else: + try: + rmdir(entry) + except OSError: + pass + for name in filenames: + entry = os.path.join(dirpath, name) + if islink(entry): + remove(entry) + + try: + rmdir(path) + except OSError: + pass + + def isdir(path): """os.path.isdir(path) wrapper with support for long paths on Windows. diff --git a/project.py b/project.py index f2fe7bc2a..5f2a98e18 100644 --- a/project.py +++ b/project.py @@ -453,9 +453,13 @@ class _LinkFile(NamedTuple): platform_utils.readlink(absDest) != relSrc ): try: - # Remove existing file first, since it might be read-only. + # Remove existing path first, since it might be read-only. if os.path.lexists(absDest): - platform_utils.remove(absDest) + # removedirs handles symlinks, empty dirs, and nested + # trees of stale linkfile dests without deleting user + # content. Falls through to symlink() if the path was + # fully removed, or raises OSError if not. + platform_utils.removedirs(absDest) else: dest_dir = os.path.dirname(absDest) if not platform_utils.isdir(dest_dir): diff --git a/subcmds/sync.py b/subcmds/sync.py index 2517694e7..291bc6243 100644 --- a/subcmds/sync.py +++ b/subcmds/sync.py @@ -1631,9 +1631,10 @@ later is required to fix a server side protocol bug. new_paths = {} new_linkfile_paths = [] new_copyfile_paths = [] - for project in self.GetProjects( + projects = self.GetProjects( None, missing_ok=True, manifest=manifest, all_manifests=False - ): + ) + for project in projects: new_linkfile_paths.extend(x.dest for x in project.linkfiles) new_copyfile_paths.extend(x.dest for x in project.copyfiles) @@ -1669,17 +1670,36 @@ later is required to fix a server side protocol bug. ) for need_remove_file in need_remove_files: - # Try to remove the updated copyfile or linkfile. - # So, if the file is not exist, nothing need to do. - platform_utils.remove( - os.path.join(self.client.topdir, need_remove_file), - missing_ok=True, + need_remove_path = os.path.join( + self.client.topdir, need_remove_file ) + if os.path.isfile(need_remove_path): + platform_utils.remove(need_remove_path) + else: + platform_utils.removedirs(need_remove_path) + + # Also try to remove empty parent directories. + parent = os.path.dirname(need_remove_path) + while parent != self.client.topdir: + try: + os.rmdir(parent) + except OSError: + break + parent = os.path.dirname(parent) # Create copy-link-files.json, save dest path of "copyfile" and # "linkfile". with open(copylinkfile_path, "w", encoding="utf-8") as fp: json.dump(new_paths, fp) + + # Retry linkfile/copyfile creation for all projects. In + # interleaved sync mode, _CopyAndLinkFiles runs before this + # cleanup, so linkfiles whose dest was blocked by an old + # directory may have failed. _CopyAndLinkFiles is idempotent + # and skips dests that are already correct. + for project in projects: + project._CopyAndLinkFiles() + return True def _SmartSyncSetup(self, opt, smart_sync_manifest_path, manifest): diff --git a/tests/test_platform_utils.py b/tests/test_platform_utils.py index e89965391..919351e7f 100644 --- a/tests/test_platform_utils.py +++ b/tests/test_platform_utils.py @@ -46,3 +46,71 @@ def test_remove_missing_ok(tmp_path: Path) -> None: path.touch() platform_utils.remove(path, missing_ok=False) assert not path.exists() + + +def test_removedirs_nonexistent(tmp_path: Path) -> None: + """removedirs should silently succeed on nonexistent paths.""" + platform_utils.removedirs(tmp_path / "does-not-exist") + + +def test_removedirs_symlink(tmp_path: Path) -> None: + """removedirs should remove a symlink.""" + link = tmp_path / "link" + link.symlink_to("target") + platform_utils.removedirs(link) + assert not link.exists() + + +def test_removedirs_empty_dir(tmp_path: Path) -> None: + """removedirs should remove an empty directory.""" + d = tmp_path / "empty" + d.mkdir() + platform_utils.removedirs(d) + assert not d.exists() + + +def test_removedirs_nested_empty_dirs(tmp_path: Path) -> None: + """removedirs should remove nested empty directories.""" + d = tmp_path / "a" / "b" / "c" + d.mkdir(parents=True) + platform_utils.removedirs(tmp_path / "a") + assert not (tmp_path / "a").exists() + + +def test_removedirs_symlinks_inside_dir(tmp_path: Path) -> None: + """removedirs should remove symlinks inside a directory.""" + d = tmp_path / "dir" + d.mkdir() + (d / "link1").symlink_to("target1") + (d / "link2").symlink_to("target2") + platform_utils.removedirs(d) + assert not d.exists() + + +def test_removedirs_preserves_user_files(tmp_path: Path) -> None: + """removedirs should not delete regular files or their parent dirs.""" + d = tmp_path / "dir" + d.mkdir() + (d / "link").symlink_to("target") + (d / "user-file.txt").write_text("keep me") + platform_utils.removedirs(d) + assert d.exists() + assert not (d / "link").exists() + assert (d / "user-file.txt").read_text() == "keep me" + + +def test_removedirs_deep_nested_with_symlinks(tmp_path: Path) -> None: + """removedirs should handle deep nesting: sub/dir/target.""" + d = tmp_path / "sub" / "dir" + d.mkdir(parents=True) + (d / "link").symlink_to("target") + platform_utils.removedirs(tmp_path / "sub") + assert not (tmp_path / "sub").exists() + + +def test_removedirs_regular_file_noop(tmp_path: Path) -> None: + """removedirs should not delete a regular file.""" + f = tmp_path / "file.txt" + f.write_text("data") + platform_utils.removedirs(f) + assert f.exists() diff --git a/tests/test_project.py b/tests/test_project.py index 1deec0726..728d5a54c 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -365,6 +365,47 @@ class LinkFile(CopyLinkTestCase): os.path.join("git-project", "foo.txt"), os.readlink(dest) ) + def test_replace_empty_dir_with_symlink(self): + """A linkfile should replace an empty real directory at the dest path. + + This is the common case: the old linkfiles inside the directory were + already cleaned up by UpdateCopyLinkfileList, leaving an empty parent + directory behind. + """ + src_dir = os.path.join(self.worktree, "dot-llms") + os.makedirs(src_dir) + + dest = os.path.join(self.topdir, "mydir") + os.makedirs(dest) + + lf = self.LinkFile("dot-llms", "mydir") + lf._Link() + self.assertTrue(os.path.islink(dest)) + self.assertEqual( + os.path.join("git-project", "dot-llms"), os.readlink(dest) + ) + + def test_nonempty_dir_not_clobbered(self): + """A linkfile must not delete a non-empty directory. + + If the user created files in a directory that a new linkfile wants + to replace, __linkIt should fail safely rather than deleting content. + """ + src_dir = os.path.join(self.worktree, "dot-llms") + os.makedirs(src_dir) + + dest = os.path.join(self.topdir, "mydir") + os.makedirs(dest) + user_file = os.path.join(dest, "user-notes.txt") + self.touch(user_file) + + lf = self.LinkFile("dot-llms", "mydir") + lf._Link() + # The directory should NOT be replaced — user content is preserved. + self.assertFalse(os.path.islink(dest)) + self.assertTrue(os.path.isdir(dest)) + self.assertTrue(os.path.exists(user_file)) + class MigrateWorkTreeTests(unittest.TestCase): """Check _MigrateOldWorkTreeGitDir handling.""" diff --git a/tests/test_subcmds_sync.py b/tests/test_subcmds_sync.py index 669027c98..ef162392b 100644 --- a/tests/test_subcmds_sync.py +++ b/tests/test_subcmds_sync.py @@ -13,6 +13,7 @@ # limitations under the License. """Unittests for the subcmds/sync.py module.""" +import json import os import shutil import tempfile @@ -1145,3 +1146,143 @@ class InterleavedSyncTest(unittest.TestCase): self.assertTrue(result.checkout_success) project.Sync_NetworkHalf.assert_called_once() project.Sync_LocalHalf.assert_not_called() + + +class UpdateCopyLinkfileListTest(unittest.TestCase): + """Tests for Sync.UpdateCopyLinkfileList.""" + + def setUp(self): + self.tempdirobj = tempfile.TemporaryDirectory(prefix="repo_tests") + self.topdir = self.tempdirobj.name + self.repodir = os.path.join(self.topdir, ".repo") + os.makedirs(self.repodir) + + manifest = mock.MagicMock() + manifest.subdir = self.repodir + self.manifest = manifest + + git_event_log = mock.MagicMock(ErrorEvent=mock.Mock(return_value=None)) + self.cmd = sync.Sync( + manifest=manifest, + outer_client=mock.MagicMock(), + git_event_log=git_event_log, + ) + self.cmd.client = mock.MagicMock(topdir=self.topdir) + + def tearDown(self): + self.tempdirobj.cleanup() + + def _write_copylinkfile_json(self, data: dict) -> None: + path = os.path.join(self.repodir, "copy-link-files.json") + with open(path, "w") as f: + json.dump(data, f) + + def _setup_projects(self, linkfile_dests: list) -> None: + project = mock.MagicMock() + project.linkfiles = [mock.MagicMock(dest=d) for d in linkfile_dests] + project.copyfiles = [] + mock.patch.object( + self.cmd, "GetProjects", return_value=[project] + ).start() + + def test_removes_old_symlink_dest(self): + """Old linkfile dests that are symlinks should be removed.""" + old_dest = os.path.join(self.topdir, "old-link") + os.symlink("target", old_dest) + + self._write_copylinkfile_json( + {"linkfile": ["old-link"], "copyfile": []} + ) + self._setup_projects([]) + + self.cmd.UpdateCopyLinkfileList(self.manifest) + self.assertFalse(os.path.lexists(old_dest)) + + def test_does_not_delete_through_new_symlink(self): + """Old dests that resolve through a new symlink must not delete files. + + When the manifest changes from individual linkfiles inside a directory + to a single directory linkfile, and _CopyAndLinkFiles has already + created the symlink (interleaved mode), cleanup must not follow the + symlink and delete real project files. + """ + project_dir = os.path.join(self.topdir, "vendor", "tools", "llms") + os.makedirs(os.path.join(project_dir, "dot-llms", "rules")) + os.makedirs(os.path.join(project_dir, "dot-llms", "skills")) + with open( + os.path.join(project_dir, "dot-llms", "rules", "basics.md"), "w" + ) as f: + f.write("# basics") + with open( + os.path.join(project_dir, "dot-llms", "skills", "repo.md"), "w" + ) as f: + f.write("# repo") + + # Simulate interleaved mode: .llms -> vendor/tools/llms/dot-llms. + llms_link = os.path.join(self.topdir, ".llms") + os.symlink("vendor/tools/llms/dot-llms", llms_link) + + self._write_copylinkfile_json( + {"linkfile": [".llms/rules", ".llms/skills"], "copyfile": []} + ) + self._setup_projects([".llms"]) + + self.cmd.UpdateCopyLinkfileList(self.manifest) + + # Real project files must still exist. + self.assertTrue( + os.path.exists( + os.path.join(project_dir, "dot-llms", "rules", "basics.md") + ) + ) + self.assertTrue( + os.path.exists( + os.path.join(project_dir, "dot-llms", "skills", "repo.md") + ), + ) + self.assertTrue(os.path.islink(llms_link)) + + def test_cleans_up_empty_parent_dirs(self): + """After removing old dests, empty parent directories are removed.""" + llms_dir = os.path.join(self.topdir, ".llms") + os.makedirs(llms_dir) + os.symlink( + "../vendor/tools/llms/rules", os.path.join(llms_dir, "rules") + ) + os.symlink( + "../vendor/tools/llms/skills", + os.path.join(llms_dir, "skills"), + ) + + self._write_copylinkfile_json( + {"linkfile": [".llms/rules", ".llms/skills"], "copyfile": []} + ) + self._setup_projects([".llms"]) + + self.cmd.UpdateCopyLinkfileList(self.manifest) + + self.assertFalse(os.path.lexists(os.path.join(llms_dir, "rules"))) + self.assertFalse(os.path.lexists(os.path.join(llms_dir, "skills"))) + # Parent directory should be removed since it's now empty. + self.assertFalse(os.path.exists(llms_dir)) + + def test_preserves_nonempty_parent_dirs(self): + """Non-empty parent directories are preserved after old dest removal.""" + llms_dir = os.path.join(self.topdir, ".llms") + os.makedirs(llms_dir) + os.symlink( + "../vendor/tools/llms/rules", os.path.join(llms_dir, "rules") + ) + with open(os.path.join(llms_dir, "my-notes.txt"), "w") as f: + f.write("user content") + + self._write_copylinkfile_json( + {"linkfile": [".llms/rules"], "copyfile": []} + ) + self._setup_projects([".llms"]) + + self.cmd.UpdateCopyLinkfileList(self.manifest) + + self.assertFalse(os.path.lexists(os.path.join(llms_dir, "rules"))) + self.assertTrue(os.path.exists(os.path.join(llms_dir, "my-notes.txt"))) + self.assertTrue(os.path.isdir(llms_dir))