Denton Liu <liu.den...@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".
