Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment
On Tue, Dec 03, 2013 at 10:21:35AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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. I am OK with (1). A related tangent is that we may have to worry about is how/if a random setting coming from GZIP in the environment (e.g. GZIP=-1v) would interfere with the test. It may be the simplest to unset $GZIP at the beginning of these tests, regardless of which of the above three is taken. I don't think we should worry about it. There are two levels to consider here. One, people may put junk in their GZIP variable, which will impact normal running of git itself (e.g., when you run git archive). And two, they may put options in which affect the test output (e.g., -v). In the former case, I do not think it is worth worrying about. If you put something in your GZIP variable that causes gzip -cn to stop working (like GZIP=-d), then it is your fault for breaking gzip (and it is not just broken for git, but everywhere). If you put in tame things like --rsyncable or -9, I think it is a _good_ thing that git's invocation of gzip is respecting your choice. Fixing that would involve git-archive clearing the GZIP variable, but I do not think it is a good idea. For tests, we could potentially clear GZIP to give us a more consistent state for running the tests. But I do not think there is anything you would put in GZIP that should negatively affect the tests. Obvious just like -d is in the same boat as above; if you break gzip completely, you deserve it. If you use -v or -q to change stderr, we handle that just fine. That leaves options which change the compressed output, like -9. I'm inclined to say that letting them affect the tests is a good thing. It is true that we do not have a consistent state, but that also means we are testing the real world a little bit better. Part of the point of git's test suite is to make sure that from commit to commit, we do not break things. But it is also to show that for a given commit, from machine to machine we do not break things. Though we try to give a consistent baseline, we must tolerate some amount of variance, and that uncovers portability bugs (e.g., tests reveal that the shell on platform X does not like our script). If somebody shows up complaining that a test fails when they have GZIP set, then that may be catching a bug, or it may be catching a fragility in the test. But since we do not have a real-world complaint yet, I'd rather leave it and judge when we have an actual case. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment
Jeff King p...@peff.net writes: On Tue, Dec 03, 2013 at 10:21:35AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: 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. ... 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. I am OK with (1). A related tangent is that we may have to worry about is how/if a random setting coming from GZIP in the environment (e.g. GZIP=-1v) would interfere with the test. It may be the simplest to unset $GZIP at the beginning of these tests, regardless of which of the above three is taken. I don't think we should worry about it. There are two levels to consider here. One, people may put junk in their GZIP variable, which will impact normal running of git itself... This wasn't something I was worried about. We should support reasonable setting of GZIP without breaking ourselves. That leaves options which change the compressed output, like -9. Yes, I was solely focusing on the stability of the tests. If somebody shows up complaining that a test fails when they have GZIP set, then that may be catching a bug, or it may be catching a fragility in the test. But since we do not have a real-world complaint yet, I'd rather leave it and judge when we have an actual case. OK. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[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 21; then
- test_set_prereq GZIP
+if $GZIPCMD --version /dev/null 21; 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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment
[cc'ing Peff, the author of these tests]
On Tue, Dec 3, 2013 at 3:57 AM, Christian Hesse m...@eworm.de wrote:
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.
Missing Signed-off-by: you
---
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 21; then
- test_set_prereq GZIP
+if $GZIPCMD --version /dev/null 21; 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.
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?
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment
On Tue, Dec 03, 2013 at 04:49:06AM -0500, Eric Sunshine wrote: -if $GZIP --version /dev/null 21; then - test_set_prereq GZIP +if $GZIPCMD --version /dev/null 21; 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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/1] tests: fix gzip with exported GZIP variable in environment
Jeff King p...@peff.net writes: 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. I am OK with (1). A related tangent is that we may have to worry about is how/if a random setting coming from GZIP in the environment (e.g. GZIP=-1v) would interfere with the test. It may be the simplest to unset $GZIP at the beginning of these tests, regardless of which of the above three is taken. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
