{"thread":{"id":"30100","subject":"t7800-difftool.sh failure on pu","startedAt":"2012-03-29T18:12:20Z","lastAt":"2012-04-03T17:53:14Z","messageCount":7,"participants":["Ramsay Jones","Junio C Hamano","David Aguilar","Tim Henigan"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"188087","messageId":"4F74A604.3040402@ramsay1.demon.co.uk","threadId":"30100","inReplyTo":null,"subject":"t7800-difftool.sh failure on pu","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-03-29T18:12:20Z","receivedAt":"2012-03-29T18:12:20Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Tim,\n\nWith the current pu branch, I have t7800.3 (difftool ignores bad --tool values)\nfailing on Linux (I haven't tried cygwin or mingw yet). The failure is caused\nby the test for the value of the exit code; for me the exit code is 9 not 1.\n\nI have investigated, briefly, and found *two* alternatives for a fix. ;-)\n\nThe first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\nsystem call with Git::command_noisy\", 22-03-2012), like so:\n\n-- >8 --\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e1754ff..49613b1 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -237,5 +237,7 @@ if (defined($dirdiff)) {\n \n \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n+\tmy @command = ('git', 'diff', @ARGV);\n+\tmy $rc = system(@command);\n+\texit($rc | ($rc >> 8));\n }\n-- 8< --\n\nThe second option is a bit of a mystery, since I don't see why it is necessary\nor why it works! :-P\n\nFirst take a look at the following:\n\n$ perl -e 'print $!+0, \" $!\\n\";'\n0 \n$ echo $?\n0\n\n$ perl -e 'use Carp qw(croak); print $!+0, \" $!\\n\";'\n9 Bad file descriptor\n$ echo $?\n0\n\n$ perl -e 'use Carp qw(croak); print $!+0, \" $!\\n\"; croak oops;'\n9 Bad file descriptor\noops at -e line 1\n$ echo $?\n9\n\n$ perl -e 'use Carp qw(croak); print $!+0, \" $!\\n\"; $!=0; croak oops;'\n9 Bad file descriptor\noops at -e line 1\n$ echo $?\n255\n\n$ perl -e 'use Carp qw(croak); print $!+0, \" $!\\n\"; $!=0;  $?=1<<8; croak oops;'\n9 Bad file descriptor\noops at -e line 1\n$ echo $?\n1\n\n$ \n\nSo, it seems that a stray non-zero errno (Bad file descriptor) is finding it's\nway to the perl exit value via the die call from within croak(). In particular,\nthe call to croak within git_cmd_try() from Git.pm.\n\nHowever, I don't see why this is happening, since just before the call to croak\nthe perl vars $! (errno) and $? (exit status) are set to the *correct* values.\nNamely, $! is zero and $? is 256 (1<<8). As you can see from the above, that\n*should* result in the correct exit code. However, unlike above, git-difftool.perl\nreturns 9.\n\nSince 'croak' exits via a 'die' call, in desperation, I tried the following\npatch - and it works! Just don't ask me why. :-D\n\n-- 8< --\ndiff --git a/perl/Git.pm b/perl/Git.pm\nindex 1c96a20..264a45f 100644\n--- a/perl/Git.pm\n+++ b/perl/Git.pm\n@@ -1258,7 +1258,7 @@ sub git_cmd_try(&$) {\n \t\t# We can't croak here since Error.pm would mangle\n \t\t# that to Error::Simple.\n \t};\n-\t$err and croak $err;\n+\t$err and die $err;\n \treturn $array ? @result : $result[0];\n }\n \n-- >8 --\n\nHTH\n\nATB,\nRamsay Jones\n"},{"id":"188099","messageId":"7v7gy3qesv.fsf@alter.siamese.dyndns.org","threadId":"30100","inReplyTo":"4F74A604.3040402@ramsay1.demon.co.uk","subject":"Re: t7800-difftool.sh failure on pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-29T21:26:40Z","receivedAt":"2012-03-29T21:26:40Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n\n> Hi Tim,\n>\n> With the current pu branch, I have t7800.3 (difftool ignores bad --tool values)\n> failing on Linux (I haven't tried cygwin or mingw yet). The failure is caused\n> by the test for the value of the exit code; for me the exit code is 9 not 1.\n>\n> I have investigated, briefly, and found *two* alternatives for a fix. ;-)\n>\n> The first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\n> system call with Git::command_noisy\", 22-03-2012), like so:\n>\n> -- >8 --\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index e1754ff..49613b1 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -237,5 +237,7 @@ if (defined($dirdiff)) {\n>  \n>  \t$ENV{GIT_PAGER} = '';\n>  \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n> -\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n> +\tmy @command = ('git', 'diff', @ARGV);\n> +\tmy $rc = system(@command);\n> +\texit($rc | ($rc >> 8));\n>  }\n> -- 8< --\n\nI would prefer this, regardless of the issue.\n\nI actually recall asking Tim about the exit status when I reviewed this\nchange.\n"},{"id":"188229","messageId":"CAJDDKr50oDwih2fyb5iuFqYDsXSD3EXje4vZJwO=1_JBvuMYsA@mail.gmail.com","threadId":"30100","inReplyTo":"7v7gy3qesv.fsf@alter.siamese.dyndns.org","subject":"Re: t7800-difftool.sh failure on pu","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-31T04:05:30Z","receivedAt":"2012-03-31T04:05:30Z","isPatch":false,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Mar 29, 2012 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n>\n>> Hi Tim,\n>>\n>> With the current pu branch, I have t7800.3 (difftool ignores bad --tool values)\n>> failing on Linux (I haven't tried cygwin or mingw yet). The failure is caused\n>> by the test for the value of the exit code; for me the exit code is 9 not 1.\n>>\n>> I have investigated, briefly, and found *two* alternatives for a fix. ;-)\n>>\n>> The first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\n>> system call with Git::command_noisy\", 22-03-2012), like so:\n>>\n>> -- >8 --\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index e1754ff..49613b1 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -237,5 +237,7 @@ if (defined($dirdiff)) {\n>>\n>>       $ENV{GIT_PAGER} = '';\n>>       $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n>> -     git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n>> +     my @command = ('git', 'diff', @ARGV);\n>> +     my $rc = system(@command);\n>> +     exit($rc | ($rc >> 8));\n>>  }\n>> -- 8< --\n>\n> I would prefer this, regardless of the issue.\n>\n> I actually recall asking Tim about the exit status when I reviewed this\n> change.\n\nI would also prefer this.\n\nA question for the msysgit/cygwin folks:\n\nwould we need to go back to use \"git.exe\" as well?\n-- \nDavid\n"},{"id":"188330","messageId":"CAFouethd9yEVNhX3_m4B3akxTmPh2RS4-NUkakYk3fcjfTB7YA@mail.gmail.com","threadId":"30100","inReplyTo":"4F74A604.3040402@ramsay1.demon.co.uk","subject":"Re: t7800-difftool.sh failure on pu","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-02T17:09:45Z","receivedAt":"2012-04-02T17:09:45Z","isPatch":false,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Hi Ramsay - thank you for running the tests and trying this out.\n\n\nOn Thu, Mar 29, 2012 at 2:12 PM, Ramsay Jones\n<ramsay@ramsay1.demon.co.uk> wrote:\n>\n> With the current pu branch, I have t7800.3 (difftool ignores bad --tool values)\n> failing on Linux (I haven't tried cygwin or mingw yet). The failure is caused\n> by the test for the value of the exit code; for me the exit code is 9 not 1.\n\nThis is interesting.  On my Ubuntu box, I am able to run all of t7800\nwithout error.  This has been my primary development platform and the\ntests have consistently passed for me on that system.\n\nI have had trouble getting the test suite to run on msysgit.  However,\nI just tried the simple test sequence that you demonstrated and found\nthat I get the same failure on that platform (i.e. \"9 Bad file\ndescriptor\").  So it appears that Carp is broken on some platforms.\n\n\n> I have investigated, briefly, and found *two* alternatives for a fix. ;-)\n>\n> The first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\n> system call with Git::command_noisy\", 22-03-2012), like so:\n\nThanks again for spending the time to dig into the problem.  Based on\nfeedback from Junio and David in later emails, I plan to revert commit\n0440ed72.\n\n\n> The second option is a bit of a mystery, since I don't see why it is necessary\n> or why it works! :-P\n>\n> First take a look at the following:\n>\n> $ perl -e 'print $!+0, \" $!\\n\";'\n> 0\n> $ echo $?\n> 0\n>\n> $ perl -e 'use Carp qw(croak); print $!+0, \" $!\\n\";'\n> 9 Bad file descriptor\n> $ echo $?\n> 0\n\nI agree that this is mysterious.  On my Ubuntu box, this Perl\none-liner prints and exits with 0 for me (i.e. no bad file descriptor\nerror).  However, I was able to replicate the failure on msysgit.\nThis makes me wonder if other users of Git.pm have had the same\nproblem.  The primary user appears to be 'git-svn.perl' and its\nrelated tests.  A quick review of those files did not show any mention\nof problems with Carp.  Perhaps they simply do not check the exact\nexit code on failure?\n"},{"id":"188331","messageId":"CAFouetg2ME6f7=p191qPq=YC7Z7ZeBpm23tgb_wDmsEmwV7etw@mail.gmail.com","threadId":"30100","inReplyTo":"CAJDDKr50oDwih2fyb5iuFqYDsXSD3EXje4vZJwO=1_JBvuMYsA@mail.gmail.com","subject":"Re: t7800-difftool.sh failure on pu","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-02T17:19:31Z","receivedAt":"2012-04-02T17:19:31Z","isPatch":false,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Sat, Mar 31, 2012 at 12:05 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Thu, Mar 29, 2012 at 2:26 PM, Junio C Hamano <gitster@pobox.com> wrote:\n>> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n>>\n>>> The first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\n>>> system call with Git::command_noisy\", 22-03-2012), like so:\n>>>\n>>> -- >8 --\n>>> diff --git a/git-difftool.perl b/git-difftool.perl\n>>> index e1754ff..49613b1 100755\n>>> --- a/git-difftool.perl\n>>> +++ b/git-difftool.perl\n>>> @@ -237,5 +237,7 @@ if (defined($dirdiff)) {\n>>>\n>>>       $ENV{GIT_PAGER} = '';\n>>>       $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n>>> -     git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n>>> +     my @command = ('git', 'diff', @ARGV);\n>>> +     my $rc = system(@command);\n>>> +     exit($rc | ($rc >> 8));\n>>>  }\n>>> -- 8< --\n>>\n>> I would prefer this, regardless of the issue.\n>>\n>> I actually recall asking Tim about the exit status when I reviewed this\n>> change.\n\nThis breakage is a surprise to me.  All the tests in t7800 have passed\nfor me since my first modifications to them.  They continue to pass\nfor me on Ubuntu.  That being said, the simple tests that Ramsay\nposted in his email (simply printing $!) also fail for me on msysgit.\n\n\n> I would also prefer this.\n>\n\nThis change will involve:\n  - Dropping 7/9 from the series\n  - Editing 8/9 to replace 'git_cmd_try' with simple system calls\n\nWould it be better to resend the entire series or just edit and resend 8/9?\n\n\n> A question for the msysgit/cygwin folks:\n>\n> would we need to go back to use \"git.exe\" as well?\n\nI would welcome some feedback from other Windows users.  I have been\nsuccessfully using this series on Win7 with msysgit, but obviously\nsome setups differ.\n"},{"id":"188337","messageId":"7vvclif0gd.fsf@alter.siamese.dyndns.org","threadId":"30100","inReplyTo":"CAFouetg2ME6f7=p191qPq=YC7Z7ZeBpm23tgb_wDmsEmwV7etw@mail.gmail.com","subject":"Re: t7800-difftool.sh failure on pu","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T18:33:22Z","receivedAt":"2012-04-02T18:33:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> This change will involve:\n>   - Dropping 7/9 from the series\n>   - Editing 8/9 to replace 'git_cmd_try' with simple system calls\n>\n> Would it be better to resend the entire series or just edit and resend 8/9?\n\nAs long as you know I have the right \"previous round\" in my tree, it would\nbe sufficient to just send an updated 8/9 and tell me clearly what to do,\ni.e. \"drop 0440ed7 and replace 6fbdb7b with this patch, leaving everything\nelse intact.\", after \"---\" line in it (if you were resending many patches\nwith an updated cover letter, the insn to me will be in 0/n instead).\n\nThanks.\n"},{"id":"188425","messageId":"4F7B390A.7050007@ramsay1.demon.co.uk","threadId":"30100","inReplyTo":"7v7gy3qesv.fsf@alter.siamese.dyndns.org","subject":"Re: t7800-difftool.sh failure on pu","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2012-04-03T17:53:14Z","receivedAt":"2012-04-03T17:53:14Z","isPatch":false,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Junio C Hamano wrote:\n> Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:\n>> The first option is to (effectively) revert commit 0440ed72 (\"difftool: replace\n>> system call with Git::command_noisy\", 22-03-2012), like so:\n>>\n>> -- >8 --\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index e1754ff..49613b1 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -237,5 +237,7 @@ if (defined($dirdiff)) {\n>>  \n>>  \t$ENV{GIT_PAGER} = '';\n>>  \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n>> -\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n>> +\tmy @command = ('git', 'diff', @ARGV);\n>> +\tmy $rc = system(@command);\n>> +\texit($rc | ($rc >> 8));\n>>  }\n>> -- 8< --\n> \n> I would prefer this, regardless of the issue.\n\nMe too! ;-P\n\nI noticed this afternoon (using my isp's web-mail interface) that Tim has\nalready sent a patch fixing this issue along these lines. [Thanks Tim]\n\nHowever, just for the record, all three of the platforms I test on failed\nin more-or-less the same way. [the exit code on cygwin was 2 (No such File\nor Directory) rather than 9.]\n\nNote that the failure occurred (on Linux, cygwin and MinGW) when pu was at\ncommit e5471b396660313b172e102b50939c9c35d4c637 (v1.7.10-rc2-260-ge5471b3).\nHowever, on Linux, having fetched an updated pu which is now at commit\nae98c9308d3a79f2b22de2f6dae614f700f7ce0c (v1.7.10-rc3-272-gae98c93), then\ntest t7800 now reliably passes!\n\n[when I say it reliably passes (fails), I mean it passes (fails) each time\non ten consecutive runs.]\n\nI created a diff of git-difftool.perl between these two commits and applied\nthe patch, in reverse, to effectively revert that file. Having done so, then\nt7800 once again reliably fails! Looking at the diff, I have no idea why!\n[I've included the diff below, just for completeness.]\n\nATB,\nRamsay Jones\n\ndt.diff:\n-- >8 --\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex e1754ff..78e93a6 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -14,14 +14,16 @@\n use 5.008;\n use strict;\n use warnings;\n-use File::Basename qw(dirname basename);\n+use File::Basename qw(dirname);\n use File::Copy;\n+use File::Find;\n use File::stat;\n use File::Path qw(mkpath);\n use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n+my @tools;\n my @working_tree;\n \n sub usage\n@@ -38,17 +40,32 @@ USAGE\n \texit($exitcode);\n }\n \n+sub filter_tool_scripts\n+{\n+\tif (-d $_) {\n+\t\tif ($_ ne \".\") {\n+\t\t\t# Ignore files in subdirectories\n+\t\t\t$File::Find::prune = 1;\n+\t\t}\n+\t} else {\n+\t\tif ((-f $_) && ($_ ne \"defaults\")) {\n+\t\t\tpush(@tools, $_);\n+\t\t}\n+\t}\n+}\n+\n sub print_tool_help\n {\n \tmy ($cmd, @found, @notfound);\n \tmy $gitpath = Git::exec_path();\n \n-\tfor (glob \"$gitpath/mergetools/*\") {\n-\t\tmy $tool = basename($_);\n-\t\tnext if ($tool eq \"defaults\");\n+\tfind(\\&filter_tool_scripts, \"$gitpath/mergetools\");\n \n-\t\t$cmd  = '. \"$(git --exec-path)/git-mergetool--lib\"';\n+\tforeach my $tool (@tools) {\n+\t\t$cmd  = \"TOOL_MODE=diff\";\n+\t\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n \t\t$cmd .= \" && get_merge_tool_path $tool >/dev/null 2>&1\";\n+\t\t$cmd .= \" && can_diff >/dev/null 2>&1\";\n \t\tif (system('sh', '-c', $cmd) == 0) {\n \t\t\tpush(@found, $tool);\n \t\t} else {\n@@ -57,10 +74,10 @@ sub print_tool_help\n \t}\n \n \tprint \"'git difftool --tool=<tool>' may be set to one of the following:\\n\";\n-\tprint \"\\t$_\\n\" for (@found);\n+\tprint \"\\t$_\\n\" for (sort(@found));\n \n \tprint \"\\nThe following tools are valid, but not currently available:\\n\";\n-\tprint \"\\t$_\\n\" for (@notfound);\n+\tprint \"\\t$_\\n\" for (sort(@notfound));\n \n \tprint \"\\nNOTE: Some of the tools listed above only work in a windowed\\n\";\n \tprint \"environment. If run in a terminal-only session, they will fail.\\n\";\n-- 8< --\n"}]}