* [PATCH] git-difftool: allow skipping file by typing 'n' at prompt @ 2011-10-04 10:53 Sitaram Chamarty 2011-10-04 15:25 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-04 10:53 UTC (permalink / raw) To: git, gitster 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 10:53 [PATCH] git-difftool: allow skipping file by typing 'n' at prompt Sitaram Chamarty @ 2011-10-04 15:25 ` Junio C Hamano 2011-10-04 17:49 ` Jeff King 2011-10-04 18:02 ` Phil Hord 0 siblings, 2 replies; 14+ messages in thread From: Junio C Hamano @ 2011-10-04 15:25 UTC (permalink / raw) To: Sitaram Chamarty; +Cc: git 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 15:25 ` Junio C Hamano @ 2011-10-04 17:49 ` Jeff King 2011-10-04 18:02 ` Phil Hord 1 sibling, 0 replies; 14+ messages in thread From: Jeff King @ 2011-10-04 17:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sitaram Chamarty, git 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 15:25 ` Junio C Hamano 2011-10-04 17:49 ` Jeff King @ 2011-10-04 18:02 ` Phil Hord 2011-10-04 19:28 ` Junio C Hamano 1 sibling, 1 reply; 14+ messages in thread From: Phil Hord @ 2011-10-04 18:02 UTC (permalink / raw) To: Junio C Hamano; +Cc: Sitaram Chamarty, git 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 ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 18:02 ` Phil Hord @ 2011-10-04 19:28 ` Junio C Hamano 2011-10-04 23:05 ` Sitaram Chamarty 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2011-10-04 19:28 UTC (permalink / raw) To: Phil Hord; +Cc: Sitaram Chamarty, git 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 19:28 ` Junio C Hamano @ 2011-10-04 23:05 ` Sitaram Chamarty 2011-10-06 12:56 ` Sitaram Chamarty 0 siblings, 1 reply; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-04 23:05 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Sitaram Chamarty, git 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. ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-04 23:05 ` Sitaram Chamarty @ 2011-10-06 12:56 ` Sitaram Chamarty 2011-10-06 17:36 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-06 12:56 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Sitaram Chamarty, git 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-06 12:56 ` Sitaram Chamarty @ 2011-10-06 17:36 ` Junio C Hamano 2011-10-06 18:15 ` Sitaram Chamarty 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2011-10-06 17:36 UTC (permalink / raw) To: Sitaram Chamarty; +Cc: Phil Hord, Sitaram Chamarty, git 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? ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-06 17:36 ` Junio C Hamano @ 2011-10-06 18:15 ` Sitaram Chamarty 2011-10-07 20:09 ` Junio C Hamano 0 siblings, 1 reply; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-06 18:15 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Sitaram Chamarty, git 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-06 18:15 ` Sitaram Chamarty @ 2011-10-07 20:09 ` Junio C Hamano 2011-10-08 13:10 ` Sitaram Chamarty 0 siblings, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2011-10-07 20:09 UTC (permalink / raw) To: Sitaram Chamarty; +Cc: Phil Hord, Sitaram Chamarty, git 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 > } ^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-07 20:09 ` Junio C Hamano @ 2011-10-08 13:10 ` Sitaram Chamarty 2011-10-09 11:26 ` Charles Bailey 2011-10-10 20:56 ` Junio C Hamano 0 siblings, 2 replies; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-08 13:10 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Sitaram Chamarty, git 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-08 13:10 ` Sitaram Chamarty @ 2011-10-09 11:26 ` Charles Bailey 2011-10-10 20:56 ` Junio C Hamano 1 sibling, 0 replies; 14+ messages in thread From: Charles Bailey @ 2011-10-09 11:26 UTC (permalink / raw) To: Sitaram Chamarty; +Cc: Junio C Hamano, Phil Hord, Sitaram Chamarty, git 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-08 13:10 ` Sitaram Chamarty 2011-10-09 11:26 ` Charles Bailey @ 2011-10-10 20:56 ` Junio C Hamano 2011-10-10 23:39 ` Sitaram Chamarty 1 sibling, 1 reply; 14+ messages in thread From: Junio C Hamano @ 2011-10-10 20:56 UTC (permalink / raw) To: Sitaram Chamarty; +Cc: Phil Hord, Sitaram Chamarty, git 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 ^ permalink raw reply related [flat|nested] 14+ messages in thread
* Re: [PATCH] git-difftool: allow skipping file by typing 'n' at prompt 2011-10-10 20:56 ` Junio C Hamano @ 2011-10-10 23:39 ` Sitaram Chamarty 0 siblings, 0 replies; 14+ messages in thread From: Sitaram Chamarty @ 2011-10-10 23:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Phil Hord, Sitaram Chamarty, git 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 :-) ^ permalink raw reply [flat|nested] 14+ messages in thread
end of thread, other threads:[~2011-10-10 23:39 UTC | newest] Thread overview: 14+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2011-10-04 10:53 [PATCH] git-difftool: allow skipping file by typing 'n' at prompt Sitaram Chamarty 2011-10-04 15:25 ` Junio C Hamano 2011-10-04 17:49 ` Jeff King 2011-10-04 18:02 ` Phil Hord 2011-10-04 19:28 ` Junio C Hamano 2011-10-04 23:05 ` Sitaram Chamarty 2011-10-06 12:56 ` Sitaram Chamarty 2011-10-06 17:36 ` Junio C Hamano 2011-10-06 18:15 ` Sitaram Chamarty 2011-10-07 20:09 ` Junio C Hamano 2011-10-08 13:10 ` Sitaram Chamarty 2011-10-09 11:26 ` Charles Bailey 2011-10-10 20:56 ` Junio C Hamano 2011-10-10 23:39 ` Sitaram Chamarty
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).