Fix ZIPPacker creating empty directory entries

[TheOathMan]

What problem(s) does this PR solve?

• Closes ZIPPacker creates empty file entries for parent directories when adding nested files #120068

Additional information

When calling start_file with a path that includes a subdirectory (e.g. "folder/file.txt"), the packer auto creates the parent directory entry. Due to a missing trailing /, the entry is stored as "folder" instead of "folder/", which ZIP tools interpret as a zero-byte file rather than a directory.

This results in the archive containing both a zero-byte file named "folder" and the intended file at "folder/file.txt". This is a duplicate name conflict that causes Windows native ZIP extraction to fail or skip entries, and breaks repacked binary formats like .aar files which are sensitive to duplicate or malformed entries.

Fix: Append "/" to the path passed to add_directory in start_file.

---

See issue #120068 for full details, test case, and root cause analysis.

Note: 4.7 specific regression. Directory and permission support was added in #115946

[bruvzg]

Storing records for directories is not required, but it is a normal behavior, so this is not a bug and no reason for this change.

Seems like Windows doesn't store directories, but most Unix tools do (probably because directories can have specific permissions set).

[TheOathMan]

Storing records for directories is not required, but it is a normal behavior, so this is not a bug and no reason for this change.

Seems like Windows doesn't store directories, but most Unix tools do (probably because directories can have specific permissions set).

It shouldn't be the default behavior, or at the very least, there should be an option to disable it. Leaving it as is exposes two major failure areas: first, Windows users cannot unzip folders compressed by Godot, and second, it is impossible to repack a file to its original state.

[TheOathMan]

repack a file to its original state is essential not only to my plugin but I imagine to so many Godot plugins out there, because they either rely on a user unzipping a file on windows or for my use case, repacking a file to its original state/format.

[bruvzg]

Windows users cannot unzip folders compressed by Godot, and second, it is impossible to repack a file to its original state.

What specific tool can't unzip it? Window should be be able to handle Unix created ZIP without issues.

If there's an issue with some tool it might be caused by names of stored directories (Unix zip seems to add / to the end of directory name, and Godot doesn't).

[TheOathMan]

What specific tool can't unzip it? Window should be be able to handle Unix created ZIP without issues.

right click -> Extract all...

[bruvzg]

I'll check it tomorrow.

[bruvzg]

If there's an issue with some tool it might be caused by names of stored directories (Unix zip seems to add / to the end of directory name, and Godot doesn't).

I can reproduce the issue on Windows, and it is fixed by adding / to the path.

diff --git a/modules/zip/zip_packer.cpp b/modules/zip/zip_packer.cpp
index 8bc13ba7c9..35da37782d 100644
--- a/modules/zip/zip_packer.cpp
+++ b/modules/zip/zip_packer.cpp
@@ -70,7 +70,7 @@ int ZIPPacker::get_compression_level() const {
 Error ZIPPacker::start_file(const String &p_path, BitField<FileAccess::UnixPermissionFlags> p_permissions, uint64_t p_modified_time) {
 	ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPPacker must be opened before use.");
 
-	if (!p_path.get_base_dir().is_empty() && !directories.has(p_path.get_base_dir())) {
+	if (!p_path.get_base_dir().is_empty() && !directories.has(p_path.get_base_dir() + "/")) {
 		add_directory(p_path.get_base_dir(), 0755, p_modified_time);
 	}
 
@@ -136,8 +136,10 @@ Error ZIPPacker::close_file() {
 }
 
 Error ZIPPacker::add_directory(const String &p_path, BitField<FileAccess::UnixPermissionFlags> p_permissions, uint64_t p_modified_time) {
+	String path = p_path.ends_with("/") ? p_path : p_path + "/";
+
 	ERR_FAIL_COND_V_MSG(fa.is_null(), FAILED, "ZIPPacker must be opened before use.");
-	ERR_FAIL_COND_V_MSG(directories.has(p_path), ERR_CANT_CREATE, vformat("Directory '%s' already exists.", p_path));
+	ERR_FAIL_COND_V_MSG(directories.has(path), ERR_CANT_CREATE, vformat("Directory '%s' already exists.", path));
 
 	uint64_t time = p_modified_time;
 	if (time == 0) {
@@ -168,7 +170,7 @@ Error ZIPPacker::add_directory(const String &p_path, BitField<FileAccess::UnixPe
 	zipfi.internal_fa = 0;
 
 	int err = zipOpenNewFileInZip4(zf,
-			p_path.utf8().get_data(),
+			path.utf8().get_data(),
 			&zipfi,
 			nullptr,
 			0,
@@ -190,7 +192,7 @@ Error ZIPPacker::add_directory(const String &p_path, BitField<FileAccess::UnixPe
 		return FAILED;
 	}
 
-	directories.insert(p_path);
+	directories.insert(path);
 	return OK;
 }

[TheOathMan]

String path = p_path.ends_with("/") ? p_path : p_path + "/";

tried your fixed, also works. Thanks

[bruvzg]

Please squash the commits, see The interactive rebase.

[TheOathMan]

Please squash the commits, see The interactive rebase.

done.

[Repiteo]

Thanks! Congratulations on your first merged contribution! 🎉