{"thread":{"id":"35460","subject":"[PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","startedAt":"2013-12-03T08:57:34Z","lastAt":"2013-12-04T22:07:08Z","messageCount":7,"participants":["Christian Hesse","Eric Sunshine","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"231415","messageId":"1386061054-30796-1-git-send-email-mail@eworm.de","threadId":"35460","inReplyTo":null,"subject":"[PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Christian Hesse","fromEmail":"mail@eworm.de","sentAt":"2013-12-03T08:57:34Z","receivedAt":"2013-12-03T08:57:34Z","isPatch":true,"sender":{"key":"mail@eworm.de","avatar":"https://avatars.githubusercontent.com/u/970853?v=4"},"body":"In t/t5000-tar-tree.sh the variable GZIP is used for the command name.\nFrom man gzip:\n\n> The environment variable GZIP can hold a set of default options for\n> gzip. These options are interpreted first and can be overwritten by\n> explicit command line parameters.\n\nSo using any other variable name fixes this.\n---\n t/t5000-tar-tree.sh | 20 ++++++++++----------\n 1 file changed, 10 insertions(+), 10 deletions(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex c2023b1..01b0ed9 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -25,7 +25,7 @@ commit id embedding:\n '\n \n . ./test-lib.sh\n-GZIP=${GZIP:-gzip}\n+GZIPCMD=${GZIPCMD:-gzip}\n GUNZIP=${GUNZIP:-gzip -d}\n \n SUBSTFORMAT=%H%n\n@@ -275,27 +275,27 @@ test_expect_success 'only enabled filters are available remotely' '\n \ttest_cmp remote.bar config.bar\n '\n \n-if $GZIP --version >/dev/null 2>&1; then\n-\ttest_set_prereq GZIP\n+if $GZIPCMD --version >/dev/null 2>&1; then\n+\ttest_set_prereq GZIPCMD\n else\n \tsay \"Skipping some tar.gz tests because gzip not found\"\n fi\n \n-test_expect_success GZIP 'git archive --format=tgz' '\n+test_expect_success GZIPCMD 'git archive --format=tgz' '\n \tgit archive --format=tgz HEAD >j.tgz\n '\n \n-test_expect_success GZIP 'git archive --format=tar.gz' '\n+test_expect_success GZIPCMD 'git archive --format=tar.gz' '\n \tgit archive --format=tar.gz HEAD >j1.tar.gz &&\n \ttest_cmp j.tgz j1.tar.gz\n '\n \n-test_expect_success GZIP 'infer tgz from .tgz filename' '\n+test_expect_success GZIPCMD 'infer tgz from .tgz filename' '\n \tgit archive --output=j2.tgz HEAD &&\n \ttest_cmp j.tgz j2.tgz\n '\n \n-test_expect_success GZIP 'infer tgz from .tar.gz filename' '\n+test_expect_success GZIPCMD 'infer tgz from .tar.gz filename' '\n \tgit archive --output=j3.tar.gz HEAD &&\n \ttest_cmp j.tgz j3.tar.gz\n '\n@@ -306,17 +306,17 @@ else\n \tsay \"Skipping some tar.gz tests because gunzip was not found\"\n fi\n \n-test_expect_success GZIP,GUNZIP 'extract tgz file' '\n+test_expect_success GZIPCMD,GUNZIP 'extract tgz file' '\n \t$GUNZIP -c <j.tgz >j.tar &&\n \ttest_cmp b.tar j.tar\n '\n \n-test_expect_success GZIP 'remote tar.gz is allowed by default' '\n+test_expect_success GZIPCMD 'remote tar.gz is allowed by default' '\n \tgit archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n \ttest_cmp j.tgz remote.tar.gz\n '\n \n-test_expect_success GZIP 'remote tar.gz can be disabled' '\n+test_expect_success GZIPCMD 'remote tar.gz can be disabled' '\n \tgit config tar.tar.gz.remote false &&\n \ttest_must_fail git archive --remote=. --format=tar.gz HEAD \\\n \t\t>remote.tar.gz\n-- \n1.8.5\n"},{"id":"231417","messageId":"CAPig+cQqKQdVEojYF+-+ZE2hQjxsH4WrgPymj8g7P6pSQzfVpw@mail.gmail.com","threadId":"35460","inReplyTo":"1386061054-30796-1-git-send-email-mail@eworm.de","subject":"Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2013-12-03T09:49:06Z","receivedAt":"2013-12-03T09:49:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"[cc'ing Peff, the author of these tests]\n\nOn Tue, Dec 3, 2013 at 3:57 AM, Christian Hesse <mail@eworm.de> wrote:\n> In t/t5000-tar-tree.sh the variable GZIP is used for the command name.\n> From man gzip:\n>\n>> The environment variable GZIP can hold a set of default options for\n>> gzip. These options are interpreted first and can be overwritten by\n>> explicit command line parameters.\n>\n> So using any other variable name fixes this.\n\nMissing Signed-off-by: <you>\n\n> ---\n>  t/t5000-tar-tree.sh | 20 ++++++++++----------\n>  1 file changed, 10 insertions(+), 10 deletions(-)\n>\n> diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\n> index c2023b1..01b0ed9 100755\n> --- a/t/t5000-tar-tree.sh\n> +++ b/t/t5000-tar-tree.sh\n> @@ -25,7 +25,7 @@ commit id embedding:\n>  '\n>\n>  . ./test-lib.sh\n> -GZIP=${GZIP:-gzip}\n> +GZIPCMD=${GZIPCMD:-gzip}\n>  GUNZIP=${GUNZIP:-gzip -d}\n>\n>  SUBSTFORMAT=%H%n\n> @@ -275,27 +275,27 @@ test_expect_success 'only enabled filters are available remotely' '\n>         test_cmp remote.bar config.bar\n>  '\n>\n> -if $GZIP --version >/dev/null 2>&1; then\n> -       test_set_prereq GZIP\n> +if $GZIPCMD --version >/dev/null 2>&1; then\n> +       test_set_prereq GZIPCMD\n\ntest_set_prereq is not actually operating on an environment variable.\nIts argument is just a generic tag, which is uppercase by convention,\nbut not otherwise related to a variable which may share the same name,\nand which does not pollute the environment. Consequently, it should\nnot be necessary to rename the argument to test_set_prereq, thus all\nchanges following this one become superfluous (since they are checking\nfor presence of tag GZIP, not referencing environment variable GZIP or\nGZIPCMD). Thus, the patch becomes much smaller.\n\nIn fact, the GZIP command does not appear to be used at all by the\ntests, so a simpler solution might be to remove the variable\naltogether, and perhaps the prerequisite. Peff?\n\n>  else\n>         say \"Skipping some tar.gz tests because gzip not found\"\n>  fi\n>\n> -test_expect_success GZIP 'git archive --format=tgz' '\n> +test_expect_success GZIPCMD 'git archive --format=tgz' '\n>         git archive --format=tgz HEAD >j.tgz\n>  '\n>\n> -test_expect_success GZIP 'git archive --format=tar.gz' '\n> +test_expect_success GZIPCMD 'git archive --format=tar.gz' '\n>         git archive --format=tar.gz HEAD >j1.tar.gz &&\n>         test_cmp j.tgz j1.tar.gz\n>  '\n>\n> -test_expect_success GZIP 'infer tgz from .tgz filename' '\n> +test_expect_success GZIPCMD 'infer tgz from .tgz filename' '\n>         git archive --output=j2.tgz HEAD &&\n>         test_cmp j.tgz j2.tgz\n>  '\n>\n> -test_expect_success GZIP 'infer tgz from .tar.gz filename' '\n> +test_expect_success GZIPCMD 'infer tgz from .tar.gz filename' '\n>         git archive --output=j3.tar.gz HEAD &&\n>         test_cmp j.tgz j3.tar.gz\n>  '\n> @@ -306,17 +306,17 @@ else\n>         say \"Skipping some tar.gz tests because gunzip was not found\"\n>  fi\n>\n> -test_expect_success GZIP,GUNZIP 'extract tgz file' '\n> +test_expect_success GZIPCMD,GUNZIP 'extract tgz file' '\n>         $GUNZIP -c <j.tgz >j.tar &&\n>         test_cmp b.tar j.tar\n>  '\n>\n> -test_expect_success GZIP 'remote tar.gz is allowed by default' '\n> +test_expect_success GZIPCMD 'remote tar.gz is allowed by default' '\n>         git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&\n>         test_cmp j.tgz remote.tar.gz\n>  '\n>\n> -test_expect_success GZIP 'remote tar.gz can be disabled' '\n> +test_expect_success GZIPCMD 'remote tar.gz can be disabled' '\n>         git config tar.tar.gz.remote false &&\n>         test_must_fail git archive --remote=. --format=tar.gz HEAD \\\n>                 >remote.tar.gz\n> --\n> 1.8.5\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at  http://vger.kernel.org/majordomo-info.html\n"},{"id":"231426","messageId":"20131203131812.GC26667@sigill.intra.peff.net","threadId":"35460","inReplyTo":"CAPig+cQqKQdVEojYF+-+ZE2hQjxsH4WrgPymj8g7P6pSQzfVpw@mail.gmail.com","subject":"Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-03T13:18:12Z","receivedAt":"2013-12-03T13:18:12Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 03, 2013 at 04:49:06AM -0500, Eric Sunshine wrote:\n\n> > -if $GZIP --version >/dev/null 2>&1; then\n> > -       test_set_prereq GZIP\n> > +if $GZIPCMD --version >/dev/null 2>&1; then\n> > +       test_set_prereq GZIPCMD\n> \n> test_set_prereq is not actually operating on an environment variable.\n> Its argument is just a generic tag, which is uppercase by convention,\n> but not otherwise related to a variable which may share the same name,\n> and which does not pollute the environment. Consequently, it should\n> not be necessary to rename the argument to test_set_prereq, thus all\n> changes following this one become superfluous (since they are checking\n> for presence of tag GZIP, not referencing environment variable GZIP or\n> GZIPCMD). Thus, the patch becomes much smaller.\n\nRight. We can get away with just changing the environment variable, and\nleaving the prereq.\n\nBy the way, we had the exact same problem with $UNZIP, fixed in ac00128\n(t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead, 2013-01-06).\nI'd probably call the new variable GIT_GZIP for consistency, but...\n\n> In fact, the GZIP command does not appear to be used at all by the\n> tests, so a simpler solution might be to remove the variable\n> altogether, and perhaps the prerequisite. Peff?\n\nYes, though it's a bit more subtle than that. The gzip tests are relying\non git's internally-configured \"tar.tgz.command\" filter, which is\nhard-coded to \"gzip -cn\". So we do depend on having a working gzip, but\nwe do _not_ depend on the one found in the $GZIP variable. It must be\ncalled \"gzip\".\n\nThere are a few options I see:\n\n  1. Drop $GZIP variable, and hard-code the prerequisite check to\n     \"gzip\", which is what is being tested.\n\n  2. Keep $GZIP (but rename it to $GIT_GZIP), and explicitly set up\n     tar.tgz.command as \"$GIT_GZIP -cn\".\n\n  3. Teach the Makefile a knob to set the value for \"gzip\" at compile\n     time, and use that for the baked-in config (and propagate it to the\n     test to check the prerequisite).\n\nI think I'd be in favor of (1). It's the simplest, and we have not seen\nany reports of people who do not actually have gzip called \"gzip\". Users\ncan still override it via config if they really want to.\n\n-Peff\n"},{"id":"231427","messageId":"20131203132140.GA31722@sigill.intra.peff.net","threadId":"35460","inReplyTo":"20131203131812.GC26667@sigill.intra.peff.net","subject":"[PATCH] t5000: simplify gzip prerequisite checks","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-03T13:21:40Z","receivedAt":"2013-12-03T13:21:40Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"In t5000, we test the built-in \".tar.gz\" config for\ngit-archive. To make our tests portable, we check that we\nhave a way to both gzip and gunzip, and we respected\nenvironment variables to point to alternate commands for\ndoing these operations.\n\nHowever, the $GZIP variable did not actually do anything, as\nchanging it would not affect the baked-in value in\narchive-tar.c. Moreover, setting the variable $GZIP\ninfluences gzip itself. From the gzip man page:\n\n  The environment variable GZIP can hold a set of default\n  options for gzip. These options are interpreted first and\n  can be overwritten by explicit command line parameters.\n\nWe could rename this variable, and use it to set up custom\nconfig (or even have a Makefile knob to affect the built\nbinary), but it is not worth the trouble; nobody has ever\nreported a problem with the baked-in default, and they can\nalways change it via config if they need to. Let's just drop\nthe variable and use \"gzip\" in the test (keeping the\nprerequisite, of course).\n\nWhile we're at it, we can drop the GUNZIP variable and\nprerequisite; it uses \"gzip -d\", so if we have GZIP, we\nwill have both.\n\nWe can also use test_lazy_prereq for the gzip prerequisite,\nwhich is simpler and behaves more consistently with the rest\nof git (e.g., by making output available when the test is\nrun with \"-v\").\n\nNoticed-by: Christian Hesse <mail@eworm.de>\nSigned-off-by: Jeff King <peff@peff.net>\n---\n t/t5000-tar-tree.sh | 20 ++++----------------\n 1 file changed, 4 insertions(+), 16 deletions(-)\n\ndiff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh\nindex c2023b1..519cc34 100755\n--- a/t/t5000-tar-tree.sh\n+++ b/t/t5000-tar-tree.sh\n@@ -25,8 +25,6 @@ commit id embedding:\n '\n \n . ./test-lib.sh\n-GZIP=${GZIP:-gzip}\n-GUNZIP=${GUNZIP:-gzip -d}\n \n SUBSTFORMAT=%H%n\n \n@@ -39,6 +37,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '\n \t)\n '\n \n+test_lazy_prereq GZIP 'gzip --version'\n+\n get_pax_header() {\n \tfile=$1\n \theader=$2=\n@@ -275,12 +275,6 @@ test_expect_success 'only enabled filters are available remotely' '\n \ttest_cmp remote.bar config.bar\n '\n \n-if $GZIP --version >/dev/null 2>&1; then\n-\ttest_set_prereq GZIP\n-else\n-\tsay \"Skipping some tar.gz tests because gzip not found\"\n-fi\n-\n test_expect_success GZIP 'git archive --format=tgz' '\n \tgit archive --format=tgz HEAD >j.tgz\n '\n@@ -300,14 +294,8 @@ test_expect_success GZIP 'infer tgz from .tar.gz filename' '\n \ttest_cmp j.tgz j3.tar.gz\n '\n \n-if $GUNZIP --version >/dev/null 2>&1; then\n-\ttest_set_prereq GUNZIP\n-else\n-\tsay \"Skipping some tar.gz tests because gunzip was not found\"\n-fi\n-\n-test_expect_success GZIP,GUNZIP 'extract tgz file' '\n-\t$GUNZIP -c <j.tgz >j.tar &&\n+test_expect_success GZIP 'extract tgz file' '\n+\tgzip -d -c <j.tgz >j.tar &&\n \ttest_cmp b.tar j.tar\n '\n \n-- \n1.8.5.rc3.493.g965953d\n"},{"id":"231443","messageId":"xmqqy541okwg.fsf@gitster.dls.corp.google.com","threadId":"35460","inReplyTo":"20131203131812.GC26667@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-03T18:21:35Z","receivedAt":"2013-12-03T18:21:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> There are a few options I see:\n>\n>   1. Drop $GZIP variable, and hard-code the prerequisite check to\n>      \"gzip\", which is what is being tested.\n>\n>   2. Keep $GZIP (but rename it to $GIT_GZIP), and explicitly set up\n>      tar.tgz.command as \"$GIT_GZIP -cn\".\n>\n>   3. Teach the Makefile a knob to set the value for \"gzip\" at compile\n>      time, and use that for the baked-in config (and propagate it to the\n>      test to check the prerequisite).\n>\n> I think I'd be in favor of (1). It's the simplest, and we have not seen\n> any reports of people who do not actually have gzip called \"gzip\". Users\n> can still override it via config if they really want to.\n\nI am OK with (1).\n\nA related tangent is that we may have to worry about is how/if a\nrandom setting coming from GZIP in the environment (e.g. \"GZIP=-1v\")\nwould interfere with the test.  It may be the simplest to unset\n$GZIP at the beginning of these tests, regardless of which of the\nabove three is taken.\n"},{"id":"231533","messageId":"20131204193232.GB11024@sigill.intra.peff.net","threadId":"35460","inReplyTo":"xmqqy541okwg.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-12-04T19:32:32Z","receivedAt":"2013-12-04T19:32:32Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Dec 03, 2013 at 10:21:35AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > There are a few options I see:\n> >\n> >   1. Drop $GZIP variable, and hard-code the prerequisite check to\n> >      \"gzip\", which is what is being tested.\n> >\n> >   2. Keep $GZIP (but rename it to $GIT_GZIP), and explicitly set up\n> >      tar.tgz.command as \"$GIT_GZIP -cn\".\n> >\n> >   3. Teach the Makefile a knob to set the value for \"gzip\" at compile\n> >      time, and use that for the baked-in config (and propagate it to the\n> >      test to check the prerequisite).\n> >\n> > I think I'd be in favor of (1). It's the simplest, and we have not seen\n> > any reports of people who do not actually have gzip called \"gzip\". Users\n> > can still override it via config if they really want to.\n> \n> I am OK with (1).\n> \n> A related tangent is that we may have to worry about is how/if a\n> random setting coming from GZIP in the environment (e.g. \"GZIP=-1v\")\n> would interfere with the test.  It may be the simplest to unset\n> $GZIP at the beginning of these tests, regardless of which of the\n> above three is taken.\n\nI don't think we should worry about it.\n\nThere are two levels to consider here. One, people may put junk in their\nGZIP variable, which will impact normal running of git itself (e.g.,\nwhen you run \"git archive\"). And two, they may put options in which\naffect the test output (e.g., \"-v\").\n\nIn the former case, I do not think it is worth worrying about. If you\nput something in your GZIP variable that causes \"gzip -cn\" to stop\nworking (like GZIP=-d), then it is your fault for breaking gzip (and it\nis not just broken for git, but everywhere). If you put in tame things\nlike \"--rsyncable\" or \"-9\", I think it is a _good_ thing that git's\ninvocation of gzip is respecting your choice.  \"Fixing\" that would\ninvolve git-archive clearing the GZIP variable, but I do not think it is\na good idea.\n\nFor tests, we could potentially clear GZIP to give us a more consistent\nstate for running the tests. But I do not think there is anything you\nwould put in GZIP that should negatively affect the tests. Obvious just\nlike \"-d\" is in the same boat as above; if you break gzip completely,\nyou deserve it. If you use \"-v\" or \"-q\" to change stderr, we handle that\njust fine.\n\nThat leaves options which change the compressed output, like \"-9\". I'm\ninclined to say that letting them affect the tests is a good thing. It\nis true that we do not have a consistent state, but that also means we\nare testing the real world a little bit better. Part of the point of\ngit's test suite is to make sure that from commit to commit, we do not\nbreak things. But it is also to show that for a given commit, from\nmachine to machine we do not break things. Though we try to give a\nconsistent baseline, we must tolerate some amount of variance, and that\nuncovers portability bugs (e.g., tests reveal that the shell on platform\nX does not like our script).\n\nIf somebody shows up complaining that a test fails when they have GZIP\nset, then that may be catching a bug, or it may be catching a fragility\nin the test. But since we do not have a real-world complaint yet, I'd\nrather leave it and judge when we have an actual case.\n\n-Peff\n"},{"id":"231544","messageId":"xmqqbo0wjmnn.fsf@gitster.dls.corp.google.com","threadId":"35460","inReplyTo":"20131204193232.GB11024@sigill.intra.peff.net","subject":"Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-12-04T22:07:08Z","receivedAt":"2013-12-04T22:07:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Dec 03, 2013 at 10:21:35AM -0800, Junio C Hamano wrote:\n>\n>> Jeff King <peff@peff.net> writes:\n>> \n>> > There are a few options I see:\n>> >\n>> >   1. Drop $GZIP variable, and hard-code the prerequisite check to\n>> >      \"gzip\", which is what is being tested.\n>> > ...\n>> > I think I'd be in favor of (1). It's the simplest, and we have not seen\n>> > any reports of people who do not actually have gzip called \"gzip\". Users\n>> > can still override it via config if they really want to.\n>> \n>> I am OK with (1).\n>> \n>> A related tangent is that we may have to worry about is how/if a\n>> random setting coming from GZIP in the environment (e.g. \"GZIP=-1v\")\n>> would interfere with the test.  It may be the simplest to unset\n>> $GZIP at the beginning of these tests, regardless of which of the\n>> above three is taken.\n>\n> I don't think we should worry about it.\n>\n> There are two levels to consider here. One, people may put junk in their\n> GZIP variable, which will impact normal running of git itself...\n\nThis wasn't something I was worried about. We should support\nreasonable setting of GZIP without breaking ourselves.\n\n> That leaves options which change the compressed output, like \"-9\".\n\nYes, I was solely focusing on the stability of the tests.\n\n> If somebody shows up complaining that a test fails when they have GZIP\n> set, then that may be catching a bug, or it may be catching a fragility\n> in the test. But since we do not have a real-world complaint yet, I'd\n> rather leave it and judge when we have an actual case.\n\nOK.\n"}]}