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

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

[Sitaram Chamarty]

Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
---

I'm using what is pretty much a universal convention to
signify that the default choice is "y"; I hope documentation
for something so small is not needed but if it is, let me
know and I'll do that also.

 git-difftool--helper.sh |    9 +++++----
 1 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 8452890..bc1b098 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
 }
 
-- 
1.7.6

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

[Junio C Hamano]

Sitaram Chamarty <sitaram@atc.tcs.com> writes:

> Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
> ---
>
> I'm using what is pretty much a universal convention to
> signify that the default choice is "y"; I hope documentation
> for something so small is not needed but if it is, let me
> know and I'll do that also.
>
> -			printf "Hit return to launch '%s': " \
> +			printf "Launch '%s' [y]/n: " \

I think I've seen this done as: "do this? [Y/n]" elsewhere.

Not telling you what to do, but trying to feel what others may think.

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

[Jeff King]

On Tue, Oct 04, 2011 at 08:25:07AM -0700, Junio C Hamano wrote:

> Sitaram Chamarty <sitaram@atc.tcs.com> writes:
> 
> > Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
> > ---
> >
> > I'm using what is pretty much a universal convention to
> > signify that the default choice is "y"; I hope documentation
> > for something so small is not needed but if it is, let me
> > know and I'll do that also.
> >
> > -			printf "Hit return to launch '%s': " \
> > +			printf "Launch '%s' [y]/n: " \
> 
> I think I've seen this done as: "do this? [Y/n]" elsewhere.
> 
> Not telling you what to do, but trying to feel what others may think.

Yes, that was my immediate thought, too (or even [Yn]).

-Peff

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

[Phil Hord]

On Tue, Oct 4, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Sitaram Chamarty <sitaram@atc.tcs.com> writes:
>
>> Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
>> ---
>>
>> I'm using what is pretty much a universal convention to
>> signify that the default choice is "y"; I hope documentation
>> for something so small is not needed but if it is, let me
>> know and I'll do that also.
>>
>> -                     printf "Hit return to launch '%s': " \
>> +                     printf "Launch '%s' [y]/n: " \
>
> I think I've seen this done as: "do this? [Y/n]" elsewhere.
>
> Not telling you what to do, but trying to feel what others may think.

I think so, too.  The [y]/n syntax is not clear enough for me to
confidently know what the default value will be.

Phil

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

[Junio C Hamano]

Phil Hord <phil.hord@gmail.com> writes:

> On Tue, Oct 4, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> I think I've seen this done as: "do this? [Y/n]" elsewhere.
>>
>> Not telling you what to do, but trying to feel what others may think.
>
> I think so, too.  The [y]/n syntax is not clear enough for me to
> confidently know what the default value will be.

One downside of "do this [Y,n,m,o,p,q]? " is that it limits us to
lowercase responses, which means we cannot assign 'q' for quitting from
the innermost nested context and assign 'Q' for quitting from the whole
interactive loop (e.g. "git add -p").

    "do this [y,n,m,o,p,q] (default=y)? "

may have been a better choice in hindsight.

No matter what we end up doing, let's try to be consistent.

Thanks.

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

[Sitaram Chamarty]

On Wed, Oct 5, 2011 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Phil Hord <phil.hord@gmail.com> writes:
>
>> On Tue, Oct 4, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> I think I've seen this done as: "do this? [Y/n]" elsewhere.
>>>
>>> Not telling you what to do, but trying to feel what others may think.
>>
>> I think so, too.  The [y]/n syntax is not clear enough for me to
>> confidently know what the default value will be.
>
> One downside of "do this [Y,n,m,o,p,q]? " is that it limits us to
> lowercase responses, which means we cannot assign 'q' for quitting from
> the innermost nested context and assign 'Q' for quitting from the whole
> interactive loop (e.g. "git add -p").
>
>    "do this [y,n,m,o,p,q] (default=y)? "

Does this even make a difference in this case?  I was going to send
out a new patch using [Y/n] instead of my original [y]/n.  There's
only one loop in this thing, and till now people have been presumably
hitting Ctrl-C to get out of it.  I see no real need to make that more
elegant; all I set out to do is add one teeny weeny bit of
functionality to a prompt that -- other than giving you a chance to
hit that Ctrl-C -- was not actually doing anything useful at all.

>
> may have been a better choice in hindsight.
>
> No matter what we end up doing, let's try to be consistent.

The only other part of git where I have ever used a prompt is 'git add
-p'.  Consistency with *that* prompt, to me, would mean colors.  And
help text.  And I'm not sure what else, really, since I only used it
superficially.

Isn't that overkill for this case?

I'll wait a few hours for any further comments then send out a patch
that is the same as my original one except it uses [Y/n] instead of
[y]/n.

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

[Sitaram Chamarty]

Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
---

(re-rolled according to earlier discussion)

 git-difftool--helper.sh |    9 +++++----
 1 files changed, 5 insertions(+), 4 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
 }
 
-- 
1.7.6

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

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
> ---
>
> (re-rolled according to earlier discussion)

Thanks. It is clear from the subject and the patch text that you are
changing "hit return to unconditionally launch" into "launch it if you
want to", but can you give justification why a choice not to launch is
needed in the log message?

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

[Sitaram Chamarty]

This is useful if you forgot to restrict the diff to the paths you want
to see, or selecting precisely the ones you want is too much typing.

Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
---

On Thu, Oct 06, 2011 at 10:36:40AM -0700, Junio C Hamano wrote:

> Thanks. It is clear from the subject and the patch text that you are
> changing "hit return to unconditionally launch" into "launch it if you
> want to", but can you give justification why a choice not to launch is
> needed in the log message?

OK; done.

 git-difftool--helper.sh |    9 +++++----
 1 files changed, 5 insertions(+), 4 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
 }
 
-- 
1.7.6

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

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> This is useful if you forgot to restrict the diff to the paths you want
> to see, or selecting precisely the ones you want is too much typing.
>
> Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
> ---
>
> On Thu, Oct 06, 2011 at 10:36:40AM -0700, Junio C Hamano wrote:
>
>> Thanks. It is clear from the subject and the patch text that you are
>> changing "hit return to unconditionally launch" into "launch it if you
>> want to", but can you give justification why a choice not to launch is
>> needed in the log message?
>
> OK; done.

Looks OK from a cursory viewing. Do we want some additional tests?

For that matter, have you run the test suite with this patch applied (I
haven't)?

>  git-difftool--helper.sh |    9 +++++----
>  1 files changed, 5 insertions(+), 4 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
>  }

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

[Sitaram Chamarty]

This is useful if you forgot to restrict the diff to the paths you want
to see, or selecting precisely the ones you want is too much typing.

Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
---

On Fri, Oct 07, 2011 at 01:09:11PM -0700, Junio C Hamano wrote:

> Looks OK from a cursory viewing. Do we want some additional tests?
> 
> For that matter, have you run the test suite with this patch applied (I
> haven't)?

OK; done.  I got some "broken" but nothing "failed":

    make aggregate-results
    make[3]: Entering directory `/home/sitaram/clones/git/t'
    for f in test-results/t*-*.counts; do \
            echo "$f"; \
    done | '/bin/sh' ./aggregate-results.sh
    fixed   0
    success 7377
    failed  0
    broken  49
    total   7461

Hope that is not a problem.

However, I'm not sure the file names that 'git difftool'
comes up with are in a predictable order.  That would mess
up the test, but I can neither make it fail not find
definitive information on the order in which the changed
files are processed.

 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
 }
 
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 395adfc..f547e0b 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -38,7 +38,18 @@ restore_test_defaults()
 prompt_given()
 {
 	prompt="$1"
-	test "$prompt" = "Hit return to launch 'test-tool': branch"
+	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
+}
+
+stdin_contains()
+{
+	grep >/dev/null "$1"
+}
+
+stdin_doesnot_contain()
+{
+	grep >/dev/null "$1" && return 1
+	return 0
 }
 
 # Create a file on master and change it on branch
@@ -265,4 +276,35 @@ test_expect_success PERL 'difftool --extcmd cat arg2' '
 	test "$diff" = branch
 '
 
+# Create a second file on master and a different version on branch
+test_expect_success PERL 'setup with 2 files different' '
+	echo m2 >file2 &&
+	git add file2 &&
+	git commit -m "added file2" &&
+
+	git checkout branch &&
+	echo br2 >file2 &&
+	git add file2 &&
+	git commit -a -m "branch changed file2" &&
+	git checkout master
+'
+
+test_expect_success PERL 'say no to the first file' '
+	diff=$((echo n; echo) | git difftool -x cat branch) &&
+
+	echo "$diff" | stdin_contains m2 &&
+	echo "$diff" | stdin_contains br2 &&
+	echo "$diff" | stdin_doesnot_contain master &&
+	echo "$diff" | stdin_doesnot_contain branch
+'
+
+test_expect_success PERL 'say no to the second file' '
+	diff=$((echo; echo n) | git difftool -x cat branch) &&
+
+	echo "$diff" | stdin_contains master &&
+	echo "$diff" | stdin_contains branch &&
+	echo "$diff" | stdin_doesnot_contain m2 &&
+	echo "$diff" | stdin_doesnot_contain br2
+'
+
 test_done
-- 
1.7.6

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

[Charles Bailey]

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

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

[Junio C Hamano]

Sitaram Chamarty <sitaramc@gmail.com> writes:

> However, I'm not sure the file names that 'git difftool'
> comes up with are in a predictable order.  That would mess
> up the test, but I can neither make it fail not find
> definitive information on the order in which the changed
> files are processed.

Hmm, that may be an issue, I would think.

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

I also found suggestion by Charles Bailey to return from the launch
function when the user says "no" easier to follow.

> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 395adfc..f547e0b 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -38,7 +38,18 @@ restore_test_defaults()
>  prompt_given()
>  {
>  	prompt="$1"
> -	test "$prompt" = "Hit return to launch 'test-tool': branch"
> +	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
> +}
> +
> +stdin_contains()
> +{
> +	grep >/dev/null "$1"
> +}
> +
> +stdin_doesnot_contain()
> +{
> +	grep >/dev/null "$1" && return 1
> +	return 0
>  }

Doesn't

	! grep >/dev/null "$1"

work in this case?        

I also wondered if this is easier to read:

	pipe | stdin_contains m2 &&
	! pipe | stdin_contains master

but I do not think it is (we cannot say "pipe | ! stdin_contains master").

In any case, here is what I ended up queuing.  Thanks.

-- >8 --
From: Sitaram Chamarty <sitaramc@gmail.com>
Date: Sat, 8 Oct 2011 18:40:15 +0530
Subject: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt

This is useful if you forgot to restrict the diff to the paths you want
to see, or selecting precisely the ones you want is too much typing.

[jc: with a change to return from the function upon 'n' by Charles Bailey
and a small tweak in stdin_doesnot_contain() in the test]

Signed-off-by: Sitaram Chamarty <sitaram@atc.tcs.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 git-difftool--helper.sh |    9 ++++++---
 t/t7800-difftool.sh     |   43 ++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index 8452890..e6558d1 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -43,12 +43,15 @@ 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
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 395adfc..7fc2b3a 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -38,7 +38,17 @@ restore_test_defaults()
 prompt_given()
 {
 	prompt="$1"
-	test "$prompt" = "Hit return to launch 'test-tool': branch"
+	test "$prompt" = "Launch 'test-tool' [Y/n]: branch"
+}
+
+stdin_contains()
+{
+	grep >/dev/null "$1"
+}
+
+stdin_doesnot_contain()
+{
+	! stdin_contains "$1"
 }
 
 # Create a file on master and change it on branch
@@ -265,4 +275,35 @@ test_expect_success PERL 'difftool --extcmd cat arg2' '
 	test "$diff" = branch
 '
 
+# Create a second file on master and a different version on branch
+test_expect_success PERL 'setup with 2 files different' '
+	echo m2 >file2 &&
+	git add file2 &&
+	git commit -m "added file2" &&
+
+	git checkout branch &&
+	echo br2 >file2 &&
+	git add file2 &&
+	git commit -a -m "branch changed file2" &&
+	git checkout master
+'
+
+test_expect_success PERL 'say no to the first file' '
+	diff=$((echo n; echo) | git difftool -x cat branch) &&
+
+	echo "$diff" | stdin_contains m2 &&
+	echo "$diff" | stdin_contains br2 &&
+	echo "$diff" | stdin_doesnot_contain master &&
+	echo "$diff" | stdin_doesnot_contain branch
+'
+
+test_expect_success PERL 'say no to the second file' '
+	diff=$((echo; echo n) | git difftool -x cat branch) &&
+
+	echo "$diff" | stdin_contains master &&
+	echo "$diff" | stdin_contains branch &&
+	echo "$diff" | stdin_doesnot_contain m2 &&
+	echo "$diff" | stdin_doesnot_contain br2
+'
+
 test_done
-- 
1.7.7.138.g7f41b6

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

[Sitaram Chamarty]

On Tue, Oct 11, 2011 at 2:26 AM, Junio C Hamano <gitster@pobox.com> wrote:

> I also wondered if this is easier to read:
>
>        pipe | stdin_contains m2 &&
>        ! pipe | stdin_contains master

> but I do not think it is (we cannot say "pipe | ! stdin_contains master").

Agreed on both counts.

"pipe | ( ! grep master )" does work, but I suspect that is an
inconsistency in the shell so I didn't want to use it.  IIRC the "(
list )" constrict is not supposed to make *that* much difference.
Have to check when I have time.

> In any case, here is what I ended up queuing.  Thanks.

> +stdin_doesnot_contain()
> +{
> +       ! stdin_contains "$1"
>  }

(facepalm) Why didn't I think of that!

Thanks :-)