Re: [PATCH v2 2/2] tests: add test for separate author and committer idents

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Jan 27 2019, Eric Sunshine wrote:

> On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason
> <avarab@gmail.com> 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@example.com &&
>>             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@evledraar.gmail.com> ("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.