{"thread":{"id":"31090","subject":"[PATCH 1/3] difftool: Handle finding mergetools/ in a path with spaces","startedAt":"2012-07-25T03:14:21Z","lastAt":"2012-07-26T19:04:20Z","messageCount":9,"participants":["David Aguilar","Junio C Hamano","Stefano Lattarini","Erik Faye-Lund"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"195692","messageId":"1343186064-49350-1-git-send-email-davvid@gmail.com","threadId":"31090","inReplyTo":null,"subject":"[PATCH 0/3] Incremental updates for da/difftool-updates","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T03:14:21Z","receivedAt":"2012-07-25T03:14:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"These are incremental updates on top of da/difftool-updates,\nwhich is currently in next.\n\nDavid Aguilar (3):\n  difftool: Handle finding mergetools/ in a path with spaces\n  difftool: Check all return codes from compare()\n  difftool: Disable --symlinks on cygwin\n\n git-difftool.perl | 41 +++++++++++++++++++++++++++++++++--------\n 1 file changed, 33 insertions(+), 8 deletions(-)\n\n-- \n1.7.12.rc0.15.g8157c39\n"},{"id":"195691","messageId":"1343186064-49350-2-git-send-email-davvid@gmail.com","threadId":"31090","inReplyTo":"1343186064-49350-1-git-send-email-davvid@gmail.com","subject":"[PATCH 1/3] difftool: Handle finding mergetools/ in a path with spaces","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T03:14:22Z","receivedAt":"2012-07-25T03:14:22Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Use the original File::Find implementation from bf73fc212a159210398b6d46ed5e9101c650e7db\nso that we properly handle mergetools/ being located in a path containing\nspaces.\n\nOne small difference is that we avoid using a global variable by\npassing a reference to the list of tools.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 27 +++++++++++++++++++++------\n 1 file changed, 21 insertions(+), 6 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex a5b371f..3057480 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -13,9 +13,10 @@\n use 5.008;\n use strict;\n use warnings;\n-use File::Basename qw(basename dirname);\n+use File::Basename qw(dirname);\n use File::Copy;\n use File::Compare;\n+use File::Find;\n use File::stat;\n use File::Path qw(mkpath);\n use File::Temp qw(tempdir);\n@@ -58,13 +59,27 @@ sub find_worktree\n \treturn $worktree;\n }\n \n+sub filter_tool_scripts\n+{\n+\tmy ($tools) = @_;\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 ($cmd, @found, @notfound, @tools);\n \tmy $gitpath = Git::exec_path();\n \n-\tmy @files = map { basename($_) } glob(\"$gitpath/mergetools/*\");\n-\tmy @tools = sort(grep { !m{^defaults$} } @files);\n+\tfind(sub { filter_tool_scripts(\\@tools) }, \"$gitpath/mergetools\");\n \n \tforeach my $tool (@tools) {\n \t\t$cmd  = \"TOOL_MODE=diff\";\n@@ -79,10 +94,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-- \n1.7.12.rc0.15.g8157c39\n"},{"id":"195694","messageId":"1343186064-49350-3-git-send-email-davvid@gmail.com","threadId":"31090","inReplyTo":"1343186064-49350-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/3] difftool: Check all return codes from compare()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T03:14:23Z","receivedAt":"2012-07-25T03:14:23Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Handle the case where compare() is unable to read its inputs.\nEmit a warning so that the user knows that something went wrong.\n\nWe may later want to restructure the code so that we can inhibit\ntempdir cleanup when this condition is reached.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 11 ++++++++++-\n 1 file changed, 10 insertions(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 3057480..591ee75 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -370,7 +370,16 @@ sub dir_diff\n \t# external tool did not replace the original link with a file.\n \tfor my $file (@worktree) {\n \t\tnext if $symlinks && -l \"$b/$file\";\n-\t\tif (-f \"$b/$file\" && compare(\"$b/$file\", \"$workdir/$file\")) {\n+\t\tnext if ! -f \"$b/$file\";\n+\n+\t\tmy $diff = compare(\"$b/$file\", \"$workdir/$file\");\n+\t\tif ($diff == 0) {\n+\t\t\tnext;\n+\t\t} elsif ($diff == -1 ) {\n+\t\t\tmy $errmsg = \"warning: could not compare \";\n+\t\t\t$errmsg += \"'$b/$file' with '$workdir/$file'\\n\";\n+\t\t\twarn $errmsg;\n+\t\t} elsif ($diff == 1) {\n \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n \t\t\tmy $mode = stat(\"$b/$file\")->mode;\n \t\t\tchmod($mode, \"$workdir/$file\") or die $!;\n-- \n1.7.12.rc0.15.g8157c39\n"},{"id":"195693","messageId":"1343186064-49350-4-git-send-email-davvid@gmail.com","threadId":"31090","inReplyTo":"1343186064-49350-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/3] difftool: Disable --symlinks on cygwin","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T03:14:24Z","receivedAt":"2012-07-25T03:14:24Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Symlinks are not ubiquitous on Windows so make --no-symlinks the default.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\nI don't have cygwin so I can't verify this one myself.\nIs 'cygwin' really the value of $^O there?\n\n git-difftool.perl | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 591ee75..10d3d97 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -291,7 +291,8 @@ sub main\n \t\tgui => undef,\n \t\thelp => undef,\n \t\tprompt => undef,\n-\t\tsymlinks => $^O ne 'MSWin32' && $^O ne 'msys',\n+\t\tsymlinks => $^O ne 'cygwin' &&\n+\t\t\t\t$^O ne 'MSWin32' && $^O ne 'msys',\n \t\ttool_help => undef,\n \t);\n \tGetOptions('g|gui!' => \\$opts{gui},\n-- \n1.7.12.rc0.15.g8157c39\n"},{"id":"195764","messageId":"7vpq7jdchw.fsf@alter.siamese.dyndns.org","threadId":"31090","inReplyTo":"1343186064-49350-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 0/3] Incremental updates for da/difftool-updates","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T16:36:11Z","receivedAt":"2012-07-25T16:36:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks.\n"},{"id":"195777","messageId":"50104B57.5050206@gmail.com","threadId":"31090","inReplyTo":"1343186064-49350-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin","fromName":"Stefano Lattarini","fromEmail":"stefano.lattarini@gmail.com","sentAt":"2012-07-25T19:39:03Z","receivedAt":"2012-07-25T19:39:03Z","isPatch":true,"sender":{"key":"stefano.lattarini@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1429199?v=4"},"body":"On 07/25/2012 05:14 AM, David Aguilar wrote:\n> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.\n> \n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> I don't have cygwin so I can't verify this one myself.\n> Is 'cygwin' really the value of $^O there?\n>\nApparently yes:\n\n  $ uname -rso\n  CYGWIN_NT-5.1 1.5.25(0.156/4/2) Cygwin\n  $ perl -e 'print $^O'\n  cygwin\n\nRegards,\n  Stefano\n"},{"id":"195845","messageId":"CABPQNSZ=yWxVivadJfh9BMPtqiiGyyEL0vZu+fvsu_Zzhd7yhw@mail.gmail.com","threadId":"31090","inReplyTo":"1343186064-49350-4-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-07-26T11:31:05Z","receivedAt":"2012-07-26T11:31:05Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:\n> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.\n>\n> Signed-off-by: David Aguilar <davvid@gmail.com>\n> ---\n> I don't have cygwin so I can't verify this one myself.\n> Is 'cygwin' really the value of $^O there?\n>\n>  git-difftool.perl | 3 ++-\n>  1 file changed, 2 insertions(+), 1 deletion(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index 591ee75..10d3d97 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -291,7 +291,8 @@ sub main\n>                 gui => undef,\n>                 help => undef,\n>                 prompt => undef,\n> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',\n> +               symlinks => $^O ne 'cygwin' &&\n> +                               $^O ne 'MSWin32' && $^O ne 'msys',\n\nI thought Cygwin supported (their own version of) symlinks? What's the\nrationale for not using it by default there?\n"},{"id":"195873","messageId":"CAJDDKr4z_ddwe7s8dY80dZJMNP=75VK6NjOV0WCqFL_LDuMpBA@mail.gmail.com","threadId":"31090","inReplyTo":"CABPQNSZ=yWxVivadJfh9BMPtqiiGyyEL0vZu+fvsu_Zzhd7yhw@mail.gmail.com","subject":"Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-26T17:39:28Z","receivedAt":"2012-07-26T17:39:28Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Jul 26, 2012 at 4:31 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n> On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:\n>> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.\n>>\n>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>> ---\n>> I don't have cygwin so I can't verify this one myself.\n>> Is 'cygwin' really the value of $^O there?\n>>\n>>  git-difftool.perl | 3 ++-\n>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index 591ee75..10d3d97 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -291,7 +291,8 @@ sub main\n>>                 gui => undef,\n>>                 help => undef,\n>>                 prompt => undef,\n>> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',\n>> +               symlinks => $^O ne 'cygwin' &&\n>> +                               $^O ne 'MSWin32' && $^O ne 'msys',\n>\n> I thought Cygwin supported (their own version of) symlinks? What's the\n> rationale for not using it by default there?\n\nI am not a Cygwin user so I cannot verify whether it is a good or bad idea.\n\nI have a few questions regarding symlinks on Cygwin:\n\nDo the symlinks work consistently with the Perl symlink() function?\nCan we always rely on this capability being available?\nDo all win32 filesystems support it?\nDo all builds of cygwin perl support it?\n\nIf any of these answers are \"no\" or \"maybe\", then an improvement\nbeyond this patch would be to perhaps support a `difftool.symlinks`\nconfiguration variable so that the user can tell us what to use as the\ndefault.\n-- \nDavid\n"},{"id":"195882","messageId":"7vhasu739n.fsf@alter.siamese.dyndns.org","threadId":"31090","inReplyTo":"CAJDDKr4z_ddwe7s8dY80dZJMNP=75VK6NjOV0WCqFL_LDuMpBA@mail.gmail.com","subject":"Re: [PATCH 3/3] difftool: Disable --symlinks on cygwin","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-26T19:04:20Z","receivedAt":"2012-07-26T19:04:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> On Thu, Jul 26, 2012 at 4:31 AM, Erik Faye-Lund <kusmabite@gmail.com> wrote:\n>> On Wed, Jul 25, 2012 at 5:14 AM, David Aguilar <davvid@gmail.com> wrote:\n>>> Symlinks are not ubiquitous on Windows so make --no-symlinks the default.\n>>>\n>>> Signed-off-by: David Aguilar <davvid@gmail.com>\n>>> ---\n>>> I don't have cygwin so I can't verify this one myself.\n>>> Is 'cygwin' really the value of $^O there?\n>>>\n>>>  git-difftool.perl | 3 ++-\n>>>  1 file changed, 2 insertions(+), 1 deletion(-)\n>>>\n>>> diff --git a/git-difftool.perl b/git-difftool.perl\n>>> index 591ee75..10d3d97 100755\n>>> --- a/git-difftool.perl\n>>> +++ b/git-difftool.perl\n>>> @@ -291,7 +291,8 @@ sub main\n>>>                 gui => undef,\n>>>                 help => undef,\n>>>                 prompt => undef,\n>>> -               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',\n>>> +               symlinks => $^O ne 'cygwin' &&\n>>> +                               $^O ne 'MSWin32' && $^O ne 'msys',\n>>\n>> I thought Cygwin supported (their own version of) symlinks? What's the\n>> rationale for not using it by default there?\n>\n> I am not a Cygwin user so I cannot verify whether it is a good or bad idea.\n>\n> I have a few questions regarding symlinks on Cygwin:\n>\n> Do the symlinks work consistently with the Perl symlink() function?\n> Can we always rely on this capability being available?\n> Do all win32 filesystems support it?\n> Do all builds of cygwin perl support it?\n>\n> If any of these answers are \"no\" or \"maybe\", then an improvement\n> beyond this patch would be to perhaps support a `difftool.symlinks`\n> configuration variable so that the user can tell us what to use as the\n> default.\n\nI would think it is an independent topic that can be used by people\nnot on Windows.  It is good to be conservative by disabling symlinks\nby default.\n\nThanks.\n"}]}