Re: [PATCH v5] Add log.abbrevCommit config variable

Jay Soffian [off-list ref] writes:

+test_expect_success 'log.abbrev-commit configuration' '
+	test_when_finished "git config --unset log.abbrevCommit" &&
+
+	git log >expect.full &&
+	git log --abbrev-commit >expect.abbrev &&
+	git log --pretty=raw >expect.raw &&
+	git log -g --abbrev-commit --pretty=oneline >expect.reflog &&
+	git whatchanged --abbrev-commit >expect.whatchanged &&
+
+	git config log.abbrevCommit true &&
+
+	git log --no-abbrev-commit >actual.full &&
+	test_cmp expect.full actual.full &&
+
+	git log >actual.abbrev &&
+	test_cmp expect.abbrev actual.abbrev &&
+
+	git log --pretty=raw >actual.raw &&
+	test_cmp expect.raw actual.raw &&
+
+	git reflog >actual.reflog &&
+	test_cmp expect.reflog actual.reflog
+
+	git whatchanged >actual.whatchanged &&
+	test_cmp expect.whatchanged actual.whatchanged
+'

I can see you are testing all the commands in the "log" family for cases
where the new configuration _should_ take effect, but I cannot see the
same throughness for the negative case where the configuration should not
kick in. Is "log" the only one that you make sure --no-abbrev-commit
defeats the configuration? Please always test both sides of the coin.

Other than that, the patch looked fine from my cursory look.

Thanks.

On Wed, May 18, 2011 at 12:22 AM, Junio C Hamano [off-list ref] wrote:

Other than that, the patch looked fine from my cursory look.

Thanks, I hope v7 addresses all concerns.

j.