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>

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):
     """

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:

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):

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:

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

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))