Fix directory deletion failing on hidden files, breaking the pack swap
The new extract/verify/swap flow surfaced a long-standing bug in Utils.clean_dir(). On macOS, browsing a folder in the Finder drops a hidden .DS_Store file in it, and clean_dir() had two flaws around that: - DirAccess listings skip hidden files by default, so .DS_Store was never listed nor removed and the directory was never actually empty. - The return value of dir.remove(subfolder) was ignored, so the failed removal of a subdirectory (non-empty because of its own hidden file) was silently swallowed and clean_dir() still reported success. As a result, delete_directory_recursive() emptied the old language pack but could not delete its directory, the rename of the new pack onto that still-existing directory failed, and the downloader showed the download-error popup. Since the version on disk was never updated, the app re-downloaded and failed again in a loop on every launch. The old pre-swap flow had the same deletion failure but masked it: it extracted directly into the existing directory and ignored rename errors. - clean_dir() now lists hidden files (include_hidden) and checks every removal, trying to remove everything (best effort, so callers like clear_all_local_data still wipe as much as possible) and reporting the first error encountered instead of a false OK. - The downloader verifies that the previous pack directory is really gone before renaming the new one into place; if not, it aborts the swap, keeps the temporary directory and shows the error instead of attempting a rename that cannot succeed. - Add regression tests covering hidden-file deletion, full content cleanup and the missing-directory error path. Verified with the GUT test suite (65/65 passing).
This commit is contained in:
@@ -201,6 +201,11 @@ func _copy_data(this: PackageDownloader) -> void:
|
||||
if DirAccess.dir_exists_absolute(current_language_path):
|
||||
Log.trace("PackageDownloader: Removing previous language directory")
|
||||
Utils.delete_directory_recursive(ProjectSettings.globalize_path(current_language_path))
|
||||
if DirAccess.dir_exists_absolute(current_language_path):
|
||||
# Keep the temporary directory so the new pack is not lost
|
||||
Log.error("PackageDownloader: Cannot remove the previous language directory, aborting swap")
|
||||
this.call_thread_safe("_show_error", 2) # Error downloading
|
||||
return
|
||||
|
||||
var error: Error = DirAccess.rename_absolute(new_pack_path, current_language_path)
|
||||
if error != OK:
|
||||
|
||||
@@ -94,14 +94,22 @@ func clean_dir(path: String) -> Error:
|
||||
return error
|
||||
if dir == null:
|
||||
return ERR_FILE_BAD_PATH
|
||||
# Hidden files (e.g. .DS_Store created by the macOS Finder) must be removed
|
||||
# too, otherwise the directory is never empty and cannot be deleted
|
||||
dir.include_hidden = true
|
||||
# Best effort: try to remove everything, report the first error encountered
|
||||
var first_error: Error = OK
|
||||
for file: String in dir.get_files():
|
||||
dir.remove(file)
|
||||
error = dir.remove(file)
|
||||
if error != OK and first_error == OK:
|
||||
first_error = error
|
||||
for subfolder: String in dir.get_directories():
|
||||
error = clean_dir(path.path_join(subfolder))
|
||||
if error != OK:
|
||||
return error
|
||||
dir.remove(subfolder)
|
||||
return OK
|
||||
if error == OK:
|
||||
error = dir.remove(subfolder)
|
||||
if error != OK and first_error == OK:
|
||||
first_error = error
|
||||
return first_error
|
||||
|
||||
|
||||
func delete_directory_recursive(path: String) -> void:
|
||||
|
||||
@@ -0,0 +1,63 @@
|
||||
extends GutTest
|
||||
|
||||
const TEST_ROOT: String = "user://test_clean_dir"
|
||||
|
||||
|
||||
func after_each() -> void:
|
||||
_force_delete_directory(TEST_ROOT)
|
||||
|
||||
|
||||
func test_delete_directory_recursive_removes_hidden_files() -> void:
|
||||
# Regression test: the macOS Finder drops .DS_Store files in browsed
|
||||
# folders; deletion used to fail silently on them, breaking the
|
||||
# language pack swap in PackageDownloader
|
||||
_create_file(TEST_ROOT.path_join("visible.txt"))
|
||||
_create_file(TEST_ROOT.path_join(".hidden"))
|
||||
_create_file(TEST_ROOT.path_join("sub").path_join(".hidden_nested"))
|
||||
|
||||
Utils.delete_directory_recursive(TEST_ROOT)
|
||||
|
||||
assert_false(DirAccess.dir_exists_absolute(TEST_ROOT), "directory with hidden files should be fully deleted")
|
||||
|
||||
|
||||
func test_clean_dir_keeps_root_and_removes_all_content() -> void:
|
||||
_create_file(TEST_ROOT.path_join("visible.txt"))
|
||||
_create_file(TEST_ROOT.path_join("sub").path_join("nested.txt"))
|
||||
|
||||
var error: Error = Utils.clean_dir(TEST_ROOT)
|
||||
|
||||
assert_eq(error, OK)
|
||||
assert_true(DirAccess.dir_exists_absolute(TEST_ROOT), "root directory should be kept")
|
||||
var dir: DirAccess = DirAccess.open(TEST_ROOT)
|
||||
dir.include_hidden = true
|
||||
assert_eq(dir.get_files().size(), 0, "all files should be removed")
|
||||
assert_eq(dir.get_directories().size(), 0, "all subdirectories should be removed")
|
||||
|
||||
|
||||
func test_clean_dir_returns_error_on_missing_directory() -> void:
|
||||
var error: Error = Utils.clean_dir("user://test_clean_dir_does_not_exist")
|
||||
|
||||
assert_ne(error, OK)
|
||||
|
||||
|
||||
func _create_file(path: String) -> void:
|
||||
DirAccess.make_dir_recursive_absolute(path.get_base_dir())
|
||||
var file: FileAccess = FileAccess.open(path, FileAccess.WRITE)
|
||||
assert_not_null(file, "test setup should be able to create %s" % path)
|
||||
file.store_string("test")
|
||||
file.close()
|
||||
|
||||
|
||||
# Cleanup helper independent from the code under test
|
||||
func _force_delete_directory(path: String) -> void:
|
||||
if not DirAccess.dir_exists_absolute(path):
|
||||
return
|
||||
var dir: DirAccess = DirAccess.open(path)
|
||||
if dir == null:
|
||||
return
|
||||
dir.include_hidden = true
|
||||
for file: String in dir.get_files():
|
||||
dir.remove(file)
|
||||
for subfolder: String in dir.get_directories():
|
||||
_force_delete_directory(path.path_join(subfolder))
|
||||
DirAccess.remove_absolute(path)
|
||||
Reference in New Issue
Block a user