| Message ID | 20210609192842.696646-2-felipe.contreras@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Make diff3 the default conflict style | expand |
On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras <felipe.contreras@gmail.com> wrote: > We want to test different combinations of merge.conflictstyle, and a new > file is the best place to do that. > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > @@ -0,0 +1,44 @@ > +fill () { > + for i > + do > + echo "$i" > + done > +} This seems to duplicate the behavior of test_write_lines()... > +test_expect_success 'merge' ' > + test_create_repo merge && > + ( > + cd merge && > + fill 1 2 3 >content && ... which could be used here instead: test_write_lines 1 2 3 >content &&
Eric Sunshine wrote: > On Wed, Jun 9, 2021 at 3:29 PM Felipe Contreras > <felipe.contreras@gmail.com> wrote: > > We want to test different combinations of merge.conflictstyle, and a new > > file is the best place to do that. > > > > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > > --- > > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > > @@ -0,0 +1,44 @@ > > +fill () { > > + for i > > + do > > + echo "$i" > > + done > > +} > > This seems to duplicate the behavior of test_write_lines()... Right, I'll update the patch. The above function is used in: t6440-config-conflict-markers.sh t7201-co.sh So those two probably should be updated. Cheers.
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. The commit message does not explain why a new file is better than adding this test to that file. There are already diff3 tests for checkout so it ends up being confusing when a checkout test gets added to this file rather than with those tests later in the series because there is no longer a single location for diff3 checkout tests. > Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> > --- > t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++ > 1 file changed, 44 insertions(+) > create mode 100755 t/t6440-config-conflict-markers.sh > > diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh > new file mode 100755 > index 0000000000..6952552c58 > --- /dev/null > +++ b/t/t6440-config-conflict-markers.sh > @@ -0,0 +1,44 @@ > +#!/bin/sh > + > +test_description='merge style conflict markers configurations' > + > +. ./test-lib.sh > + > +fill () { > + for i > + do > + echo "$i" > + done > +} > + > +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. This is quite a weak test, something like "^|||||| " would be a stronger test for conflict markers Best Wishes Phillip > + git reset --hard && > + test_must_fail git -c merge.conflictstyle=diff3 merge r && > + grep -E "\|+" content && > + > + git reset --hard && > + test_must_fail git -c merge.conflictstyle=merge merge r && > + ! grep -E "\|+" content > + ) > +' > + > +test_done >
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. > > 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.
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. >
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. >>[...] >>> 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". I don't understand. What are you expecting content to contain? Why doesn't "\|+" fail in that case? >> 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. So test for what you actually expect, you don't need to have the same check in all the tests if the expected output is different. Best Wishes Phillip > Cheers. >
Phillip Wood wrote: > On 10/06/2021 14:26, Felipe Contreras wrote: > > Phillip Wood wrote: > >> 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 That is one of the things it's testing, it's not the only thing it's testing, it's testing other things as well.
Phillip Wood wrote: > 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. > >>[...] > >>> 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". > > I don't understand. What are you expecting content to contain? Not a sequence of |. > Why doesn't "\|+" fail in that case? It would, perhaps "\|\|+" would be better, or maybe "\|{2,}". > >> 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. > > So test for what you actually expect, you don't need to have the same > check in all the tests if the expected output is different. I don't need to, but it makes the tests simpler, and as you pointed out in another comment: more tests are needed. Perhaps once we know exactly what we want to test, and how to fix the current issues it would make sense to revisit these.
On 10/06/2021 17:47, Felipe Contreras wrote: > Phillip Wood wrote: >> 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. >>>> [...] >>>>> 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". >> >> I don't understand. What are you expecting content to contain? > > Not a sequence of |. > >> Why doesn't "\|+" fail in that case? > > It would, perhaps "\|\|+" would be better, or maybe "\|{2,}". The point of my original comment was that you do not need an ERE - 'grep "||"' matches the same set of lines as 'grep -E "\|\|+"'. As it is testing for conflict markers anchoring the pattern to the beginning of a line would probably be a good idea. Best Wishes Phillip >>>> 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. >> >> So test for what you actually expect, you don't need to have the same >> check in all the tests if the expected output is different. > > I don't need to, but it makes the tests simpler, and as you pointed out > in another comment: more tests are needed. > > Perhaps once we know exactly what we want to test, and how to fix the > current issues it would make sense to revisit these. >
Phillip Wood wrote: > On 10/06/2021 17:47, Felipe Contreras wrote: > > Phillip Wood wrote: > >> 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. > >>>> [...] > >>>>> 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". > >> > >> I don't understand. What are you expecting content to contain? > > > > Not a sequence of |. > > > >> Why doesn't "\|+" fail in that case? > > > > It would, perhaps "\|\|+" would be better, or maybe "\|{2,}". > > The point of my original comment was that you do not need an ERE - 'grep > "||"' matches the same set of lines as 'grep -E "\|\|+"'. As it is > testing for conflict markers anchoring the pattern to the beginning of a > line would probably be a good idea. Right, I didn't add that because I saw some tests not doing ^ when they clearly should, so I thought perhaps there was a compatibility issue, but now I see that ^ is already used in many tests, therefore "^\|\|+" makes sense.
diff --git a/t/t6440-config-conflict-markers.sh b/t/t6440-config-conflict-markers.sh new file mode 100755 index 0000000000..6952552c58 --- /dev/null +++ b/t/t6440-config-conflict-markers.sh @@ -0,0 +1,44 @@ +#!/bin/sh + +test_description='merge style conflict markers configurations' + +. ./test-lib.sh + +fill () { + for i + do + echo "$i" + done +} + +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 && + + git reset --hard && + test_must_fail git -c merge.conflictstyle=diff3 merge r && + grep -E "\|+" content && + + git reset --hard && + test_must_fail git -c merge.conflictstyle=merge merge r && + ! grep -E "\|+" content + ) +' + +test_done
We want to test different combinations of merge.conflictstyle, and a new file is the best place to do that. Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com> --- t/t6440-config-conflict-markers.sh | 44 ++++++++++++++++++++++++++++++ 1 file changed, 44 insertions(+) create mode 100755 t/t6440-config-conflict-markers.sh