threads / patch / 42002

patchdifftool/mergetool: make the form of yes/no questions consistent

Subject: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

## tl;dr

6 messages between Apr 12, 2016 and Apr 12, 2016. Diffs are folded; open one to read it.

replies: 5people: 4as markdown or json

Nikola Forró· Apr 12, 2016, 12:59 UTC · lore

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ó <nforro@redhat.com>
---
 git-difftool--helper.sh | 4 ++--
 git-mergetool--lib.sh   | 2 +-
 git-mergetool.sh        | 2 +-
 3 files changed, 4 insertions(+), 4 deletions(-)

Show changes to 3 files +4 −4

git-difftool--helper.sh, git-mergetool--lib.sh, git-mergetool.sh

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· Apr 12, 2016, 13:27 UTC · re: Nikola Forró · lore

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

On Tue, Apr 12, 2016 at 02:59:42PM +0200, Nikola Forrrr wrote:

Show 28 quoted lines

> 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ó <nforro@redhat.com>
> ---
>  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.

Show 31 quoted lines

>  		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ó· Apr 12, 2016, 13:53 UTC · re: John Keeping · lore

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

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· Apr 12, 2016, 14:01 UTC · re: Nikola Forró · lore

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Forrrr wrote:

Show 8 quoted lines

> 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 <john@keeping.me.uk>

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.

David Aguilar· Apr 12, 2016, 18:05 UTC · re: John Keeping · lore

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

On Tue, Apr 12, 2016 at 03:01:19PM +0100, John Keeping wrote:

Show 17 quoted lines

> 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 <john@keeping.me.uk>
> 
> 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 <davvid@gmail.com>

cheers,

-- 
David

Junio C Hamano· Apr 12, 2016, 16:15 UTC · re: Nikola Forró · lore

Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent

Nikola Forró <nforro@redhat.com> writes:

Show 9 quoted lines

> 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ó <nforro@redhat.com>
> ---

Looks good from a cursory read.  David?

Show 48 quoted lines

>  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]*)