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>
diff --git a/git_config.py b/git_config.py
index 778e81a..bc70d16 100644
--- a/git_config.py
+++ b/git_config.py
@@ -352,8 +352,8 @@
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 @@
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 135c91f..86f2020 100644
--- a/manifest_xml.py
+++ b/manifest_xml.py
@@ -270,8 +270,7 @@
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 5741f4d..0203249 100644
--- a/platform_utils.py
+++ b/platform_utils.py
@@ -127,28 +127,27 @@
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 fe88a50..634d88c 100644
--- a/project.py
+++ b/project.py
@@ -1182,10 +1182,8 @@
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 @@
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 @@
# 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 c99b06c..3211cbb 100644
--- a/subcmds/sync.py
+++ b/subcmds/sync.py
@@ -767,13 +767,9 @@
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 @@
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 @@
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 0000000..55b7805
--- /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))
