Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt

[Charles Bailey <charles@hashpling.org>]

On Sat, Oct 08, 2011 at 06:40:15PM +0530, Sitaram Chamarty wrote:
> 
>  git-difftool--helper.sh |    9 +++++----
>  t/t7800-difftool.sh     |   44 +++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 48 insertions(+), 5 deletions(-)
> 
> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
> index 8452890..0468446 100755
> --- a/git-difftool--helper.sh
> +++ b/git-difftool--helper.sh
> @@ -38,15 +38,16 @@ launch_merge_tool () {
>  
>  	# $LOCAL and $REMOTE are temporary files so prompt
>  	# the user with the real $MERGED name before launching $merge_tool.
> +	ans=y
>  	if should_prompt
>  	then
>  		printf "\nViewing: '$MERGED'\n"
>  		if use_ext_cmd
>  		then
> -			printf "Hit return to launch '%s': " \
> +			printf "Launch '%s' [Y/n]: " \
>  				"$GIT_DIFFTOOL_EXTCMD"
>  		else
> -			printf "Hit return to launch '%s': " "$merge_tool"
> +			printf "Launch '%s' [Y/n]: " "$merge_tool"
>  		fi
>  		read ans
>  	fi
> @@ -54,9 +55,9 @@ launch_merge_tool () {
>  	if use_ext_cmd
>  	then
>  		export BASE
> -		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
> +		test "$ans" != "n" && eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
>  	else
> -		run_merge_tool "$merge_tool"
> +		test "$ans" != "n" && run_merge_tool "$merge_tool"
>  	fi
>  }

It's a minor point but for me, this looks a little more difficult to
follow than it needs to be.

Why do we need to hold on to 'ans' for so long? With the new prompt,
if we ever 'read ans' we always want to return from the
launch_merge_tool without doing anything else if we read "n". I think
it's easier to follow if we just change 'read ans' and leave the 'if
use_ext_cmd' clauses alone. Perhaps some people don't like the early
return, though?

Charles.


E.g. (for discussion, untested):

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 8452890..b668a12 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -43,12 +43,16 @@ launch_merge_tool () {
                printf "\nViewing: '$MERGED'\n"
                if use_ext_cmd
                then
-                       printf "Hit return to launch '%s': " \
+                       printf "Launch '%s' [Y/n]: " \
                                "$GIT_DIFFTOOL_EXTCMD"
                else
-                       printf "Hit return to launch '%s': " "$merge_tool"
+                       printf "Launch '%s' [Y/n]: " "$merge_tool"
+               fi
+
+               if read ans && test "$ans" = "n"
+               then
+                       return
                fi
-               read ans
        fi

        if use_ext_cmd