Re: git archive --format zip utf-8 issues

René Scharfe [off-list ref] writes:

But now for the patch, which is a bit confusing as well.  I'm curious to
hear about results for more platforms, extractors and character classes.
Based on that we can see if we need to generate the extra fields instead
of relying on the new flag.

Thanks for keeping the ball rolling.

Do we want to treat \033 as "ascii" in this codepath?  The function
primarily is used by the log formatter to see if we need 8-bit CTE
when writing out in the e-mail format.

Am 04.09.2012 23:03, schrieb Junio C Hamano:

René Scharfe [off-list ref] writes:

quoted

+	if (has_non_ascii(path)) {

Do we want to treat \033 as "ascii" in this codepath?  The function
primarily is used by the log formatter to see if we need 8-bit CTE
when writing out in the e-mail format.

Argh, yes, I'd think so. The function name mislead me.  This won't 
matter for compatibility testing, but should be corrected before inclusion.

Just checked: unzip strips the ESC character when extracting (whether 
the UTF-8 flag is set or not) and 7-Zip replaces it with an underscore. 
The built-in ZIP extractor of Windows 7 skips such a file; it doesn't 
even show up in archive directory listings.

René

Hello again,

so two weeks have passed, and I've moved at a glacial pace towards a 
method how to measure compatibility of our generated ZIP files.  Sorry, 
I just keep getting distracted.

Anyway, the idea is to have a bunch of files with names using different 
scripts, zip them with several packers (including git archive), unzip 
them and compare the result with the original files.

As test corpus I used files named like the pangrams on this UTF-8 
sampler page, the exact commands are attached:

    http://www.columbia.edu/~fdc/utf8/index.html#quickbrownfox

The numbers below are how many lines the output of diff -ru contains for 
this pair of packer and unpacker.  There are 37 files, so the worst 
result is 74 lines of difference ("Only in [...]" for both sides), while 
0 indicates a perfect score.

Hmm, come to think of it, an empty directory would show up as 37, so 
this metric is not ideal.  A better one would be to simply give one 
point for each correctly unpacked file.

                                          Windows    Info-ZIP unzip
                             7-Zip PeaZip builtin Linux msysgit Windows
7-Zip 9.20                      0      0      46    26      43      43
PeaZip 4.7.1 win64              0      0      46    26      42      42
Info-ZIP zip 3.0 Linux          0      0      72     0      43      43
Info-ZIP zip 3.0 Windows       45     45     n/a     0      43      43
git-master                     72     72      72    60      72      72
git-master-patch1               0      0      72    60      72      72
git-master-patch2               0      0      72     0      72      72
git-v1.7.11.msysgit.1          72     72      72    60      72      72
git-v1.7.11.msysgit.1-patch1    0      0      72    60      72      72
git-v1.7.11.msysgit.1-patch2    0      0      72     0      72      72

Info-ZIP's programs don't work too well on Windows.  The built-in 
unzipper of Windows 7 even refuses to open the file created by the 
Windows version of zip.  Speaking of which, this is the worst of the 
unpackers.

With the two patches applied, we can say "use 7-Zip or PeaZip on Windows 
and unzip on Linux" and filenames with all tested characters will be 
preserved.  I was surprised to see this working fine with msysgit like 
that, even though no reencoding is introduced by the patches.

I wonder what 7-Zip and PeaZip do that gives them a slightly nicer score 
with the Windows-internal unzipper.  Umlauts, Nordic characters and 
accents are preserved by that combination.  It seems that unzip on Linux 
fails to unpack exactly these names, so perhaps they employ a dirty 
trick like using the local encoding in the ZIP file, which makes it 
unportable.

I'll reply with the two patches, which contain basically the same code 
as the previous patch, only split up.  The second one declares that 
filenames with UTF-8 encoding came from Unix (instead of FAT), which 
makes unzip happy.  This, however, implies that we contain Unix 
permissions for these entries, which is a bit ugly.

René

Set general purpose flag 11 if we encounter a path that contains
non-ASCII characters.  We assume that all paths are given as UTF-8; no
conversion is done.

Signed-off-by: Rene Scharfe <redacted>
---
Changes from previous version: Stop using has_non_ascii(), which does
slightly too much for our purposes, and split off creator version
change into a separate patch.

 archive-zip.c | 22 +++++++++++++++++++++-
 1 file changed, 21 insertions(+), 1 deletion(-)

diff --git a/archive-zip.c b/archive-zip.c
index f5af81f..0f763e8 100644
--- a/archive-zip.c
+++ b/archive-zip.c

@@ -4,6 +4,7 @@
 #include "cache.h"
 #include "archive.h"
 #include "streaming.h"
+#include "utf8.h"
 
 static int zip_date;
 static int zip_time;

@@ -16,7 +17,8 @@ static unsigned int zip_dir_offset;
 static unsigned int zip_dir_entries;
 
 #define ZIP_DIRECTORY_MIN_SIZE	(1024 * 1024)
-#define ZIP_STREAM (8)
+#define ZIP_STREAM	(1 <<  3)
+#define ZIP_UTF8	(1 << 11)
 
 struct zip_local_header {
 	unsigned char magic[4];

@@ -164,6 +166,17 @@ static void set_zip_header_data_desc(struct zip_local_header *header,
 	copy_le32(header->size, size);
 }
 
+static int has_only_ascii(const char *s)
+{
+	for (;;) {
+		int c = *s++;
+		if (c == '\0')
+			return 1;
+		if (!isascii(c))
+			return 0;
+	}
+}
+
 #define STREAM_BUFFER_SIZE (1024 * 16)
 
 static int write_zip_entry(struct archiver_args *args,

@@ -187,6 +200,13 @@ static int write_zip_entry(struct archiver_args *args,
 
 	crc = crc32(0, NULL, 0);
 
+	if (!has_only_ascii(path)) {
+		if (is_utf8(path))
+			flags |= ZIP_UTF8;
+		else
+			warning("Path is not valid UTF-8: %s", path);
+	}
+
 	if (pathlen > 0xffff) {
 		return error("path too long (%d chars, SHA1: %s): %s",
 				(int)pathlen, sha1_to_hex(sha1), path);

-- 
1.7.12

The UTF-8 flag seems to be ignored by unzip unless we also mark the
archive entry as coming from a Unix system.  This is done by setting the
field creator_version ("version made by" in the standard[1]) to 0x03NN.

The NN part represents the version of the standard supported by us, and
this patch sets it to 3f (for version 6.3) for Unix paths.  We keep
creator_version set to 0 (FAT filesystem, standard version 0) in the
non-special cases, as before.

But when we declare a file to have a Unix path, then we have to set the
file mode as well, or unzip will extract the files with the permission
set 0000, i.e. inaccessible by all.

[1] http://www.pkware.com/documents/casestudies/APPNOTE.TXT
---
No sign-off for this, yet.  Perhaps there is a better way to convince
unzip to respect the flag?  And if not, do we need to offer umask
settings for ZIP as well as we have for tar?  And perhaps declare all
files as being from a Unix filesystem, for consistency?

 archive-zip.c | 15 ++++++++++-----
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 0f763e8..e9b3dc9 100644
--- a/archive-zip.c
+++ b/archive-zip.c

@@ -186,7 +186,8 @@ static int write_zip_entry(struct archiver_args *args,
 {
 	struct zip_local_header header;
 	struct zip_dir_header dirent;
-	unsigned long attr2;
+	unsigned int creator_version = 0;
+	unsigned long attr2 = 0;
 	unsigned long compressed_size;
 	unsigned long crc;
 	unsigned long direntsize;

@@ -224,10 +225,15 @@ static int write_zip_entry(struct archiver_args *args,
 		enum object_type type = sha1_object_info(sha1, &size);
 
 		method = 0;
-		attr2 = S_ISLNK(mode) ? ((mode | 0777) << 16) :
-			(mode & 0111) ? ((mode) << 16) : 0;
 		if (S_ISREG(mode) && args->compression_level != 0 && size > 0)
 			method = 8;
+		if (S_ISLNK(mode) || (mode & 0111) || (flags & ZIP_UTF8)) {
+			creator_version = 0x033f;
+			attr2 = mode;
+			if (S_ISLNK(mode))
+				attr2 |= 0777;
+			attr2 <<= 16;
+		}
 		compressed_size = size;
 
 		if (S_ISREG(mode) && type == OBJ_BLOB && !args->convert &&

@@ -274,8 +280,7 @@ static int write_zip_entry(struct archiver_args *args,
 	}
 
 	copy_le32(dirent.magic, 0x02014b50);
-	copy_le16(dirent.creator_version,
-		S_ISLNK(mode) || (S_ISREG(mode) && (mode & 0111)) ? 0x0317 : 0);
+	copy_le16(dirent.creator_version, creator_version);
 	copy_le16(dirent.version, 10);
 	copy_le16(dirent.flags, flags);
 	copy_le16(dirent.compression_method, method);

-- 
1.7.12

Am 18.09.2012 21:40, schrieb René Scharfe:

                                           Windows    Info-ZIP unzip
                              7-Zip PeaZip builtin Linux msysgit Windows
git-master-patch1               0      0      72    60      72      72
git-master-patch2               0      0      72     0      72      72

Oh, and when I wrote Windows, I meant a German Windows 7 Home Premium 
x64, and Linux is Ubuntu 12.04 x86.

I've also attached generated ZIP files for the two archivers above, for 
easier testing.  Let's see if they make it through.

René

File modification times in ZIP files are encoded in DOS format: local
time with a granularity of two seconds.  Add an extra field to all
archive entries to also record the mtime in Unix' fashion, as UTC with
a granularity of one second.

This has the desirable side-effect of convincing Info-ZIP unzip 6.00
to respect general purpose flag 11, which is used to indicate that a
file name is encoded in UTF-8.  Any extra field would do, actually,
but the extended timestamp is a reasonably small one (22 bytes per
entry).  Archives created by Info-ZIP zip 3.0 contain it, too (but
with ctime and atime as well).

Signed-off-by: Rene Scharfe <redacted>
---
 archive-zip.c | 27 ++++++++++++++++++++++++---
 1 file changed, 24 insertions(+), 3 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 0f763e8..55f66b4 100644
--- a/archive-zip.c
+++ b/archive-zip.c

@@ -76,6 +76,14 @@ struct zip_dir_trailer {
 	unsigned char _end[1];
 };
 
+struct zip_extra_mtime {
+	unsigned char magic[2];
+	unsigned char extra_size[2];
+	unsigned char flags[1];
+	unsigned char mtime[4];
+	unsigned char _end[1];
+};
+
 /*
  * On ARM, padding is added at the end of the struct, so a simple
  * sizeof(struct ...) reports two bytes more than the payload size

@@ -85,6 +93,9 @@ struct zip_dir_trailer {
 #define ZIP_DATA_DESC_SIZE	offsetof(struct zip_data_desc, _end)
 #define ZIP_DIR_HEADER_SIZE	offsetof(struct zip_dir_header, _end)
 #define ZIP_DIR_TRAILER_SIZE	offsetof(struct zip_dir_trailer, _end)
+#define ZIP_EXTRA_MTIME_SIZE	offsetof(struct zip_extra_mtime, _end)
+#define ZIP_EXTRA_MTIME_PAYLOAD_SIZE \
+	(ZIP_EXTRA_MTIME_SIZE - offsetof(struct zip_extra_mtime, flags))
 
 static void copy_le16(unsigned char *dest, unsigned int n)
 {

@@ -186,6 +197,7 @@ static int write_zip_entry(struct archiver_args *args,
 {
 	struct zip_local_header header;
 	struct zip_dir_header dirent;
+	struct zip_extra_mtime extra;
 	unsigned long attr2;
 	unsigned long compressed_size;
 	unsigned long crc;

@@ -266,8 +278,13 @@ static int write_zip_entry(struct archiver_args *args,
 		}
 	}
 
+	copy_le16(extra.magic, 0x5455);
+	copy_le16(extra.extra_size, ZIP_EXTRA_MTIME_PAYLOAD_SIZE);
+	extra.flags[0] = 1;	/* just mtime */
+	copy_le32(extra.mtime, args->time);
+
 	/* make sure we have enough free space in the dictionary */
-	direntsize = ZIP_DIR_HEADER_SIZE + pathlen;
+	direntsize = ZIP_DIR_HEADER_SIZE + pathlen + ZIP_EXTRA_MTIME_SIZE;
 	while (zip_dir_size < zip_dir_offset + direntsize) {
 		zip_dir_size += ZIP_DIRECTORY_MIN_SIZE;
 		zip_dir = xrealloc(zip_dir, zip_dir_size);

@@ -283,7 +300,7 @@ static int write_zip_entry(struct archiver_args *args,
 	copy_le16(dirent.mdate, zip_date);
 	set_zip_dir_data_desc(&dirent, size, compressed_size, crc);
 	copy_le16(dirent.filename_length, pathlen);
-	copy_le16(dirent.extra_length, 0);
+	copy_le16(dirent.extra_length, ZIP_EXTRA_MTIME_SIZE);
 	copy_le16(dirent.comment_length, 0);
 	copy_le16(dirent.disk, 0);
 	copy_le16(dirent.attr1, 0);

@@ -301,11 +318,13 @@ static int write_zip_entry(struct archiver_args *args,
 	else
 		set_zip_header_data_desc(&header, size, compressed_size, crc);
 	copy_le16(header.filename_length, pathlen);
-	copy_le16(header.extra_length, 0);
+	copy_le16(header.extra_length, ZIP_EXTRA_MTIME_SIZE);
 	write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
 	zip_offset += ZIP_LOCAL_HEADER_SIZE;
 	write_or_die(1, path, pathlen);
 	zip_offset += pathlen;
+	write_or_die(1, &extra, ZIP_EXTRA_MTIME_SIZE);
+	zip_offset += ZIP_EXTRA_MTIME_SIZE;
 	if (stream && method == 0) {
 		unsigned char buf[STREAM_BUFFER_SIZE];
 		ssize_t readlen;

@@ -402,6 +421,8 @@ static int write_zip_entry(struct archiver_args *args,
 	zip_dir_offset += ZIP_DIR_HEADER_SIZE;
 	memcpy(zip_dir + zip_dir_offset, path, pathlen);
 	zip_dir_offset += pathlen;
+	memcpy(zip_dir + zip_dir_offset, &extra, ZIP_EXTRA_MTIME_SIZE);
+	zip_dir_offset += ZIP_EXTRA_MTIME_SIZE;
 	zip_dir_entries++;
 
 	return 0;

-- 
1.7.12