On Tue, Dec 03, 2013 at 04:49:06AM -0500, Eric Sunshine wrote:
> > -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
--
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
