Re: [PATCH] mergetool: Skip autoresolved paths

From: Charles Bailey <charles@hashpling.org>
Date: Aug 20, 2010, 09:57 UTC
Message-ID: <4C6E519E.1080700@hashpling.org>
In-Reply-To: <20100820035236.GA18267@gmail.com>

On 20/08/2010 04:52, David Aguilar wrote:

Show 32 quoted lines

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