Re: [PATCH 1/7] test: add merge style config test

From: Phillip Wood <phillip.wood123@gmail.com>
To: Felipe Contreras <felipe.contreras@gmail.com>, git@vger.kernel.org
Cc: "David Aguilar" <davvid@gmail.com>,
	"Junio C Hamano" <gitster@pobox.com>,
	"Sergey Organov" <sorganov@gmail.com>,
	"Bagas Sanjaya" <bagasdotme@gmail.com>,
	"Elijah Newren" <newren@gmail.com>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Denton Liu" <liu.denton@gmail.com>
Subject: Re: [PATCH 1/7] test: add merge style config test
Date: Thu, 10 Jun 2021 15:54:41 +0100	[thread overview]
Message-ID: <a819dd7d-e690-baa7-b6c8-2b1fabe51522@gmail.com> (raw)
In-Reply-To: <60c2130178831_b25b1208f5@natae.notmuch>

On 10/06/2021 14:26, Felipe Contreras wrote:
> Phillip Wood wrote:
>> On 09/06/2021 20:28, Felipe Contreras wrote:
>>> We want to test different combinations of merge.conflictstyle, and a new
>>> file is the best place to do that.
>>
>> I'm not sure what this particular tests adds over the existing ones in
>> t6427-diff3-conflict-markers.sh.
> 
> That file is for diff3 conflict markers. The tests in this file are not.
> 
>> The commit message does not explain why a new file is better than
>> adding this test to that file.
> 
> Because there's no file that is testing for this.
> 
>> There are already diff3 tests for checkout
> 
> This file is not doing diff3 tests.
> 
> 
> As stated above, it's testing different *combinations* of
> merge.conflictstyle, diff3 is only *one* of the possibilities, another
> possibility is:
> 
>    git -c merge.conflictstyle=diff3 checkout -m --conflict=merge
> 
> That is *not* a diff3 test.

I think that is an artificial distinction, it is testing the behavior of 
checkout when merge.conflictStyle=diff3 just like the other tests, it 
just happens to be checking that the config option can be combined with 
a command line option.

Best Wishes

Phillip

>>> diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh
>>> new file mode 100755
> 
>>> +test_expect_success 'merge' '
>>> +	test_create_repo merge &&
>>> +	(
>>> +		cd merge &&
>>> +
>>> +		fill 1 2 3 >content &&
>>> +		git add content &&
>>> +		git commit -m base &&
>>> +
>>> +		git checkout -b r &&
>>> +		echo six >>content &&
>>> +		git commit -a -m right &&
>>> +
>>> +		git checkout master &&
>>> +		echo 7 >>content &&
>>> +		git commit -a -m left &&
>>> +
>>> +		test_must_fail git merge r &&
>>> +		! grep -E "\|+" content &&
>>
>> ! grep "|"  would be simpler and just as effective.
> 
> But that would fail if there's a "command1 | command2".
> 
>> This is quite a weak
>> test, something like "^|||||| " would be a stronger test for conflict
>> markers
> 
> But that doesn't work in all the tests.
> 
> Cheers.
> 