[PATCH 1/5] difftool: Simplify print_tool_help()

[PATCH 1/5] difftool: Simplify print_tool_help()

[David Aguilar]

Eliminate a global variable and File::Find usage by building upon
basename() and glob() instead.

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

diff --git a/git-difftool.perl b/git-difftool.perl
index c079854..ac0ed63 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -13,17 +13,15 @@
 use 5.008;
 use strict;
 use warnings;
-use File::Basename qw(dirname);
+use File::Basename qw(basename dirname);
 use File::Copy;
 use File::Compare;
-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;
 my $rc;
 my $repo = Git->repository();
@@ -65,26 +63,13 @@ sub find_worktree
 
 my $workdir = find_worktree();
 
-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();
 
-	find(\&filter_tool_scripts, "$gitpath/mergetools");
+	my @files = map { basename($_) } glob("$gitpath/mergetools/*");
+	my @tools = sort(grep { !m{^defaults$} } @files);
 
 	foreach my $tool (@tools) {
 		$cmd  = "TOOL_MODE=diff";
@@ -99,10 +84,10 @@ sub print_tool_help
 	}
 
 	print "'git difftool --tool=<tool>' may be set to one of the following:\n";
-	print "\t$_\n" for (sort(@found));
+	print "\t$_\n" for (@found);
 
 	print "\nThe following tools are valid, but not currently available:\n";
-	print "\t$_\n" for (sort(@notfound));
+	print "\t$_\n" for (@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.11.2.255.g5f133da

[PATCH 2/5] difftool: Eliminate global variables

[David Aguilar]

Organize the script so that it has a single main() function which
calls out to dir_diff() and file_diff() functions. This eliminates
"dir-diff"-specific variables that do not need to be calculated when
performing a regular file-diff.

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

diff --git a/git-difftool.perl b/git-difftool.perl
index ac0ed63..41ba932 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,11 +22,6 @@ use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
-my @working_tree;
-my $rc;
-my $repo = Git->repository();
-my $repo_path = $repo->repo_path();
-
 sub usage
 {
 	my $exitcode = shift;
@@ -43,6 +38,8 @@ USAGE
 
 sub find_worktree
 {
+	my ($repo) = @_;
+
 	# Git->repository->wc_path() does not honor changes to the working
 	# tree location made by $ENV{GIT_WORK_TREE} or the 'core.worktree'
 	# config variable.
@@ -61,8 +58,6 @@ sub find_worktree
 	return $worktree;
 }
 
-my $workdir = find_worktree();
-
 sub print_tool_help
 {
 	my ($cmd, @found, @notfound);
@@ -97,10 +92,13 @@ sub print_tool_help
 
 sub setup_dir_diff
 {
+	my ($repo, $workdir) = @_;
+
 	# Run the diff; exit immediately if no diff found
 	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
 	# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used
 	# by Git->repository->command*.
+	my $repo_path = $repo->repo_path();
 	my $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);
 	my $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);
 	exit(0) if (length($diffrtn) == 0);
@@ -121,6 +119,7 @@ sub setup_dir_diff
 	my $rindex = '';
 	my %submodule;
 	my %symlink;
+	my @working_tree = ();
 	my @rawdiff = split('\0', $diffrtn);
 
 	my $i = 0;
@@ -188,7 +187,7 @@ sub setup_dir_diff
 	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
 	print($inpipe $lindex);
 	$repo->command_close_pipe($inpipe, $ctx);
-	$rc = system('git', 'checkout-index', '--all', "--prefix=$ldir/");
+	my $rc = system('git', 'checkout-index', '--all', "--prefix=$ldir/");
 	exit($rc | ($rc >> 8)) if ($rc != 0);
 
 	$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
@@ -238,7 +237,7 @@ sub setup_dir_diff
 		}
 	}
 
-	return ($ldir, $rdir);
+	return ($ldir, $rdir, @working_tree);
 }
 
 sub write_to_file
@@ -261,54 +260,70 @@ sub write_to_file
 	close($fh);
 }
 
-# parse command-line options. all unrecognized options and arguments
-# are passed through to the 'git diff' command.
-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);
-GetOptions('g|gui!' => \$gui,
-	'd|dir-diff' => \$dirdiff,
-	'h' => \$help,
-	'prompt!' => \$prompt,
-	'y' => sub { $prompt = 0; },
-	't|tool:s' => \$difftool_cmd,
-	'tool-help' => \$tool_help,
-	'x|extcmd:s' => \$extcmd);
-
-if (defined($help)) {
-	usage(0);
-}
-if (defined($tool_help)) {
-	print_tool_help();
-}
-if (defined($difftool_cmd)) {
-	if (length($difftool_cmd) > 0) {
-		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
-	} else {
-		print "No <tool> given for --tool=<tool>\n";
-		usage(1);
+sub main
+{
+	# parse command-line options. all unrecognized options and arguments
+	# are passed through to the 'git diff' command.
+	my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);
+	GetOptions('g|gui!' => \$gui,
+		'd|dir-diff' => \$dirdiff,
+		'h' => \$help,
+		'prompt!' => \$prompt,
+		'y' => sub { $prompt = 0; },
+		't|tool:s' => \$difftool_cmd,
+		'tool-help' => \$tool_help,
+		'x|extcmd:s' => \$extcmd);
+
+	if (defined($help)) {
+		usage(0);
 	}
-}
-if (defined($extcmd)) {
-	if (length($extcmd) > 0) {
-		$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
-	} else {
-		print "No <cmd> given for --extcmd=<cmd>\n";
-		usage(1);
+	if (defined($tool_help)) {
+		print_tool_help();
 	}
-}
-if ($gui) {
-	my $guitool = '';
-	$guitool = Git::config('diff.guitool');
-	if (length($guitool) > 0) {
-		$ENV{GIT_DIFF_TOOL} = $guitool;
+	if (defined($difftool_cmd)) {
+		if (length($difftool_cmd) > 0) {
+			$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
+		} else {
+			print "No <tool> given for --tool=<tool>\n";
+			usage(1);
+		}
+	}
+	if (defined($extcmd)) {
+		if (length($extcmd) > 0) {
+			$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
+		} else {
+			print "No <cmd> given for --extcmd=<cmd>\n";
+			usage(1);
+		}
+	}
+	if ($gui) {
+		my $guitool = '';
+		$guitool = Git::config('diff.guitool');
+		if (length($guitool) > 0) {
+			$ENV{GIT_DIFF_TOOL} = $guitool;
+		}
+	}
+
+	# In directory diff mode, 'git-difftool--helper' is called once
+	# to compare the a/b directories.  In file diff mode, 'git diff'
+	# will invoke a separate instance of 'git-difftool--helper' for
+	# each file that changed.
+	if (defined($dirdiff)) {
+		dir_diff($extcmd);
+	} else {
+		file_diff($prompt);
 	}
 }
 
-# In directory diff mode, 'git-difftool--helper' is called once
-# to compare the a/b directories.  In file diff mode, 'git diff'
-# will invoke a separate instance of 'git-difftool--helper' for
-# each file that changed.
-if (defined($dirdiff)) {
-	my ($a, $b) = setup_dir_diff();
+sub dir_diff
+{
+	my ($extcmd) = @_;
+
+	my $rc;
+	my $repo = Git->repository();
+
+	my $workdir = find_worktree($repo);
+	my ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir);
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);
 	} else {
@@ -327,7 +342,12 @@ if (defined($dirdiff)) {
 			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
 		}
 	}
-} else {
+}
+
+sub file_diff
+{
+	my ($prompt) = @_;
+
 	if (defined($prompt)) {
 		if ($prompt) {
 			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
@@ -347,3 +367,5 @@ if (defined($dirdiff)) {
 	my $rc = system('git', 'diff', @ARGV);
 	exit($rc | ($rc >> 8));
 }
+
+main();
-- 
1.7.11.2.255.g5f133da

[PATCH 3/5] difftool: Move option values into a hash

[David Aguilar]

Shorten the "my" declaration for all of the option-specific variables
by wrapping all of them in a hash.  This makes also gives us a place
to specify default values, should we need them.

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

diff --git a/git-difftool.perl b/git-difftool.perl
index 41ba932..0ce6168 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -264,41 +264,48 @@ sub main
 {
 	# parse command-line options. all unrecognized options and arguments
 	# are passed through to the 'git diff' command.
-	my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);
-	GetOptions('g|gui!' => \$gui,
-		'd|dir-diff' => \$dirdiff,
-		'h' => \$help,
-		'prompt!' => \$prompt,
-		'y' => sub { $prompt = 0; },
-		't|tool:s' => \$difftool_cmd,
-		'tool-help' => \$tool_help,
-		'x|extcmd:s' => \$extcmd);
-
-	if (defined($help)) {
+	my %opts = (
+		difftool_cmd => undef,
+		dirdiff => undef,
+		extcmd => undef,
+		gui => undef,
+		help => undef,
+		prompt => undef,
+		tool_help => undef,
+	);
+	GetOptions('g|gui!' => \$opts{gui},
+		'd|dir-diff' => \$opts{dirdiff},
+		'h' => \$opts{help},
+		'prompt!' => \$opts{prompt},
+		'y' => sub { $opts{prompt} = 0; },
+		't|tool:s' => \$opts{difftool_cmd},
+		'tool-help' => \$opts{tool_help},
+		'x|extcmd:s' => \$opts{extcmd});
+
+	if (defined($opts{help})) {
 		usage(0);
 	}
-	if (defined($tool_help)) {
+	if (defined($opts{tool_help})) {
 		print_tool_help();
 	}
-	if (defined($difftool_cmd)) {
-		if (length($difftool_cmd) > 0) {
-			$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
+	if (defined($opts{difftool_cmd})) {
+		if (length($opts{difftool_cmd}) > 0) {
+			$ENV{GIT_DIFF_TOOL} = $opts{difftool_cmd};
 		} else {
 			print "No <tool> given for --tool=<tool>\n";
 			usage(1);
 		}
 	}
-	if (defined($extcmd)) {
-		if (length($extcmd) > 0) {
-			$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;
+	if (defined($opts{extcmd})) {
+		if (length($opts{extcmd}) > 0) {
+			$ENV{GIT_DIFFTOOL_EXTCMD} = $opts{extcmd};
 		} else {
 			print "No <cmd> given for --extcmd=<cmd>\n";
 			usage(1);
 		}
 	}
-	if ($gui) {
-		my $guitool = '';
-		$guitool = Git::config('diff.guitool');
+	if ($opts{gui}) {
+		my $guitool = Git::config('diff.guitool');
 		if (length($guitool) > 0) {
 			$ENV{GIT_DIFF_TOOL} = $guitool;
 		}
@@ -308,10 +315,10 @@ sub main
 	# to compare the a/b directories.  In file diff mode, 'git diff'
 	# will invoke a separate instance of 'git-difftool--helper' for
 	# each file that changed.
-	if (defined($dirdiff)) {
-		dir_diff($extcmd);
+	if (defined($opts{dirdiff})) {
+		dir_diff($opts{extcmd});
 	} else {
-		file_diff($prompt);
+		file_diff($opts{prompt});
 	}
 }
 
-- 
1.7.11.2.255.g5f133da

[PATCH 5/5] difftool: Use symlinks when diffing against the worktree

[David Aguilar]

Teach difftool's --dir-diff mode to use symlinks to represent
files from the working copy, and make it the default behavior
for the non-Windows platforms.

Using symlinks is simpler and safer since we do not need to
worry about copying files back into the worktree.
The old behavior is still available as --no-symlinks.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/git-difftool.txt |  8 ++++++++
 git-difftool.perl              | 30 +++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 31fc2e3..313d54e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -66,6 +66,14 @@ of the diff post-image.  `$MERGED` is the name of the file which is
 being compared. `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$MERGED`.
 
+--symlinks::
+--no-symlinks::
+	'git difftool''s default behavior is create symlinks to the
+	working tree when run in `--dir-diff` mode.
++
+	Specifying `--no-symlinks` instructs 'git difftool' to create
+	copies instead.  `--no-symlinks` is the default on Windows.
+
 --tool-help::
 	Print a list of diff tools that may be used with `--tool`.
 
diff --git a/git-difftool.perl b/git-difftool.perl
index 2ae344c..b8f8057 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -92,7 +92,7 @@ sub print_tool_help
 
 sub setup_dir_diff
 {
-	my ($repo, $workdir) = @_;
+	my ($repo, $workdir, $symlinks) = @_;
 
 	# Run the diff; exit immediately if no diff found
 	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
@@ -209,8 +209,13 @@ sub setup_dir_diff
 		unless (-d "$rdir/$dir") {
 			mkpath("$rdir/$dir") or die $!;
 		}
-		copy("$workdir/$file", "$rdir/$file") or die $!;
-		chmod(stat("$workdir/$file")->mode, "$rdir/$file") or die $!;
+		if ($symlinks) {
+			symlink("$workdir/$file", "$rdir/$file") or die $!;
+		} else {
+			copy("$workdir/$file", "$rdir/$file") or die $!;
+			my $mode = stat("$workdir/$file")->mode;
+			chmod($mode, "$rdir/$file") or die $!;
+		}
 	}
 
 	# Changes to submodules require special treatment. This loop writes a
@@ -271,6 +276,7 @@ sub main
 		gui => undef,
 		help => undef,
 		prompt => undef,
+		symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
 		tool_help => undef,
 	);
 	GetOptions('g|gui!' => \$opts{gui},
@@ -278,6 +284,8 @@ sub main
 		'h' => \$opts{help},
 		'prompt!' => \$opts{prompt},
 		'y' => sub { $opts{prompt} = 0; },
+		'symlinks' => \$opts{symlinks},
+		'no-symlinks' => sub { $opts{symlinks} = 0; },
 		't|tool:s' => \$opts{difftool_cmd},
 		'tool-help' => \$opts{tool_help},
 		'x|extcmd:s' => \$opts{extcmd});
@@ -316,7 +324,7 @@ sub main
 	# will invoke a separate instance of 'git-difftool--helper' for
 	# each file that changed.
 	if (defined($opts{dirdiff})) {
-		dir_diff($opts{extcmd});
+		dir_diff($opts{extcmd}, $opts{symlinks});
 	} else {
 		file_diff($opts{prompt});
 	}
@@ -324,13 +332,13 @@ sub main
 
 sub dir_diff
 {
-	my ($extcmd) = @_;
+	my ($extcmd, $symlinks) = @_;
 
 	my $rc;
 	my $repo = Git->repository();
 
 	my $workdir = find_worktree($repo);
-	my ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir);
+	my ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir, $symlinks);
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);
 	} else {
@@ -340,15 +348,23 @@ sub dir_diff
 
 	exit($rc | ($rc >> 8)) if ($rc != 0);
 
+	# Do not copy back files when symlinks are used
+	if ($symlinks) {
+		exit(0);
+	}
+
 	# If the diff including working copy files and those
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
+
 	for my $file (@working_tree) {
 		if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {
 			copy("$b/$file", "$workdir/$file") or die $!;
-			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+			my $mode = stat("$b/$file")->mode;
+			chmod($mode, "$workdir/$file") or die $!;
 		}
 	}
+	exit(0);
 }
 
 sub file_diff
-- 
1.7.11.2.255.g5f133da

Re: [PATCH 3/5] difftool: Move option values into a hash

[David Aguilar]

On Sun, Jul 22, 2012 at 8:42 PM, David Aguilar <davvid@gmail.com> wrote:
> ... This makes also gives us a place to specify default values

Oops, please drop the word "makes" from the commit message here if you
apply this, or I'll fix it in a re-roll if review finds other issues.

Thanks,
-- 
David

Re: [PATCH 2/5] difftool: Eliminate global variables

[Sebastian Schuberth]

On 23.07.2012 05:42, David Aguilar wrote:

> Organize the script so that it has a single main() function which
> calls out to dir_diff() and file_diff() functions. This eliminates
> "dir-diff"-specific variables that do not need to be calculated when
> performing a regular file-diff.

Funny, I just have prepared a patch along the same lines so that one can 
call git-difftool -h and --tool-help also outside a repository, see 
below. Does you patch offer the same? If so, I'll drop mine.

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

diff --git a/git-difftool.perl b/git-difftool.perl
index ae1e052..e7b71c9 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -25,8 +25,9 @@ use Git;
  my @tools;
  my @working_tree;
  my $rc;
-my $repo = Git->repository();
-my $repo_path = $repo->repo_path();
+my $repo;
+my $repo_path;
+my $workdir;

  sub usage
  {
@@ -62,8 +63,6 @@ sub find_worktree
  	return $worktree;
  }

-my $workdir = find_worktree();
-
  sub filter_tool_scripts
  {
  	if (-d $_) {
@@ -293,6 +292,11 @@ if (defined($help)) {
  if (defined($tool_help)) {
  	print_tool_help();
  }
+
+$repo = Git->repository();
+$repo_path = $repo->repo_path();
+$workdir = find_worktree();
+
  if (defined($difftool_cmd)) {
  	if (length($difftool_cmd) > 0) {
  		$ENV{GIT_DIFF_TOOL} = $difftool_cmd;
-- 
1.7.11.msysgit.2

Re: [PATCH 2/5] difftool: Eliminate global variables

[David Aguilar]

On Sun, Jul 22, 2012 at 11:13 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On 23.07.2012 05:42, David Aguilar wrote:
>
>> Organize the script so that it has a single main() function which
>> calls out to dir_diff() and file_diff() functions. This eliminates
>> "dir-diff"-specific variables that do not need to be calculated when
>> performing a regular file-diff.
>
>
> Funny, I just have prepared a patch along the same lines so that one can
> call git-difftool -h and --tool-help also outside a repository, see below.
> Does you patch offer the same? If so, I'll drop mine.

It *should*. I did not consider this case but that is indeed the
desired behavior.

What my patch did not offer was a test to ensure this behavior...


>
> ---
>  git-difftool.perl | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index ae1e052..e7b71c9 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -25,8 +25,9 @@ use Git;
>  my @tools;
>  my @working_tree;
>
>  my $rc;
> -my $repo = Git->repository();
> -my $repo_path = $repo->repo_path();
> +my $repo;
> +my $repo_path;
> +my $workdir;
>
>  sub usage
>  {
> @@ -62,8 +63,6 @@ sub find_worktree
>
>         return $worktree;
>  }
>
> -my $workdir = find_worktree();
> -
>  sub filter_tool_scripts
>  {
>         if (-d $_) {
> @@ -293,6 +292,11 @@ if (defined($help)) {
>  if (defined($tool_help)) {
>         print_tool_help();
>  }
> +
> +$repo = Git->repository();
> +$repo_path = $repo->repo_path();
> +$workdir = find_worktree();
> +
>  if (defined($difftool_cmd)) {
>         if (length($difftool_cmd) > 0) {
>                 $ENV{GIT_DIFF_TOOL} = $difftool_cmd;
> --
> 1.7.11.msysgit.2



-- 
David

Re: [PATCH 2/5] difftool: Eliminate global variables

[Sebastian Schuberth]

On Mon, Jul 23, 2012 at 8:21 AM, David Aguilar <davvid@gmail.com> wrote:

>> Funny, I just have prepared a patch along the same lines so that one can
>> call git-difftool -h and --tool-help also outside a repository, see below.
>> Does you patch offer the same? If so, I'll drop mine.
>
> It *should*. I did not consider this case but that is indeed the
> desired behavior.
>
> What my patch did not offer was a test to ensure this behavior...

Well, I'm not asking for a test. From my side, I'd be happy if you
could just try it and confirm that it works, as I currently cannot
easily apply your patch.

-- 
Sebastian Schuberth

Re: [PATCH 2/5] difftool: Eliminate global variables

[David Aguilar]

On Sun, Jul 22, 2012 at 11:30 PM, Sebastian Schuberth
<sschuberth@gmail.com> wrote:
> On Mon, Jul 23, 2012 at 8:21 AM, David Aguilar <davvid@gmail.com> wrote:
>
>>> Funny, I just have prepared a patch along the same lines so that one can
>>> call git-difftool -h and --tool-help also outside a repository, see below.
>>> Does you patch offer the same? If so, I'll drop mine.
>>
>> It *should*. I did not consider this case but that is indeed the
>> desired behavior.
>>
>> What my patch did not offer was a test to ensure this behavior...
>
> Well, I'm not asking for a test. From my side, I'd be happy if you
> could just try it and confirm that it works, as I currently cannot
> easily apply your patch.

Heheh.. I was hoping you could provide a test ;-)

I just tried it now.  They work outside of a repository.
-- 
David

Re: [PATCH 2/5] difftool: Eliminate global variables

[Sebastian Schuberth]

On Mon, Jul 23, 2012 at 8:44 AM, David Aguilar <davvid@gmail.com> wrote:

>> Well, I'm not asking for a test. From my side, I'd be happy if you
>> could just try it and confirm that it works, as I currently cannot
>> easily apply your patch.
>
> Heheh.. I was hoping you could provide a test ;-)

;-)

> I just tried it now.  They work outside of a repository.

Great, thanks! I'm dropping my patch then.

-- 
Sebastian Schuberth

Re: [PATCH 1/5] difftool: Simplify print_tool_help()

[Tim Henigan]

On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:
> Eliminate a global variable and File::Find usage by building upon
> basename() and glob() instead.

glob was used in an early revision of the patch that led to bf73fc2
(difftool: print list of valid tools with '--tool-help') as well [1].
However, if the path to git or the path under 'mergetools' includes
spaces, glob fails.  To work around the problem, File::Find was used
instead [2].

Does this implementation handle that case?  I'm sorry, but I haven't
had time to apply and test myself.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925
[2]: http://thread.gmane.org/gmane.comp.version-control.git/194158

Re: [PATCH 1/5] difftool: Simplify print_tool_help()

[David Aguilar]

On Tue, Jul 24, 2012 at 5:43 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:
>> Eliminate a global variable and File::Find usage by building upon
>> basename() and glob() instead.
>
> glob was used in an early revision of the patch that led to bf73fc2
> (difftool: print list of valid tools with '--tool-help') as well [1].
> However, if the path to git or the path under 'mergetools' includes
> spaces, glob fails.  To work around the problem, File::Find was used
> instead [2].
>
> Does this implementation handle that case?  I'm sorry, but I haven't
> had time to apply and test myself.
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925
> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158

Good catch.  Eliminating the globals should be the goal, then.
I'll have to re-roll.
-- 
David

Re: [PATCH 1/5] difftool: Simplify print_tool_help()

[David Aguilar]

On Tue, Jul 24, 2012 at 6:52 PM, David Aguilar <davvid@gmail.com> wrote:
> On Tue, Jul 24, 2012 at 5:43 AM, Tim Henigan <tim.henigan@gmail.com> wrote:
>> On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:
>>> Eliminate a global variable and File::Find usage by building upon
>>> basename() and glob() instead.
>>
>> glob was used in an early revision of the patch that led to bf73fc2
>> (difftool: print list of valid tools with '--tool-help') as well [1].
>> However, if the path to git or the path under 'mergetools' includes
>> spaces, glob fails.  To work around the problem, File::Find was used
>> instead [2].
>>
>> Does this implementation handle that case?  I'm sorry, but I haven't
>> had time to apply and test myself.
>>
>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925
>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158
>
> Good catch.  Eliminating the globals should be the goal, then.
> I'll have to re-roll.

These landed in next.

Junio, would you prefer follow-up patches or a rebased series?
-- 
David

Re: [PATCH 1/5] difftool: Simplify print_tool_help()

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

>>> Does this implementation handle that case?  I'm sorry, but I haven't
>>> had time to apply and test myself.
>>>
>>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925
>>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158
>>
>> Good catch.  Eliminating the globals should be the goal, then.
>> I'll have to re-roll.
>
> Junio, would you prefer follow-up patches or a rebased series?

Incremental patches, please.

Thanks.

Re: [PATCH 1/5] difftool: Simplify print_tool_help()

[David Aguilar]

On Tue, Jul 24, 2012 at 7:40 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>>>> Does this implementation handle that case?  I'm sorry, but I haven't
>>>> had time to apply and test myself.
>>>>
>>>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925
>>>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158
>>>
>>> Good catch.  Eliminating the globals should be the goal, then.
>>> I'll have to re-roll.
>>
>> Junio, would you prefer follow-up patches or a rebased series?
>
> Incremental patches, please.
>
> Thanks.

Okay, I saw this just before hitting send-email (and sent the whole series).

I'll prep the incremental ones shortly, so please ignore the one I just sent...
-- 
David

Re: [PATCH 5/5] difftool: Use symlinks when diffing against the worktree

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> +	# Do not copy back files when symlinks are used
> +	if ($symlinks) {
> +		exit(0);
> +	}
> +

Isn't this a bit risky, depending on the behaviour of the tool that
eventually lead the user to invoke his favorite editor to muck with
the files in the temporary directory?  I think most sane people and
their editors would follow symlinks and update the file the symlink
points at when writing out the modified contents, but it should not
be too much trouble to detect the case in which the editor unlinked
the symlink and recreated a regular file in its place, and copy the
file back when that happened, to make it even safer, no?

The most lazy solution would be to just remove the above block, and
let the compare() compare the symlink $b/$file and the working tree
file $workdir/$file that is pointed by it. We will find data losing
case where the editor unlinks and creates that way automatically.

Optionally, you can update

	if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {

with

	if (! -l "$b/$file" && -f _ && compare("$b/$file", "$workdir/$file")) {

to avoid the cost of comparison.

>  	# If the diff including working copy files and those
>  	# files were modified during the diff, then the changes
>  	# should be copied back to the working tree
> +
>  	for my $file (@working_tree) {
>  		if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {
>  			copy("$b/$file", "$workdir/$file") or die $!;
> -			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
> +			my $mode = stat("$b/$file")->mode;
> +			chmod($mode, "$workdir/$file") or die $!;
>  		}
>  	}
> +	exit(0);
>  }

Other than that, the series looked well thought-out.

Thanks.

[PATCH 5/5] difftool: Use symlinks when diffing against the worktree

[David Aguilar]

Teach difftool's --dir-diff mode to use symlinks to represent
files from the working copy, and make it the default behavior
for the non-Windows platforms.

Using symlinks is simpler and safer since we do not need to
worry about copying files back into the worktree.
The old behavior is still available as --no-symlinks.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Same as before, resending because gmail ate patch 4/5

 Documentation/git-difftool.txt |  8 ++++++++
 git-difftool.perl              | 30 +++++++++++++++++++++++-------
 2 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 31fc2e3..313d54e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -66,6 +66,14 @@ of the diff post-image.  `$MERGED` is the name of the file which is
 being compared. `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$MERGED`.
 
+--symlinks::
+--no-symlinks::
+	'git difftool''s default behavior is create symlinks to the
+	working tree when run in `--dir-diff` mode.
++
+	Specifying `--no-symlinks` instructs 'git difftool' to create
+	copies instead.  `--no-symlinks` is the default on Windows.
+
 --tool-help::
 	Print a list of diff tools that may be used with `--tool`.
 
diff --git a/git-difftool.perl b/git-difftool.perl
index 2ae344c..b8f8057 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -92,7 +92,7 @@ sub print_tool_help
 
 sub setup_dir_diff
 {
-	my ($repo, $workdir) = @_;
+	my ($repo, $workdir, $symlinks) = @_;
 
 	# Run the diff; exit immediately if no diff found
 	# 'Repository' and 'WorkingCopy' must be explicitly set to insure that
@@ -209,8 +209,13 @@ sub setup_dir_diff
 		unless (-d "$rdir/$dir") {
 			mkpath("$rdir/$dir") or die $!;
 		}
-		copy("$workdir/$file", "$rdir/$file") or die $!;
-		chmod(stat("$workdir/$file")->mode, "$rdir/$file") or die $!;
+		if ($symlinks) {
+			symlink("$workdir/$file", "$rdir/$file") or die $!;
+		} else {
+			copy("$workdir/$file", "$rdir/$file") or die $!;
+			my $mode = stat("$workdir/$file")->mode;
+			chmod($mode, "$rdir/$file") or die $!;
+		}
 	}
 
 	# Changes to submodules require special treatment. This loop writes a
@@ -271,6 +276,7 @@ sub main
 		gui => undef,
 		help => undef,
 		prompt => undef,
+		symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
 		tool_help => undef,
 	);
 	GetOptions('g|gui!' => \$opts{gui},
@@ -278,6 +284,8 @@ sub main
 		'h' => \$opts{help},
 		'prompt!' => \$opts{prompt},
 		'y' => sub { $opts{prompt} = 0; },
+		'symlinks' => \$opts{symlinks},
+		'no-symlinks' => sub { $opts{symlinks} = 0; },
 		't|tool:s' => \$opts{difftool_cmd},
 		'tool-help' => \$opts{tool_help},
 		'x|extcmd:s' => \$opts{extcmd});
@@ -316,7 +324,7 @@ sub main
 	# will invoke a separate instance of 'git-difftool--helper' for
 	# each file that changed.
 	if (defined($opts{dirdiff})) {
-		dir_diff($opts{extcmd});
+		dir_diff($opts{extcmd}, $opts{symlinks});
 	} else {
 		file_diff($opts{prompt});
 	}
@@ -324,13 +332,13 @@ sub main
 
 sub dir_diff
 {
-	my ($extcmd) = @_;
+	my ($extcmd, $symlinks) = @_;
 
 	my $rc;
 	my $repo = Git->repository();
 
 	my $workdir = find_worktree($repo);
-	my ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir);
+	my ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir, $symlinks);
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);
 	} else {
@@ -340,15 +348,23 @@ sub dir_diff
 
 	exit($rc | ($rc >> 8)) if ($rc != 0);
 
+	# Do not copy back files when symlinks are used
+	if ($symlinks) {
+		exit(0);
+	}
+
 	# If the diff including working copy files and those
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
+
 	for my $file (@working_tree) {
 		if (-e "$b/$file" && compare("$b/$file", "$workdir/$file")) {
 			copy("$b/$file", "$workdir/$file") or die $!;
-			chmod(stat("$b/$file")->mode, "$workdir/$file") or die $!;
+			my $mode = stat("$b/$file")->mode;
+			chmod($mode, "$workdir/$file") or die $!;
 		}
 	}
+	exit(0);
 }
 
 sub file_diff
-- 
1.7.11.2.255.g5f133da