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

From: Charles Bailey <charles@hashpling.org>
Date: Oct 9, 2011, 11:26 UTC
Message-ID: <20111009112623.GA30585@hashpling.org>
In-Reply-To: <20111008131015.GA28213@sita-lt.atc.tcs.com>

On Sat, Oct 08, 2011 at 06:40:15PM +0530, Sitaram Chamarty wrote:

Show 39 quoted lines

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