Status of conflicted files resolved with rerere

Status of conflicted files resolved with rerere

[Magnus Bäck]

I played around with git rerere today and was surprised by the results.
When a conflict has been resolved automatically by rerere, the file
isn't added to the index like other files are where Git just used one of
the regular merge resolution algorithms. What's worse, if git mergetool
is invoked -- which is what I normally do when git merge needs help --
it has no idea that the file actually has been merged already, and
launches the merge tool with the three files involved in the merge. If
the user hasn't been paying attention to each line of the git merge
output (stating the files who were automatically resolved) it's easy to
trash rerere's work.

Would it make sense for git merge to add rerere'd files (where all hunks
were recognized and resolved) to the index automatically? That would
make it possible for the user to run git mergetool without reading every
single line of output from previous commands just to figure out which
files should be added to the index straight off and which files require
merging.

For users resolving conflicts by editing the file and fiddling with the
<<<<<<<<< marks etc such a change wouldn't make that big difference, but
for mergetool users it seems like a quite useful improvement. Or is
there something I'm failing to grasp here? This is on Git 1.7.0.3 by the
way.

-- 
Magnus Bäck                      Opinions are my own and do not necessarily
SW Configuration Manager         represent the ones of my employer, etc.
Sony Ericsson

Re: Status of conflicted files resolved with rerere

[Avery Pennarun]

On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck
<magnus.back@sonyericsson.com> wrote:
> I played around with git rerere today and was surprised by the results.
> When a conflict has been resolved automatically by rerere, the file
> isn't added to the index like other files are where Git just used one of
> the regular merge resolution algorithms. What's worse, if git mergetool
> is invoked -- which is what I normally do when git merge needs help --
> it has no idea that the file actually has been merged already, and
> launches the merge tool with the three files involved in the merge. If
> the user hasn't been paying attention to each line of the git merge
> output (stating the files who were automatically resolved) it's easy to
> trash rerere's work.

The motivation behind the current behaviour, as I understand it, is
that rerere speeds things up, but you don't necessarily want to trust
that it has resolved your merge conflicts correctly.  After all, they
were unarguably *conflicts*, not just normal merge results, so you
can't quite trust them.

That said, I've never had a problem where rerere did the wrong thing
for me.  Maybe there could be an option to override it.

Anyway, I never use a mergetool, so like you suspected, this has never
been a major problem for me.

It sounds like the real problem here though it the mergetool stuff.
Why is it disregarding all the automated merging that git has done and
starting over from scratch?  If git, in its infinite cleverness, has
resolved *some* parts of the file but not others, wouldn't we want it
to keep those resolutions?  It sounds like mergetool is actually
making things *more* work instead of less.

Is there some way to teach the mergetool stuff to be smarter?  At the
very least, having it auto-skip files that have no *remaining*
conflicts might be a good idea.

Have fun,

Avery

Re: Status of conflicted files resolved with rerere

[Jay Soffian]

On Thu, Aug 12, 2010 at 5:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:
> That said, I've never had a problem where rerere did the wrong thing
> for me.  Maybe there could be an option to override it.

$ git config rerere.autoupdate true

j.

Re: Status of conflicted files resolved with rerere

[David Aguilar]

On Aug 12, 2010, at 2:36 PM, Avery Pennarun <apenwarr@gmail.com> wrote:

> On Thu, Aug 12, 2010 at 5:28 PM, Magnus Bäck
> <magnus.back@sonyericsson.com> wrote:
>> I played around with git rerere today and was surprised by the  
>> results.
>> When a conflict has been resolved automatically by rerere, the file
>> isn't added to the index like other files are where Git just used  
>> one of
>> the regular merge resolution algorithms. What's worse, if git  
>> mergetool
>> is invoked -- which is what I normally do when git merge needs help  
>> --
>> it has no idea that the file actually has been merged already, and
>> launches the merge tool with the three files involved in the merge.  
>> If
>> the user hasn't been paying attention to each line of the git merge
>> output (stating the files who were automatically resolved) it's  
>> easy to
>> trash rerere's work.
> [...]
> It sounds like the real problem here though it the mergetool stuff.
> Why is it disregarding all the automated merging that git has done and
> starting over from scratch?  If git, in its infinite cleverness, has
> resolved *some* parts of the file but not others, wouldn't we want it
> to keep those resolutions?  It sounds like mergetool is actually
> making things *more* work instead of less.
>
> Is there some way to teach the mergetool stuff to be smarter?  At the
> very least, having it auto-skip files that have no *remaining*
> conflicts might be a good idea.
>
> Have fun,
>
> Avery

Right, that would be a great enhancement.

The problem is that mergetool always uses local, remote, and base.  It  
uses the unmerged flag in the index when deciding which files to  
consider.

Here's what we'd need in order to improve rerere and mergetool  
interaction:  the ability to answer the question, "has this file been  
rerere merged?"

Once we have that then we can make mergetool skip these files by  
default. We would also need a command line flag to override the  
behaviour.

Perhaps a naive grep for merge markers in the worktree file would  
suffice?  Heuristics have gone a long way in git so doing something  
like that wouldn't seem too atrocious.

It'd also have the benefit that mergetool would skip files merged by  
$EDITOR.

If anyone has any tips I'm all ears.  Does this seem reasonable, and  
if so, what would be a good name for the command line flag to get the  
existing behavior?  My gut feeling is that it shouldn't have a  
corresponding config variable.

Cheers,
-- 
         David

Re: Status of conflicted files resolved with rerere

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> Here's what we'd need in order to improve rerere and mergetool  
> interaction:  the ability to answer the question, "has this file been  
> rerere merged?"

I do not quite understand why the user _runs_ mergetool on a file that has
been already merged; isn't it an option not to do so in the first place?

Having said that.

I think you can use the fact that:

 - "ls-files -u" will list paths with conflicts; and

 - "rerere status" won't mention the ones that have been autoresolved

if rerere is in effect (check for presense of .git/MERGE_RR).

Re: Status of conflicted files resolved with rerere

[Magnus Bäck]

On Sunday, August 15, 2010 at 08:59 CEST,
     Junio C Hamano <gitster@pobox.com> wrote:

> David Aguilar <davvid@gmail.com> writes:
> 
> > Here's what we'd need in order to improve rerere and mergetool
> > interaction:  the ability to answer the question, "has this file
> > been rerere merged?"
> 
> I do not quite understand why the user _runs_ mergetool on a file that
> has been already merged; isn't it an option not to do so in the first
> place?

You have a point, but if there are conflicts in many files where only a
subset were autoresolved I think it would be prudent to help the user.
Grepping after remaining conflict markers or keeping the "git merge"
output somewhere to see which files actually were autoresolved works
but I think we can do better.

On the other hand, hinting mergetool users about rerere.autoupdate
is perhaps good enough? Doesn't help users who want to inspect the
autoresolved results yet also want hassle-free mergetool usage, though.

> Having said that.
> 
> I think you can use the fact that:
> 
>  - "ls-files -u" will list paths with conflicts; and
> 
>  - "rerere status" won't mention the ones that have been autoresolved
> 
> if rerere is in effect (check for presense of .git/MERGE_RR).

Okay, I'll have a stab at a patch.

-- 
Magnus Bäck                      Opinions are my own and do not necessarily
SW Configuration Manager         represent the ones of my employer, etc.
Sony Ericsson

[PATCH] mergetool: Skip autoresolved paths

[David Aguilar]

When mergetool is run without path limiters it loops
over each entry in 'git ls-files -u'.  This includes
autoresolved paths.

Teach mergetool to only merge files listed in 'rerere status'
when rerere is enabled.

There are some subtle but harmless changes in behavior.
We now call cd_to_toplevel when no paths are given.
We do this because 'rerere status' paths are always relative
to the root.  This is beneficial for the non-rerere use as
well in that mergetool now runs against all unmerged files
regardless of the current directory.

This also slightly tweaks the output when run without paths
to be more readable.

The old output:

Merging the files: foo
bar
baz

The new output:

Merging:
foo
bar
baz

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-mergetool.sh     |   28 +++++++++++++++++++++++-----
 t/t7610-mergetool.sh |   46 ++++++++++++++++++++++++++++++++++------------
 2 files changed, 57 insertions(+), 17 deletions(-)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index b52a741..bd7ab02 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -264,17 +264,35 @@ merge_keep_temporaries="$(git config --bool mergetool.keepTemporaries || echo fa
 
 last_status=0
 rollup_status=0
+rerere=false
+
+files_to_merge() {
+    if test "$rerere" = true
+    then
+	git rerere status
+    else
+	git ls-files -u | sed -e 's/^[^	]*	//' | sort -u
+    fi
+}
+
 
 if test $# -eq 0 ; then
-    files=$(git ls-files -u | sed -e 's/^[^	]*	//' | sort -u)
+    cd_to_toplevel
+
+    if test -e "$GIT_DIR/MERGE_RR"
+    then
+	rerere=true
+    fi
+
+    files=$(files_to_merge)
     if test -z "$files" ; then
 	echo "No files need merging"
 	exit 0
     fi
-    echo Merging the files: "$files"
-    git ls-files -u |
-    sed -e 's/^[^	]*	//' |
-    sort -u |
+    printf "Merging:\n"
+    printf "$files\n"
+
+    files_to_merge |
     while IFS= read i
     do
 	if test $last_status -ne 0; then
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index e768c3e..f5a7bf4 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -14,6 +14,7 @@ Testing basic merge tool invocation'
 # running mergetool
 
 test_expect_success 'setup' '
+    git config rerere.enabled true &&
     echo master >file1 &&
     mkdir subdir &&
     echo master sub >subdir/file3 &&
@@ -71,19 +72,40 @@ test_expect_success 'mergetool in subdir' '
     cd subdir && (
     test_must_fail git merge master >/dev/null 2>&1 &&
     ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
-    test "$(cat file3)" = "master new sub" )
+    test "$(cat file3)" = "master new sub") &&
+    cd ..
 '
 
-# We can't merge files from parent directories when running mergetool
-# from a subdir. Is this a bug?
-#
-#test_expect_failure 'mergetool in subdir' '
-#    cd subdir && (
-#    ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
-#    ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
-#    test "$(cat ../file1)" = "master updated" &&
-#    test "$(cat ../file2)" = "master new" &&
-#    git commit -m "branch1 resolved with mergetool - subdir" )
-#'
+test_expect_success 'mergetool on file in parent dir' '
+    cd subdir && (
+    ( yes "" | git mergetool ../file1 >/dev/null 2>&1 ) &&
+    ( yes "" | git mergetool ../file2 >/dev/null 2>&1 ) &&
+    test "$(cat ../file1)" = "master updated" &&
+    test "$(cat ../file2)" = "master new" &&
+    git commit -m "branch1 resolved with mergetool - subdir") &&
+    cd ..
+'
+
+test_expect_success 'mergetool skips autoresolved' '
+    git checkout -b test4 branch1 &&
+    test_must_fail git merge master &&
+    test -n "$(git ls-files -u)" &&
+    output="$(git mergetool --no-prompt)" &&
+    test "$output" = "No files need merging" &&
+    git reset --hard
+'
+
+test_expect_success 'mergetool merges all from subdir' '
+    cd subdir && (
+    git config rerere.enabled false &&
+    test_must_fail git merge master &&
+    git mergetool --no-prompt &&
+    test "$(cat ../file1)" = "master updated" &&
+    test "$(cat ../file2)" = "master new" &&
+    test "$(cat file3)" = "master new sub" &&
+    git add ../file1 ../file2 file3 &&
+    git commit -m "branch2 resolved by mergetool from subdir") &&
+    cd ..
+'
 
 test_done
-- 
1.7.2.1.98.gd9365.dirty

Re: [PATCH] mergetool: Skip autoresolved paths

[Thomas Rast]

David Aguilar wrote:
> When mergetool is run without path limiters it loops
> over each entry in 'git ls-files -u'.  This includes
> autoresolved paths.
[...]
> +test_expect_success 'mergetool merges all from subdir' '
> +    cd subdir && (
> +    git config rerere.enabled false &&
> +    test_must_fail git merge master &&
> +    git mergetool --no-prompt &&
> +    test "$(cat ../file1)" = "master updated" &&
> +    test "$(cat ../file2)" = "master new" &&
> +    test "$(cat file3)" = "master new sub" &&
> +    git add ../file1 ../file2 file3 &&
> +    git commit -m "branch2 resolved by mergetool from subdir") &&
> +    cd ..
> +'

This test never worked in my automatic testing (it fails and bisects
to this commit).

It might be because the cronjob doesn't have a tty, as I'm seeing the
output below (note the error at the end).  Any insights?

expecting success: 
    cd subdir && (
    git config rerere.enabled false &&
    test_must_fail git merge master &&
    git mergetool --no-prompt &&
    test "$(cat ../file1)" = "master updated" &&
    test "$(cat ../file2)" = "master new" &&
    test "$(cat file3)" = "master new sub" &&
    git add ../file1 ../file2 file3 &&
    git commit -m "branch2 resolved by mergetool from subdir") &&
    cd ..

Merging:
a8bf666 branch1 changes
virtual master
found 1 common ancestor(s):
775c381 added file1
Auto-merging file1
CONFLICT (content): Merge conflict in file1
Auto-merging file2
CONFLICT (add/add): Merge conflict in file2
Auto-merging subdir/file3
CONFLICT (content): Merge conflict in subdir/file3
Automatic merge failed; fix conflicts and then commit the result.
Merging:
file1
file2
subdir/file3

/local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device
 or address
/local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device
 or address

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH] mergetool: Skip autoresolved paths

[David Aguilar]

On Thu, Aug 19, 2010 at 12:02:36PM +0200, Thomas Rast wrote:
> David Aguilar wrote:
> > When mergetool is run without path limiters it loops
> > over each entry in 'git ls-files -u'.  This includes
> > autoresolved paths.
> [...]
> > +test_expect_success 'mergetool merges all from subdir' '
> > +    cd subdir && (
> > +    git config rerere.enabled false &&
> > +    test_must_fail git merge master &&
> > +    git mergetool --no-prompt &&
> > +    test "$(cat ../file1)" = "master updated" &&
> > +    test "$(cat ../file2)" = "master new" &&
> > +    test "$(cat file3)" = "master new sub" &&
> > +    git add ../file1 ../file2 file3 &&
> > +    git commit -m "branch2 resolved by mergetool from subdir") &&
> > +    cd ..
> > +'
> 
> This test never worked in my automatic testing (it fails and bisects
> to this commit).
> 
> It might be because the cronjob doesn't have a tty, as I'm seeing the
> output below (note the error at the end).  Any insights?

It must be the tty.


> expecting success: 
>     cd subdir && (
>     git config rerere.enabled false &&
>     test_must_fail git merge master &&
>     git mergetool --no-prompt &&
>     test "$(cat ../file1)" = "master updated" &&
>     test "$(cat ../file2)" = "master new" &&
>     test "$(cat file3)" = "master new sub" &&
>     git add ../file1 ../file2 file3 &&
>     git commit -m "branch2 resolved by mergetool from subdir") &&
>     cd ..
> [...]
> /local/home/trast/git/t/valgrind/bin/git-mergetool: line 302: /dev/tty: No such device
>  or address
> /local/home/trast/git/t/valgrind/bin/git-mergetool: line 299: /dev/tty: No such device
>  or address


git-mergetool lines 295-307:

    files_to_merge |
    while IFS= read i
    do
	if test $last_status -ne 0; then
	    prompt_after_failed_merge < /dev/tty || exit 1
	fi
	printf "\n"
	merge_file "$i" < /dev/tty > /dev/tty
	last_status=$?
	if test $last_status -ne 0; then
	    rollup_status=1
	fi
    done

The reason the test fails without a tty is that we've never
exercised this code in the past.

This commit did not introduce the "< /dev/tty > /dev/tty"
idiom.  It was introduced in b0169d84 by Charles Bailey.
What this commit did do was add test coverage to it,
which is good because it uncovered this problem :-)

Charles, is there another way we can write this?
Is there a reason why we need the tty redirection?
Can we drop it or is there a portability concern?

FWIW, the merge_file call in the else clause that follows
this section does not use tty redirection.

-- 

	David

Re: [PATCH] mergetool: Skip autoresolved paths

[Charles Bailey]

On 20/08/2010 04:52, David Aguilar wrote:
>
> git-mergetool lines 295-307:
>
>      files_to_merge |
>      while IFS= read i
>      do
> 	if test $last_status -ne 0; then
> 	    prompt_after_failed_merge<  /dev/tty || exit 1
> 	fi
> 	printf "\n"
> 	merge_file "$i"<  /dev/tty>  /dev/tty
> 	last_status=$?
> 	if test $last_status -ne 0; then
> 	    rollup_status=1
> 	fi
>      done
>
> The reason the test fails without a tty is that we've never
> exercised this code in the past.
>
> This commit did not introduce the "<  /dev/tty>  /dev/tty"
> idiom.  It was introduced in b0169d84 by Charles Bailey.
> What this commit did do was add test coverage to it,
> which is good because it uncovered this problem :-)
>
> Charles, is there another way we can write this?
> Is there a reason why we need the tty redirection?
> Can we drop it or is there a portability concern?
>
> FWIW, the merge_file call in the else clause that follows
> this section does not use tty redirection.
>

Actually, it's been like this since c4b4a5af which is when mergetool was 
introduced.

(b0169d84 didn't change this line, 0eea3451 but made only whitespace 
changes, it comes from the original mergetool code.)

When you say "drop it" what are you proposing to replace it with? We're 
in the middle of a shell pipe which has replaced stdin and merge_file 
needs access to the human on it's stdin; hence the </dev/tty. Strictly. 
I believe that the >/dev/tty isn't needed.

Is there some way of juggling file descriptors in shell? I had a quick 
play with this but suspect it's a bashism (and it might make mergetool 
less readable!).

echo hidden | { echo lost | cat 0<&3- ; } 3<&0

mergetool has never really been very approachable for automatic testing 
as it's fundamentally an interactive script. It would be nice if 
sufficient of the guts of mergetool were in testable library code and 
mergetool was just an obviously correct slim shell UI.

merge_file in the 'else' doesn't need the redirection as nobody has 
redirected the original stdin.

Charles.

Re: [PATCH] mergetool: Skip autoresolved paths

[Jonathan Nieder]

Charles Bailey wrote:

> We're in the middle of a shell pipe which has replaced stdin and
> merge_file needs access to the human on it's stdin; hence the
> </dev/tty. Strictly.
[...]
> Is there some way of juggling file descriptors in shell?

You can duplicate important fds, like so:

 exec 3<&0

 foo |
 (
	bar
	baz
	quuz <&3
 )

> I had a
> quick play with this but suspect it's a bashism

It's standard, luckily.  See http://unix.org/2008edition/

Hope that helps.

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

[Charles Bailey]

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.

Signed-off-by: Charles Bailey <charles@hashpling.org>
---

This works on my fedora 12 box with bash. The redirects should be
standard but this could do with some testing on other bourne shell
implementations.

 git-mergetool--lib.sh |    2 +-
 git-mergetool.sh      |    7 ++++---
 2 files changed, 5 insertions(+), 4 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 51dd0d6..b5e1943 100644
--- a/git-mergetool--lib.sh
+++ 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
 			case "$answer" in
 			y*|Y*) status=0; break ;;
 			n*|N*) status=1; break ;;
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bd7ab02..84edf7d 100755
--- a/git-mergetool.sh
+++ 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 |
     while IFS= read i
     do
 	if test $last_status -ne 0; then
-	    prompt_after_failed_merge < /dev/tty || exit 1
+	    prompt_after_failed_merge <&3 || exit 1
 	fi
 	printf "\n"
-	merge_file "$i" < /dev/tty > /dev/tty
+	merge_file "$i" <&3
 	last_status=$?
 	if test $last_status -ne 0; then
 	    rollup_status=1
-- 
1.7.2.2.110.gf04b9.dirty

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

[Jonathan Nieder]

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
-- 

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

[Charles Bailey]

On 20/08/2010 13:27, Jonathan Nieder wrote:
>> +++ 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?

No special reason, I just thought it was more natural to save it at the 
time that we do the redirect..

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

... but given that this approach is evidently less portable your way is 
clearly better.

> With the patch below on top, it passes with dash and ksh.

Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK?

Charles.

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

[Jonathan Nieder]

Charles Bailey wrote:
> On 20/08/2010 13:27, Jonathan Nieder wrote:

>> With the patch below on top, it passes with dash and ksh.
>
> Thanks, I'll re-roll in a bit at squash your fixes in, if that's OK?

Yeah, that's okay. :)

Thanks for your work.

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

[Charles Bailey]

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.

Includes additional compatibility fix from Jonathan Nieder.

Tested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Charles Bailey <charles@hashpling.org>
---

Now works on ksh as well as bash and dash.

 git-mergetool--lib.sh |    2 +-
 git-mergetool.sh      |    7 +++++--
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 51dd0d6..b5e1943 100644
--- a/git-mergetool--lib.sh
+++ 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
 			case "$answer" in
 			y*|Y*) status=0; break ;;
 			n*|N*) status=1; break ;;
diff --git a/git-mergetool.sh b/git-mergetool.sh
index bd7ab02..165b700 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -279,6 +279,9 @@ files_to_merge() {
 if test $# -eq 0 ; then
     cd_to_toplevel
 
+    # Save original stdin
+    exec 3<&0
+
     if test -e "$GIT_DIR/MERGE_RR"
     then
 	rerere=true
@@ -296,10 +299,10 @@ if test $# -eq 0 ; then
     while IFS= read i
     do
 	if test $last_status -ne 0; then
-	    prompt_after_failed_merge < /dev/tty || exit 1
+	    prompt_after_failed_merge <&3 || exit 1
 	fi
 	printf "\n"
-	merge_file "$i" < /dev/tty > /dev/tty
+	merge_file "$i" <&3
 	last_status=$?
 	if test $last_status -ne 0; then
 	    rollup_status=1
-- 
1.7.2.2.110.gf04b9.dirty