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 <ccross@android.com>
Tested-by: Mike Frysinger <vapier@google.com>

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',
     }
 
-    # Now walk the paths and sync the .git/ to .repo/projects/.
+    # 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 platform_utils.islink(dotgit_path):
-          platform_utils.remove(dotgit_path)
-        else:
-          raise GitError(f'{dotgit_path}: should be a symlink')
+        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 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')
+        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)
+
+      # 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)
+        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())