Re: [PATCH 03/12] t: drop useless sane_unset GIT_* calls

From: Jeff King <peff@peff.net>
Date: Mar 24, 2014, 21:56 UTC
Message-ID: <20140324215638.GH13728@sigill.intra.peff.net>
In-Reply-To: <xmqqy503s0s0.fsf@gitster.dls.corp.google.com>

On Fri, Mar 21, 2014 at 02:24:31PM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> > Unsetting these is not only useless, but can be confusing to
> > a reader, who may wonder why some tests in a script unset
> > them and others do not (t0001 is particularly guilty of this
> > inconsistency, probably because many of its tests predate
> > the test-lib.sh environment-cleansing).
> [...]
> > I suppose one could make an argument that test-lib.sh may later change
> > the set of variables it clears, and these unsets are documenting an
> > explicit need of each test. I'd find that more compelling if it were
> > actually applied consistently.
> 
> Hmph.  I am looking at "git show HEAD^:t/t0001-init.sh" after
> applying this patch, and it does look consistently done with
> GIT_CONFIG and GIT_DIR (I am not sure about GIT_WORK_TREE but from a
> cursory read it is done consistently for tests on non-bare
> repositories).

I don't understand why we stop bothering with the unsets starting with "init with --template". Are those variables not important to the outcome of that and later tests, or did the author simply not bother because they are noops?

Show 5 quoted lines

> So I would actually agree with your alternative interpretation
> "Unsetting these is useless, but it does serve documentation
> purpose---without having to see what the state of the environment
> when the subprocess is started, the reader can understand what is
> being tested", rather than the one in the log message.

I'd agree with that if I were convinced that the presence of them there versus the absence of them later was meaningful.

Show 12 quoted lines

> Having said that, I am perfectly OK with the change to t0001 in this
> patch, if we added at the very beginning of the test sequence a
> comment that says:
> 
>     Below, creation and use of repositories are tested with various
>     combinations of environment settings and command line flags.
>     They are done inside subshells to avoid leaking temporary
>     environment settings to later tests *and* assumes that the
>     initial environment does not have have GIT_DIR, GIT_CONFIG, and
>     GIT_WORK_TREE defined.
> 
> or something.

I do not have a problem with that, as it implicitly covers all of the tests following it. I do not think it is particularly necessary, though. Assuming we start with a known test environment and avoiding polluting it for further tests are basic principles of _all_ test scripts.

-Peff