threads / patch / 31090
Subject: [PATCH 1/3] difftool: Handle finding mergetools/ in a path with spaces
David Aguilar[PATCH 0/3] Incremental updates for da/difftool-updates
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
David AguilarUse 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
David Aguilar[PATCH 2/3] difftool: Check all return codes from compare()
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
David Aguilar[PATCH 3/3] difftool: Disable --symlinks on cygwin
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
Stefano LattariniRe: [PATCH 3/3] difftool: Disable --symlinks on cygwin
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
Erik Faye-LundRe: [PATCH 3/3] difftool: Disable --symlinks on cygwin
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?
David AguilarRe: [PATCH 3/3] difftool: Disable --symlinks on cygwin
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
Junio C HamanoRe: [PATCH 3/3] difftool: Disable --symlinks on cygwin
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.