On Fri, Mar 21, 2014 at 02:24:31PM -0700, Junio C Hamano wrote: > > 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? > 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. > 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html