make file removal a bit more robust

Some of the file removal calls are subject to race conditions (if
something else deletes the file), so extend our remove API to have
an option to ignore ENOENT errors.  Then update a bunch of random
call sites to use this new functionality.

Change-Id: I31a9090e135452033135337a202a4fc2dbf8b63c
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/319195
Reviewed-by: Sean McAllister <smcallis@google.com>
Tested-by: Mike Frysinger <vapier@google.com>

6 files changed, 80 insertions, 51 deletions

diff --git a/git_config.py b/git_config.py
index 778e81a4..bc70d160 100644
--- a/git_config.py
+++ b/git_config.py
@@ -352,8 +352,8 @@ class GitConfig(object):
       Trace(': parsing %s', self.file)
       with open(self._json) as fd:
         return json.load(fd)
-    except (IOError, ValueError):
-      platform_utils.remove(self._json)
+    except (IOError, ValueErrorl):
+      platform_utils.remove(self._json, missing_ok=True)
       return None
 
   def _SaveJson(self, cache):
@@ -361,8 +361,7 @@ class GitConfig(object):
       with open(self._json, 'w') as fd:
         json.dump(cache, fd, indent=2)
     except (IOError, TypeError):
-      if os.path.exists(self._json):
-        platform_utils.remove(self._json)
+      platform_utils.remove(self._json, missing_ok=True)
 
   def _ReadGit(self):
     """
diff --git a/manifest_xml.py b/manifest_xml.py
index 135c91fb..86f20202 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -270,8 +270,7 @@ class XmlManifest(object):
     self.Override(name)
 
     # Old versions of repo would generate symlinks we need to clean up.
-    if os.path.lexists(self.manifestFile):
-      platform_utils.remove(self.manifestFile)
+    platform_utils.remove(self.manifestFile, missing_ok=True)
     # This file is interpreted as if it existed inside the manifest repo.
     # That allows us to use <include> with the relative file name.
     with open(self.manifestFile, 'w') as fp:
diff --git a/platform_utils.py b/platform_utils.py
index 5741f4d3..0203249a 100644
--- a/platform_utils.py
+++ b/platform_utils.py
@@ -127,28 +127,27 @@ def rename(src, dst):
     shutil.move(src, dst)
 
 
-def remove(path):
+def remove(path, missing_ok=False):
   """Remove (delete) the file path. This is a replacement for os.remove that
   allows deleting read-only files on Windows, with support for long paths and
   for deleting directory symbolic links.
 
   Availability: Unix, Windows."""
-  if isWindows():
-    longpath = _makelongpath(path)
-    try:
-      os.remove(longpath)
-    except OSError as e:
-      if e.errno == errno.EACCES:
-        os.chmod(longpath, stat.S_IWRITE)
-        # Directory symbolic links must be deleted with 'rmdir'.
-        if islink(longpath) and isdir(longpath):
-          os.rmdir(longpath)
-        else:
-          os.remove(longpath)
+  longpath = _makelongpath(path) if isWindows() else path
+  try:
+    os.remove(longpath)
+  except OSError as e:
+    if e.errno == errno.EACCES:
+      os.chmod(longpath, stat.S_IWRITE)
+      # Directory symbolic links must be deleted with 'rmdir'.
+      if islink(longpath) and isdir(longpath):
+        os.rmdir(longpath)
       else:
-        raise
-  else:
-    os.remove(path)
+        os.remove(longpath)
+    elif missing_ok and e.errno == errno.ENOENT:
+      pass
+    else:
+      raise
 
 
 def walk(top, topdown=True, onerror=None, followlinks=False):
diff --git a/project.py b/project.py
index fe88a505..634d88c5 100644
--- a/project.py
+++ b/project.py
@@ -1182,10 +1182,8 @@ class Project(object):
       self._InitMRef()
     else:
       self._InitMirrorHead()
-      try:
-        platform_utils.remove(os.path.join(self.gitdir, 'FETCH_HEAD'))
-      except OSError:
-        pass
+      platform_utils.remove(os.path.join(self.gitdir, 'FETCH_HEAD'),
+                            missing_ok=True)
     return True
 
   def PostRepoUpgrade(self):
@@ -2307,15 +2305,12 @@ class Project(object):
     cmd.append('+refs/tags/*:refs/tags/*')
 
     ok = GitCommand(self, cmd, bare=True).Wait() == 0
-    if os.path.exists(bundle_dst):
-      platform_utils.remove(bundle_dst)
-    if os.path.exists(bundle_tmp):
-      platform_utils.remove(bundle_tmp)
+    platform_utils.remove(bundle_dst, missing_ok=True)
+    platform_utils.remove(bundle_tmp, missing_ok=True)
     return ok
 
   def _FetchBundle(self, srcUrl, tmpPath, dstPath, quiet, verbose):
-    if os.path.exists(dstPath):
-      platform_utils.remove(dstPath)
+    platform_utils.remove(dstPath, missing_ok=True)
 
     cmd = ['curl', '--fail', '--output', tmpPath, '--netrc', '--location']
     if quiet:
@@ -2739,10 +2734,7 @@ class Project(object):
         # If the source file doesn't exist, ensure the destination
         # file doesn't either.
         if name in symlink_files and not os.path.lexists(src):
-          try:
-            platform_utils.remove(dst)
-          except OSError:
-            pass
+          platform_utils.remove(dst, missing_ok=True)
 
       except OSError as e:
         if e.errno == errno.EPERM:
diff --git a/subcmds/sync.py b/subcmds/sync.py
index c99b06ca..3211cbb1 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -767,13 +767,9 @@ later is required to fix a server side protocol bug.
           set(new_copyfile_paths))
 
       for need_remove_file in need_remove_files:
-        try:
-          platform_utils.remove(need_remove_file)
-        except OSError as e:
-          if e.errno == errno.ENOENT:
-            # Try to remove the updated copyfile or linkfile.
-            # So, if the file is not exist, nothing need to do.
-            pass
+        # Try to remove the updated copyfile or linkfile.
+        # So, if the file is not exist, nothing need to do.
+        platform_utils.remove(need_remove_file, missing_ok=True)
 
     # Create copy-link-files.json, save dest path of "copyfile" and "linkfile".
     with open(copylinkfile_path, 'w', encoding='utf-8') as fp:
@@ -1171,10 +1167,7 @@ class _FetchTimes(object):
         with open(self._path) as f:
           self._times = json.load(f)
       except (IOError, ValueError):
-        try:
-          platform_utils.remove(self._path)
-        except OSError:
-          pass
+        platform_utils.remove(self._path, missing_ok=True)
         self._times = {}
 
   def Save(self):
@@ -1192,10 +1185,7 @@ class _FetchTimes(object):
       with open(self._path, 'w') as f:
         json.dump(self._times, f, indent=2)
     except (IOError, TypeError):
-      try:
-        platform_utils.remove(self._path)
-      except OSError:
-        pass
+      platform_utils.remove(self._path, missing_ok=True)
 
 # This is a replacement for xmlrpc.client.Transport using urllib2
 # and supporting persistent-http[s]. It cannot change hosts from
diff --git a/tests/test_platform_utils.py b/tests/test_platform_utils.py
new file mode 100644
index 00000000..55b7805c
--- /dev/null
+++ b/tests/test_platform_utils.py
@@ -0,0 +1,50 @@
+# Copyright 2021 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the platform_utils.py module."""
+
+import os
+import tempfile
+import unittest
+
+import platform_utils
+
+
+class RemoveTests(unittest.TestCase):
+  """Check remove() helper."""
+
+  def testMissingOk(self):
+    """Check missing_ok handling."""
+    with tempfile.TemporaryDirectory() as tmpdir:
+      path = os.path.join(tmpdir, 'test')
+
+      # Should not fail.
+      platform_utils.remove(path, missing_ok=True)
+
+      # Should fail.
+      self.assertRaises(OSError, platform_utils.remove, path)
+      self.assertRaises(OSError, platform_utils.remove, path, missing_ok=False)
+
+      # Should not fail if it exists.
+      open(path, 'w').close()
+      platform_utils.remove(path, missing_ok=True)
+      self.assertFalse(os.path.exists(path))
+
+      open(path, 'w').close()
+      platform_utils.remove(path)
+      self.assertFalse(os.path.exists(path))
+
+      open(path, 'w').close()
+      platform_utils.remove(path, missing_ok=False)
+      self.assertFalse(os.path.exists(path))