Re: [PATCH] mergetool: Skip autoresolved paths

From: David Aguilar <davvid@gmail.com>
Date: Aug 20, 2010, 03:52 UTC
Message-ID: <20100820035236.GA18267@gmail.com>
In-Reply-To: <201008191202.36508.trast@student.ethz.ch>

On Thu, Aug 19, 2010 at 12:02:36PM +0200, Thomas Rast wrote:

Show 23 quoted lines

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

Show 16 quoted lines

> 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