Re: [PATCH 6/9] t4013: add tests for log.diffMerges config

From: Sergey Organov <sorganov@gmail.com>
To: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, Jeff King <peff@peff.net>,
	Philip Oakley <philipoakley@iee.email>,
	Elijah Newren <newren@gmail.com>,
	Felipe Contreras <felipe.contreras@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH 6/9] t4013: add tests for log.diffMerges config
Date: Thu, 08 Apr 2021 17:25:24 +0300	[thread overview]
Message-ID: <87a6q8dfaj.fsf@osv.gnss.ru> (raw)
In-Reply-To: <87v98xitjh.fsf@evledraar.gmail.com> (=?utf-8?B?IsOGdmFyIEFy?= =?utf-8?B?bmZqw7Zyw7A=?= Bjarmason"'s message of "Thu, 08 Apr 2021 01:06:26 +0200")

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> On Thu, Apr 08 2021, Sergey Organov wrote:

[...]

>> +test_expect_success 'deny wrong log.diffMerges config' '
>> +	git config log.diffMerges wrong-value &&
>> +	test_expect_code 128 git log &&
>> +	git config --unset log.diffMerges
>
> Don't use "git config", but "test_config" at the start, then you don't
> need the --unset at the end, it'll happen automatically. Ditto for the
> following tests.

[...]

Junio C Hamano <gitster@pobox.com> writes:

> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
>
>>> +test_expect_success 'deny wrong log.diffMerges config' '
>>> +	git config log.diffMerges wrong-value &&
>>> +	test_expect_code 128 git log &&
>>> +	git config --unset log.diffMerges
>>
>> Don't use "git config", but "test_config" at the start, then you don't
>> need the --unset at the end, it'll happen automatically. Ditto for the
>> following tests.
>
> More importantly, test_config arranges the unset to happen even if
> a step in the middle (e.g. test_expect_code in the above example)
> fails.  In the posted version, the control would not reach the
> "git config --unset" and leaves the configuration behind.
>
> And that is the biggest reason why the above should use test_config.

Yeah, thanks for pointing, – will fix for the next re-roll.

-- Sergey Organov