From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 18 May 2011 04:22:18 GMT
Subject: Re: [PATCH v5] Add log.abbrevCommit config variable
Message-ID: <7vmxikeiyt.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1305681991-11699-1-git-send-email-jaysoffian@gmail.com>

Jay Soffian <jaysoffian@gmail.com> 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.