[LHF] making t5000 "tar xf" tests more lenient

[LHF] making t5000 "tar xf" tests more lenient

[Junio C Hamano]

I've been running testsuite on a few platforms that are unfamiliar
to me, and was bitten by BSD implementation of tar that do not grok
the extended pax headers.  I've already fixed one in t9502 [*1*]
where we produce a tarball with "git archive" and then try to
validate it with the platform implementation of "tar" so that the
test won't barf when it sees the extended pax header.

t5000 is littered with similar tests that break unnecessarily with
BSD implementations.  I think what it wants to test are:

 - "archive" produces tarball, and "tar" can extract from it.  If
   your "tar" does not understand pax header, you may get it as an
   extra file that shouldn't be there, but that should not cause the
   test to fail---in real life, people without a pax-aware "tar"
   will just ignore and remove the header and can go on.

 - "get-tar-commmit-id" can inspect a tarball produced by "archive"
   to recover the object name of the commit even on a platform
   without a pax-aware "tar".

Perhaps t5000 can be restructured like so:

 - create a tarball with the commit-id and test with
   "get-tar-commit-id" to validate it; also create a tarball out of
   a tree to make sure it does not have commit-id and check with
   "get-tar-commit-id".  Do this on any and all platform, even on
   the ones without a pax-aware "tar".

 - check platform implementation of "tar" early to see if extracting
   a simple output from "git archive" results in an extra pax header
   file.  If so, remember this fact and produce any and all tarballs
   used in the remainder of the test by forcing ^{tree}.

so that people on platforms without pax-aware "tar" do not have to
install GNU tar only to pass this test.

It would be a good exercise during the holiday week for somebody on
BSD (it seems NetBSD is more troublesome than OpenBSD) to come up
with a patch to help users on these platforms.

Thanks.


[Footnote]

*1* http://thread.gmane.org/gmane.comp.version-control.git/211803

Re: [LHF] making t5000 "tar xf" tests more lenient

[René Scharfe]

Am 24.12.2012 21:49, schrieb Junio C Hamano:
> I've been running testsuite on a few platforms that are unfamiliar
> to me, and was bitten by BSD implementation of tar that do not grok
> the extended pax headers.  I've already fixed one in t9502 [*1*]
> where we produce a tarball with "git archive" and then try to
> validate it with the platform implementation of "tar" so that the
> test won't barf when it sees the extended pax header.
> 
> t5000 is littered with similar tests that break unnecessarily with
> BSD implementations.  I think what it wants to test are:
> 
>   - "archive" produces tarball, and "tar" can extract from it.  If
>     your "tar" does not understand pax header, you may get it as an
>     extra file that shouldn't be there, but that should not cause the
>     test to fail---in real life, people without a pax-aware "tar"
>     will just ignore and remove the header and can go on.
> 
>   - "get-tar-commmit-id" can inspect a tarball produced by "archive"
>     to recover the object name of the commit even on a platform
>     without a pax-aware "tar".
> 
> Perhaps t5000 can be restructured like so:
> 
>   - create a tarball with the commit-id and test with
>     "get-tar-commit-id" to validate it; also create a tarball out of
>     a tree to make sure it does not have commit-id and check with
>     "get-tar-commit-id".  Do this on any and all platform, even on
>     the ones without a pax-aware "tar".
> 
>   - check platform implementation of "tar" early to see if extracting
>     a simple output from "git archive" results in an extra pax header
>     file.  If so, remember this fact and produce any and all tarballs
>     used in the remainder of the test by forcing ^{tree}.
> 
> so that people on platforms without pax-aware "tar" do not have to
> install GNU tar only to pass this test.
> 
> It would be a good exercise during the holiday week for somebody on
> BSD (it seems NetBSD is more troublesome than OpenBSD) to come up
> with a patch to help users on these platforms.

I got around to building a virtual machine with NetBSD 6.0.1, which
involved a bit of cursing because networking only seems to work when I
turn off ACPI and SMP -- and running tests is a lot more pleasant with
more than one CPU.

Anyway, I don't think the pax headers are to blame here.  The patch below
fixes the tar failures for me, but I'm not sure why.  There must be
something special about some (not all!) directory entries with trailing
slashes after directory names.

Several ZIP tests still fail, however, because NetBSD's unzip doesn't
seem to support (our flavour of) symlinks and streamed files.

I'll take a deeper look into it over the weekend.

René


---
 archive-tar.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/archive-tar.c b/archive-tar.c
index 0ba3f25..fd2d3e8 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -215,6 +215,8 @@ static int write_tar_entry(struct archiver_args *args,
 	if (S_ISDIR(mode) || S_ISGITLINK(mode)) {
 		*header.typeflag = TYPEFLAG_DIR;
 		mode = (mode | 0777) & ~tar_umask;
+		if (pathlen && path[pathlen - 1] == '/')
+			pathlen--;
 	} else if (S_ISLNK(mode)) {
 		*header.typeflag = TYPEFLAG_LNK;
 		mode |= 0777;
-- 
1.7.12

Re: [LHF] making t5000 "tar xf" tests more lenient

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Anyway, I don't think the pax headers are to blame here.  The patch below
> fixes the tar failures for me, but I'm not sure why.  There must be
> something special about some (not all!) directory entries with trailing
> slashes after directory names.
>
> Several ZIP tests still fail, however, because NetBSD's unzip doesn't
> seem to support (our flavour of) symlinks and streamed files.

Just FYI, I found that unzip that comes with base distro and the one
you can add via pkg_add (or pkgin) are different, and the latter
seemed to behave better.

Re: [LHF] making t5000 "tar xf" tests more lenient

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> Anyway, I don't think the pax headers are to blame here.

Hmph, I am reasonably sure I saw a test that created an archive from
a commit (hence with pax header), asked platform tar to either list
the contents or actually extracted to the filesystem, and tried to
ensure nothing but the paths in the repository existed in the
archive.  When the platform tar implementation treated the pax
header as an extra file, such a test sees something not in the
repository and fails.

This was on (and I am still on) NetBSD 6.0 not 6.0.1, by the way.

[PATCH] archive-tar: split long paths more carefully

[René Scharfe]

The name field of a tar header has a size of 100 characters.  This limit
was extended long ago in a backward compatible way by providing the
additional prefix field, which can hold 155 additional characters.  The
actual path is constructed at extraction time by concatenating the prefix
field, a slash and the name field.

get_path_prefix() is used to determine which slash in the path is used as
the cutting point and thus which part of it is placed into the field
prefix and which into the field name.  It tries to cram as much into the
prefix field as possible.  (And only if we can't fit a path into the
provided 255 characters we use a pax extended header to store it.)

If a path is longer than 100 but shorter than 156 characters and ends
with a slash (i.e. is for a directory) then get_path_prefix() puts the
whole path in the prefix field and leaves the name field empty.  GNU tar
reconstructs the path without complaint, but the tar included with
NetBSD 6 does not: It reports the header to be invalid.

For compatibility with this version of tar, make sure to never leave the
name field empty.  In order to do that, trim the trailing slash from the
part considered as possible prefix, if it exists -- that way the last
path component (or more, but not less) will end up in the name field.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-tar.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/archive-tar.c b/archive-tar.c
index 0ba3f25..923daf5 100644
--- a/archive-tar.c
+++ b/archive-tar.c
@@ -153,6 +153,8 @@ static unsigned int ustar_header_chksum(const struct ustar_header *header)
 static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
 {
 	size_t i = pathlen;
+	if (i > 1 && path[i - 1] == '/')
+		i--;
 	if (i > maxlen)
 		i = maxlen;
 	do {
-- 
1.7.12

Re: [LHF] making t5000 "tar xf" tests more lenient

[René Scharfe]

Am 05.01.2013 21:11, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Anyway, I don't think the pax headers are to blame here.
>
> Hmph, I am reasonably sure I saw a test that created an archive from
> a commit (hence with pax header), asked platform tar to either list
> the contents or actually extracted to the filesystem, and tried to
> ensure nothing but the paths in the repository existed in the
> archive.  When the platform tar implementation treated the pax
> header as an extra file, such a test sees something not in the
> repository and fails.

t5000 avoids that issue by comparing only the contents of a 
subdirectory.  The script could do with a little cleanup, in any case. 
Moving ZIP testing into its own file, more explicit pax header file 
handling, speaking file and directory names, and modern coding style 
would all be nice. :)

René

Re: [PATCH] archive-tar: split long paths more carefully

[Jonathan Nieder]

René Scharfe wrote:

> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -153,6 +153,8 @@ static unsigned int ustar_header_chksum(const struct ustar_header *header)
>  static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
>  {
>  	size_t i = pathlen;
> +	if (i > 1 && path[i - 1] == '/')
> +		i--;

Beautiful.

Re: [PATCH] archive-tar: split long paths more carefully

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> The name field of a tar header has a size of 100 characters.  This limit
> was extended long ago in a backward compatible way by providing the
> additional prefix field, which can hold 155 additional characters.  The
> actual path is constructed at extraction time by concatenating the prefix
> field, a slash and the name field.
>
> get_path_prefix() is used to determine which slash in the path is used as
> the cutting point and thus which part of it is placed into the field
> prefix and which into the field name.  It tries to cram as much into the
> prefix field as possible.  (And only if we can't fit a path into the
> provided 255 characters we use a pax extended header to store it.)
>
> If a path is longer than 100 but shorter than 156 characters and ends
> with a slash (i.e. is for a directory) then get_path_prefix() puts the
> whole path in the prefix field and leaves the name field empty.  GNU tar
> reconstructs the path without complaint, but the tar included with
> NetBSD 6 does not: It reports the header to be invalid.
>
> For compatibility with this version of tar, make sure to never leave the
> name field empty.  In order to do that, trim the trailing slash from the
> part considered as possible prefix, if it exists -- that way the last
> path component (or more, but not less) will end up in the name field.

Nicely explained; thanks.

Makes me wonder what we should do for a file inside a directory
whose name is 10 bytes long, and whose filename is 120 bytes long,
though.

Sounds like people on NetBSD are SOL due to the 155+'/'+100 in such
a case and there is nothing we can do, I guess.

> Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
> ---
>  archive-tar.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/archive-tar.c b/archive-tar.c
> index 0ba3f25..923daf5 100644
> --- a/archive-tar.c
> +++ b/archive-tar.c
> @@ -153,6 +153,8 @@ static unsigned int ustar_header_chksum(const struct ustar_header *header)
>  static size_t get_path_prefix(const char *path, size_t pathlen, size_t maxlen)
>  {
>  	size_t i = pathlen;
> +	if (i > 1 && path[i - 1] == '/')
> +		i--;
>  	if (i > maxlen)
>  		i = maxlen;
>  	do {

[PATCH] archive-zip: write uncompressed size into header even with streaming

[René Scharfe]

We record the uncompressed and compressed sizes and the CRC of streamed
files as zero in the local header of the file.  The actual values are
recorded in an extra data descriptor after the file content, and in the
usual ZIP directory entry at the end of the archive.

While we know the compressed size and the CRC only after we processed
the contents, we actually know the uncompressed size right from the
start.  And for files that we store uncompressed we also already know
their final size.

Do it like InfoZIP's zip and recored the known values, even though they
can be reconstructed using the ZIP directory and the data descriptors
alone.  InfoZIP's unzip worked fine before, but NetBSD's version
actually depends on these fields.

The uncompressed size is already set by sha1_object_info().  We just
need to initialize the compressed size to zero or the uncompressed size
depending on the compression method (0 means storing).  The CRC was
propertly initialized already.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 archive-zip.c | 7 ++-----
 1 file changed, 2 insertions(+), 5 deletions(-)

diff --git a/archive-zip.c b/archive-zip.c
index 55f66b4..d3aef53 100644
--- a/archive-zip.c
+++ b/archive-zip.c
@@ -240,7 +240,7 @@ static int write_zip_entry(struct archiver_args *args,
 			(mode & 0111) ? ((mode) << 16) : 0;
 		if (S_ISREG(mode) && args->compression_level != 0 && size > 0)
 			method = 8;
-		compressed_size = size;
+		compressed_size = (method == 0) ? size : 0;
 
 		if (S_ISREG(mode) && type == OBJ_BLOB && !args->convert &&
 		    size > big_file_threshold) {
@@ -313,10 +313,7 @@ static int write_zip_entry(struct archiver_args *args,
 	copy_le16(header.compression_method, method);
 	copy_le16(header.mtime, zip_time);
 	copy_le16(header.mdate, zip_date);
-	if (flags & ZIP_STREAM)
-		set_zip_header_data_desc(&header, 0, 0, 0);
-	else
-		set_zip_header_data_desc(&header, size, compressed_size, crc);
+	set_zip_header_data_desc(&header, size, compressed_size, crc);
 	copy_le16(header.filename_length, pathlen);
 	copy_le16(header.extra_length, ZIP_EXTRA_MTIME_SIZE);
 	write_or_die(1, &header, ZIP_LOCAL_HEADER_SIZE);
-- 
1.7.12

[PATCH 0/4] ZIP test fixes

[René Scharfe]

Fix a bug in two scripts that call unzip, use the opportunity for a small
cleanup, move all ZIP related tests out of t5000 and finally skip testing
symlinks if unzip doesn't support them.

The first one allows running t5000 with the unzip from pkgsrc manually on
NetBSD, which succeeds.  The last one -- together with the archive-zip
streaming patch I sent earlier today -- makes the ZIP tests succeed on
that platform out of the box.

René


  t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead
  t0024, t5000: use test_lazy_prereq for UNZIP
  t5000, t5002: move ZIP tests into their own script
  t5002: check if unzip supports symlinks

 t/t0024-crlf-archive.sh      |  16 +++---
 t/t5000-tar-tree.sh          |  71 -----------------------
 t/t5002-archive-zip.sh       | 131 +++++++++++++++++++++++++++++++++++++++++++
 t/t5002/infozip-symlinks.zip | Bin 0 -> 328 bytes
 t/test-lib.sh                |   2 +
 5 files changed, 140 insertions(+), 80 deletions(-)
 create mode 100755 t/t5002-archive-zip.sh
 create mode 100644 t/t5002/infozip-symlinks.zip

-- 
1.7.12

[PATCH 1/4] t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead

[René Scharfe]

InfoZIP's unzip takes default parameters from the environment variable
UNZIP.  Unset it in the test library and use GIT_UNZIP for specifying
alternate versions of the unzip command instead.

t0024 wasn't even using variable for the actual extraction.  t5000
was, but when setting it to InfoZIP's unzip it would try to extract
from itself (because it treats the contents of $UNZIP as parameters),
which failed of course.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t0024-crlf-archive.sh |  6 +++---
 t/t5000-tar-tree.sh     | 10 +++++-----
 t/test-lib.sh           |  2 ++
 3 files changed, 10 insertions(+), 8 deletions(-)

diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index ec6c1b3..080fe5c 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -3,7 +3,7 @@
 test_description='respect crlf in git archive'
 
 . ./test-lib.sh
-UNZIP=${UNZIP:-unzip}
+GIT_UNZIP=${GIT_UNZIP:-unzip}
 
 test_expect_success setup '
 
@@ -26,7 +26,7 @@ test_expect_success 'tar archive' '
 
 '
 
-"$UNZIP" -v >/dev/null 2>&1
+"$GIT_UNZIP" -v >/dev/null 2>&1
 if [ $? -eq 127 ]; then
 	say "Skipping ZIP test, because unzip was not found"
 else
@@ -37,7 +37,7 @@ test_expect_success UNZIP 'zip archive' '
 
 	git archive --format=zip HEAD >test.zip &&
 
-	( mkdir unzipped && cd unzipped && unzip ../test.zip ) &&
+	( mkdir unzipped && cd unzipped && "$GIT_UNZIP" ../test.zip ) &&
 
 	test_cmp sample unzipped/sample
 
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index ecf00ed..1f7593d 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -25,7 +25,7 @@ commit id embedding:
 '
 
 . ./test-lib.sh
-UNZIP=${UNZIP:-unzip}
+GIT_UNZIP=${GIT_UNZIP:-unzip}
 GZIP=${GZIP:-gzip}
 GUNZIP=${GUNZIP:-gzip -d}
 
@@ -37,9 +37,9 @@ check_zip() {
 	dir=$1
 	dir_with_prefix=$dir/$2
 
-	test_expect_success UNZIP " extract ZIP archive" "
-		(mkdir $dir && cd $dir && $UNZIP ../$zipfile)
-	"
+	test_expect_success UNZIP " extract ZIP archive" '
+		(mkdir $dir && cd $dir && "$GIT_UNZIP" ../$zipfile)
+	'
 
 	test_expect_success UNZIP " validate filenames" "
 		(cd ${dir_with_prefix}a && find .) | sort >$listfile &&
@@ -201,7 +201,7 @@ test_expect_success \
       test_cmp a/substfile2 g/prefix/a/substfile2
 '
 
-$UNZIP -v >/dev/null 2>&1
+"$GIT_UNZIP" -v >/dev/null 2>&1
 if [ $? -eq 127 ]; then
 	say "Skipping ZIP tests, because unzip was not found"
 else
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8a12cbb..d8ec408 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -85,6 +85,7 @@ unset VISUAL EMAIL LANGUAGE COLUMNS $("$PERL_PATH" -e '
 		.*_TEST
 		PROVE
 		VALGRIND
+		UNZIP
 		PERF_AGGREGATING_LATER
 	));
 	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
@@ -128,6 +129,7 @@ fi
 unset CDPATH
 
 unset GREP_OPTIONS
+unset UNZIP
 
 case $(echo $GIT_TRACE |tr "[A-Z]" "[a-z]") in
 1|2|true)
-- 
1.7.12

[PATCH 2/4] t0024, t5000: use test_lazy_prereq for UNZIP

[René Scharfe]

This change makes the code smaller and we can put it at the top of
the script, its rightful place as setup code.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t0024-crlf-archive.sh | 12 +++++-------
 t/t5000-tar-tree.sh     | 12 +++++-------
 2 files changed, 10 insertions(+), 14 deletions(-)

diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index 080fe5c..ba397bb 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -5,6 +5,11 @@ test_description='respect crlf in git archive'
 . ./test-lib.sh
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 
+test_lazy_prereq UNZIP '
+	"$GIT_UNZIP" -v >/dev/null 2>&1
+	test $? -ne 127
+'
+
 test_expect_success setup '
 
 	git config core.autocrlf true &&
@@ -26,13 +31,6 @@ test_expect_success 'tar archive' '
 
 '
 
-"$GIT_UNZIP" -v >/dev/null 2>&1
-if [ $? -eq 127 ]; then
-	say "Skipping ZIP test, because unzip was not found"
-else
-	test_set_prereq UNZIP
-fi
-
 test_expect_success UNZIP 'zip archive' '
 
 	git archive --format=zip HEAD >test.zip &&
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 1f7593d..6702157 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -31,6 +31,11 @@ GUNZIP=${GUNZIP:-gzip -d}
 
 SUBSTFORMAT=%H%n
 
+test_lazy_prereq UNZIP '
+	"$GIT_UNZIP" -v >/dev/null 2>&1
+	test $? -ne 127
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst
@@ -201,13 +206,6 @@ test_expect_success \
       test_cmp a/substfile2 g/prefix/a/substfile2
 '
 
-"$GIT_UNZIP" -v >/dev/null 2>&1
-if [ $? -eq 127 ]; then
-	say "Skipping ZIP tests, because unzip was not found"
-else
-	test_set_prereq UNZIP
-fi
-
 test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
-- 
1.7.12

[PATCH 3/4] t5000, t5002: move ZIP tests into their own script

[René Scharfe]

This makes ZIP specific tweaks easier.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t5000-tar-tree.sh    |  69 ----------------------------
 t/t5002-archive-zip.sh | 119 +++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 119 insertions(+), 69 deletions(-)
 create mode 100755 t/t5002-archive-zip.sh

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 6702157..e7c240f 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -25,37 +25,11 @@ commit id embedding:
 '
 
 . ./test-lib.sh
-GIT_UNZIP=${GIT_UNZIP:-unzip}
 GZIP=${GZIP:-gzip}
 GUNZIP=${GUNZIP:-gzip -d}
 
 SUBSTFORMAT=%H%n
 
-test_lazy_prereq UNZIP '
-	"$GIT_UNZIP" -v >/dev/null 2>&1
-	test $? -ne 127
-'
-
-check_zip() {
-	zipfile=$1.zip
-	listfile=$1.lst
-	dir=$1
-	dir_with_prefix=$dir/$2
-
-	test_expect_success UNZIP " extract ZIP archive" '
-		(mkdir $dir && cd $dir && "$GIT_UNZIP" ../$zipfile)
-	'
-
-	test_expect_success UNZIP " validate filenames" "
-		(cd ${dir_with_prefix}a && find .) | sort >$listfile &&
-		test_cmp a.lst $listfile
-	"
-
-	test_expect_success UNZIP " validate file contents" "
-		diff -r a ${dir_with_prefix}a
-	"
-}
-
 test_expect_success \
     'populate workdir' \
     'mkdir a b c &&
@@ -206,55 +180,12 @@ test_expect_success \
       test_cmp a/substfile2 g/prefix/a/substfile2
 '
 
-test_expect_success \
-    'git archive --format=zip' \
-    'git archive --format=zip HEAD >d.zip'
-
-check_zip d
-
-test_expect_success \
-    'git archive --format=zip in a bare repo' \
-    '(cd bare.git && git archive --format=zip HEAD) >d1.zip'
-
-test_expect_success \
-    'git archive --format=zip vs. the same in a bare repo' \
-    'test_cmp d.zip d1.zip'
-
-test_expect_success 'git archive --format=zip with --output' \
-    'git archive --format=zip --output=d2.zip HEAD &&
-    test_cmp d.zip d2.zip'
-
-test_expect_success 'git archive with --output, inferring format' '
-	git archive --output=d3.zip HEAD &&
-	test_cmp d.zip d3.zip
-'
-
 test_expect_success 'git archive with --output, override inferred format' '
 	git archive --format=tar --output=d4.zip HEAD &&
 	test_cmp b.tar d4.zip
 '
 
 test_expect_success \
-    'git archive --format=zip with prefix' \
-    'git archive --format=zip --prefix=prefix/ HEAD >e.zip'
-
-check_zip e prefix/
-
-test_expect_success 'git archive -0 --format=zip on large files' '
-	test_config core.bigfilethreshold 1 &&
-	git archive -0 --format=zip HEAD >large.zip
-'
-
-check_zip large
-
-test_expect_success 'git archive --format=zip on large files' '
-	test_config core.bigfilethreshold 1 &&
-	git archive --format=zip HEAD >large-compressed.zip
-'
-
-check_zip large-compressed
-
-test_expect_success \
     'git archive --list outside of a git repo' \
     'GIT_DIR=some/non-existing/directory git archive --list'
 
diff --git a/t/t5002-archive-zip.sh b/t/t5002-archive-zip.sh
new file mode 100755
index 0000000..ac9c6d4
--- /dev/null
+++ b/t/t5002-archive-zip.sh
@@ -0,0 +1,119 @@
+#!/bin/sh
+
+test_description='git archive --format=zip test'
+
+. ./test-lib.sh
+GIT_UNZIP=${GIT_UNZIP:-unzip}
+
+SUBSTFORMAT=%H%n
+
+test_lazy_prereq UNZIP '
+	"$GIT_UNZIP" -v >/dev/null 2>&1
+	test $? -ne 127
+'
+
+check_zip() {
+	zipfile=$1.zip
+	listfile=$1.lst
+	dir=$1
+	dir_with_prefix=$dir/$2
+
+	test_expect_success UNZIP " extract ZIP archive" '
+		(mkdir $dir && cd $dir && "$GIT_UNZIP" ../$zipfile)
+	'
+
+	test_expect_success UNZIP " validate filenames" "
+		(cd ${dir_with_prefix}a && find .) | sort >$listfile &&
+		test_cmp a.lst $listfile
+	"
+
+	test_expect_success UNZIP " validate file contents" "
+		diff -r a ${dir_with_prefix}a
+	"
+}
+
+test_expect_success \
+    'populate workdir' \
+    'mkdir a b c &&
+     echo simple textfile >a/a &&
+     mkdir a/bin &&
+     cp /bin/sh a/bin &&
+     printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile1 &&
+     printf "A not substituted O" >a/substfile2 &&
+     if test_have_prereq SYMLINKS; then
+	ln -s a a/l1
+     else
+	printf %s a > a/l1
+     fi &&
+     (p=long_path_to_a_file && cd a &&
+      for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
+      echo text >file_with_long_path) &&
+     (cd a && find .) | sort >a.lst'
+
+test_expect_success \
+    'add ignored file' \
+    'echo ignore me >a/ignored &&
+     echo ignored export-ignore >.git/info/attributes'
+
+test_expect_success \
+    'add files to repository' \
+    'find a -type f | xargs git update-index --add &&
+     find a -type l | xargs git update-index --add &&
+     treeid=`git write-tree` &&
+     echo $treeid >treeid &&
+     git update-ref HEAD $(TZ=GMT GIT_COMMITTER_DATE="2005-05-27 22:00:00" \
+     git commit-tree $treeid </dev/null)'
+
+test_expect_success \
+    'create bare clone' \
+    'git clone --bare . bare.git &&
+     cp .git/info/attributes bare.git/info/attributes'
+
+test_expect_success \
+    'remove ignored file' \
+    'rm a/ignored'
+
+test_expect_success \
+    'git archive --format=zip' \
+    'git archive --format=zip HEAD >d.zip'
+
+check_zip d
+
+test_expect_success \
+    'git archive --format=zip in a bare repo' \
+    '(cd bare.git && git archive --format=zip HEAD) >d1.zip'
+
+test_expect_success \
+    'git archive --format=zip vs. the same in a bare repo' \
+    'test_cmp d.zip d1.zip'
+
+test_expect_success 'git archive --format=zip with --output' \
+    'git archive --format=zip --output=d2.zip HEAD &&
+    test_cmp d.zip d2.zip'
+
+test_expect_success 'git archive with --output, inferring format' '
+	git archive --output=d3.zip HEAD &&
+	test_cmp d.zip d3.zip
+'
+
+test_expect_success \
+    'git archive --format=zip with prefix' \
+    'git archive --format=zip --prefix=prefix/ HEAD >e.zip'
+
+check_zip e prefix/
+
+test_expect_success 'git archive -0 --format=zip on large files' '
+	test_config core.bigfilethreshold 1 &&
+	git archive -0 --format=zip HEAD >large.zip
+'
+
+check_zip large
+
+test_expect_success 'git archive --format=zip on large files' '
+	test_config core.bigfilethreshold 1 &&
+	git archive --format=zip HEAD >large-compressed.zip
+'
+
+check_zip large-compressed
+
+test_done
-- 
1.7.12

[PATCH 4/4] t5002: check if unzip supports symlinks

[René Scharfe]

Only add a symlink to the repository if both the filesystem and
unzip support symlinks.  To check the latter, add a ZIP file
containing a symlink, created like this with InfoZIP zip 3.0:

	$ echo sample text >textfile
	$ ln -s textfile symlink
	$ zip -y infozip-symlinks.zip textfile symlink

If we can extract it successfully, we add a symlink to the test
repository for git archive --format=zip, or otherwise skip that
step.  Users can see the skipped test and perhaps run it again
with a different unzip version.

Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
 t/t5002-archive-zip.sh       |  26 +++++++++++++++++++-------
 t/t5002/infozip-symlinks.zip | Bin 0 -> 328 bytes
 2 files changed, 19 insertions(+), 7 deletions(-)
 create mode 100644 t/t5002/infozip-symlinks.zip

diff --git a/t/t5002-archive-zip.sh b/t/t5002-archive-zip.sh
index ac9c6d4..d35aa24 100755
--- a/t/t5002-archive-zip.sh
+++ b/t/t5002-archive-zip.sh
@@ -12,6 +12,15 @@ test_lazy_prereq UNZIP '
 	test $? -ne 127
 '
 
+test_lazy_prereq UNZIP_SYMLINKS '
+	(
+		mkdir unzip-symlinks &&
+		cd unzip-symlinks &&
+		"$GIT_UNZIP" "$TEST_DIRECTORY"/t5002/infozip-symlinks.zip &&
+		test -h symlink
+	)
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst
@@ -40,15 +49,18 @@ test_expect_success \
      cp /bin/sh a/bin &&
      printf "A\$Format:%s\$O" "$SUBSTFORMAT" >a/substfile1 &&
      printf "A not substituted O" >a/substfile2 &&
-     if test_have_prereq SYMLINKS; then
-	ln -s a a/l1
-     else
-	printf %s a > a/l1
-     fi &&
      (p=long_path_to_a_file && cd a &&
       for depth in 1 2 3 4 5; do mkdir $p && cd $p; done &&
-      echo text >file_with_long_path) &&
-     (cd a && find .) | sort >a.lst'
+      echo text >file_with_long_path)
+'
+
+test_expect_success SYMLINKS,UNZIP_SYMLINKS 'add symlink' '
+	ln -s a a/symlink_to_a
+'
+
+test_expect_success 'prepare file list' '
+	(cd a && find .) | sort >a.lst
+'
 
 test_expect_success \
     'add ignored file' \
diff --git a/t/t5002/infozip-symlinks.zip b/t/t5002/infozip-symlinks.zip
new file mode 100644
index 0000000000000000000000000000000000000000..065728c631cf1f7ab20a045a83abc3e08455eeba
GIT binary patch
literal 328
zcmWIWW@h1H0D(ty)tzkeJdg4K*&xipAj43ST2YdgnUfkC!pXp_F7Y}5gi9;985mh!
zFf%Z)qyW_wC*~I9q$+@vas|Lmdj&M@9kbsh4zNiK4D3MDiYs$-GV`**hM5Bm0%0`6
zU={{=Gcw6B<8qh;&`<^jMj&3&2x7r>g@&*~oQY;CvT2wOgO~;~=j}p2APILS&@e1c
U4De=U11V+#!r4H2I*7vn0CeC%rvLx|

literal 0
HcmV?d00001

-- 
1.7.12

Re: [PATCH 2/4] t0024, t5000: use test_lazy_prereq for UNZIP

[Matt Kraai]

On Sun, Jan 06, 2013 at 06:49:00PM +0100, René Scharfe wrote:
> This change makes the code smaller and we can put it at the top of
> the script, its rightful place as setup code.

Would it be better to add the setting of GIT_UNZIP and
test_lazy_prereq to test-lib.sh so they aren't duplicated in both
t0024-crlf-archive.sh and t5000-tar-tree.sh, something like the
following (modulo UNZIP/GIT_UNZIP)?

-- 
Matt Kraai
https://ftbfs.org/kraai

diff --git a/t/t0024-crlf-archive.sh b/t/t0024-crlf-archive.sh
index ec6c1b3..084f33c 100755
--- a/t/t0024-crlf-archive.sh
+++ b/t/t0024-crlf-archive.sh
@@ -3,7 +3,6 @@
 test_description='respect crlf in git archive'
 
 . ./test-lib.sh
-UNZIP=${UNZIP:-unzip}
 
 test_expect_success setup '
 
@@ -26,13 +25,6 @@ test_expect_success 'tar archive' '
 
 '
 
-"$UNZIP" -v >/dev/null 2>&1
-if [ $? -eq 127 ]; then
-	say "Skipping ZIP test, because unzip was not found"
-else
-	test_set_prereq UNZIP
-fi
-
 test_expect_success UNZIP 'zip archive' '
 
 	git archive --format=zip HEAD >test.zip &&
diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index ecf00ed..85b64ae 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -25,7 +25,6 @@ commit id embedding:
 '
 
 . ./test-lib.sh
-UNZIP=${UNZIP:-unzip}
 GZIP=${GZIP:-gzip}
 GUNZIP=${GUNZIP:-gzip -d}
 
@@ -201,13 +200,6 @@ test_expect_success \
       test_cmp a/substfile2 g/prefix/a/substfile2
 '
 
-$UNZIP -v >/dev/null 2>&1
-if [ $? -eq 127 ]; then
-	say "Skipping ZIP tests, because unzip was not found"
-else
-	test_set_prereq UNZIP
-fi
-
 test_expect_success \
     'git archive --format=zip' \
     'git archive --format=zip HEAD >d.zip'
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 8a12cbb..4ceabad 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -752,6 +752,13 @@ test_lazy_prereq AUTOIDENT '
 	git var GIT_AUTHOR_IDENT
 '
 
+UNZIP=${UNZIP:-unzip}
+
+test_lazy_prereq UNZIP '
+	"$UNZIP" -v >/dev/null 2>&1
+	test $? -ne 127
+'
+
 # When the tests are run as root, permission tests will report that
 # things are writable when they shouldn't be.
 test -w / || test_set_prereq SANITY

Re: [PATCH 2/4] t0024, t5000: use test_lazy_prereq for UNZIP

[René Scharfe]

Am 06.01.2013 19:06, schrieb Matt Kraai:
> On Sun, Jan 06, 2013 at 06:49:00PM +0100, René Scharfe wrote:
>> This change makes the code smaller and we can put it at the top of
>> the script, its rightful place as setup code.
>
> Would it be better to add the setting of GIT_UNZIP and
> test_lazy_prereq to test-lib.sh so they aren't duplicated in both
> t0024-crlf-archive.sh and t5000-tar-tree.sh, something like the
> following (modulo UNZIP/GIT_UNZIP)?

We could do that in a follow-up patch, but I'm not sure it's worth it 
for the two use cases.

René

Re: [PATCH 1/4] t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead

[Jonathan Nieder]

René Scharfe wrote:

> InfoZIP's unzip takes default parameters from the environment variable
> UNZIP.  Unset it in the test library and use GIT_UNZIP for specifying
> alternate versions of the unzip command instead.
>
> t0024 wasn't even using variable for the actual extraction.  t5000
> was, but when setting it to InfoZIP's unzip it would try to extract
> from itself (because it treats the contents of $UNZIP as parameters),
> which failed of course.

That would only happen if the UNZIP variable was already exported,
right?

The patch makes sense and takes care of all uses of ${UNZIP} I can
find, and it even makes the quoting consistent so a person can put
their copy of unzip under "/Program Files".  For what it's worth,

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 2/4] t0024, t5000: use test_lazy_prereq for UNZIP

[Jonathan Nieder]

René Scharfe wrote:

> --- a/t/t0024-crlf-archive.sh
> +++ b/t/t0024-crlf-archive.sh
> @@ -5,6 +5,11 @@ test_description='respect crlf in git archive'
>  . ./test-lib.sh
>  GIT_UNZIP=${GIT_UNZIP:-unzip}
>  
> +test_lazy_prereq UNZIP '
> +	"$GIT_UNZIP" -v >/dev/null 2>&1
> +	test $? -ne 127

Micronit: now that this is part of a test, there is no more need to
silence its output.  The "unzip -v" output could be useful to people
debugging with "t0024-crlf-archive.sh -v -i".

With or without that change, this is a nice cleanup and obviously
correct, so
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Re: [PATCH 4/4] t5002: check if unzip supports symlinks

[Jonathan Nieder]

René Scharfe wrote:

> Only add a symlink to the repository if both the filesystem and
> unzip support symlinks.  To check the latter, add a ZIP file
> containing a symlink, created like this with InfoZIP zip 3.0:
>
>	$ echo sample text >textfile
>	$ ln -s textfile symlink
>	$ zip -y infozip-symlinks.zip textfile symlink

Hm.  Do some implementations of "unzip" not support symlinks, or is
the problem that some systems build Info-ZIP without the SYMLINKS
option?

Re: [PATCH 1/4] t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead

[René Scharfe]

Am 07.01.2013 06:16, schrieb Jonathan Nieder:
> René Scharfe wrote:
>
>> InfoZIP's unzip takes default parameters from the environment variable
>> UNZIP.  Unset it in the test library and use GIT_UNZIP for specifying
>> alternate versions of the unzip command instead.
>>
>> t0024 wasn't even using variable for the actual extraction.  t5000
>> was, but when setting it to InfoZIP's unzip it would try to extract
>> from itself (because it treats the contents of $UNZIP as parameters),
>> which failed of course.
>
> That would only happen if the UNZIP variable was already exported,
> right?

We don't want any parameters a user may have been specified influence 
the test.  I'm not sure if someone actually sets that variable for that 
purpose, though.

My main use case is running individual test scripts with an alternative 
unzip binary, and with the patch this works as expected:

	$ cd t
	$ GIT_UNZIP=/usr/pkg/bin/unzip ./t5000-tar-tree.sh

> The patch makes sense and takes care of all uses of ${UNZIP} I can
> find, and it even makes the quoting consistent so a person can put
> their copy of unzip under "/Program Files".  For what it's worth,
>
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks!

René

Re: [PATCH 2/4] t0024, t5000: use test_lazy_prereq for UNZIP

[René Scharfe]

Am 07.01.2013 09:45, schrieb Jonathan Nieder:
> René Scharfe wrote:
>
>> --- a/t/t0024-crlf-archive.sh
>> +++ b/t/t0024-crlf-archive.sh
>> @@ -5,6 +5,11 @@ test_description='respect crlf in git archive'
>>   . ./test-lib.sh
>>   GIT_UNZIP=${GIT_UNZIP:-unzip}
>>
>> +test_lazy_prereq UNZIP '
>> +	"$GIT_UNZIP" -v >/dev/null 2>&1
>> +	test $? -ne 127
>
> Micronit: now that this is part of a test, there is no more need to
> silence its output.  The "unzip -v" output could be useful to people
> debugging with "t0024-crlf-archive.sh -v -i".

Oh, yes, good point.

> With or without that change, this is a nice cleanup and obviously
> correct, so
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks,
René

Re: [PATCH 4/4] t5002: check if unzip supports symlinks

[René Scharfe]

Am 07.01.2013 09:52, schrieb Jonathan Nieder:
> René Scharfe wrote:
>
>> Only add a symlink to the repository if both the filesystem and
>> unzip support symlinks.  To check the latter, add a ZIP file
>> containing a symlink, created like this with InfoZIP zip 3.0:
>>
>> 	$ echo sample text >textfile
>> 	$ ln -s textfile symlink
>> 	$ zip -y infozip-symlinks.zip textfile symlink
>
> Hm.  Do some implementations of "unzip" not support symlinks, or is
> the problem that some systems build Info-ZIP without the SYMLINKS
> option?

The unzip supplied with NetBSD 6.0.1, which is based on libarchive, 
doesn't support symlinks.  It creates a file with the link target path 
as its only content for such entries.

I assume that Info-ZIP is compiled with the SYMLINKS option on all 
platforms whose default filesystem supports symbolic links.  Except on 
Windows perhaps, where it's complicated.

For the test script there is no difference: If we don't have a tool to 
verify symlinks in archives, we better skip that part.

René

Re: [PATCH 4/4] t5002: check if unzip supports symlinks

[Jonathan Nieder]

René Scharfe wrote:
> Am 07.01.2013 09:52, schrieb Jonathan Nieder:

>> Hm.  Do some implementations of "unzip" not support symlinks, or is
>> the problem that some systems build Info-ZIP without the SYMLINKS
>> option?
>
> The unzip supplied with NetBSD 6.0.1, which is based on libarchive, doesn't
> support symlinks.  It creates a file with the link target path as its only
> content for such entries.

Ok, that makes sense.  A quick search finds
<https://code.google.com/p/libarchive/issues/detail?id=104>, which if
I understand correctly was fixed in libarchive 3.0.2.  NetBSD 6 uses a
patched 2.8.4.

[...]
> For the test script there is no difference: If we don't have a tool to
> verify symlinks in archives, we better skip that part.

Yeah, I just wanted to see if there were other parts of the world that
needed fixing while at it.  Thanks for explaining.

Ciao,
Jonathan