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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Jan 27, 2019, 04:48 UTC
Message-ID: <CAPig+cQKKqL7QD_nwy8tvHaxuGqBXATVt2Mo+gELpif9aULc6A@mail.gmail.com>
In-Reply-To: <874l9v4x6p.fsf@evledraar.gmail.com>

On Sat, Jan 26, 2019 at 3:53 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

Show 11 quoted lines

> 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.