threads / patch / 28435

patchgit-mergetool--lib.sh: make check_unchanged return 1 on invalid read

Subject: [PATCH] git-mergetool--lib.sh: make check_unchanged return 1 on invalid read

## tl;dr

5 messages between Sep 19, 2011 and Sep 20, 2011. Diffs are folded; open one to read it.

replies: 4people: 3as markdown or json

Jay Soffian· Sep 19, 2011, 20:03 UTC · lore

Else when the user hits ctrl-c at the "Was the merge successful? [y/n]" prompt, mergetool goes into an infinite loop asking for input.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
 git-mergetool--lib.sh |    6 +++++-
 1 files changed, 5 insertions(+), 1 deletions(-)
Show changes to git-mergetool--lib.sh +5 −1
diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 8fc65d0400..0eb424484c 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -21,7 +21,11 @@ check_unchanged () {
 		do
 			echo "$MERGED seems unchanged."
 			printf "Was the merge successful? [y/n] "
-			read answer
+			if ! read answer
+			then
+				status=1
+				break
+			fi
 			case "$answer" in
 			y*|Y*) status=0; break ;;
 			n*|N*) status=1; break ;;
-- 
1.7.7.rc2.2.gf185
Junio C Hamano· Sep 19, 2011, 20:37 UTC · re: Jay Soffian · lore

Re: [PATCH] git-mergetool--lib.sh: make check_unchanged return 1 on invalid read

Jay Soffian <jaysoffian@gmail.com> writes:
Show 5 quoted lines
> Else when the user hits ctrl-c at the "Was the merge successful?
> [y/n]" prompt, mergetool goes into an infinite loop asking
> for input.
>
> Signed-off-by: Jay Soffian <jaysoffian@gmail.com>

We still seem to miss one "read" unchecked in resolve_symlink_merge(), even with this patch.

Show 17 quoted lines
>  git-mergetool--lib.sh |    6 +++++-
>  1 files changed, 5 insertions(+), 1 deletions(-)
>
> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
> index 8fc65d0400..0eb424484c 100644
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -21,7 +21,11 @@ check_unchanged () {
>  		do
>  			echo "$MERGED seems unchanged."
>  			printf "Was the merge successful? [y/n] "
> -			read answer
> +			if ! read answer
> +			then
> +				status=1
> +				break
> +			fi

I suspect that it would be more consistent with 6b44577 (mergetool: check return value from read, 2011-07-01), which this patch is a follow-up to, to do:

	read answer || return 1
here.
Thanks.
Jay Soffian· Sep 19, 2011, 23:40 UTC · re: Junio C Hamano · lore

[PATCH] git-mergetool: check return value from read

Mostly fixed already by 6b44577 (mergetool: check return value from read, 2011-07-01). Catch two uses it missed.

Signed-off-by: Jay Soffian <jaysoffian@gmail.com>
---
On Mon, Sep 19, 2011 at 4:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 10 quoted lines
> We still seem to miss one "read" unchecked in resolve_symlink_merge(),
> even with this patch.
> [...]
> I suspect that it would be more consistent with 6b44577 (mergetool: check
> return value from read, 2011-07-01), which this patch is a follow-up to,
> to do:
>
> 	read answer || return 1
>
> here.
Thanks, sorry I missed that.
 git-mergetool--lib.sh |    2 +-
 git-mergetool.sh      |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)
Show changes to 2 files +2 −2

git-mergetool--lib.sh, git-mergetool.sh

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 8fc65d0400..ed630b208a 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -21,7 +21,7 @@ check_unchanged () {
 		do
 			echo "$MERGED seems unchanged."
 			printf "Was the merge successful? [y/n] "
-			read answer
+			read answer || return 1
 			case "$answer" in
 			y*|Y*) status=0; break ;;
 			n*|N*) status=1; break ;;
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 3c157bcd26..b6d463f0d0 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -72,7 +72,7 @@ describe_file () {
 resolve_symlink_merge () {
     while true; do
 	printf "Use (l)ocal or (r)emote, or (a)bort? "
-	read ans
+	read ans || return 1
 	case "$ans" in
 	    [lL]*)
 		git checkout-index -f --stage=2 -- "$MERGED"
-- 
1.7.7.rc2.2.gdf97720
Junio C Hamano· Sep 20, 2011, 00:41 UTC · re: Jay Soffian · lore

Re: [PATCH] git-mergetool: check return value from read

Jay Soffian <jaysoffian@gmail.com> writes:
Show 9 quoted lines
>> I suspect that it would be more consistent with 6b44577 (mergetool: check
>> return value from read, 2011-07-01), which this patch is a follow-up to,
>> to do:
>>
>> 	read answer || return 1
>>
>> here.
>
> Thanks, sorry I missed that.
Thank _you_ for spotting these unchecked "read"s.  Will queue.
Andrew Ardill· Sep 20, 2011, 01:20 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-mergetool: check return value from read

On 20 September 2011 10:41, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
> Jay Soffian <jaysoffian@gmail.com> writes:
>
>>> I suspect that it would be more consistent with 6b44577 (mergetool: check
>>> return value from read, 2011-07-01), which this patch is a follow-up to,
>>> to do:
>>>
>>>      read answer || return 1
>>>
>>> here.
>>
>> Thanks, sorry I missed that.
>
> Thank _you_ for spotting these unchecked "read"s.  Will queue.
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
>

I got hit by the ctrl+c bug while using mergetool just the other day. Thanks for the fix :)

← back to recent threads