Re: [PATCH] mergetool: Remove explicit references to /dev/tty

[Jonathan Nieder <jrnieder@gmail.com>]

Charles Bailey wrote:

> mergetool used /dev/tty to switch back to receiving input from the user
> via inside a block with a redirected stdin.
> 
> This harms testability, so change mergetool to save its original stdin
> to an alternative fd in this block and restore it for those sub-commands
> that need the original stdin.

Sounds good.

> +++ b/git-mergetool--lib.sh
> @@ -35,7 +35,7 @@ check_unchanged () {
>  		while true; do
>  			echo "$MERGED seems unchanged."
>  			printf "Was the merge successful? [y/n] "
> -			read answer < /dev/tty
> +			read answer

Part of the run_merge_tool codepath.  The only place this is called
with TOOL_MODE=merge is by merge_file which has stdin redirected,
so this should be safe.  Good.

> +++ b/git-mergetool.sh
> @@ -292,14 +292,15 @@ if test $# -eq 0 ; then
>      printf "Merging:\n"
>      printf "$files\n"
>  
> -    files_to_merge |
> +    # Save original stdin to fd 3
> +    files_to_merge 3<&0 |

I would think this should work, but it doesn't feel idiomatic.  Why
not save stdin a little earlier, so the reader does not have to track
down whether it has been redirected?

The test quietly passes for me with dash but fails with ksh:

 /home/jrn/src/git4/git-mergetool: line 303: 3: cannot open [Bad file descriptor]

With the patch below on top, it passes with dash and ksh.
---
diff --git a/git-mergetool.sh b/git-mergetool.sh
index 84edf7d..2e82522 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -275,10 +275,13 @@ files_to_merge() {
     fi
 }
 
 
 if test $# -eq 0 ; then
     cd_to_toplevel
 
+    # Save original stdin
+    exec 3<&0
+
     if test -e "$GIT_DIR/MERGE_RR"
     then
 	rerere=true
@@ -292,8 +294,7 @@ if test $# -eq 0 ; then
     printf "Merging:\n"
     printf "$files\n"
 
-    # Save original stdin to fd 3
-    files_to_merge 3<&0 |
+    files_to_merge |
     while IFS= read i
     do
 	if test $last_status -ne 0; then
--