{"thread":{"id":"42002","subject":"[PATCH] difftool/mergetool: make the form of yes/no questions consistent","startedAt":"2016-04-12T12:59:42Z","lastAt":"2016-04-12T18:05:46Z","messageCount":6,"participants":["Nikola Forró","John Keeping","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"283181","messageId":"1460465982.3214.7.camel@redhat.com","threadId":"42002","inReplyTo":null,"subject":"[PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"Nikola Forró","fromEmail":"nforro@redhat.com","sentAt":"2016-04-12T12:59:42Z","receivedAt":"2016-04-12T12:59:42Z","isPatch":true,"sender":{"key":"nforro@redhat.com","avatar":"https://avatars.githubusercontent.com/u/14923067?v=4"},"body":"Every yes/no question in difftool/mergetool scripts has slightly\ndifferent form, and none of them is consistent with the form git\nitself uses.\n\nMake the form of all the questions consistent with the form used\nby git, i.e. \"Question [y/n]? \".\n\nSigned-off-by: Nikola Forró <nforro@redhat.com>\n---\n git-difftool--helper.sh | 4 ++--\n git-mergetool--lib.sh   | 2 +-\n git-mergetool.sh        | 2 +-\n 3 files changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex 2b11b1d..84d6cc0 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -44,10 +44,10 @@ launch_merge_tool () {\n \t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n \t\tif use_ext_cmd\n \t\tthen\n-\t\t\tprintf \"Launch '%s' [Y/n]: \" \\\n+\t\t\tprintf \"Launch '%s' [Y/n]? \" \\\n \t\t\t\t\"$GIT_DIFFTOOL_EXTCMD\"\n \t\telse\n-\t\t\tprintf \"Launch '%s' [Y/n]: \" \"$merge_tool\"\n+\t\t\tprintf \"Launch '%s' [Y/n]? \" \"$merge_tool\"\n \t\tfi\n \t\tread ans || return\n \t\tif test \"$ans\" = n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 54ac8e4..92adcc0 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -100,7 +100,7 @@ check_unchanged () {\n \t\twhile true\n \t\tdo\n \t\t\techo \"$MERGED seems unchanged.\"\n-\t\t\tprintf \"Was the merge successful? [y/n] \"\n+\t\t\tprintf \"Was the merge successful [y/n]? \"\n \t\t\tread answer || return 1\n \t\t\tcase \"$answer\" in\n \t\t\ty*|Y*) return 0 ;;\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex 9f77e3a..2e0635a 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -396,7 +396,7 @@ done\n prompt_after_failed_merge () {\n \twhile true\n \tdo\n-\t\tprintf \"Continue merging other unresolved paths (y/n) ? \"\n+\t\tprintf \"Continue merging other unresolved paths [y/n]? \"\n \t\tread ans || return 1\n \t\tcase \"$ans\" in\n \t\t[yY]*)\n-- \n2.4.11\n"},{"id":"283183","messageId":"20160412132737.GB11013@serenity.lan","threadId":"42002","inReplyTo":"1460465982.3214.7.camel@redhat.com","subject":"Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-04-12T13:27:37Z","receivedAt":"2016-04-12T13:27:37Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Apr 12, 2016 at 02:59:42PM +0200, Nikola Forrrr wrote:\n> Every yes/no question in difftool/mergetool scripts has slightly\n> different form, and none of them is consistent with the form git\n> itself uses.\n> \n> Make the form of all the questions consistent with the form used\n> by git, i.e. \"Question [y/n]? \".\n> \n> Signed-off-by: Nikola Forró <nforro@redhat.com>\n> ---\n>  git-difftool--helper.sh | 4 ++--\n>  git-mergetool--lib.sh   | 2 +-\n>  git-mergetool.sh        | 2 +-\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n> \n> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> index 2b11b1d..84d6cc0 100755\n> --- a/git-difftool--helper.sh\n> +++ b/git-difftool--helper.sh\n> @@ -44,10 +44,10 @@ launch_merge_tool () {\n>  \t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n>  \t\tif use_ext_cmd\n>  \t\tthen\n> -\t\t\tprintf \"Launch '%s' [Y/n]: \" \\\n> +\t\t\tprintf \"Launch '%s' [Y/n]? \" \\\n>  \t\t\t\t\"$GIT_DIFFTOOL_EXTCMD\"\n>  \t\telse\n> -\t\t\tprintf \"Launch '%s' [Y/n]: \" \"$merge_tool\"\n> +\t\t\tprintf \"Launch '%s' [Y/n]? \" \"$merge_tool\"\n\nI think the case in these two is correct as-is.  The \"Y\" is capitalised\nbecause it is the default and will take effect if the user just presses\nENTER.\n\nFrom a quick look, the two cases below do not have this behaviour (the\nuser must enter either \"y\" or \"n\"), so it is correct that they are not\ncapitalized.\n\nThe change from \":\" to \"?\" and normalization of \"?\" placement below\nseems reasonable.\n\n>  \t\tfi\n>  \t\tread ans || return\n>  \t\tif test \"$ans\" = n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 54ac8e4..92adcc0 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -100,7 +100,7 @@ check_unchanged () {\n>  \t\twhile true\n>  \t\tdo\n>  \t\t\techo \"$MERGED seems unchanged.\"\n> -\t\t\tprintf \"Was the merge successful? [y/n] \"\n> +\t\t\tprintf \"Was the merge successful [y/n]? \"\n>  \t\t\tread answer || return 1\n>  \t\t\tcase \"$answer\" in\n>  \t\t\ty*|Y*) return 0 ;;\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 9f77e3a..2e0635a 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -396,7 +396,7 @@ done\n>  prompt_after_failed_merge () {\n>  \twhile true\n>  \tdo\n> -\t\tprintf \"Continue merging other unresolved paths (y/n) ? \"\n> +\t\tprintf \"Continue merging other unresolved paths [y/n]? \"\n>  \t\tread ans || return 1\n>  \t\tcase \"$ans\" in\n>  \t\t[yY]*)\n> -- \n> 2.4.11\n"},{"id":"283184","messageId":"1460469192.3214.17.camel@redhat.com","threadId":"42002","inReplyTo":"20160412132737.GB11013@serenity.lan","subject":"Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"Nikola Forró","fromEmail":"nforro@redhat.com","sentAt":"2016-04-12T13:53:12Z","receivedAt":"2016-04-12T13:53:12Z","isPatch":true,"sender":{"key":"nforro@redhat.com","avatar":"https://avatars.githubusercontent.com/u/14923067?v=4"},"body":"On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote:\n> I think the case in these two is correct as-is.  The \"Y\" is capitalised\n> because it is the default and will take effect if the user just presses\n> ENTER.\n\nThanks John, I'm aware of that. That's why the patch doesn't change\nthe case. Maybe I should have mention that explicitly in the commit\nmessage.\n"},{"id":"283185","messageId":"20160412140119.GC11013@serenity.lan","threadId":"42002","inReplyTo":"1460469192.3214.17.camel@redhat.com","subject":"Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2016-04-12T14:01:19Z","receivedAt":"2016-04-12T14:01:19Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Forrrr wrote:\n> On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote:\n> > I think the case in these two is correct as-is.  The \"Y\" is capitalised\n> > because it is the default and will take effect if the user just presses\n> > ENTER.\n> \n> Thanks John, I'm aware of that. That's why the patch doesn't change\n> the case. Maybe I should have mention that explicitly in the commit\n> message.\n\nSorry, I completely missed that.  Your patch does in fact look good, so:\n\n\tReviewed-by: John Keeping <john@keeping.me.uk>\n\nI think I was taken in by the commit message saying 'i.e. \"Question\n[y/n]? \"' and didn't examine the patch carefully enough.  It might be\nbetter just to drop the example since it's obvious what the patch does.\n"},{"id":"283197","messageId":"xmqq8u0isszw.fsf@gitster.mtv.corp.google.com","threadId":"42002","inReplyTo":"1460465982.3214.7.camel@redhat.com","subject":"Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-04-12T16:15:47Z","receivedAt":"2016-04-12T16:15:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nikola Forró <nforro@redhat.com> writes:\n\n> Every yes/no question in difftool/mergetool scripts has slightly\n> different form, and none of them is consistent with the form git\n> itself uses.\n>\n> Make the form of all the questions consistent with the form used\n> by git, i.e. \"Question [y/n]? \".\n>\n> Signed-off-by: Nikola Forró <nforro@redhat.com>\n> ---\n\nLooks good from a cursory read.  David?\n\n>  git-difftool--helper.sh | 4 ++--\n>  git-mergetool--lib.sh   | 2 +-\n>  git-mergetool.sh        | 2 +-\n>  3 files changed, 4 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\n> index 2b11b1d..84d6cc0 100755\n> --- a/git-difftool--helper.sh\n> +++ b/git-difftool--helper.sh\n> @@ -44,10 +44,10 @@ launch_merge_tool () {\n>  \t\t\t\"$GIT_DIFF_PATH_TOTAL\" \"$MERGED\"\n>  \t\tif use_ext_cmd\n>  \t\tthen\n> -\t\t\tprintf \"Launch '%s' [Y/n]: \" \\\n> +\t\t\tprintf \"Launch '%s' [Y/n]? \" \\\n>  \t\t\t\t\"$GIT_DIFFTOOL_EXTCMD\"\n>  \t\telse\n> -\t\t\tprintf \"Launch '%s' [Y/n]: \" \"$merge_tool\"\n> +\t\t\tprintf \"Launch '%s' [Y/n]? \" \"$merge_tool\"\n>  \t\tfi\n>  \t\tread ans || return\n>  \t\tif test \"$ans\" = n\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index 54ac8e4..92adcc0 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -100,7 +100,7 @@ check_unchanged () {\n>  \t\twhile true\n>  \t\tdo\n>  \t\t\techo \"$MERGED seems unchanged.\"\n> -\t\t\tprintf \"Was the merge successful? [y/n] \"\n> +\t\t\tprintf \"Was the merge successful [y/n]? \"\n>  \t\t\tread answer || return 1\n>  \t\t\tcase \"$answer\" in\n>  \t\t\ty*|Y*) return 0 ;;\n> diff --git a/git-mergetool.sh b/git-mergetool.sh\n> index 9f77e3a..2e0635a 100755\n> --- a/git-mergetool.sh\n> +++ b/git-mergetool.sh\n> @@ -396,7 +396,7 @@ done\n>  prompt_after_failed_merge () {\n>  \twhile true\n>  \tdo\n> -\t\tprintf \"Continue merging other unresolved paths (y/n) ? \"\n> +\t\tprintf \"Continue merging other unresolved paths [y/n]? \"\n>  \t\tread ans || return 1\n>  \t\tcase \"$ans\" in\n>  \t\t[yY]*)\n"},{"id":"283210","messageId":"20160412180546.GA27558@gmail.com","threadId":"42002","inReplyTo":"20160412140119.GC11013@serenity.lan","subject":"Re: [PATCH] difftool/mergetool: make the form of yes/no questions consistent","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2016-04-12T18:05:46Z","receivedAt":"2016-04-12T18:05:46Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Apr 12, 2016 at 03:01:19PM +0100, John Keeping wrote:\n> On Tue, Apr 12, 2016 at 03:53:12PM +0200, Nikola Forrrr wrote:\n> > On Tue, 2016-04-12 at 14:27 +0100, John Keeping wrote:\n> > > I think the case in these two is correct as-is.  The \"Y\" is capitalised\n> > > because it is the default and will take effect if the user just presses\n> > > ENTER.\n> > \n> > Thanks John, I'm aware of that. That's why the patch doesn't change\n> > the case. Maybe I should have mention that explicitly in the commit\n> > message.\n> \n> Sorry, I completely missed that.  Your patch does in fact look good, so:\n> \n> \tReviewed-by: John Keeping <john@keeping.me.uk>\n> \n> I think I was taken in by the commit message saying 'i.e. \"Question\n> [y/n]? \"' and didn't examine the patch carefully enough.  It might be\n> better just to drop the example since it's obvious what the patch does.\n\nThanks for reviewing.\n\nAcked-by: David Aguilar <davvid@gmail.com>\n\n\ncheers,\n-- \nDavid\n"}]}