Re: [PATCH] test-lib: stricter unzip(1) check

Jeff King [off-list ref] writes:

My Debian version of unzip (which is derived from Info-zip) seems to
give return code 0 for just "unzip". So for the first check, we could
possibly drop "-v"; we don't care about "-v", but just wanted some way
to say "does unzip exist on the path?". Another option would just be
checking whether "unzip" returns something besides 127 (so what we have
now, minus "-v").

To test for "-a", I think we'd have to actually feed it a sample zip
file, though. My unzip returns "10", which its manpage explains as
"invalid command line options" (presumably because of the missing
zipfile argument). But that seems like it probably isn't portable.  And
it's also what I might expect another unzip to return if it doesn't
support "-a".

So while this patch does solve the immediate problem, I think it does so
by overly skipping tests that we _could_ run.

Hmm, how about taking Dscho's "default GIT_UNZIP to /usr/local/bin/unzip
on FreeBSD" thing, together with something like this, then?

I suspect that 4 checks that look at $extracted/* after running
unzip -a should probably be inside a single test that runs unzip -a,
simply because they do not make any sense if the extraction failed,
but I did not fix that with this.

-- >8 --
test: check "unzip" and "unzip -a"

Different platforms have implementations "unzip" that behave
differently.  Most of the tests we use GIT_UNZIP we only care about
the command to be able to extract from *.zip archive, but one test
in t5003 wants it to also be able to grok the "-a" option.

Prepare a sample zip file that has a single text file in it, and try
extracting its contents to see GIT_UNZIP is usable. when setting
UNZIP prerequisite.  Similarly, set UNZIP_AUTOTEXT prerequisite by
running GIT_UNZIP with the "-a" option.

---
 t/t5003-archive-zip.sh   |  19 ++++++++++++++-----
 t/t5003/infozip-text.zip | Bin 0 -> 163 bytes
 t/test-lib.sh            |   4 ++--
 3 files changed, 16 insertions(+), 7 deletions(-)

diff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh
index 14744b2..43c0cfd 100755
--- a/t/t5003-archive-zip.sh
+++ b/t/t5003-archive-zip.sh

@@ -15,6 +15,15 @@ test_lazy_prereq UNZIP_SYMLINKS '
 	)
 '
 
+test_lazy_prereq UNZIP_AUTOTEXT '
+	(
+		mkdir unzip-autotext &&
+		cd unzip-autotext
+		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-text.zip &&
+		test -f text
+	)
+'
+
 check_zip() {
 	zipfile=$1.zip
 	listfile=$1.lst

@@ -39,27 +48,27 @@ check_zip() {
 	extracted=${dir_with_prefix}a
 	original=a
 
-	test_expect_success UNZIP " extract ZIP archive with EOL conversion" '
+	test_expect_success UNZIP_AUTOTEXT " extract ZIP archive with EOL conversion" '
 		(mkdir $dir && cd $dir && "$GIT_UNZIP" -a ../$zipfile)
 	'
 
-	test_expect_success UNZIP " validate that text files are converted" "
+	test_expect_success UNZIP_AUTOTEXT " validate that text files are converted" "
 		test_cmp_bin $extracted/text.cr $extracted/text.crlf &&
 		test_cmp_bin $extracted/text.cr $extracted/text.lf
 	"
 
-	test_expect_success UNZIP " validate that binary files are unchanged" "
+	test_expect_success UNZIP_AUTOTEXT " validate that binary files are unchanged" "
 		test_cmp_bin $original/binary.cr   $extracted/binary.cr &&
 		test_cmp_bin $original/binary.crlf $extracted/binary.crlf &&
 		test_cmp_bin $original/binary.lf   $extracted/binary.lf
 	"
 
-	test_expect_success UNZIP " validate that diff files are converted" "
+	test_expect_success UNZIP_AUTOTEXT " validate that diff files are converted" "
 		test_cmp_bin $extracted/diff.cr $extracted/diff.crlf &&
 		test_cmp_bin $extracted/diff.cr $extracted/diff.lf
 	"
 
-	test_expect_success UNZIP " validate that -diff files are unchanged" "
+	test_expect_success UNZIP_AUTOTEXT " validate that -diff files are unchanged" "
 		test_cmp_bin $original/nodiff.cr   $extracted/nodiff.cr &&
 		test_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&
 		test_cmp_bin $original/nodiff.lf   $extracted/nodiff.lf

diff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip
new file mode 100644
index 0000000..a019acb
Binary files /dev/null and b/t/t5003/infozip-text.zip differ
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11201e9..9907b3f 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh

@@ -1102,8 +1102,8 @@ test_lazy_prereq SANITY '
 
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 test_lazy_prereq UNZIP '
-	"$GIT_UNZIP" -v
-	test $? -ne 127
+	"$GIT_UNZIP" "$TEST_DIRECTORY/t5003/infozip-text.zip" &&
+	test -f text
 '
 
 run_with_limited_cmdline () {

Junio C Hamano [off-list ref] wrote:

+test_lazy_prereq UNZIP_AUTOTEXT '
+	(
+		mkdir unzip-autotext &&
+		cd unzip-autotext
+		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-text.zip &&
+		test -f text
+	)

/usr/bin/unzip actually takes -a on FreeBSD, just not in the
same way the Info-ZIP version does, so I suspect "test -f"
here is not enough.

I would test this, but I can't apply it:

On Mon, Jul 18, 2016 at 12:43:27PM -0700, Junio C Hamano wrote:

-- >8 --
test: check "unzip" and "unzip -a"

Different platforms have implementations "unzip" that behave
differently.  Most of the tests we use GIT_UNZIP we only care about
the command to be able to extract from *.zip archive, but one test
in t5003 wants it to also be able to grok the "-a" option.

Prepare a sample zip file that has a single text file in it, and try
extracting its contents to see GIT_UNZIP is usable. when setting
UNZIP prerequisite.  Similarly, set UNZIP_AUTOTEXT prerequisite by
running GIT_UNZIP with the "-a" option.

I like the direction here, modulo the problems with "-a" that Eric
pointed out. Maybe "zip -l" would be a better approach.

One nit:

+test_lazy_prereq UNZIP_AUTOTEXT '
+	(
+		mkdir unzip-autotext &&
+		cd unzip-autotext
+		"$GIT_UNZIP" -a "$TEST_DIRECTORY"/t5003/infozip-text.zip &&
+		test -f text
+	)
+'

I don't think we need the extra directory or the subshell here.
test_lazy_prereq takes care of that for us.

Couldn't apply this locally without --binary, of course. :)

-Peff