* [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
@ 2017-03-03 13:40 Denton Liu
2017-03-03 19:39 ` Junio C Hamano
0 siblings, 1 reply; 3+ messages in thread
From: Denton Liu @ 2017-03-03 13:40 UTC (permalink / raw)
To: git; +Cc: davvid, Johannes.Schindelin
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 <liu.denton@gmail.com>
---
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
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
2017-03-03 13:40 [PATCH v2 2/5] Use -y where possible in test t7610-mergetool Denton Liu
@ 2017-03-03 19:39 ` Junio C Hamano
2017-03-03 21:21 ` Denton Liu
0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2017-03-03 19:39 UTC (permalink / raw)
To: Denton Liu; +Cc: git, davvid, Johannes.Schindelin
Denton Liu <liu.denton@gmail.com> 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 <ENTER>" 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".
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
2017-03-03 19:39 ` Junio C Hamano
@ 2017-03-03 21:21 ` Denton Liu
0 siblings, 0 replies; 3+ messages in thread
From: Denton Liu @ 2017-03-03 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, davvid, Johannes.Schindelin
On Fri, Mar 03, 2017 at 11:39:30AM -0800, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> 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.
^ permalink raw reply related [flat|nested] 3+ messages in thread
end of thread, other threads:[~2017-03-03 21:22 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-03 13:40 [PATCH v2 2/5] Use -y where possible in test t7610-mergetool Denton Liu
2017-03-03 19:39 ` Junio C Hamano
2017-03-03 21:21 ` Denton Liu
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).