{"thread":{"id":"45277","subject":"[PATCH v2 2/5] Use -y where possible in test t7610-mergetool","startedAt":"2017-03-03T13:48:16Z","lastAt":"2017-03-03T21:22:18Z","messageCount":3,"participants":["Denton Liu","Junio C Hamano"],"isPatch":true,"patchVersion":2,"patchTotal":5},"messages":[{"id":"313172","messageId":"20170303134022.GA6258@arch-attack.localdomain","threadId":"45277","inReplyTo":null,"subject":"[PATCH v2 2/5] Use -y where possible in test t7610-mergetool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2017-03-03T13:40:22Z","receivedAt":"2017-03-03T13:48:16Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"In these tests, there are many situations where\n'echo \"\" | git mergetool' is used. This replaces all of those\noccurrences with 'git mergetool -y' for simplicity and readability.\n\nSigned-off-by: Denton Liu <liu.denton@gmail.com>\n---\n t/t7610-mergetool.sh | 62 ++++++++++++++++++++++++++--------------------------\n 1 file changed, 31 insertions(+), 31 deletions(-)\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex a9e274add..01c1d44a9 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -131,10 +131,10 @@ test_expect_success 'custom mergetool' '\n \tgit checkout -b test$test_count branch1 &&\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool file1 file1 ) &&\n-\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n+\tgit mergetool -y both >/dev/null 2>&1 &&\n+\tgit mergetool -y file1 file1 &&\n+\tgit mergetool -y file2 \"spaced name\" >/dev/null 2>&1 &&\n+\tgit mergetool -y subdir/file3 >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n@@ -154,11 +154,11 @@ test_expect_success 'mergetool crlf' '\n \ttest_config core.autocrlf true &&\n \tgit checkout -b test$test_count branch1 &&\n \ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n-\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 >/dev/null 2>&1 &&\n+\tgit mergetool file2 >/dev/null 2>&1 &&\n+\tgit mergetool \"spaced name\" >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n+\tgit mergetool subdir/file3 >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod >/dev/null 2>&1 ) &&\n@@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' '\n \t(\n \t\tcd subdir &&\n \t\ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n+\t\tgit mergetool file3 >/dev/null 2>&1 &&\n \t\ttest \"$(cat file3)\" = \"master new sub\"\n \t)\n '\n@@ -189,10 +189,10 @@ test_expect_success 'mergetool on file in parent dir' '\n \t(\n \t\tcd subdir &&\n \t\ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n-\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file1 >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 ) &&\n-\t\t( yes \"\" | git mergetool ../both >/dev/null 2>&1 ) &&\n+\t\tgit mergetool file3 >/dev/null 2>&1 &&\n+\t\tgit mergetool ../file1 >/dev/null 2>&1 &&\n+\t\tgit mergetool ../file2 ../spaced\\ name >/dev/null 2>&1 &&\n+\t\tgit mergetool ../both >/dev/null 2>&1 &&\n \t\t( yes \"d\" | git mergetool ../file11 >/dev/null 2>&1 ) &&\n \t\t( yes \"d\" | git mergetool ../file12 >/dev/null 2>&1 ) &&\n \t\t( yes \"l\" | git mergetool ../submod >/dev/null 2>&1 ) &&\n@@ -302,7 +302,7 @@ test_expect_success 'mergetool takes partial path' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge master &&\n \n-\t( yes \"\" | git mergetool subdir ) &&\n+\tgit mergetool subdir &&\n \n \ttest \"$(cat subdir/file3)\" = \"master new sub\"\n '\n@@ -370,8 +370,8 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit checkout -b test$test_count.a test$test_count &&\n \ttest_expect_code 1 git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n@@ -387,8 +387,8 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n@@ -401,8 +401,8 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest ! -e submod &&\n@@ -417,8 +417,8 @@ test_expect_success 'deleted vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n@@ -441,8 +441,8 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.a branch1 &&\n \ttest_expect_code 1 git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \trmdir submod && mv submod-movedaside submod &&\n@@ -457,8 +457,8 @@ test_expect_success 'file vs modified submodule' '\n \tgit checkout -b test$test_count.b test$test_count &&\n \ttest_expect_code 1 git merge master &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \tgit submodule update -N &&\n@@ -473,8 +473,8 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both >/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"r\" | git mergetool submod ) &&\n \ttest -d submod.orig &&\n@@ -489,8 +489,8 @@ test_expect_success 'file vs modified submodule' '\n \tgit submodule update -N &&\n \ttest_expect_code 1 git merge test$test_count &&\n \ttest -n \"$(git ls-files -u)\" &&\n-\t( yes \"\" | git mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 ) &&\n-\t( yes \"\" | git mergetool both>/dev/null 2>&1 ) &&\n+\tgit mergetool file1 file2 spaced\\ name subdir/file3 >/dev/null 2>&1 &&\n+\tgit mergetool both>/dev/null 2>&1 &&\n \t( yes \"d\" | git mergetool file11 file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod ) &&\n \ttest \"$(cat submod/bar)\" = \"master submodule\" &&\n-- \n2.12.0.5.gfbc750a84\n\n"},{"id":"313226","messageId":"xmqqd1dy880d.fsf@gitster.mtv.corp.google.com","threadId":"45277","inReplyTo":"20170303134022.GA6258@arch-attack.localdomain","subject":"Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-03-03T19:39:30Z","receivedAt":"2017-03-03T19:40:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu <liu.denton@gmail.com> writes:\n\n> In these tests, there are many situations where\n> 'echo \"\" | git mergetool' is used. This replaces all of those\n> occurrences with 'git mergetool -y' for simplicity and readability.\n\n\"-y where _possible_\" is not necessarily a good thing to do in\ntests.  We do want to make sure that both \"yes\" from the input and\n\"-y\" from the command line work.  Changes to \"-y\" done only for the\ntests that are not about accepting the interactive input from the\nend-user is a very good idea, but \"replaces all of those\" makes me\nworried.\n\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool file1 file1 ) &&\n> -\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> +\tgit mergetool -y both >/dev/null 2>&1 &&\n> +\tgit mergetool -y file1 file1 &&\n> +\tgit mergetool -y file2 \"spaced name\" >/dev/null 2>&1 &&\n> +\tgit mergetool -y subdir/file3 >/dev/null 2>&1 &&\n\nSo these replace \"the user interactively keeps typing <ENTER>\" with\n\"-y\", which sounds good.  These are obviously more about what the\ncode actually does.\n\n> -\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> -\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> +\tgit mergetool file1 >/dev/null 2>&1 &&\n> +\tgit mergetool file2 >/dev/null 2>&1 &&\n> +\tgit mergetool \"spaced name\" >/dev/null 2>&1 &&\n> +\tgit mergetool both >/dev/null 2>&1 &&\n> +\tgit mergetool subdir/file3 >/dev/null 2>&1 &&\n\nThe reason for the lack of \"-y\" in the rewrite, in contrast to what\nwas done in the previous hunk we saw, is not quite obvious.\n\n> @@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' '\n>  \t(\n>  \t\tcd subdir &&\n>  \t\ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n> -\t\t( yes \"\" | git mergetool file3 >/dev/null 2>&1 ) &&\n> +\t\tgit mergetool file3 >/dev/null 2>&1 &&\n\nLikewise, and likewise for the remainder of the patch where the\nupdated command line does not say \"-y\".\n"},{"id":"313236","messageId":"20170303212150.GA13790@arch-attack.localdomain","threadId":"45277","inReplyTo":"xmqqd1dy880d.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2017-03-03T21:21:50Z","receivedAt":"2017-03-03T21:22:18Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"On Fri, Mar 03, 2017 at 11:39:30AM -0800, Junio C Hamano wrote:\n> Denton Liu <liu.denton@gmail.com> writes:\n> \n> > In these tests, there are many situations where\n> > 'echo \"\" | git mergetool' is used. This replaces all of those\n> > occurrences with 'git mergetool -y' for simplicity and readability.\n> \n> \"-y where _possible_\" is not necessarily a good thing to do in\n> tests.  We do want to make sure that both \"yes\" from the input and\n> \"-y\" from the command line work.  Changes to \"-y\" done only for the\n> tests that are not about accepting the interactive input from the\n> end-user is a very good idea, but \"replaces all of those\" makes me\n> worried.\nThe 'custom mergetool' test case seems like a good place to introduce an\ninteractive test. Would the following patch to my patch work?\n\ndiff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh\nindex b6a419258..71624583c 100755\n--- a/t/t7610-mergetool.sh\n+++ b/t/t7610-mergetool.sh\n@@ -135,8 +135,8 @@ test_expect_success 'custom mergetool' '\n \ttest_expect_code 1 git merge master >/dev/null 2>&1 &&\n \tgit mergetool -y both >/dev/null 2>&1 &&\n \tgit mergetool -y file1 file1 &&\n-\tgit mergetool -y file2 \"spaced name\" >/dev/null 2>&1 &&\n-\tgit mergetool -y subdir/file3 >/dev/null 2>&1 &&\n+\t( yes \"\" | git mergetool file2 \"spaced name\" >/dev/null 2>&1 ) &&\n+\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file11 >/dev/null 2>&1 ) &&\n \t( yes \"d\" | git mergetool file12 >/dev/null 2>&1 ) &&\n \t( yes \"l\" | git mergetool submod >/dev/null 2>&1 ) &&\n\n> > -\t( yes \"\" | git mergetool file1 >/dev/null 2>&1 ) &&\n> > -\t( yes \"\" | git mergetool file2 >/dev/null 2>&1 ) &&\n> > -\t( yes \"\" | git mergetool \"spaced name\" >/dev/null 2>&1 ) &&\n> > -\t( yes \"\" | git mergetool both >/dev/null 2>&1 ) &&\n> > -\t( yes \"\" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&\n> > +\tgit mergetool file1 >/dev/null 2>&1 &&\n> > +\tgit mergetool file2 >/dev/null 2>&1 &&\n> > +\tgit mergetool \"spaced name\" >/dev/null 2>&1 &&\n> > +\tgit mergetool both >/dev/null 2>&1 &&\n> > +\tgit mergetool subdir/file3 >/dev/null 2>&1 &&\n> \n> The reason for the lack of \"-y\" in the rewrite, in contrast to what\n> was done in the previous hunk we saw, is not quite obvious.\n> \nSorry, it was my mistake. I had forgotten to replace the '-y'. I have\ncorrected this for a future revision.\n"}]}