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