From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 03 Mar 2017 19:39:30 GMT
Subject: Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
Message-ID: <xmqqd1dy880d.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170303134022.GA6258@arch-attack.localdomain>

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".