{"thread":{"id":"42870","subject":"[PATCH] test-lib: stricter unzip(1) check","startedAt":"2016-07-18T06:44:39Z","lastAt":"2016-07-19T17:27:22Z","messageCount":14,"participants":["Eric Wong","Jeff King","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"291643","messageId":"20160718064431.GA10819@starla","threadId":"42870","inReplyTo":null,"subject":"[PATCH] test-lib: stricter unzip(1) check","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-18T06:44:31Z","receivedAt":"2016-07-18T06:44:39Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip\nexists, but is insufficient for t5003 due to its non-standard\nhandling of the -a option[1]. This version of unzip exits\nwith \"1\" when given the \"-v\" flag.\n\nHowever, the common Info-ZIP version may be installed at\n/usr/local/bin/unzip (via \"pkg install unzip\") to pass t5003.\nThis Info-ZIP version exits with \"0\" when given \"-v\",\nso limit the prereq to only versions which return 0 on \"-v\".\n\n[1] https://www.freebsd.org/cgi/man.cgi?query=unzip&sektion=1&manpath=FreeBSD+10.3-RELEASE\n\nSigned-off-by: Eric Wong \n---\n t/test-lib.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 11201e9..938f788 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1103,7 +1103,7 @@ test_lazy_prereq SANITY '\n GIT_UNZIP=${GIT_UNZIP:-unzip}\n test_lazy_prereq UNZIP '\n \t\"$GIT_UNZIP\" -v\n-\ttest $? -ne 127\n+\ttest $? -eq 0\n '\n \n run_with_limited_cmdline () {\n-- \nEW\n"},{"id":"291649","messageId":"20160718130405.GA19751@sigill.intra.peff.net","threadId":"42870","inReplyTo":"20160718064431.GA10819@starla","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-18T13:04:05Z","receivedAt":"2016-07-18T13:04:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2016 at 06:44:31AM +0000, Eric Wong wrote:\n\n> On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip\n> exists, but is insufficient for t5003 due to its non-standard\n> handling of the -a option[1]. This version of unzip exits\n> with \"1\" when given the \"-v\" flag.\n> \n> However, the common Info-ZIP version may be installed at\n> /usr/local/bin/unzip (via \"pkg install unzip\") to pass t5003.\n> This Info-ZIP version exits with \"0\" when given \"-v\",\n> so limit the prereq to only versions which return 0 on \"-v\".\n\nI guess I am cc'd because of f838ce5 (test-lib: factor out $GIT_UNZIP\nsetup, 2013-03-10). But really this check for 127 dates all the way back\nto Johannes's 3a86f36 (t5000: skip ZIP tests if unzip was not found,\n2007-06-06), and was just pulled along as it got refactored into a\nvarious incarnations of prerequisite by me and René.\n\nIt's possible that there is some version of unzip that does not like\n\"-v\" but otherwise is OK with our tests, but we would skip tests using\nthis patch. Even with the FreeBSD version you mention, I'd expect you\ncould run all of the tests except for the single \"-a\" test.\n\nSo I wonder if we could more directly test the two levels we care about:\n\n - do you appear to have a working \"unzip\" at all?\n\n - does your unzip support \"-a\"?\n\nMy Debian version of unzip (which is derived from Info-zip) seems to\ngive return code 0 for just \"unzip\". So for the first check, we could\npossibly drop \"-v\"; we don't care about \"-v\", but just wanted some way\nto say \"does unzip exist on the path?\". Another option would just be\nchecking whether \"unzip\" returns something besides 127 (so what we have\nnow, minus \"-v\").\n\nTo test for \"-a\", I think we'd have to actually feed it a sample zip\nfile, though. My unzip returns \"10\", which its manpage explains as\n\"invalid command line options\" (presumably because of the missing\nzipfile argument). But that seems like it probably isn't portable. And\nit's also what I might expect another unzip to return if it doesn't\nsupport \"-a\".\n\nSo while this patch does solve the immediate problem, I think it does so\nby overly skipping tests that we _could_ run.\n\nIf we do go with this patch, though:\n\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 11201e9..938f788 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -1103,7 +1103,7 @@ test_lazy_prereq SANITY '\n> GIT_UNZIP=${GIT_UNZIP:-unzip}\n> test_lazy_prereq UNZIP '\n> \t\"$GIT_UNZIP\" -v\n> -\ttest $? -ne 127\n> +\ttest $? -eq 0\n> '\n\n...you can simply drop the \"test\" line, as testing $? against 0 is\nessentially a noop. If it is 0, then test will return 0; if it is not,\nthen test will return non-zero. You can just return the value directly\ninstead.\n\n-Peff\n"},{"id":"291651","messageId":"alpine.DEB.2.20.1607181536540.3472@virtualbox","threadId":"42870","inReplyTo":"20160718130405.GA19751@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-18T13:52:31Z","receivedAt":"2016-07-18T13:52:57Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Peff & Eric,\n\nOn Mon, 18 Jul 2016, Jeff King wrote:\n\n> On Mon, Jul 18, 2016 at 06:44:31AM +0000, Eric Wong wrote:\n> \n> > On FreeBSD 10.3 (but presumably any FreeBSD 8+), /usr/bin/unzip\n> > exists, but is insufficient for t5003 due to its non-standard\n> > handling of the -a option[1]. This version of unzip exits\n> > with \"1\" when given the \"-v\" flag.\n> > \n> > However, the common Info-ZIP version may be installed at\n> > /usr/local/bin/unzip (via \"pkg install unzip\") to pass t5003.\n> > This Info-ZIP version exits with \"0\" when given \"-v\",\n> > so limit the prereq to only versions which return 0 on \"-v\".\n\nHrm. That sounds a little magical, and fragile, to me. What if the next\nperson's unzip returns 0 and *still* cannot handle -a?\n\nI'd rather do something like\n\n-- snipsnap --\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 0055ebb..5b9521e 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -929,7 +929,8 @@ yes () {\n }\n \n # Fix some commands on Windows\n-case $(uname -s) in\n+uname_s=$(uname -s)\n+case $uname_s in\n *MINGW*)\n \t# Windows has its own (incompatible) sort and find\n \tsort () {\n@@ -1100,6 +1101,7 @@ test_lazy_prereq SANITY '\n \treturn $status\n '\n \n+test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}\n GIT_UNZIP=${GIT_UNZIP:-unzip}\n test_lazy_prereq UNZIP '\n \t\"$GIT_UNZIP\" -v\n"},{"id":"291663","messageId":"xmqqtwfmkduk.fsf@gitster.mtv.corp.google.com","threadId":"42870","inReplyTo":"alpine.DEB.2.20.1607181536540.3472@virtualbox","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T18:20:19Z","receivedAt":"2016-07-18T18:20:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Hrm. That sounds a little magical, and fragile, to me. What if the next\n> person's unzip returns 0 and *still* cannot handle -a?\n\nThat is a very sensible line of thought.\n\n> I'd rather do something like\n\n... but the patch presented as an alternative does not seem to\nfollow that line of thought. After reading that sensible line of\nthought I would have expected to see an auto-detection of the path\nand support for features we care about.\n\nStepping back a bit, why do we even care if \"unzip -a\" works on\n\"../$zipfile\" and converts things correctly in that check_zip() test\nin t5003 in the first place? It looks more like a test on \"unzip\"\nthan making sure we correctly generate a zip archive to me...\n\n> -- snipsnap --\n> diff --git a/t/test-lib.sh b/t/test-lib.sh\n> index 0055ebb..5b9521e 100644\n> --- a/t/test-lib.sh\n> +++ b/t/test-lib.sh\n> @@ -929,7 +929,8 @@ yes () {\n> }\n> \n> # Fix some commands on Windows\n> -case $(uname -s) in\n> +uname_s=$(uname -s)\n> +case $uname_s in\n> *MINGW*)\n> \t# Windows has its own (incompatible) sort and find\n> \tsort () {\n> @@ -1100,6 +1101,7 @@ test_lazy_prereq SANITY '\n> \treturn $status\n> '\n> \n> +test FreeBSD != $uname_s || GIT_UNZIP=${GIT_UNZIP:-/usr/local/bin/unzip}\n> GIT_UNZIP=${GIT_UNZIP:-unzip}\n> test_lazy_prereq UNZIP '\n> \t\"$GIT_UNZIP\" -v\n"},{"id":"291670","messageId":"20160718185634.GA24726@sigill.intra.peff.net","threadId":"42870","inReplyTo":"xmqqtwfmkduk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-18T18:56:35Z","receivedAt":"2016-07-18T18:56:42Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2016 at 11:20:19AM -0700, Junio C Hamano wrote:\n\n> Stepping back a bit, why do we even care if \"unzip -a\" works on\n> \"../$zipfile\" and converts things correctly in that check_zip() test\n> in t5003 in the first place? It looks more like a test on \"unzip\"\n> than making sure we correctly generate a zip archive to me...\n\nI think it is testing that we generated an archive with the correct \"I\nam text\" flags so that an unzip implementation can do the\nauto-conversion.\n\n-Peff\n"},{"id":"291674","messageId":"xmqqy44yiwmf.fsf@gitster.mtv.corp.google.com","threadId":"42870","inReplyTo":"20160718185634.GA24726@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T19:17:44Z","receivedAt":"2016-07-18T19:17:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Mon, Jul 18, 2016 at 11:20:19AM -0700, Junio C Hamano wrote:\n>\n>> Stepping back a bit, why do we even care if \"unzip -a\" works on\n>> \"../$zipfile\" and converts things correctly in that check_zip() test\n>> in t5003 in the first place? It looks more like a test on \"unzip\"\n>> than making sure we correctly generate a zip archive to me...\n>\n> I think it is testing that we generated an archive with the correct \"I\n> am text\" flags so that an unzip implementation can do the\n> auto-conversion.\n\nYes, I understand that. I was hoping for a response along the lines\nof \"we want to make sure we mark text as text, and 'zip -l' has an\noption to let us check the attributes without having to actually\nchecking things out\" ;-)\n\n\n"},{"id":"291676","messageId":"xmqqshv6ivfk.fsf@gitster.mtv.corp.google.com","threadId":"42870","inReplyTo":"20160718130405.GA19751@sigill.intra.peff.net","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T19:43:27Z","receivedAt":"2016-07-18T19:43:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> My Debian version of unzip (which is derived from Info-zip) seems to\n> give return code 0 for just \"unzip\". So for the first check, we could\n> possibly drop \"-v\"; we don't care about \"-v\", but just wanted some way\n> to say \"does unzip exist on the path?\". Another option would just be\n> checking whether \"unzip\" returns something besides 127 (so what we have\n> now, minus \"-v\").\n>\n> To test for \"-a\", I think we'd have to actually feed it a sample zip\n> file, though. My unzip returns \"10\", which its manpage explains as\n> \"invalid command line options\" (presumably because of the missing\n> zipfile argument). But that seems like it probably isn't portable. And\n> it's also what I might expect another unzip to return if it doesn't\n> support \"-a\".\n>\n> So while this patch does solve the immediate problem, I think it does so\n> by overly skipping tests that we _could_ run.\n\nHmm, how about taking Dscho's \"default GIT_UNZIP to /usr/local/bin/unzip\non FreeBSD\" thing, together with something like this, then?\n\nI suspect that 4 checks that look at $extracted/* after running\nunzip -a should probably be inside a single test that runs unzip -a,\nsimply because they do not make any sense if the extraction failed,\nbut I did not fix that with this.\n\n-- >8 --\ntest: check \"unzip\" and \"unzip -a\"\n\nDifferent platforms have implementations \"unzip\" that behave\ndifferently. Most of the tests we use GIT_UNZIP we only care about\nthe command to be able to extract from *.zip archive, but one test\nin t5003 wants it to also be able to grok the \"-a\" option.\n\nPrepare a sample zip file that has a single text file in it, and try\nextracting its contents to see GIT_UNZIP is usable. when setting\nUNZIP prerequisite. Similarly, set UNZIP_AUTOTEXT prerequisite by\nrunning GIT_UNZIP with the \"-a\" option.\n\n---\n t/t5003-archive-zip.sh | 19 ++++++++++++++-----\n t/t5003/infozip-text.zip | Bin 0 -> 163 bytes\n t/test-lib.sh | 4 ++--\n 3 files changed, 16 insertions(+), 7 deletions(-)\n\ndiff --git a/t/t5003-archive-zip.sh b/t/t5003-archive-zip.sh\nindex 14744b2..43c0cfd 100755\n--- a/t/t5003-archive-zip.sh\n+++ b/t/t5003-archive-zip.sh\n@@ -15,6 +15,15 @@ test_lazy_prereq UNZIP_SYMLINKS '\n \t)\n '\n \n+test_lazy_prereq UNZIP_AUTOTEXT '\n+\t(\n+\t\tmkdir unzip-autotext &&\n+\t\tcd unzip-autotext\n+\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-text.zip &&\n+\t\ttest -f text\n+\t)\n+'\n+\n check_zip() {\n \tzipfile=$1.zip\n \tlistfile=$1.lst\n@@ -39,27 +48,27 @@ check_zip() {\n \textracted=${dir_with_prefix}a\n \toriginal=a\n \n-\ttest_expect_success UNZIP \" extract ZIP archive with EOL conversion\" '\n+\ttest_expect_success UNZIP_AUTOTEXT \" extract ZIP archive with EOL conversion\" '\n \t\t(mkdir $dir && cd $dir && \"$GIT_UNZIP\" -a ../$zipfile)\n \t'\n \n-\ttest_expect_success UNZIP \" validate that text files are converted\" \"\n+\ttest_expect_success UNZIP_AUTOTEXT \" validate that text files are converted\" \"\n \t\ttest_cmp_bin $extracted/text.cr $extracted/text.crlf &&\n \t\ttest_cmp_bin $extracted/text.cr $extracted/text.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that binary files are unchanged\" \"\n+\ttest_expect_success UNZIP_AUTOTEXT \" validate that binary files are unchanged\" \"\n \t\ttest_cmp_bin $original/binary.cr $extracted/binary.cr &&\n \t\ttest_cmp_bin $original/binary.crlf $extracted/binary.crlf &&\n \t\ttest_cmp_bin $original/binary.lf $extracted/binary.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that diff files are converted\" \"\n+\ttest_expect_success UNZIP_AUTOTEXT \" validate that diff files are converted\" \"\n \t\ttest_cmp_bin $extracted/diff.cr $extracted/diff.crlf &&\n \t\ttest_cmp_bin $extracted/diff.cr $extracted/diff.lf\n \t\"\n \n-\ttest_expect_success UNZIP \" validate that -diff files are unchanged\" \"\n+\ttest_expect_success UNZIP_AUTOTEXT \" validate that -diff files are unchanged\" \"\n \t\ttest_cmp_bin $original/nodiff.cr $extracted/nodiff.cr &&\n \t\ttest_cmp_bin $original/nodiff.crlf $extracted/nodiff.crlf &&\n \t\ttest_cmp_bin $original/nodiff.lf $extracted/nodiff.lf\ndiff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip\nnew file mode 100644\nindex 0000000..a019acb\nBinary files /dev/null and b/t/t5003/infozip-text.zip differ\ndiff --git a/t/test-lib.sh b/t/test-lib.sh\nindex 11201e9..9907b3f 100644\n--- a/t/test-lib.sh\n+++ b/t/test-lib.sh\n@@ -1102,8 +1102,8 @@ test_lazy_prereq SANITY '\n \n GIT_UNZIP=${GIT_UNZIP:-unzip}\n test_lazy_prereq UNZIP '\n-\t\"$GIT_UNZIP\" -v\n-\ttest $? -ne 127\n+\t\"$GIT_UNZIP\" \"$TEST_DIRECTORY/t5003/infozip-text.zip\" &&\n+\ttest -f text\n '\n \n run_with_limited_cmdline () {\n"},{"id":"291679","messageId":"20160718200351.GA61232@plume","threadId":"42870","inReplyTo":"xmqqshv6ivfk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Eric Wong","fromEmail":"e@80x24.org","sentAt":"2016-07-18T20:03:51Z","receivedAt":"2016-07-18T20:03:56Z","isPatch":true,"sender":{"key":"e@80x24.org","avatar":null},"body":"Junio C Hamano wrote:\n> +test_lazy_prereq UNZIP_AUTOTEXT '\n> +\t(\n> +\t\tmkdir unzip-autotext &&\n> +\t\tcd unzip-autotext\n> +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-text.zip &&\n> +\t\ttest -f text\n> +\t)\n\n/usr/bin/unzip actually takes -a on FreeBSD, just not in the\nsame way the Info-ZIP version does, so I suspect \"test -f\"\nhere is not enough.\n\nI would test this, but I can't apply it:\n\n> diff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip\n> new file mode 100644\n> index 0000000..a019acb\n> Binary files /dev/null and b/t/t5003/infozip-text.zip differ\n"},{"id":"291681","messageId":"xmqqk2giitrn.fsf@gitster.mtv.corp.google.com","threadId":"42870","inReplyTo":"20160718200351.GA61232@plume","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T20:19:24Z","receivedAt":"2016-07-18T20:19:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Wong writes:\n\n> Junio C Hamano wrote:\n>> +test_lazy_prereq UNZIP_AUTOTEXT '\n>> +\t(\n>> +\t\tmkdir unzip-autotext &&\n>> +\t\tcd unzip-autotext\n>> +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-text.zip &&\n>> +\t\ttest -f text\n>> +\t)\n>\n> /usr/bin/unzip actually takes -a on FreeBSD, just not in the\n> same way the Info-ZIP version does, so I suspect \"test -f\"\n> here is not enough.\n\nHmph. So it only and always does \"CRLF -> LF\", while Info-ZIP\nversion does something like autocrlf?\n\n> I would test this, but I can't apply it:\n>\n>> diff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip\n>> new file mode 100644\n>> index 0000000..a019acb\n>> Binary files /dev/null and b/t/t5003/infozip-text.zip differ\n\nHeh. It was created like so:\n\n\t$ printf 'text\\r\\n' >text && zip -ll infozip-text.zip text\n\t$ zipinfo infozip-text.zip text\n -rw-r----- 3.0 unx 5 tx stor 16-Jul-18 13:12 text\n\n t/t5003/infozip-text.zip | Bin 0 -> 163 bytes\n 1 file changed, 0 insertions(+), 0 deletions(-)\n\ndiff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip\nnew file mode 100644\nindex 0000000000000000000000000000000000000000..7119bbb62699f4cb613f3675f57aa9c9dc021ea0\nGIT binary patch\nliteral 163\nzcmWIWW@h1H0D&2qpFGrWy)kD6vO$=IL586uwW1_6gp+~U-l8`ggi9;985mjSu4iOm\nz=@4cB%X0;IGcw6B<1$17WHtjM5HDy1u^>jWLX1Q+F2I|W4Wxz<2)%%`Gl;_g0G&b|\nAeE wrote:\n> Eric Wong writes:\n> > Junio C Hamano wrote:\n> >> +test_lazy_prereq UNZIP_AUTOTEXT '\n> >> +\t(\n> >> +\t\tmkdir unzip-autotext &&\n> >> +\t\tcd unzip-autotext\n> >> +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-text.zip &&\n> >> +\t\ttest -f text\n> >> +\t)\n> >\n> > /usr/bin/unzip actually takes -a on FreeBSD, just not in the\n> > same way the Info-ZIP version does, so I suspect \"test -f\"\n> > here is not enough.\n> \n> Hmph. So it only and always does \"CRLF -> LF\", while Info-ZIP\n> version does something like autocrlf?\n\nNo, it does CRLF -> LF based on the absence of non-ASCII\ncharacters and ignoring metadata set in the zipfile.\nThe unzip manpage states:\n\n Normally, the -a option should only affect files which are\n marked as text files in the zipfile's central directory. Since\n the archive(3) library reads zipfiles sequentially, and does\n not use the central directory, that information is not available\n to the unzip utility. Instead, the unzip utility will assume\n that a file is a text file if no non-ASCII characters are\n present within the first block of data decompressed for that\n file. If non-ASCII characters appear in subsequent blocks of\n data, a warning will be issued.\n\nhttps://www.freebsd.org/cgi/man.cgi?query=unzip&sektion=1&manpath=FreeBSD+10.3-RELEASE\n\n> Heh. It was created like so:\n> \n> \t$ printf 'text\\r\\n' >text && zip -ll infozip-text.zip text\n> \t$ zipinfo infozip-text.zip text\n> -rw-r----- 3.0 unx 5 tx stor 16-Jul-18 13:12 text\n> \n\nThanks, but I think the test file is too small. I tried\nsetting up a test to store the text file as binary in the\nzip to check for inadvertant CRLF conversions:\n\n printf 'text\\r\\n' >binary && zip -B infozip-binary.zip binary\n\nBut zip -B/--binary only works on VM/CMS and MVS...\n\nSo I'm inclined to go with Dscho's patch.\n\n\n(apologies for messing up René's name in my previous email;\n on my new FreeBSD setup: mutt displays it fine, as does vim when\n taking it from .mailmap, but something gets lost when mutt\n populates the file for vim. Perhaps some Debian patch didn't\n make it upstream...)\n"},{"id":"291688","messageId":"CAPc5daWnfA7C23Qa07Y-aYwWBebdn33qZMo4Sj+Evm3-fVUDcg@mail.gmail.com","threadId":"42870","inReplyTo":"20160718211951.GA62657@plume","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-18T21:27:06Z","receivedAt":"2016-07-18T21:27:30Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"On Mon, Jul 18, 2016 at 2:19 PM, Eric Wong wrote:\n\n> Thanks, but I think the test file is too small. I tried\n> setting up a test to store the text file as binary in the\n> zip to check for inadvertant CRLF conversions:\n>\n> printf 'text\\r\\n' >binary && zip -B infozip-binary.zip binary\n>\n> But zip -B/--binary only works on VM/CMS and MVS...\n>\n> So I'm inclined to go with Dscho's patch.\n\nOK, I'll wait for the final reroll and queue it near 'next' when it happens.\n\nThanks.\n"},{"id":"291698","messageId":"20160718234121.GA26152@sigill.intra.peff.net","threadId":"42870","inReplyTo":"xmqqshv6ivfk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-07-18T23:41:21Z","receivedAt":"2016-07-18T23:42:14Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 18, 2016 at 12:43:27PM -0700, Junio C Hamano wrote:\n\n> -- >8 --\n> test: check \"unzip\" and \"unzip -a\"\n> \n> Different platforms have implementations \"unzip\" that behave\n> differently. Most of the tests we use GIT_UNZIP we only care about\n> the command to be able to extract from *.zip archive, but one test\n> in t5003 wants it to also be able to grok the \"-a\" option.\n> \n> Prepare a sample zip file that has a single text file in it, and try\n> extracting its contents to see GIT_UNZIP is usable. when setting\n> UNZIP prerequisite. Similarly, set UNZIP_AUTOTEXT prerequisite by\n> running GIT_UNZIP with the \"-a\" option.\n\nI like the direction here, modulo the problems with \"-a\" that Eric\npointed out. Maybe \"zip -l\" would be a better approach.\n\nOne nit:\n\n> +test_lazy_prereq UNZIP_AUTOTEXT '\n> +\t(\n> +\t\tmkdir unzip-autotext &&\n> +\t\tcd unzip-autotext\n> +\t\t\"$GIT_UNZIP\" -a \"$TEST_DIRECTORY\"/t5003/infozip-text.zip &&\n> +\t\ttest -f text\n> +\t)\n> +'\n\nI don't think we need the extra directory or the subshell here.\ntest_lazy_prereq takes care of that for us.\n\n> diff --git a/t/t5003/infozip-text.zip b/t/t5003/infozip-text.zip\n> new file mode 100644\n> index 0000000..a019acb\n> Binary files /dev/null and b/t/t5003/infozip-text.zip differ\n\nCouldn't apply this locally without --binary, of course. :)\n\n-Peff\n"},{"id":"291715","messageId":"alpine.DEB.2.20.1607191327260.3472@virtualbox","threadId":"42870","inReplyTo":"xmqqtwfmkduk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-19T11:27:30Z","receivedAt":"2016-07-19T11:27:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 18 Jul 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Hrm. That sounds a little magical, and fragile, to me. What if the next\n> > person's unzip returns 0 and *still* cannot handle -a?\n> \n> That is a very sensible line of thought.\n> \n> > I'd rather do something like\n> \n> ... but the patch presented as an alternative does not seem to\n> follow that line of thought.\n\nRight. I tried to see whether I could come up with a test, but did not\nimmediately succeed. The patch I presented was the best I could do...\n\nCiao,\nDscho\n"},{"id":"291733","messageId":"xmqqfur5h730.fsf@gitster.mtv.corp.google.com","threadId":"42870","inReplyTo":"alpine.DEB.2.20.1607191327260.3472@virtualbox","subject":"Re: [PATCH] test-lib: stricter unzip(1) check","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-19T17:26:59Z","receivedAt":"2016-07-19T17:27:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> On Mon, 18 Jul 2016, Junio C Hamano wrote:\n>\n>> Johannes Schindelin writes:\n>> \n>> > Hrm. That sounds a little magical, and fragile, to me. What if the next\n>> > person's unzip returns 0 and *still* cannot handle -a?\n>> \n>> That is a very sensible line of thought.\n>> \n>> > I'd rather do something like\n>> \n>> ... but the patch presented as an alternative does not seem to\n>> follow that line of thought.\n>\n> Right. I tried to see whether I could come up with a test, but did not\n> immediately succeed. The patch I presented was the best I could do...\n\nAh, OK, it makes sense.\n"}]}