[PATCH 0/3] Incremental updates for da/difftool-updates

[PATCH 0/3] Incremental updates for da/difftool-updates

[David Aguilar]

These are incremental updates on top of da/difftool-updates,
which is currently in next.

David Aguilar (3):
  difftool: Handle finding mergetools/ in a path with spaces
  difftool: Check all return codes from compare()
  difftool: Disable --symlinks on cygwin

 git-difftool.perl | 41 +++++++++++++++++++++++++++++++++--------
 1 file changed, 33 insertions(+), 8 deletions(-)

-- 
1.7.12.rc0.15.g8157c39

[PATCH 1/3] difftool: Handle finding mergetools/ in a path with spaces

[David Aguilar]

Use the original File::Find implementation from bf73fc212a159210398b6d46ed5e9101c650e7db
so that we properly handle mergetools/ being located in a path containing
spaces.

One small difference is that we avoid using a global variable by
passing a reference to the list of tools.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool.perl | 27 +++++++++++++++++++++------
 1 file changed, 21 insertions(+), 6 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index a5b371f..3057480 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,9 +13,10 @@
 use 5.008;
 use strict;
 use warnings;
-use File::Basename qw(basename dirname);
+use File::Basename qw(dirname);
 use File::Copy;
 use File::Compare;
+use File::Find;
 use File::stat;
 use File::Path qw(mkpath);
 use File::Temp qw(tempdir);
@@ -58,13 +59,27 @@ sub find_worktree
 	return $worktree;
 }
 
+sub filter_tool_scripts
+{
+	my ($tools) = @_;
+	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 ($cmd, @found, @notfound, @tools);
 	my $gitpath = Git::exec_path();
 
-	my @files = map { basename($_) } glob("$gitpath/mergetools/*");
-	my @tools = sort(grep { !m{^defaults$} } @files);
+	find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
 
 	foreach my $tool (@tools) {
 		$cmd  = "TOOL_MODE=diff";
@@ -79,10 +94,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";
-- 
1.7.12.rc0.15.g8157c39

[PATCH 2/3] difftool: Check all return codes from compare()

[David Aguilar]

Handle the case where compare() is unable to read its inputs.
Emit a warning so that the user knows that something went wrong.

We may later want to restructure the code so that we can inhibit
tempdir cleanup when this condition is reached.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool.perl | 11 ++++++++++-
 1 file changed, 10 insertions(+), 1 deletion(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 3057480..591ee75 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -370,7 +370,16 @@ sub dir_diff
 	# external tool did not replace the original link with a file.
 	for my $file (@worktree) {
 		next if $symlinks && -l "$b/$file";
-		if (-f "$b/$file" && compare("$b/$file", "$workdir/$file")) {
+		next if ! -f "$b/$file";
+
+		my $diff = compare("$b/$file", "$workdir/$file");
+		if ($diff == 0) {
+			next;
+		} elsif ($diff == -1 ) {
+			my $errmsg = "warning: could not compare ";
+			$errmsg += "'$b/$file' with '$workdir/$file'\n";
+			warn $errmsg;
+		} elsif ($diff == 1) {
 			copy("$b/$file", "$workdir/$file") or die $!;
 			my $mode = stat("$b/$file")->mode;
 			chmod($mode, "$workdir/$file") or die $!;
-- 
1.7.12.rc0.15.g8157c39

[PATCH 3/3] difftool: Disable --symlinks on cygwin

[David Aguilar]

Symlinks are not ubiquitous on Windows so make --no-symlinks the default.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
I don't have cygwin so I can't verify this one myself.
Is 'cygwin' really the value of $^O there?

 git-difftool.perl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 591ee75..10d3d97 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -291,7 +291,8 @@ sub main
 		gui => undef,
 		help => undef,
 		prompt => undef,
-		symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
+		symlinks => $^O ne 'cygwin' &&
+				$^O ne 'MSWin32' && $^O ne 'msys',
 		tool_help => undef,
 	);
 	GetOptions('g|gui!' => \$opts{gui},
-- 
1.7.12.rc0.15.g8157c39

Re: [PATCH 0/3] Incremental updates for da/difftool-updates

[Junio C Hamano]

Thanks.

Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin

[Stefano Lattarini]

On 07/25/2012 05:14 AM, David Aguilar wrote:
> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.
> 
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> I don't have cygwin so I can't verify this one myself.
> Is 'cygwin' really the value of $^O there?
>
Apparently yes:

  $ uname -rso
  CYGWIN_NT-5.1 1.5.25(0.156/4/2) Cygwin
  $ perl -e 'print $^O'
  cygwin

Regards,
  Stefano

Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin

[Erik Faye-Lund]

On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:
> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.
>
> Signed-off-by: David Aguilar <davvid@gmail.com>
> ---
> I don't have cygwin so I can't verify this one myself.
> Is 'cygwin' really the value of $^O there?
>
>  git-difftool.perl | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 591ee75..10d3d97 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -291,7 +291,8 @@ sub main
>                 gui => undef,
>                 help => undef,
>                 prompt => undef,
> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
> +               symlinks => $^O ne 'cygwin' &&
> +                               $^O ne 'MSWin32' && $^O ne 'msys',

I thought Cygwin supported (their own version of) symlinks? What's the
rationale for not using it by default there?

Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin

[David Aguilar]

On Thu, Jul 26, 2012 at 4:31 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:
>> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.
>>
>> Signed-off-by: David Aguilar <davvid@gmail.com>
>> ---
>> I don't have cygwin so I can't verify this one myself.
>> Is 'cygwin' really the value of $^O there?
>>
>>  git-difftool.perl | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-difftool.perl b/git-difftool.perl
>> index 591ee75..10d3d97 100755
>> --- a/git-difftool.perl
>> +++ b/git-difftool.perl
>> @@ -291,7 +291,8 @@ sub main
>>                 gui => undef,
>>                 help => undef,
>>                 prompt => undef,
>> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
>> +               symlinks => $^O ne 'cygwin' &&
>> +                               $^O ne 'MSWin32' && $^O ne 'msys',
>
> I thought Cygwin supported (their own version of) symlinks? What's the
> rationale for not using it by default there?

I am not a Cygwin user so I cannot verify whether it is a good or bad idea.

I have a few questions regarding symlinks on Cygwin:

Do the symlinks work consistently with the Perl symlink() function?
Can we always rely on this capability being available?
Do all win32 filesystems support it?
Do all builds of cygwin perl support it?

If any of these answers are "no" or "maybe", then an improvement
beyond this patch would be to perhaps support a `difftool.symlinks`
configuration variable so that the user can tell us what to use as the
default.
-- 
David

Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> On Thu, Jul 26, 2012 at 4:31 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
>> On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:
>>> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.
>>>
>>> Signed-off-by: David Aguilar <davvid@gmail.com>
>>> ---
>>> I don't have cygwin so I can't verify this one myself.
>>> Is 'cygwin' really the value of $^O there?
>>>
>>>  git-difftool.perl | 3 ++-
>>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/git-difftool.perl b/git-difftool.perl
>>> index 591ee75..10d3d97 100755
>>> --- a/git-difftool.perl
>>> +++ b/git-difftool.perl
>>> @@ -291,7 +291,8 @@ sub main
>>>                 gui => undef,
>>>                 help => undef,
>>>                 prompt => undef,
>>> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
>>> +               symlinks => $^O ne 'cygwin' &&
>>> +                               $^O ne 'MSWin32' && $^O ne 'msys',
>>
>> I thought Cygwin supported (their own version of) symlinks? What's the
>> rationale for not using it by default there?
>
> I am not a Cygwin user so I cannot verify whether it is a good or bad idea.
>
> I have a few questions regarding symlinks on Cygwin:
>
> Do the symlinks work consistently with the Perl symlink() function?
> Can we always rely on this capability being available?
> Do all win32 filesystems support it?
> Do all builds of cygwin perl support it?
>
> If any of these answers are "no" or "maybe", then an improvement
> beyond this patch would be to perhaps support a `difftool.symlinks`
> configuration variable so that the user can tell us what to use as the
> default.

I would think it is an independent topic that can be used by people
not on Windows.  It is good to be conservative by disabling symlinks
by default.

Thanks.