Re: [PATCH v2 2/2] log: add log.showSignature configuration variable

> On Sat, Jun 18, 2016 at 8:25 AM, Mehul Jain <mehul.jain2029@gmail.com> wrote:
>> diff --git a/t/t4202-log.sh b/t/t4202-log.sh
>> @@ -900,6 +900,31 @@ test_expect_success GPG '--no-show-signature overrides --show-signature' '
>> +test_expect_success GPG 'log.showsignature=true behaves like --show-signature' '
>> +       git checkout -b test_sign master &&
>
> It appears that you copied the bulk of this test from the 'log --graph
> --show-signature' test and changed it to create a new branch named
> 'test_sign' rather than 'signed', however...
>
>> +       echo foo >foo &&
>> +       git add foo &&
>> +       git commit -S -m signed_commit &&
>> +       test_config log.showsignature true &&
>> +       git log -1 signed >actual &&
>
> ... you're incorrectly testing against the 'signed' branch rather than
> the 'test_sign' created specifically for this test.

>> +       grep "gpg: Signature made" actual &&
>> +       grep "gpg: Good signature" actual
>> +'
>> +
>> +test_expect_success GPG '--no-show-signature overrides log.showsignature=true' '
>> +       test_config log.showsignature true &&
>> +       git log -1 --no-show-signature signed >actual &&
>> +       ! grep "^gpg:" actual
>> +'
>> +
>> +test_expect_success GPG '--show-signature overrides log.showsignature=false' '
>> +       test_when_finished "git reset --hard && git checkout master" &&
>
> So, in the first of these three new tests, you're setting up some
> state by creating and checking out a new branch named 'test_sign', and
> leaving that branch checked out while these three tests run, and
> finally use test_when_finished() in the last of the three tests to
> restore sanity (by returning to the 'master' branch) when that test
> exits.
>
> This is ugly and couples these three tests too tightly. It would be
> better to make each test more self-contained, not relying upon state
> left over from previous tests.
>
>> +       test_config log.showsignature false &&
>> +       git log -1 --show-signature signed >actual &&
>> +       grep "gpg: Signature made" actual &&
>> +       grep "gpg: Good signature" actual
>> +'
>
> In fact, the original 'log --graph --show-signature' test which
> created the 'signed' branch, the new --no-show-signature test added in
> patch 1/2, and the three new tests here could all just work against
> the same 'signed' branch. There is no need to create a new 'test_sign'
> branch for these three tests, or a 'nosign' branch for the patch 1/2
> test.
>
> Therefore, it probably would make more sense to add a new distinct
> 'setup signed' test (or just enhance the existing 'setup' test) which
> creates the 'signed' branch and update the original 'log --graph
> --show-signature' to take advantage of it, as well as each of the new
> tests introduced by this patch series. And since each test would
> mention 'signed' explicitly in its git-log invocation, there's no need
> to leave that branch checked out, so the setup test itself only needs
> test_when_finished() to ensure that the current branch is restored to
> 'master'.