Re: [PATCH v3 3/4] t7800: modernize tests
Am 3/21/2013 8:41, schrieb Johannes Sixt:
Am 3/20/2013 23:59, schrieb David Aguilar:
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 tracked down the difference between Windows and Linux, and it is...
for my $file (@worktree) {
next if $symlinks -l $b/$file;
... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,
And that is likely by design. From the docs:
--symlinks
--no-symlinks
git difftool's default behavior is create symlinks to the working
tree when run in --dir-diff mode.
Specifying `--no-symlinks` instructs 'git difftool' to create
copies instead. `--no-symlinks` is the default on Windows.
And indeed, we have this initialization:
my %opts = (
...
symlinks = $^O ne 'cygwin'
$^O ne 'MSWin32' $^O ne 'msys',
...
);
Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?
Perhaps the right solution is this:
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index c6d6b1c..19238f6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -328,14 +328,16 @@ test_expect_success PERL 'setup change in subdirectory' '
git commit -m modified both
'
-test_expect_success PERL 'difftool -d' '
- git difftool -d --extcmd ls branch output
+# passing --symlinks helps Cygwin, which defaults to --no-symlinks
+
+test_expect_success PERL,SYMLINKS 'difftool -d' '
+ git difftool -d --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
-test_expect_success PERL 'difftool --dir-diff' '
- git difftool --dir-diff --extcmd ls branch output
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '
+ git difftool --dir-diff --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
@@ -362,16 +364,16 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff
--symlink without unstage
test_cmp actual expect
'
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
- git difftool --dir-diff --prompt --extcmd ls branch output
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '
+ git difftool --dir-diff --symlinks --prompt --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff from subdirectory' '
(
cd sub
- git difftool --dir-diff --extcmd ls branch output
+ git difftool --dir-diff --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
)
(Only tested on MinGW, which skips the tests.) I leave it to you
to write --no-symlinks tests.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
Am 3/21/2013 8:41, schrieb Johannes Sixt:
Am 3/20/2013 23:59, schrieb David Aguilar:
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 tracked down the difference between Windows and Linux, and it is...
for my $file (@worktree) {
next if $symlinks -l $b/$file;
... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,
And that is likely by design. From the docs:
--symlinks
--no-symlinks
git difftool's default behavior is create symlinks to the working
tree when run in --dir-diff mode.
Specifying `--no-symlinks` instructs 'git difftool' to create
copies instead. `--no-symlinks` is the default on Windows.
And indeed, we have this initialization:
my %opts = (
...
symlinks = $^O ne 'cygwin'
$^O ne 'MSWin32' $^O ne 'msys',
...
);
Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?
Perhaps the right solution is this:
We already have tests that explicitly pass '--symlinks'. I wonder if it
would be better to change output to .git/output, which should avoid
the problem by moving the output file out of the working tree.
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index c6d6b1c..19238f6 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -328,14 +328,16 @@ test_expect_success PERL 'setup change in subdirectory'
'
git commit -m modified both
'
-test_expect_success PERL 'difftool -d' '
- git difftool -d --extcmd ls branch output
+# passing --symlinks helps Cygwin, which defaults to --no-symlinks
+
+test_expect_success PERL,SYMLINKS 'difftool -d' '
+ git difftool -d --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
-test_expect_success PERL 'difftool --dir-diff' '
- git difftool --dir-diff --extcmd ls branch output
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff' '
+ git difftool --dir-diff --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
@@ -362,16 +364,16 @@ test_expect_success PERL,SYMLINKS 'difftool --dir-diff
--symlink without unstage
test_cmp actual expect
'
-test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
- git difftool --dir-diff --prompt --extcmd ls branch output
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff ignores --prompt' '
+ git difftool --dir-diff --symlinks --prompt --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
'
-test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+test_expect_success PERL,SYMLINKS 'difftool --dir-diff from subdirectory' '
(
cd sub
- git difftool --dir-diff --extcmd ls branch output
+ git difftool --dir-diff --symlinks --extcmd ls branch output
stdin_contains sub output
stdin_contains file output
)
(Only tested on MinGW, which skips the tests.) I leave it to you
to write --no-symlinks tests.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
Am 3/22/2013 11:00, schrieb John Keeping:
On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
Am 3/21/2013 8:41, schrieb Johannes Sixt:
Am 3/20/2013 23:59, schrieb David Aguilar:
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 tracked down the difference between Windows and Linux, and it is...
for my $file (@worktree) {
next if $symlinks -l $b/$file;
... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,
And that is likely by design. From the docs:
--symlinks
--no-symlinks
git difftool's default behavior is create symlinks to the working
tree when run in --dir-diff mode.
Specifying `--no-symlinks` instructs 'git difftool' to create
copies instead. `--no-symlinks` is the default on Windows.
And indeed, we have this initialization:
my %opts = (
...
symlinks = $^O ne 'cygwin'
$^O ne 'MSWin32' $^O ne 'msys',
...
);
Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?
Perhaps the right solution is this:
We already have tests that explicitly pass '--symlinks'. I wonder if it
would be better to change output to .git/output, which should avoid
the problem by moving the output file out of the working tree.
The point of using --symlinks is not to test the effect of the option, but
to test the same thing on Unix and on Cygwin, because the latter uses
--no-symlinks by default. Therefore, I think that this sketch is the right
thing to do.
But the real problem seems to be that output should not be among the
files treated in the cited pieces of code (unless I'm wrong, of course; I
know next to nothing about git-difftool). It should not matter where the
file lives. Just add --no-symlinks to the difftool invocation of test
difftool -d and watch it fail on Linux, too.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
On Fri, Mar 22, 2013 at 12:14:27PM +0100, Johannes Sixt wrote:
Am 3/22/2013 11:00, schrieb John Keeping:
On Fri, Mar 22, 2013 at 08:13:46AM +0100, Johannes Sixt wrote:
Am 3/21/2013 8:41, schrieb Johannes Sixt:
Am 3/20/2013 23:59, schrieb David Aguilar:
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 tracked down the difference between Windows and Linux, and it is...
for my $file (@worktree) {
next if $symlinks -l $b/$file;
... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop,
And that is likely by design. From the docs:
--symlinks
--no-symlinks
git difftool's default behavior is create symlinks to the working
tree when run in --dir-diff mode.
Specifying `--no-symlinks` instructs 'git difftool' to create
copies instead. `--no-symlinks` is the default on Windows.
And indeed, we have this initialization:
my %opts = (
...
symlinks = $^O ne 'cygwin'
$^O ne 'MSWin32' $^O ne 'msys',
...
);
Can the --dir-diff tests case pass on Cygwin when neither --symlinks nor
--no-symlinks is passed?
Perhaps the right solution is this:
We already have tests that explicitly pass '--symlinks'. I wonder if it
would be better to change output to .git/output, which should avoid
the problem by moving the output file out of the working tree.
The point of using --symlinks is not to test the effect of the option, but
to test the same thing on Unix and on Cygwin, because the latter uses
--no-symlinks by default. Therefore, I think that this sketch is the right
thing to do.
So shouldn't we be running all of the tests with both --symlinks and
--no-symlinks (except those which explicitly check that these options do
the right thing)?
But the real problem seems to be that output should not be among the
files treated in the cited pieces of code (unless I'm wrong, of course; I
know next to nothing about git-difftool). It should not matter where the
file lives. Just add --no-symlinks to the difftool invocation of test
difftool -d and watch it fail on Linux, too.
I fired up strace and it looks like difftool sees it as a working tree
file (i.e. the working tree content matches the RHS of the diff) in the
output of git diff --raw --no-abbrev -z branch and copies it over to
the temporary directories. Then when difftool completes it copies back
the working tree files from there.
When the file is copied to the temporary directory, the diff command
hasn't yet run so output is empty. When it's copied back it is after
the ls has run and so we overwrite the output of the command with the
original empty file.
So sticking the output file under .git does solve this issue since it
then is not treated as a working tree file.
Whether the current behaviour of difftool is entirely sensible is a
different question. I think we should at the very least by refusing to
overwrite working tree files that have been modified since they were
copied to the temporary directory
If I ever end up on a system using --no-symlinks I can see myself being
bitten by this by editing the original working tree file while
inspecting the diff in a separate window. I suspect this is just as
common a workflow as people editing the file in their diff tool and
wanting the changes copied back to the working tree.
John
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
Am 3/20/2013 23:59, schrieb David Aguilar:
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 tracked down the difference between Windows and Linux, and it is...
for my $file (@worktree) {
next if $symlinks -l $b/$file;
... this line in sub dir_diff. On Linux, we take the short-cut, but on
Windows we proceed through the rest of the loop, which ultimately finds a
difference here:
my $diff = compare($b/$file, $workdir/$file);
and attempts to copy a file here:
copy($b/$file, $workdir/$file) or
where one of the files is the locked output file.
I don't know how essential symlinks are for the operation of git-difftool
and whether something can be done about it. The immediate fix is
apparently to protect the tests with SYMLINKS.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
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?
Same for some later tests.
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.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 3/4] t7800: modernize tests
On Wed, Mar 20, 2013 at 2:48 AM, Johannes Sixt j.s...@viscovery.net wrote:
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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
