Re: difftool--helper: exit when reading a prompt answer fails

From: David Aguilar <davvid@gmail.com>
Date: Oct 27, 2014, 00:41 UTC
Message-ID: <20141027004148.GB81127@gmail.com>
In-Reply-To: <544CAC30.7000607@kdbg.org>

On Sun, Oct 26, 2014 at 09:09:20AM +0100, Johannes Sixt wrote:

Show 8 quoted lines

> An attempt to quit difftool by hitting Ctrl-D (EOF) at its prompt does
> not quit it, but is treated as if 'yes' was answered to the prompt and
> all following prompts, which is contrary to the user's intent. Fix the
> error check.
> 
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
>  Found while reviewing your latest patch.

Thanks for the careful review. I have one small question about the test below.

I'll base the reroll of my patch on top of this one so that I'm including your new test while developing.

Junio, can you please queue this patch before my upcoming v3 patch when I reroll?

I also noticed that I forgot to "git reset --hard" after doing "git add" in one of the tests I introduced so I'll be adjusting that as well in v3.

Show 39 quoted lines

>  I chose the 'foo || return' idiom for the error check, but left the
>  'if' for the interesting check, because I feel it is more readable
>  than 'test ... && return'.
> 
>  -- Hannes
> 
>  git-difftool--helper.sh | 3 ++-
>  t/t7800-difftool.sh     | 8 ++++++++
>  2 files changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 7ef36b9..aca0413 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -49,7 +49,8 @@ launch_merge_tool () {
>  		else
>  			printf "Launch '%s' [Y/n]: " "$merge_tool"
>  		fi
> -		if read ans && test "$ans" = n
> +		read ans || return
> +		if test "$ans" = n
>  		then
>  			return
>  		fi
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index dc30a51..9cf5dc9 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -301,6 +301,14 @@ test_expect_success PERL 'say no to the second file' '
>  	! grep br2 output
>  '
>  
> +test_expect_success PERL 'ending prompt input with EOF' '
> +	git difftool -x cat branch </dev/null >output &&
> +	! grep master output &&
> +	! grep branch output &&
> +	! grep m2 output &&
> +	! grep br2 output
> +'

Should we use "test_must_fail grep ..." instead of "! grep ..." here?

-- 
David