Re: t7800-difftool.sh failure on pu

Ramsay Jones [off-list ref] writes:

I would prefer this, regardless of the issue.

I actually recall asking Tim about the exit status when I reviewed this
change.

On Thu, Mar 29, 2012 at 2:26 PM, Junio C Hamano [off-list ref] wrote:

Ramsay Jones [off-list ref] writes:

quoted

Hi Tim,

With the current pu branch, I have t7800.3 (difftool ignores bad --tool values)
failing on Linux (I haven't tried cygwin or mingw yet). The failure is caused
by the test for the value of the exit code; for me the exit code is 9 not 1.

I have investigated, briefly, and found *two* alternatives for a fix. ;-)

The first option is to (effectively) revert commit 0440ed72 ("difftool: replace
system call with Git::command_noisy", 22-03-2012), like so:

-- >8 --

diff --git a/git-difftool.perl b/git-difftool.perl
index e1754ff..49613b1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -237,5 +237,7 @@ if (defined($dirdiff)) {

      $ENV{GIT_PAGER} = '';
      $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-     git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+     my @command = ('git', 'diff', @ARGV);
+     my $rc = system(@command);
+     exit($rc | ($rc >> 8));
 }
-- 8< --

I would prefer this, regardless of the issue.

I actually recall asking Tim about the exit status when I reviewed this
change.

I would also prefer this.

A question for the msysgit/cygwin folks:

would we need to go back to use "git.exe" as well?
-- 
David

On Sat, Mar 31, 2012 at 12:05 AM, David Aguilar [off-list ref] wrote:

On Thu, Mar 29, 2012 at 2:26 PM, Junio C Hamano [off-list ref] wrote:

quoted

Ramsay Jones [off-list ref] writes:

quoted

The first option is to (effectively) revert commit 0440ed72 ("difftool: replace
system call with Git::command_noisy", 22-03-2012), like so:

-- >8 --

diff --git a/git-difftool.perl b/git-difftool.perl
index e1754ff..49613b1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -237,5 +237,7 @@ if (defined($dirdiff)) {

      $ENV{GIT_PAGER} = '';
      $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-     git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+     my @command = ('git', 'diff', @ARGV);
+     my $rc = system(@command);
+     exit($rc | ($rc >> 8));
 }
-- 8< --

I would prefer this, regardless of the issue.

I actually recall asking Tim about the exit status when I reviewed this
change.

This breakage is a surprise to me.  All the tests in t7800 have passed
for me since my first modifications to them.  They continue to pass
for me on Ubuntu.  That being said, the simple tests that Ramsay
posted in his email (simply printing $!) also fail for me on msysgit.

I would also prefer this.

This change will involve:
  - Dropping 7/9 from the series
  - Editing 8/9 to replace 'git_cmd_try' with simple system calls

Would it be better to resend the entire series or just edit and resend 8/9?

A question for the msysgit/cygwin folks:

would we need to go back to use "git.exe" as well?

I would welcome some feedback from other Windows users.  I have been
successfully using this series on Win7 with msysgit, but obviously
some setups differ.

Junio C Hamano wrote:

Ramsay Jones [off-list ref] writes:

quoted

The first option is to (effectively) revert commit 0440ed72 ("difftool: replace
system call with Git::command_noisy", 22-03-2012), like so:

-- >8 --

diff --git a/git-difftool.perl b/git-difftool.perl
index e1754ff..49613b1 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -237,5 +237,7 @@ if (defined($dirdiff)) {
 
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-	git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+	my @command = ('git', 'diff', @ARGV);
+	my $rc = system(@command);
+	exit($rc | ($rc >> 8));
 }
-- 8< --

I would prefer this, regardless of the issue.

Me too! ;-P

I noticed this afternoon (using my isp's web-mail interface) that Tim has
already sent a patch fixing this issue along these lines. [Thanks Tim]

However, just for the record, all three of the platforms I test on failed
in more-or-less the same way. [the exit code on cygwin was 2 (No such File
or Directory) rather than 9.]

Note that the failure occurred (on Linux, cygwin and MinGW) when pu was at
commit e5471b396660313b172e102b50939c9c35d4c637 (v1.7.10-rc2-260-ge5471b3).
However, on Linux, having fetched an updated pu which is now at commit
ae98c9308d3a79f2b22de2f6dae614f700f7ce0c (v1.7.10-rc3-272-gae98c93), then
test t7800 now reliably passes!

[when I say it reliably passes (fails), I mean it passes (fails) each time
on ten consecutive runs.]

I created a diff of git-difftool.perl between these two commits and applied
the patch, in reverse, to effectively revert that file. Having done so, then
t7800 once again reliably fails! Looking at the diff, I have no idea why!
[I've included the diff below, just for completeness.]

ATB,
Ramsay Jones

dt.diff:
-- >8 --

diff --git a/git-difftool.perl b/git-difftool.perl
index e1754ff..78e93a6 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl

@@ -14,14 +14,16 @@
 use 5.008;
 use strict;
 use warnings;
-use File::Basename qw(dirname basename);
+use File::Basename qw(dirname);
 use File::Copy;
+use File::Find;
 use File::stat;
 use File::Path qw(mkpath);
 use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
+my @tools;
 my @working_tree;
 
 sub usage

@@ -38,17 +40,32 @@ USAGE
 	exit($exitcode);
 }
 
+sub filter_tool_scripts
+{
+	if (-d $_) {
+		if ($_ ne ".") {
+			# Ignore files in subdirectories
+			$File::Find::prune = 1;
+		}
+	} else {
+		if ((-f $_) && ($_ ne "defaults")) {
+			push(@tools, $_);
+		}
+	}
+}
+
 sub print_tool_help
 {
 	my ($cmd, @found, @notfound);
 	my $gitpath = Git::exec_path();
 
-	for (glob "$gitpath/mergetools/*") {
-		my $tool = basename($_);
-		next if ($tool eq "defaults");
+	find(\&filter_tool_scripts, "$gitpath/mergetools");
 
-		$cmd  = '. "$(git --exec-path)/git-mergetool--lib"';
+	foreach my $tool (@tools) {
+		$cmd  = "TOOL_MODE=diff";
+		$cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
 		$cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
+		$cmd .= " && can_diff >/dev/null 2>&1";
 		if (system('sh', '-c', $cmd) == 0) {
 			push(@found, $tool);
 		} else {

@@ -57,10 +74,10 @@ sub print_tool_help
 	}
 
 	print "'git difftool --tool=<tool>' may be set to one of the following:\n";
-	print "\t$_\n" for (@found);
+	print "\t$_\n" for (sort(@found));
 
 	print "\nThe following tools are valid, but not currently available:\n";
-	print "\t$_\n" for (@notfound);
+	print "\t$_\n" for (sort(@notfound));
 
 	print "\nNOTE: Some of the tools listed above only work in a windowed\n";
 	print "environment. If run in a terminal-only session, they will fail.\n";
-- 8< --