Hi Eric, Thanks for your review. On Sun, Jun 19, 2016 at 12:29 PM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Sat, Jun 18, 2016 at 8:25 AM, Mehul Jain <mehul.jain2029@xxxxxxxxx> 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. Yes, I made a mistake here. >> + 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'. Adding a new test 'setup signed' will work, where I will create a new 'signed' branch and use that branch in new --no-show-signature test introduced in patch 1/2, and the three tests in current patch 2/2. Also by creating a preparatory patch for this series, I will modify the 'log --graph --show-signature' test, so that it can also take advantage of new 'setup signed' test. Though I'm wondering if whether there is a need to create the new 'setup signed' test. In 'log --graph --show-signature' test, we already have the 'signed' branch, which could be used in the test introduced here. But this will couple the tests, 'log --graph ...' and new ones, tightly. Because if in future someone changes the 'log --graph ...' test, then there is a possibility of new tests (introduced in patch 1/2 and 2/2) to fail. So creating a new test for creation of 'signed' branch seems fair. Thanks, Mehul -- 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