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

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

[Eric Wong]

On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip
exists, but is insufficient for t5003 due to its non-standard
handling of the -a option[1].  This version of unzip exits
with "1" when given the "-v" flag.

However, the common Info-ZIP version may be installed at
/usr/local/bin/unzip (via "pkg install unzip") to pass t5003.
This Info-ZIP version exits with "0" when given "-v",
so limit the prereq to only versions which return 0 on "-v".

[1] https://www.freebsd.org/cgi/man.cgi?query=unzip&sektion=1&manpath=FreeBSD+10.3-RELEASE

Signed-off-by: Eric Wong <e@80x24.org>
---
 t/test-lib.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 11201e9..938f788 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -1103,7 +1103,7 @@ test_lazy_prereq SANITY '
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 test_lazy_prereq UNZIP '
 	"$GIT_UNZIP" -v
-	test $? -ne 127
+	test $? -eq 0
 '
 
 run_with_limited_cmdline () {
-- 
EW

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

[Jeff King]

On Mon, Jul 18, 2016 at 06:44:31AM +0000, Eric Wong wrote:

> On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip
> exists, but is insufficient for t5003 due to its non-standard
> handling of the -a option[1].  This version of unzip exits
> with "1" when given the "-v" flag.
> 
> However, the common Info-ZIP version may be installed at
> /usr/local/bin/unzip (via "pkg install unzip") to pass t5003.
> This Info-ZIP version exits with "0" when given "-v",
> so limit the prereq to only versions which return 0 on "-v".

I guess I am cc'd because of f838ce5 (test-lib: factor out $GIT_UNZIP
setup, 2013-03-10). But really this check for 127 dates all the way back
to Johannes's 3a86f36 (t5000: skip ZIP tests if unzip was not found,
2007-06-06), and was just pulled along as it got refactored into a
various incarnations of prerequisite by me and René.

It's possible that there is some version of unzip that does not like
"-v" but otherwise is OK with our tests, but we would skip tests using
this patch. Even with the FreeBSD version you mention, I'd expect you
could run all of the tests except for the single "-a" test.

So I wonder if we could more directly test the two levels we care about:

  - do you appear to have a working "unzip" at all?

  - does your unzip support "-a"?

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.

If we do go with this patch, though:

> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 11201e9..938f788 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -1103,7 +1103,7 @@ test_lazy_prereq SANITY '
>  GIT_UNZIP=${GIT_UNZIP:-unzip}
>  test_lazy_prereq UNZIP '
>  	"$GIT_UNZIP" -v
> -	test $? -ne 127
> +	test $? -eq 0
>  '

...you can simply drop the "test" line, as testing $? against 0 is
essentially a noop. If it is 0, then test will return 0; if it is not,
then test will return non-zero. You can just return the value directly
instead.

-Peff

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

[Johannes Schindelin]

Hi Peff & Eric,

On Mon, 18 Jul 2016, Jeff King wrote:

> On Mon, Jul 18, 2016 at 06:44:31AM +0000, Eric Wong wrote:
> 
> > On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip
> > exists, but is insufficient for t5003 due to its non-standard
> > handling of the -a option[1].  This version of unzip exits
> > with "1" when given the "-v" flag.
> > 
> > However, the common Info-ZIP version may be installed at
> > /usr/local/bin/unzip (via "pkg install unzip") to pass t5003.
> > This Info-ZIP version exits with "0" when given "-v",
> > so limit the prereq to only versions which return 0 on "-v".

Hrm. That sounds a little magical, and fragile, to me. What if the next
person's unzip returns 0 and *still* cannot handle -a?

I'd rather do something like

-- snipsnap --
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 0055ebb..5b9521e 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -929,7 +929,8 @@ yes () {
 }
 
 # Fix some commands on Windows
-case $(uname -s) in
+uname_s=$(uname -s)
+case $uname_s in
 *MINGW*)
 	# Windows has its own (incompatible) sort and find
 	sort () {
@@ -1100,6 +1101,7 @@ test_lazy_prereq SANITY '
 	return $status
 '
 
+test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
 GIT_UNZIP=${GIT_UNZIP:-unzip}
 test_lazy_prereq UNZIP '
 	"$GIT_UNZIP" -v

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Hrm. That sounds a little magical, and fragile, to me. What if the next
> person's unzip returns 0 and *still* cannot handle -a?

That is a very sensible line of thought.

> I'd rather do something like

... but the patch presented as an alternative does not seem to
follow that line of thought.  After reading that sensible line of
thought I would have expected to see an auto-detection of the path
and support for features we care about.

Stepping back a bit, why do we even care if "unzip -a" works on
"../$zipfile" and converts things correctly in that check_zip() test
in t5003 in the first place?  It looks more like a test on "unzip"
than making sure we correctly generate a zip archive to me...

> -- snipsnap --
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 0055ebb..5b9521e 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -929,7 +929,8 @@ yes () {
>  }
>  
>  # Fix some commands on Windows
> -case $(uname -s) in
> +uname_s=$(uname -s)
> +case $uname_s in
>  *MINGW*)
>  	# Windows has its own (incompatible) sort and find
>  	sort () {
> @@ -1100,6 +1101,7 @@ test_lazy_prereq SANITY '
>  	return $status
>  '
>  
> +test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}
>  GIT_UNZIP=${GIT_UNZIP:-unzip}
>  test_lazy_prereq UNZIP '
>  	"$GIT_UNZIP" -v

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

[Jeff King]

On Mon, Jul 18, 2016 at 11:20:19AM -0700, Junio C Hamano wrote:

> Stepping back a bit, why do we even care if "unzip -a" works on
> "../$zipfile" and converts things correctly in that check_zip() test
> in t5003 in the first place?  It looks more like a test on "unzip"
> than making sure we correctly generate a zip archive to me...

I think it is testing that we generated an archive with the correct "I
am text" flags so that an unzip implementation can do the
auto-conversion.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Jul 18, 2016 at 11:20:19AM -0700, Junio C Hamano wrote:
>
>> Stepping back a bit, why do we even care if "unzip -a" works on
>> "../$zipfile" and converts things correctly in that check_zip() test
>> in t5003 in the first place?  It looks more like a test on "unzip"
>> than making sure we correctly generate a zip archive to me...
>
> I think it is testing that we generated an archive with the correct "I
> am text" flags so that an unzip implementation can do the
> auto-conversion.

Yes, I understand that.  I was hoping for a response along the lines
of "we want to make sure we mark text as text, and 'zip -l' has an
option to let us check the attributes without having to actually
checking things out" ;-)

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

[Johannes Schindelin]

Hi Junio,

On Mon, 18 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Hrm. That sounds a little magical, and fragile, to me. What if the next
> > person's unzip returns 0 and *still* cannot handle -a?
> 
> That is a very sensible line of thought.
> 
> > I'd rather do something like
> 
> ... but the patch presented as an alternative does not seem to
> follow that line of thought.

Right. I tried to see whether I could come up with a test, but did not
immediately succeed. The patch I presented was the best I could do...

Ciao,
Dscho

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

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Mon, 18 Jul 2016, Junio C Hamano wrote:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>> 
>> > Hrm. That sounds a little magical, and fragile, to me. What if the next
>> > person's unzip returns 0 and *still* cannot handle -a?
>> 
>> That is a very sensible line of thought.
>> 
>> > I'd rather do something like
>> 
>> ... but the patch presented as an alternative does not seem to
>> follow that line of thought.
>
> Right. I tried to see whether I could come up with a test, but did not
> immediately succeed. The patch I presented was the best I could do...

Ah, OK, it makes sense.

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

[Junio C Hamano]

Jeff King <peff@peff.net> 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 () {

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> 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:

> 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

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

[Junio C Hamano]

Eric Wong <e@80x24.org> writes:

> Junio C Hamano <gitster@pobox.com> 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.

Hmph.  So it only and always does "CRLF -> LF", while Info-ZIP
version does something like autocrlf?

> I would test this, but I can't apply it:
>
>> 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

Heh.  It was created like so:

	$ printf 'text\r\n' >text && zip -ll infozip-text.zip text
	$ zipinfo infozip-text.zip text
        -rw-r-----  3.0 unx        5 tx stor 16-Jul-18 13:12 text

 t/t5003/infozip-text.zip | Bin 0 -> 163 bytes
 1 file changed, 0 insertions(+), 0 deletions(-)

diff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip
new file mode 100644
index 0000000000000000000000000000000000000000..7119bbb62699f4cb613f3675f57aa9c9dc021ea0
GIT binary patch
literal 163
zcmWIWW@h1H0D&2qpFGrWy)kD6vO$=IL586uwW1_6gp+~U-l8`ggi9;985mjSu4iOm
z=@4cB%X0;IGcw6B<1$17WHtjM5HDy1u^>jWLX1Q+F2I|W4Wxz<2)%%`Gl;_g0G&b|
AeE<Le

literal 0
HcmV?d00001

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

[Eric Wong]

Junio C Hamano <gitster@pobox.com> wrote:
> Eric Wong <e@80x24.org> writes:
> > Junio C Hamano <gitster@pobox.com> 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.
> 
> Hmph.  So it only and always does "CRLF -> LF", while Info-ZIP
> version does something like autocrlf?

No, it does CRLF -> LF based on the absence of non-ASCII
characters and ignoring metadata set in the zipfile.
The unzip manpage states:

     Normally, the -a option should only affect files which are
     marked as text files in the zipfile's central directory.  Since
     the archive(3) library reads zipfiles sequentially, and does
     not use the central directory, that information is not available
     to the unzip utility.  Instead, the unzip utility will assume
     that a file is a text file if no non-ASCII characters are
     present within the first block of data decompressed for that
     file.  If non-ASCII characters appear in subsequent blocks of
     data, a warning will be issued.

https://www.freebsd.org/cgi/man.cgi?query=unzip&sektion=1&manpath=FreeBSD+10.3-RELEASE

> Heh.  It was created like so:
> 
> 	$ printf 'text\r\n' >text && zip -ll infozip-text.zip text
> 	$ zipinfo infozip-text.zip text
>         -rw-r-----  3.0 unx        5 tx stor 16-Jul-18 13:12 text
> 

Thanks, but I think the test file is too small.  I tried
setting up a test to store the text file as binary in the
zip to check for inadvertant CRLF conversions:

  printf 'text\r\n' >binary && zip -B infozip-binary.zip binary

But zip -B/--binary only works on VM/CMS and MVS...

So I'm inclined to go with Dscho's patch.


(apologies for messing up René's name in my previous email;
 on my new FreeBSD setup: mutt displays it fine, as does vim when
 taking it from .mailmap, but something gets lost when mutt
 populates the file for vim.  Perhaps some Debian patch didn't
 make it upstream...)

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

[Junio C Hamano]

On Mon, Jul 18, 2016 at 2:19 PM, Eric Wong <e@80x24.org> wrote:

> Thanks, but I think the test file is too small.  I tried
> setting up a test to store the text file as binary in the
> zip to check for inadvertant CRLF conversions:
>
>   printf 'text\r\n' >binary && zip -B infozip-binary.zip binary
>
> But zip -B/--binary only works on VM/CMS and MVS...
>
> So I'm inclined to go with Dscho's patch.

OK, I'll wait for the final reroll and queue it near 'next' when it happens.

Thanks.

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

[Jeff King]

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.

> 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

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

-Peff