# [PATCH v2 2/5] Use -y where possible in test t7610-mergetool 3 messages from 2017-03-03 to 2017-03-03. Participants: Denton Liu, Junio C Hamano. Thread: https://gitlist.dev/t/45277 ## Denton Liu, 2017-03-03 13:40 Subject: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool Message-ID: <20170303134022.GA6258@arch-attack.localdomain> URL: https://gitlist.dev/e/20170303134022.GA6258%40arch-attack.localdomain ``` In these tests, there are many situations where 'echo "" | git mergetool' is used. This replaces all of those occurrences with 'git mergetool -y' for simplicity and readability. Signed-off-by: Denton Liu --- t/t7610-mergetool.sh | 62 ++++++++++++++++++++++++++-------------------------- 1 file changed, 31 insertions(+), 31 deletions(-) diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index a9e274add..01c1d44a9 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -131,10 +131,10 @@ test_expect_success 'custom mergetool' ' git checkout -b test$test_count branch1 && git submodule update -N && test_expect_code 1 git merge master >/dev/null 2>&1 && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && - ( yes "" | git mergetool file1 file1 ) && - ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) && - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && + git mergetool -y both >/dev/null 2>&1 && + git mergetool -y file1 file1 && + git mergetool -y file2 "spaced name" >/dev/null 2>&1 && + git mergetool -y subdir/file3 >/dev/null 2>&1 && ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) && ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod >/dev/null 2>&1 ) && @@ -154,11 +154,11 @@ test_expect_success 'mergetool crlf' ' test_config core.autocrlf true && git checkout -b test$test_count branch1 && test_expect_code 1 git merge master >/dev/null 2>&1 && - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) && - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) && - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && + git mergetool file1 >/dev/null 2>&1 && + git mergetool file2 >/dev/null 2>&1 && + git mergetool "spaced name" >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && + git mergetool subdir/file3 >/dev/null 2>&1 && ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) && ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) && ( yes "r" | git mergetool submod >/dev/null 2>&1 ) && @@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' ' ( cd subdir && test_expect_code 1 git merge master >/dev/null 2>&1 && - ( yes "" | git mergetool file3 >/dev/null 2>&1 ) && + git mergetool file3 >/dev/null 2>&1 && test "$(cat file3)" = "master new sub" ) ' @@ -189,10 +189,10 @@ test_expect_success 'mergetool on file in parent dir' ' ( cd subdir && test_expect_code 1 git merge master >/dev/null 2>&1 && - ( yes "" | git mergetool file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) && - ( yes "" | git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 ) && - ( yes "" | git mergetool ../both >/dev/null 2>&1 ) && + git mergetool file3 >/dev/null 2>&1 && + git mergetool ../file1 >/dev/null 2>&1 && + git mergetool ../file2 ../spaced\ name >/dev/null 2>&1 && + git mergetool ../both >/dev/null 2>&1 && ( yes "d" | git mergetool ../file11 >/dev/null 2>&1 ) && ( yes "d" | git mergetool ../file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool ../submod >/dev/null 2>&1 ) && @@ -302,7 +302,7 @@ test_expect_success 'mergetool takes partial path' ' git submodule update -N && test_expect_code 1 git merge master && - ( yes "" | git mergetool subdir ) && + git mergetool subdir && test "$(cat subdir/file3)" = "master new sub" ' @@ -370,8 +370,8 @@ test_expect_success 'deleted vs modified submodule' ' git checkout -b test$test_count.a test$test_count && test_expect_code 1 git merge master && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "r" | git mergetool submod ) && rmdir submod && mv submod-movedaside submod && @@ -387,8 +387,8 @@ test_expect_success 'deleted vs modified submodule' ' git submodule update -N && test_expect_code 1 git merge master && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod ) && test ! -e submod && @@ -401,8 +401,8 @@ test_expect_success 'deleted vs modified submodule' ' git submodule update -N && test_expect_code 1 git merge test$test_count && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "r" | git mergetool submod ) && test ! -e submod && @@ -417,8 +417,8 @@ test_expect_success 'deleted vs modified submodule' ' git submodule update -N && test_expect_code 1 git merge test$test_count && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod ) && test "$(cat submod/bar)" = "master submodule" && @@ -441,8 +441,8 @@ test_expect_success 'file vs modified submodule' ' git checkout -b test$test_count.a branch1 && test_expect_code 1 git merge master && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "r" | git mergetool submod ) && rmdir submod && mv submod-movedaside submod && @@ -457,8 +457,8 @@ test_expect_success 'file vs modified submodule' ' git checkout -b test$test_count.b test$test_count && test_expect_code 1 git merge master && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod ) && git submodule update -N && @@ -473,8 +473,8 @@ test_expect_success 'file vs modified submodule' ' git submodule update -N && test_expect_code 1 git merge test$test_count && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both >/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both >/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "r" | git mergetool submod ) && test -d submod.orig && @@ -489,8 +489,8 @@ test_expect_success 'file vs modified submodule' ' git submodule update -N && test_expect_code 1 git merge test$test_count && test -n "$(git ls-files -u)" && - ( yes "" | git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 ) && - ( yes "" | git mergetool both>/dev/null 2>&1 ) && + git mergetool file1 file2 spaced\ name subdir/file3 >/dev/null 2>&1 && + git mergetool both>/dev/null 2>&1 && ( yes "d" | git mergetool file11 file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod ) && test "$(cat submod/bar)" = "master submodule" && -- 2.12.0.5.gfbc750a84 ``` ## Junio C Hamano, 2017-03-03 19:39 Subject: Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool Message-ID: URL: https://gitlist.dev/e/xmqqd1dy880d.fsf%40gitster.mtv.corp.google.com In-Reply-To: <20170303134022.GA6258@arch-attack.localdomain> ``` Denton Liu writes: > In these tests, there are many situations where > 'echo "" | git mergetool' is used. This replaces all of those > occurrences with 'git mergetool -y' for simplicity and readability. "-y where _possible_" is not necessarily a good thing to do in tests. We do want to make sure that both "yes" from the input and "-y" from the command line work. Changes to "-y" done only for the tests that are not about accepting the interactive input from the end-user is a very good idea, but "replaces all of those" makes me worried. > - ( yes "" | git mergetool both >/dev/null 2>&1 ) && > - ( yes "" | git mergetool file1 file1 ) && > - ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) && > - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && > + git mergetool -y both >/dev/null 2>&1 && > + git mergetool -y file1 file1 && > + git mergetool -y file2 "spaced name" >/dev/null 2>&1 && > + git mergetool -y subdir/file3 >/dev/null 2>&1 && So these replace "the user interactively keeps typing " with "-y", which sounds good. These are obviously more about what the code actually does. > - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) && > - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) && > - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) && > - ( yes "" | git mergetool both >/dev/null 2>&1 ) && > - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && > + git mergetool file1 >/dev/null 2>&1 && > + git mergetool file2 >/dev/null 2>&1 && > + git mergetool "spaced name" >/dev/null 2>&1 && > + git mergetool both >/dev/null 2>&1 && > + git mergetool subdir/file3 >/dev/null 2>&1 && The reason for the lack of "-y" in the rewrite, in contrast to what was done in the previous hunk we saw, is not quite obvious. > @@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' ' > ( > cd subdir && > test_expect_code 1 git merge master >/dev/null 2>&1 && > - ( yes "" | git mergetool file3 >/dev/null 2>&1 ) && > + git mergetool file3 >/dev/null 2>&1 && Likewise, and likewise for the remainder of the patch where the updated command line does not say "-y". ``` ## Denton Liu, 2017-03-03 21:21 Subject: Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool Message-ID: <20170303212150.GA13790@arch-attack.localdomain> URL: https://gitlist.dev/e/20170303212150.GA13790%40arch-attack.localdomain In-Reply-To: ``` On Fri, Mar 03, 2017 at 11:39:30AM -0800, Junio C Hamano wrote: > Denton Liu writes: > > > In these tests, there are many situations where > > 'echo "" | git mergetool' is used. This replaces all of those > > occurrences with 'git mergetool -y' for simplicity and readability. > > "-y where _possible_" is not necessarily a good thing to do in > tests. We do want to make sure that both "yes" from the input and > "-y" from the command line work. Changes to "-y" done only for the > tests that are not about accepting the interactive input from the > end-user is a very good idea, but "replaces all of those" makes me > worried. The 'custom mergetool' test case seems like a good place to introduce an interactive test. Would the following patch to my patch work? diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh index b6a419258..71624583c 100755 --- a/t/t7610-mergetool.sh +++ b/t/t7610-mergetool.sh @@ -135,8 +135,8 @@ test_expect_success 'custom mergetool' ' test_expect_code 1 git merge master >/dev/null 2>&1 && git mergetool -y both >/dev/null 2>&1 && git mergetool -y file1 file1 && - git mergetool -y file2 "spaced name" >/dev/null 2>&1 && - git mergetool -y subdir/file3 >/dev/null 2>&1 && + ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) && + ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && ( yes "d" | git mergetool file11 >/dev/null 2>&1 ) && ( yes "d" | git mergetool file12 >/dev/null 2>&1 ) && ( yes "l" | git mergetool submod >/dev/null 2>&1 ) && > > - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) && > > - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) && > > - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) && > > - ( yes "" | git mergetool both >/dev/null 2>&1 ) && > > - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) && > > + git mergetool file1 >/dev/null 2>&1 && > > + git mergetool file2 >/dev/null 2>&1 && > > + git mergetool "spaced name" >/dev/null 2>&1 && > > + git mergetool both >/dev/null 2>&1 && > > + git mergetool subdir/file3 >/dev/null 2>&1 && > > The reason for the lack of "-y" in the rewrite, in contrast to what > was done in the previous hunk we saw, is not quite obvious. > Sorry, it was my mistake. I had forgotten to replace the '-y'. I have corrected this for a future revision. ```