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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html