From 89ed8acdbe468fd76d531cd8b7b2ace5b414f0bd Mon Sep 17 00:00:00 2001 From: Mike Frysinger Date: Thu, 6 Jan 2022 05:42:24 -0500 Subject: [PATCH] project: abort a bit earlier before migrating .git/ Verify all the .git/ paths will be handled by the migration logic before starting the migration. This way we still abort & log an error, but the user gets to see it before we put the tree into a state that they have to manually recover. Also add a few more known-safe-to-clobber paths. Bug: https://crbug.com/gerrit/15273 Change-Id: If49d69b341bc960ddcafa30da333fb5ec7145b51 Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/327557 Reviewed-by: Colin Cross Tested-by: Mike Frysinger --- project.py | 38 +++++++++++++++++++++++++++----------- tests/test_project.py | 28 ++++++++++++++++++++++++++-- 2 files changed, 53 insertions(+), 13 deletions(-) diff --git a/project.py b/project.py index de593c83..48287858 100644 --- a/project.py +++ b/project.py @@ -2818,24 +2818,40 @@ class Project(object): } # Paths that we know will be in both, but are safe to clobber in .repo/projects/. SAFE_TO_CLOBBER = { - 'COMMIT_EDITMSG', 'FETCH_HEAD', 'HEAD', 'index', 'ORIG_HEAD', + 'COMMIT_EDITMSG', 'FETCH_HEAD', 'HEAD', 'gitk.cache', 'index', 'ORIG_HEAD', } + # First see if we'd succeed before starting the migration. + unknown_paths = [] + for name in platform_utils.listdir(dotgit): + # Ignore all temporary/backup names. These are common with vim & emacs. + if name.endswith('~') or (name[0] == '#' and name[-1] == '#'): + continue + + dotgit_path = os.path.join(dotgit, name) + if name in KNOWN_LINKS: + if not platform_utils.islink(dotgit_path): + unknown_paths.append(f'{dotgit_path}: should be a symlink') + else: + gitdir_path = os.path.join(gitdir, name) + if name not in SAFE_TO_CLOBBER and os.path.exists(gitdir_path): + unknown_paths.append(f'{dotgit_path}: unknown file; please file a bug') + if unknown_paths: + raise GitError('Aborting migration: ' + '\n'.join(unknown_paths)) + # Now walk the paths and sync the .git/ to .repo/projects/. for name in platform_utils.listdir(dotgit): dotgit_path = os.path.join(dotgit, name) - if name in KNOWN_LINKS: - if platform_utils.islink(dotgit_path): - platform_utils.remove(dotgit_path) - else: - raise GitError(f'{dotgit_path}: should be a symlink') + + # Ignore all temporary/backup names. These are common with vim & emacs. + if name.endswith('~') or (name[0] == '#' and name[-1] == '#'): + platform_utils.remove(dotgit_path) + elif name in KNOWN_LINKS: + platform_utils.remove(dotgit_path) else: gitdir_path = os.path.join(gitdir, name) - if name in SAFE_TO_CLOBBER or not os.path.exists(gitdir_path): - platform_utils.remove(gitdir_path, missing_ok=True) - platform_utils.rename(dotgit_path, gitdir_path) - else: - raise GitError(f'{dotgit_path}: unknown file; please file a bug') + platform_utils.remove(gitdir_path, missing_ok=True) + platform_utils.rename(dotgit_path, gitdir_path) # Now that the dir should be empty, clear it out, and symlink it over. platform_utils.rmdir(dotgit) diff --git a/tests/test_project.py b/tests/test_project.py index d578fe84..4f449227 100644 --- a/tests/test_project.py +++ b/tests/test_project.py @@ -347,6 +347,10 @@ class MigrateWorkTreeTests(unittest.TestCase): } _FILES = { 'COMMIT_EDITMSG', 'FETCH_HEAD', 'HEAD', 'index', 'ORIG_HEAD', + 'unknown-file-should-be-migrated', + } + _CLEAN_FILES = { + 'a-vim-temp-file~', '#an-emacs-temp-file#', } @classmethod @@ -365,10 +369,9 @@ class MigrateWorkTreeTests(unittest.TestCase): dotgit.mkdir(parents=True) for name in cls._SYMLINKS: (dotgit / name).symlink_to(f'../../../.repo/projects/src/test.git/{name}') - for name in cls._FILES: + for name in cls._FILES | cls._CLEAN_FILES: (dotgit / name).write_text(name) - subprocess.run(['tree', '-a', str(dotgit)]) yield tempdir def test_standard(self): @@ -385,3 +388,24 @@ class MigrateWorkTreeTests(unittest.TestCase): gitdir = tempdir / '.repo/projects/src/test.git' for name in self._FILES: self.assertEqual(name, (gitdir / name).read_text()) + # Make sure files were removed. + for name in self._CLEAN_FILES: + self.assertFalse((gitdir / name).exists()) + + def test_unknown(self): + """A checkout with unknown files should abort.""" + with self._simple_layout() as tempdir: + dotgit = tempdir / 'src/test/.git' + (tempdir / '.repo/projects/src/test.git/random-file').write_text('one') + (dotgit / 'random-file').write_text('two') + with self.assertRaises(error.GitError): + project.Project._MigrateOldWorkTreeGitDir(str(dotgit)) + + # Make sure no content was actually changed. + self.assertTrue(dotgit.is_dir()) + for name in self._FILES: + self.assertTrue((dotgit / name).is_file()) + for name in self._CLEAN_FILES: + self.assertTrue((dotgit / name).is_file()) + for name in self._SYMLINKS: + self.assertTrue((dotgit / name).is_symlink())