[PATCH 1/1] tests: fix gzip with exported GZIP variable in environment

In t/t5000-tar-tree.sh the variable GZIP is used for the command name.
From man gzip:

The environment variable GZIP can hold a set of default options for
gzip. These options are interpreted first and can be overwritten by
explicit command line parameters.

So using any other variable name fixes this.
---
 t/t5000-tar-tree.sh | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index c2023b1..01b0ed9 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh

@@ -25,7 +25,7 @@ commit id embedding:
 '
 
 . ./test-lib.sh
-GZIP=${GZIP:-gzip}
+GZIPCMD=${GZIPCMD:-gzip}
 GUNZIP=${GUNZIP:-gzip -d}
 
 SUBSTFORMAT=%H%n

@@ -275,27 +275,27 @@ test_expect_success 'only enabled filters are available remotely' '
 	test_cmp remote.bar config.bar
 '
 
-if $GZIP --version >/dev/null 2>&1; then
-	test_set_prereq GZIP
+if $GZIPCMD --version >/dev/null 2>&1; then
+	test_set_prereq GZIPCMD
 else
 	say "Skipping some tar.gz tests because gzip not found"
 fi
 
-test_expect_success GZIP 'git archive --format=tgz' '
+test_expect_success GZIPCMD 'git archive --format=tgz' '
 	git archive --format=tgz HEAD >j.tgz
 '
 
-test_expect_success GZIP 'git archive --format=tar.gz' '
+test_expect_success GZIPCMD 'git archive --format=tar.gz' '
 	git archive --format=tar.gz HEAD >j1.tar.gz &&
 	test_cmp j.tgz j1.tar.gz
 '
 
-test_expect_success GZIP 'infer tgz from .tgz filename' '
+test_expect_success GZIPCMD 'infer tgz from .tgz filename' '
 	git archive --output=j2.tgz HEAD &&
 	test_cmp j.tgz j2.tgz
 '
 
-test_expect_success GZIP 'infer tgz from .tar.gz filename' '
+test_expect_success GZIPCMD 'infer tgz from .tar.gz filename' '
 	git archive --output=j3.tar.gz HEAD &&
 	test_cmp j.tgz j3.tar.gz
 '

@@ -306,17 +306,17 @@ else
 	say "Skipping some tar.gz tests because gunzip was not found"
 fi
 
-test_expect_success GZIP,GUNZIP 'extract tgz file' '
+test_expect_success GZIPCMD,GUNZIP 'extract tgz file' '
 	$GUNZIP -c <j.tgz >j.tar &&
 	test_cmp b.tar j.tar
 '
 
-test_expect_success GZIP 'remote tar.gz is allowed by default' '
+test_expect_success GZIPCMD 'remote tar.gz is allowed by default' '
 	git archive --remote=. --format=tar.gz HEAD >remote.tar.gz &&
 	test_cmp j.tgz remote.tar.gz
 '
 
-test_expect_success GZIP 'remote tar.gz can be disabled' '
+test_expect_success GZIPCMD 'remote tar.gz can be disabled' '
 	git config tar.tar.gz.remote false &&
 	test_must_fail git archive --remote=. --format=tar.gz HEAD \
 		>remote.tar.gz

-- 
1.8.5

[cc'ing Peff, the author of these tests]

On Tue, Dec 3, 2013 at 3:57 AM, Christian Hesse [off-list ref] wrote:

In t/t5000-tar-tree.sh the variable GZIP is used for the command name.
From man gzip:

quoted

The environment variable GZIP can hold a set of default options for
gzip. These options are interpreted first and can be overwritten by
explicit command line parameters.

So using any other variable name fixes this.

Missing Signed-off-by: <you>

test_set_prereq is not actually operating on an environment variable.
Its argument is just a generic tag, which is uppercase by convention,
but not otherwise related to a variable which may share the same name,
and which does not pollute the environment. Consequently, it should
not be necessary to rename the argument to test_set_prereq, thus all
changes following this one become superfluous (since they are checking
for presence of tag GZIP, not referencing environment variable GZIP or
GZIPCMD). Thus, the patch becomes much smaller.

In fact, the GZIP command does not appear to be used at all by the
tests, so a simpler solution might be to remove the variable
altogether, and perhaps the prerequisite. Peff?

On Tue, Dec 03, 2013 at 04:49:06AM -0500, Eric Sunshine wrote:

quoted

-if $GZIP --version >/dev/null 2>&1; then
-       test_set_prereq GZIP
+if $GZIPCMD --version >/dev/null 2>&1; then
+       test_set_prereq GZIPCMD

test_set_prereq is not actually operating on an environment variable.
Its argument is just a generic tag, which is uppercase by convention,
but not otherwise related to a variable which may share the same name,
and which does not pollute the environment. Consequently, it should
not be necessary to rename the argument to test_set_prereq, thus all
changes following this one become superfluous (since they are checking
for presence of tag GZIP, not referencing environment variable GZIP or
GZIPCMD). Thus, the patch becomes much smaller.

Right. We can get away with just changing the environment variable, and
leaving the prereq.

By the way, we had the exact same problem with $UNZIP, fixed in ac00128
(t0024, t5000: clear variable UNZIP, use GIT_UNZIP instead, 2013-01-06).
I'd probably call the new variable GIT_GZIP for consistency, but...

In fact, the GZIP command does not appear to be used at all by the
tests, so a simpler solution might be to remove the variable
altogether, and perhaps the prerequisite. Peff?

Yes, though it's a bit more subtle than that. The gzip tests are relying
on git's internally-configured "tar.tgz.command" filter, which is
hard-coded to "gzip -cn". So we do depend on having a working gzip, but
we do _not_ depend on the one found in the $GZIP variable. It must be
called "gzip".

There are a few options I see:

  1. Drop $GZIP variable, and hard-code the prerequisite check to
     "gzip", which is what is being tested.

  2. Keep $GZIP (but rename it to $GIT_GZIP), and explicitly set up
     tar.tgz.command as "$GIT_GZIP -cn".

  3. Teach the Makefile a knob to set the value for "gzip" at compile
     time, and use that for the baked-in config (and propagate it to the
     test to check the prerequisite).

I think I'd be in favor of (1). It's the simplest, and we have not seen
any reports of people who do not actually have gzip called "gzip". Users
can still override it via config if they really want to.

-Peff

In t5000, we test the built-in ".tar.gz" config for
git-archive. To make our tests portable, we check that we
have a way to both gzip and gunzip, and we respected
environment variables to point to alternate commands for
doing these operations.

However, the $GZIP variable did not actually do anything, as
changing it would not affect the baked-in value in
archive-tar.c. Moreover, setting the variable $GZIP
influences gzip itself. From the gzip man page:

  The environment variable GZIP can hold a set of default
  options for gzip. These options are interpreted first and
  can be overwritten by explicit command line parameters.

We could rename this variable, and use it to set up custom
config (or even have a Makefile knob to affect the built
binary), but it is not worth the trouble; nobody has ever
reported a problem with the baked-in default, and they can
always change it via config if they need to. Let's just drop
the variable and use "gzip" in the test (keeping the
prerequisite, of course).

While we're at it, we can drop the GUNZIP variable and
prerequisite; it uses "gzip -d", so if we have GZIP, we
will have both.

We can also use test_lazy_prereq for the gzip prerequisite,
which is simpler and behaves more consistently with the rest
of git (e.g., by making output available when the test is
run with "-v").

Noticed-by: Christian Hesse [off-list ref]
Signed-off-by: Jeff King <redacted>
---
 t/t5000-tar-tree.sh | 20 ++++----------------
 1 file changed, 4 insertions(+), 16 deletions(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index c2023b1..519cc34 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh

@@ -25,8 +25,6 @@ commit id embedding:
 '
 
 . ./test-lib.sh
-GZIP=${GZIP:-gzip}
-GUNZIP=${GUNZIP:-gzip -d}
 
 SUBSTFORMAT=%H%n
 

@@ -39,6 +37,8 @@ test_lazy_prereq TAR_NEEDS_PAX_FALLBACK '
 	)
 '
 
+test_lazy_prereq GZIP 'gzip --version'
+
 get_pax_header() {
 	file=$1
 	header=$2=

@@ -275,12 +275,6 @@ test_expect_success 'only enabled filters are available remotely' '
 	test_cmp remote.bar config.bar
 '
 
-if $GZIP --version >/dev/null 2>&1; then
-	test_set_prereq GZIP
-else
-	say "Skipping some tar.gz tests because gzip not found"
-fi
-
 test_expect_success GZIP 'git archive --format=tgz' '
 	git archive --format=tgz HEAD >j.tgz
 '

@@ -300,14 +294,8 @@ test_expect_success GZIP 'infer tgz from .tar.gz filename' '
 	test_cmp j.tgz j3.tar.gz
 '
 
-if $GUNZIP --version >/dev/null 2>&1; then
-	test_set_prereq GUNZIP
-else
-	say "Skipping some tar.gz tests because gunzip was not found"
-fi
-
-test_expect_success GZIP,GUNZIP 'extract tgz file' '
-	$GUNZIP -c <j.tgz >j.tar &&
+test_expect_success GZIP 'extract tgz file' '
+	gzip -d -c <j.tgz >j.tar &&
 	test_cmp b.tar j.tar
 '
 

-- 
1.8.5.rc3.493.g965953d