Re: [PATCH] mergetool: Skip autoresolved paths

[Charles Bailey <charles@hashpling.org>]

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.