Re: [PATCH v3 3/4] t7800: modernize tests

From: David Aguilar <davvid@gmail.com>
Date: Mar 20, 2013, 22:59 UTC
Message-ID: <CAJDDKr4O-OU57j0f7F2E2_4EvtCTE1gFP5uLv+fvD6VcpPiifg@mail.gmail.com>
In-Reply-To: <514985FB.8080806@viscovery.net>

On Wed, Mar 20, 2013 at 2:48 AM, Johannes Sixt <j.sixt@viscovery.net> wrote:

Show 28 quoted lines

> Am 2/21/2013 5:03, schrieb David Aguilar:
>>  test_expect_success PERL 'difftool -d' '
>> -     diff=$(git difftool -d --extcmd ls branch) &&
>> -     echo "$diff" | stdin_contains sub &&
>> -     echo "$diff" | stdin_contains file
>> +     git difftool -d --extcmd ls branch >output &&
>> +     stdin_contains sub <output &&
>> +     stdin_contains file <output
>>  '
>
> This test is broken on Windows. There is this code in git-difftool.perl
>
>         for my $file (@worktree) {
> ...
>                         copy("$b/$file", "$workdir/$file") or
>                         exit_cleanup($tmpdir, 1);
> ...
>         }
>
> @worktree is populated with all files in the worktree. At this point,
> "output" is among them. Then follows an attempt to copy a file over
> "$workdir/$file". I guess that is some link+remove magic going on behind
> the scenes. At any rate, this fails on Windows with
> "D:/Src/mingw-git/t/trash directory.t7800-difftool/../../git-difftool line
> 408: Bad file number", because files that are open cannot be written from
> outside (the file is open due to the redirection in the test snippet).
>
> What is going on here? Why can this ever succeed even on Unix?

Thanks for the report.  Yes, these do pass on Unix.

Hmm I wonder what's going on here?

I started digging in and the @worktree_files (aka @worktree above) is populated from the output of "git diff --raw ...".

Seeing the "output" filename in "diff --raw" implies that one of the tests added "output" to the index somehow. I do not see that happening anywhere, though, so I do not know how it would end up in the @worktree array if it is not reported by "diff --raw".

My current understanding of how it could possibly be open twice:

1. via the >output redirect
2. via the copy() perl code which is fed by @worktree

So I'm confused.  Why would we get different results on Windows?

I just re-ran these tests from "next" to check my sanity and they passed on both Linux and OS X.

> Same for some later tests.

Ditto.

> BTW, while debugging this, I found the use of the helper function
> stdin_contains() highly unhelpful; it just resolves to a 'grep' that on
> top of all hides stdout. Please don't do that. Just use unadorned grep
> like we do everywhere else.

I'm not too opposed to that. The one small advantage to the helper is that you can tweak the redirect in one central place, so it's not all for naught.

Sitaram, you added this back in:

ba959de1 git-difftool: allow skipping file by typing 'n' at prompt

Do you have any thoughts?

It seems like removing the stdout redirect could be helpful for debugging, and if we did that then there's really no point in having the helper (aside from the indirection which can sometimes help during debugging). I don't really feel too strongly either way, but it did bother you while debugging the test script, so unadorned grep seems like the way to go.

I'll wait and see if anybody else has any Windows-specific clues that we can use to narrow down this problem.

In lieu of an immediate fix, are there any test prerequisite we can use to skip these tests on windows? One or both of NOT_CYGWIN,NOT_MINGW?

-- 
David