git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode
@ 2012-07-23  3:57 David Aguilar
  2012-07-23  3:57 ` [PATCH v2 1/5] difftool: Simplify print_tool_help() David Aguilar
  2012-07-23  5:14 ` [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode Junio C Hamano
  0 siblings, 2 replies; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

Teach the difftool script to use symlinks when doing
directory diffs in --dir-diff mode.

This is v2 of the patch because I had a typo in one of the
commit messages and gmail ate 4/5 in the last round.

David Aguilar (5):
  difftool: Simplify print_tool_help()
  difftool: Eliminate global variables
  difftool: Move option values into a hash
  difftool: Call the temp directory "git-difftool"
  difftool: Use symlinks when diffing against the worktree

 Documentation/git-difftool.txt |   8 ++
 git-difftool.perl              | 184 ++++++++++++++++++++++++-----------------
 2 files changed, 115 insertions(+), 77 deletions(-)

-- 
1.7.11.2.255.g5f133da

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v2 1/5] difftool: Simplify print_tool_help()
  2012-07-23  3:57 [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode David Aguilar
@ 2012-07-23  3:57 ` David Aguilar
  2012-07-23  3:57   ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
  2012-07-23  5:14 ` [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---

Same as before, resending because gmail ate patch 4/5

 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  3:57 ` [PATCH v2 1/5] difftool: Simplify print_tool_help() David Aguilar
@ 2012-07-23  3:57   ` David Aguilar
  2012-07-23  3:57     ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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>
---
Same as before, resending because gmail ate patch 4/5

 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 3/5] difftool: Move option values into a hash
  2012-07-23  3:57   ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
@ 2012-07-23  3:57     ` David Aguilar
  2012-07-23  3:57       ` [PATCH 4/5] difftool: Call the temp directory "git-difftool" David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Fixed typo in the commit message

 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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 4/5] difftool: Call the temp directory "git-difftool"
  2012-07-23  3:57     ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
@ 2012-07-23  3:57       ` David Aguilar
  2012-07-23  3:57         ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

The "diffall" name was left over from when this functionality was part of
the "git-diffall" script in contrib/.  Make the naming consistent.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
Reworded the commit message to get through gmail filters.

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

diff --git a/git-difftool.perl b/git-difftool.perl
index 0ce6168..2ae344c 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -104,7 +104,7 @@ sub setup_dir_diff
 	exit(0) if (length($diffrtn) == 0);
 
 	# Setup temp directories
-	my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);
+	my $tmpdir = tempdir('git-difftool.XXXXX', CLEANUP => 1, TMPDIR => 1);
 	my $ldir = "$tmpdir/left";
 	my $rdir = "$tmpdir/right";
 	mkpath($ldir) or die $!;
-- 
1.7.11.2.255.g5f133da

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* [PATCH 5/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  3:57       ` [PATCH 4/5] difftool: Call the temp directory "git-difftool" David Aguilar
@ 2012-07-23  3:57         ` David Aguilar
  2012-07-23  4:57           ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: David Aguilar @ 2012-07-23  3:57 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH 5/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  3:57         ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
@ 2012-07-23  4:57           ` Junio C Hamano
  2012-07-23  6:05             ` [PATCH v3 4/5] " David Aguilar
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2012-07-23  4:57 UTC (permalink / raw)
  To: David Aguilar; +Cc: Tim Henigan, git

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.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode
  2012-07-23  3:57 [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode David Aguilar
  2012-07-23  3:57 ` [PATCH v2 1/5] difftool: Simplify print_tool_help() David Aguilar
@ 2012-07-23  5:14 ` Junio C Hamano
  2012-07-23  5:34   ` David Aguilar
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2012-07-23  5:14 UTC (permalink / raw)
  To: David Aguilar; +Cc: Tim Henigan, git

David Aguilar <davvid@gmail.com> writes:

> Teach the difftool script to use symlinks when doing
> directory diffs in --dir-diff mode.
>
> This is v2 of the patch because I had a typo in one of the
> commit messages and gmail ate 4/5 in the last round.

FWIW, I received all including 4/5 in my inboxes (at pobox and
gmail---I am doubly subscribed).  I still haven't figured out what
in the original 4/5 was so special to be dropped somewhere in
between.

> David Aguilar (5):
>   difftool: Simplify print_tool_help()
>   difftool: Eliminate global variables
>   difftool: Move option values into a hash
>   difftool: Call the temp directory "git-difftool"
>   difftool: Use symlinks when diffing against the worktree
>
>  Documentation/git-difftool.txt |   8 ++
>  git-difftool.perl              | 184 ++++++++++++++++++++++++-----------------
>  2 files changed, 115 insertions(+), 77 deletions(-)

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode
  2012-07-23  5:14 ` [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode Junio C Hamano
@ 2012-07-23  5:34   ` David Aguilar
  0 siblings, 0 replies; 13+ messages in thread
From: David Aguilar @ 2012-07-23  5:34 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

On Sun, Jul 22, 2012 at 10:14 PM, Junio C Hamano <gitster@pobox.com> wrote:
> David Aguilar <davvid@gmail.com> writes:
>
>> Teach the difftool script to use symlinks when doing
>> directory diffs in --dir-diff mode.
>>
>> This is v2 of the patch because I had a typo in one of the
>> commit messages and gmail ate 4/5 in the last round.
>
> FWIW, I received all including 4/5 in my inboxes (at pobox and
> gmail---I am doubly subscribed).  I still haven't figured out what
> in the original 4/5 was so special to be dropped somewhere in
> between.

I hastily blamed gmail but of course it was vger's spam filters.
The original subject said "git-difftool.XXXXX".
The exes triggered it.
-- 
David

^ permalink raw reply	[flat|nested] 13+ messages in thread

* [PATCH v3 4/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  4:57           ` Junio C Hamano
@ 2012-07-23  6:05             ` David Aguilar
  2012-07-23 16:38               ` Junio C Hamano
  2012-07-24 13:35               ` Tim Henigan
  0 siblings, 2 replies; 13+ messages in thread
From: David Aguilar @ 2012-07-23  6:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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>
---
Handles the case where an editor unlinks the original symlink,
replacing it with a file.

 Documentation/git-difftool.txt |    8 ++++++++
 git-difftool.perl              |   33 +++++++++++++++++++++++----------
 2 files changed, 31 insertions(+), 10 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..a5b371f 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, @worktree) = setup_dir_diff($repo, $workdir, $symlinks);
 	if (defined($extcmd)) {
 		$rc = system($extcmd, $a, $b);
 	} else {
@@ -342,13 +350,18 @@ sub dir_diff
 
 	# 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")) {
+	# should be copied back to the working tree.
+	# Do not copy back files when symlinks are used and the
+	# 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")) {
 			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.7.2.448.gee6df

^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  6:05             ` [PATCH v3 4/5] " David Aguilar
@ 2012-07-23 16:38               ` Junio C Hamano
  2012-07-24 13:35               ` Tim Henigan
  1 sibling, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2012-07-23 16:38 UTC (permalink / raw)
  To: David Aguilar; +Cc: Tim Henigan, git

David Aguilar <davvid@gmail.com> writes:

> 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>
> ---
> Handles the case where an editor unlinks the original symlink,
> replacing it with a file.

Thanks; will replace.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  6:05             ` [PATCH v3 4/5] " David Aguilar
  2012-07-23 16:38               ` Junio C Hamano
@ 2012-07-24 13:35               ` Tim Henigan
  2012-07-24 15:57                 ` Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: Tim Henigan @ 2012-07-24 13:35 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, git

I'm sorry I am so late to see and comment on this...I am just getting
caught up after a few busy weeks due to $dayjob and vacation.


On Mon, Jul 23, 2012 at 2:05 AM, David Aguilar <davvid@gmail.com> wrote:
>
> diff --git a/git-difftool.perl b/git-difftool.perl
> index 2ae344c..a5b371f 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
>
> @@ -271,6 +276,7 @@ sub main
>                 gui => undef,
>                 help => undef,
>                 prompt => undef,
> +               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',

Should this test for cygwin as well?


> @@ -342,13 +350,18 @@ sub dir_diff
>
>         # 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")) {
> +       # should be copied back to the working tree.
> +       # Do not copy back files when symlinks are used and the
> +       # 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")) {

compare returns '-1' if an error is encountered while reading a file.
In this (unlikely) case, should it still overwrite the working copy
file?  I think the answer is 'yes', but thought it was worth
mentioning.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH v3 4/5] difftool: Use symlinks when diffing against the worktree
  2012-07-24 13:35               ` Tim Henigan
@ 2012-07-24 15:57                 ` Junio C Hamano
  0 siblings, 0 replies; 13+ messages in thread
From: Junio C Hamano @ 2012-07-24 15:57 UTC (permalink / raw)
  To: Tim Henigan; +Cc: David Aguilar, git

Tim Henigan <tim.henigan@gmail.com> writes:

> I'm sorry I am so late to see and comment on this...I am just getting
> caught up after a few busy weeks due to $dayjob and vacation.
>
>
> On Mon, Jul 23, 2012 at 2:05 AM, David Aguilar <davvid@gmail.com> wrote:
>>
>> diff --git a/git-difftool.perl b/git-difftool.perl
>> index 2ae344c..a5b371f 100755
>> --- a/git-difftool.perl
>> +++ b/git-difftool.perl
>>
>> @@ -271,6 +276,7 @@ sub main
>>                 gui => undef,
>>                 help => undef,
>>                 prompt => undef,
>> +               symlinks => $^O ne 'MSWin32' && $^O ne 'msys',
>
> Should this test for cygwin as well?
>
>
>> @@ -342,13 +350,18 @@ sub dir_diff
>>
>>         # 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")) {
>> +       # should be copied back to the working tree.
>> +       # Do not copy back files when symlinks are used and the
>> +       # 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")) {
>
> compare returns '-1' if an error is encountered while reading a file.
> In this (unlikely) case, should it still overwrite the working copy
> file?  I think the answer is 'yes', but thought it was worth
> mentioning.

It probably is safer to report the error, not touch anything and let
the user take an appropriate action.

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2012-07-24 15:57 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-23  3:57 [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode David Aguilar
2012-07-23  3:57 ` [PATCH v2 1/5] difftool: Simplify print_tool_help() David Aguilar
2012-07-23  3:57   ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
2012-07-23  3:57     ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
2012-07-23  3:57       ` [PATCH 4/5] difftool: Call the temp directory "git-difftool" David Aguilar
2012-07-23  3:57         ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
2012-07-23  4:57           ` Junio C Hamano
2012-07-23  6:05             ` [PATCH v3 4/5] " David Aguilar
2012-07-23 16:38               ` Junio C Hamano
2012-07-24 13:35               ` Tim Henigan
2012-07-24 15:57                 ` Junio C Hamano
2012-07-23  5:14 ` [PATCH v2 0/5] difftool: Use symlinks in dir-diff mode Junio C Hamano
2012-07-23  5:34   ` David Aguilar

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).