threads / patch / 28569

patchgit-difftool: allow skipping file by typing 'n' at prompt

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

## tl;dr

14 messages between Oct 4, 2011 and Oct 10, 2011. Diffs are folded; open one to read it.

replies: 13people: 6as markdown or json

Sitaram Chamarty· Oct 4, 2011, 10:53 UTC · lore
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(-)
Show changes to git-difftool--helper.sh +5 −4
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
Junio C Hamano· Oct 4, 2011, 15:25 UTC · re: Sitaram Chamarty · lore

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

Sitaram Chamarty <sitaram@atc.tcs.com> writes:
Show 10 quoted lines
> 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.
Jeff King· Oct 4, 2011, 17:49 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 04, 2011 at 08:25:07AM -0700, Junio C Hamano wrote:
Show 16 quoted lines
> 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
Phil Hord· Oct 4, 2011, 18:02 UTC · re: Junio C Hamano · lore

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

On Tue, Oct 4, 2011 at 11:25 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 16 quoted lines
> 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
Junio C Hamano· Oct 4, 2011, 19:28 UTC · re: Phil Hord · lore

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

Phil Hord <phil.hord@gmail.com> writes:
Show 8 quoted lines
> 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.
Sitaram Chamarty· Oct 4, 2011, 23:05 UTC · re: Junio C Hamano · lore

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

On Wed, Oct 5, 2011 at 12:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 17 quoted lines
> 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.

Sitaram Chamarty· Oct 6, 2011, 12:56 UTC · re: Sitaram Chamarty · lore
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(-)
Show changes to git-difftool--helper.sh +5 −4
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
Junio C Hamano· Oct 6, 2011, 17:36 UTC · re: Sitaram Chamarty · lore

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

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?

Sitaram Chamarty· Oct 6, 2011, 18:15 UTC · re: Junio C Hamano · lore

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(-)
Show changes to git-difftool--helper.sh +5 −4
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
Junio C Hamano· Oct 7, 2011, 20:09 UTC · re: Sitaram Chamarty · lore

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

Sitaram Chamarty <sitaramc@gmail.com> writes:
Show 14 quoted lines
> 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)?

Show 37 quoted lines
>  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
>  }
Sitaram Chamarty· Oct 8, 2011, 13:10 UTC · re: Junio C Hamano · lore

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(-)
Show changes to 2 files +48 −5

git-difftool--helper.sh, t/t7800-difftool.sh

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
Charles Bailey· Oct 9, 2011, 11:26 UTC · re: Sitaram Chamarty · lore

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

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):
Show changes to git-difftool--helper.sh +7 −3
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
Junio C Hamano· Oct 10, 2011, 20:56 UTC · re: Sitaram Chamarty · lore

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

Sitaram Chamarty <sitaramc@gmail.com> writes:
Show 5 quoted lines
> 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.
Show 38 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
>  }

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

Show 22 quoted lines
> 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(-)
Show changes to 2 files +48 −4

git-difftool--helper.sh, t/t7800-difftool.sh

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
Sitaram Chamarty· Oct 10, 2011, 23:39 UTC · re: Junio C Hamano · lore

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

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

← back to recent threads