On Sun, Jan 27 2019, Eric Sunshine wrote:
> On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason
> <avarab@xxxxxxxxx> wrote:
>> Which, looking at this again, you'd only want if a previous test in the
>> file was leaking its state. That's not the case, so this isn't needed
>> and you can just apply this on top:
>>
>> test_expect_success \
>> 'author and committer config settings override user config settings' '
>> - sane_unset GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL &&
>> - sane_unset GIT_COMMITTER_NAME GIT_COMMITTER_EMAIL &&
>> git config user.name user &&
>> git config user.email user@xxxxxxxxxxx &&
>> git config author.name author &&
>
> Aside from future-proofing against a test being inserted before this
> one which does set those environment variables, these invocations of
> sane_unset() serve the additional purpose of documenting the interplay
> of configuration and environment, and further indicate to readers that
> the test author took this into consideration (rather than merely
> slapping together the test without thought). As a reviewer and reader
> of the test, I appreciate the additional context the sane_unset()
> calls provide, thus think it makes sense to retain them.
As noted in <875zuc49uj.fsf@xxxxxxxxxxxxxxxxxxx> ("various override
interactions") there should definitely be more tests where the
combination of config & env is tested for.
But I don't see how it makes things clearer to unset a bunch of
variables previous tests didn't set. If we applied that to our test
suite much of it would be pointlessly unsetting various GIT_*
variables.
Better to assume other tests have cleaned up their own state, and when
it's not the case fix it.
