# [PATCH] difftool/mergetool: make the form of yes/no questions consistent 6 messages from 2016-04-12 to 2016-04-12. Participants: Nikola Forró, John Keeping, Junio C Hamano, David Aguilar. Thread: https://gitlist.dev/t/42002 ## Nikola Forró, 2016-04-12 12:59 Subject: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: <1460465982.3214.7.camel@redhat.com> URL: https://gitlist.dev/e/1460465982.3214.7.camel%40redhat.com ``` Every yes/no question in difftool/mergetool scripts has slightly different form, and none of them is consistent with the form git itself uses. Make the form of all the questions consistent with the form used by git, i.e. "Question [y/n]? ". Signed-off-by: Nikola Forró --- git-difftool--helper.sh | 4 ++-- git-mergetool--lib.sh | 2 +- git-mergetool.sh | 2 +- 3 files changed, 4 insertions(+), 4 deletions(-) diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh index 2b11b1d..84d6cc0 100755 --- a/git-difftool--helper.sh +++ b/git-difftool--helper.sh @@ -44,10 +44,10 @@ launch_merge_tool () { "$GIT_DIFF_PATH_TOTAL" "$MERGED" if use_ext_cmd then - printf "Launch '%s' [Y/n]: " \ + printf "Launch '%s' [Y/n]? " \ "$GIT_DIFFTOOL_EXTCMD" else - printf "Launch '%s' [Y/n]: " "$merge_tool" + printf "Launch '%s' [Y/n]? " "$merge_tool" fi read ans || return if test "$ans" = n diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh index 54ac8e4..92adcc0 100644 --- a/git-mergetool--lib.sh +++ b/git-mergetool--lib.sh @@ -100,7 +100,7 @@ check_unchanged () { while true do echo "$MERGED seems unchanged." - printf "Was the merge successful? [y/n] " + printf "Was the merge successful [y/n]? " read answer || return 1 case "$answer" in y*|Y*) return 0 ;; diff --git a/git-mergetool.sh b/git-mergetool.sh index 9f77e3a..2e0635a 100755 --- a/git-mergetool.sh +++ b/git-mergetool.sh @@ -396,7 +396,7 @@ done prompt_after_failed_merge () { while true do - printf "Continue merging other unresolved paths (y/n) ? " + printf "Continue merging other unresolved paths [y/n]? " read ans || return 1 case "$ans" in [yY]*) -- 2.4.11 ``` ## John Keeping, 2016-04-12 13:27 Subject: Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: <20160412132737.GB11013@serenity.lan> URL: https://gitlist.dev/e/20160412132737.GB11013%40serenity.lan In-Reply-To: <1460465982.3214.7.camel@redhat.com> ``` On Tue, Apr 12, 2016 at 02:59:42PM +0200, Nikola Forrrr wrote: > Every yes/no question in difftool/mergetool scripts has slightly > different form, and none of them is consistent with the form git > itself uses. > > Make the form of all the questions consistent with the form used > by git, i.e. "Question [y/n]? ". > > Signed-off-by: Nikola Forró > --- > git-difftool--helper.sh | 4 ++-- > git-mergetool--lib.sh | 2 +- > git-mergetool.sh | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh > index 2b11b1d..84d6cc0 100755 > --- a/git-difftool--helper.sh > +++ b/git-difftool--helper.sh > @@ -44,10 +44,10 @@ launch_merge_tool () { > "$GIT_DIFF_PATH_TOTAL" "$MERGED" > if use_ext_cmd > then > - printf "Launch '%s' [Y/n]: " \ > + printf "Launch '%s' [Y/n]? " \ > "$GIT_DIFFTOOL_EXTCMD" > else > - printf "Launch '%s' [Y/n]: " "$merge_tool" > + printf "Launch '%s' [Y/n]? " "$merge_tool" I think the case in these two is correct as-is. The "Y" is capitalised because it is the default and will take effect if the user just presses ENTER. From a quick look, the two cases below do not have this behaviour (the user must enter either "y" or "n"), so it is correct that they are not capitalized. The change from ":" to "?" and normalization of "?" placement below seems reasonable. > fi > read ans || return > if test "$ans" = n > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 54ac8e4..92adcc0 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -100,7 +100,7 @@ check_unchanged () { > while true > do > echo "$MERGED seems unchanged." > - printf "Was the merge successful? [y/n] " > + printf "Was the merge successful [y/n]? " > read answer || return 1 > case "$answer" in > y*|Y*) return 0 ;; > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 9f77e3a..2e0635a 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -396,7 +396,7 @@ done > prompt_after_failed_merge () { > while true > do > - printf "Continue merging other unresolved paths (y/n) ? " > + printf "Continue merging other unresolved paths [y/n]? " > read ans || return 1 > case "$ans" in > [yY]*) > -- > 2.4.11 ``` ## Nikola Forró, 2016-04-12 13:53 Subject: Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: <1460469192.3214.17.camel@redhat.com> URL: https://gitlist.dev/e/1460469192.3214.17.camel%40redhat.com In-Reply-To: <20160412132737.GB11013@serenity.lan> ``` On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote: > I think the case in these two is correct as-is. The "Y" is capitalised > because it is the default and will take effect if the user just presses > ENTER. Thanks John, I'm aware of that. That's why the patch doesn't change the case. Maybe I should have mention that explicitly in the commit message. ``` ## John Keeping, 2016-04-12 14:01 Subject: Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: <20160412140119.GC11013@serenity.lan> URL: https://gitlist.dev/e/20160412140119.GC11013%40serenity.lan In-Reply-To: <1460469192.3214.17.camel@redhat.com> ``` On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Forrrr wrote: > On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote: > > I think the case in these two is correct as-is. The "Y" is capitalised > > because it is the default and will take effect if the user just presses > > ENTER. > > Thanks John, I'm aware of that. That's why the patch doesn't change > the case. Maybe I should have mention that explicitly in the commit > message. Sorry, I completely missed that. Your patch does in fact look good, so: Reviewed-by: John Keeping I think I was taken in by the commit message saying 'i.e. "Question [y/n]? "' and didn't examine the patch carefully enough. It might be better just to drop the example since it's obvious what the patch does. ``` ## Junio C Hamano, 2016-04-12 16:15 Subject: Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: URL: https://gitlist.dev/e/xmqq8u0isszw.fsf%40gitster.mtv.corp.google.com In-Reply-To: <1460465982.3214.7.camel@redhat.com> ``` Nikola Forró writes: > Every yes/no question in difftool/mergetool scripts has slightly > different form, and none of them is consistent with the form git > itself uses. > > Make the form of all the questions consistent with the form used > by git, i.e. "Question [y/n]? ". > > Signed-off-by: Nikola Forró > --- Looks good from a cursory read. David? > git-difftool--helper.sh | 4 ++-- > git-mergetool--lib.sh | 2 +- > git-mergetool.sh | 2 +- > 3 files changed, 4 insertions(+), 4 deletions(-) > > diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh > index 2b11b1d..84d6cc0 100755 > --- a/git-difftool--helper.sh > +++ b/git-difftool--helper.sh > @@ -44,10 +44,10 @@ launch_merge_tool () { > "$GIT_DIFF_PATH_TOTAL" "$MERGED" > if use_ext_cmd > then > - printf "Launch '%s' [Y/n]: " \ > + printf "Launch '%s' [Y/n]? " \ > "$GIT_DIFFTOOL_EXTCMD" > else > - printf "Launch '%s' [Y/n]: " "$merge_tool" > + printf "Launch '%s' [Y/n]? " "$merge_tool" > fi > read ans || return > if test "$ans" = n > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh > index 54ac8e4..92adcc0 100644 > --- a/git-mergetool--lib.sh > +++ b/git-mergetool--lib.sh > @@ -100,7 +100,7 @@ check_unchanged () { > while true > do > echo "$MERGED seems unchanged." > - printf "Was the merge successful? [y/n] " > + printf "Was the merge successful [y/n]? " > read answer || return 1 > case "$answer" in > y*|Y*) return 0 ;; > diff --git a/git-mergetool.sh b/git-mergetool.sh > index 9f77e3a..2e0635a 100755 > --- a/git-mergetool.sh > +++ b/git-mergetool.sh > @@ -396,7 +396,7 @@ done > prompt_after_failed_merge () { > while true > do > - printf "Continue merging other unresolved paths (y/n) ? " > + printf "Continue merging other unresolved paths [y/n]? " > read ans || return 1 > case "$ans" in > [yY]*) ``` ## David Aguilar, 2016-04-12 18:05 Subject: Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent Message-ID: <20160412180546.GA27558@gmail.com> URL: https://gitlist.dev/e/20160412180546.GA27558%40gmail.com In-Reply-To: <20160412140119.GC11013@serenity.lan> ``` On Tue, Apr 12, 2016 at 03:01:19PM +0100, John Keeping wrote: > On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Forrrr wrote: > > On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote: > > > I think the case in these two is correct as-is. The "Y" is capitalised > > > because it is the default and will take effect if the user just presses > > > ENTER. > > > > Thanks John, I'm aware of that. That's why the patch doesn't change > > the case. Maybe I should have mention that explicitly in the commit > > message. > > Sorry, I completely missed that. Your patch does in fact look good, so: > > Reviewed-by: John Keeping > > I think I was taken in by the commit message saying 'i.e. "Question > [y/n]? "' and didn't examine the patch carefully enough. It might be > better just to drop the example since it's obvious what the patch does. Thanks for reviewing. Acked-by: David Aguilar cheers, -- David ```