git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [PATCH 1/5] difftool: Simplify print_tool_help()
@ 2012-07-23  3:42 David Aguilar
  2012-07-23  3:42 ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
                   ` (3 more replies)
  0 siblings, 4 replies; 15+ messages in thread
From: David Aguilar @ 2012-07-23  3:42 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>
---
 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] 15+ messages in thread

* [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  3:42 [PATCH 1/5] difftool: Simplify print_tool_help() David Aguilar
@ 2012-07-23  3:42 ` David Aguilar
  2012-07-23  6:13   ` Sebastian Schuberth
  2012-07-23  3:42 ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
                   ` (2 subsequent siblings)
  3 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-23  3:42 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>
---
 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] 15+ messages in thread

* [PATCH 3/5] difftool: Move option values into a hash
  2012-07-23  3:42 [PATCH 1/5] difftool: Simplify print_tool_help() David Aguilar
  2012-07-23  3:42 ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
@ 2012-07-23  3:42 ` David Aguilar
  2012-07-23  3:49   ` David Aguilar
  2012-07-23  3:42 ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
  2012-07-24 12:43 ` [PATCH 1/5] difftool: Simplify print_tool_help() Tim Henigan
  3 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-23  3:42 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 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

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

* [PATCH 5/5] difftool: Use symlinks when diffing against the worktree
  2012-07-23  3:42 [PATCH 1/5] difftool: Simplify print_tool_help() David Aguilar
  2012-07-23  3:42 ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
  2012-07-23  3:42 ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
@ 2012-07-23  3:42 ` David Aguilar
  2012-07-24 12:43 ` [PATCH 1/5] difftool: Simplify print_tool_help() Tim Henigan
  3 siblings, 0 replies; 15+ messages in thread
From: David Aguilar @ 2012-07-23  3:42 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>
---
 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] 15+ messages in thread

* Re: [PATCH 3/5] difftool: Move option values into a hash
  2012-07-23  3:42 ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
@ 2012-07-23  3:49   ` David Aguilar
  0 siblings, 0 replies; 15+ messages in thread
From: David Aguilar @ 2012-07-23  3:49 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Tim Henigan, git

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

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

* Re: [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  3:42 ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
@ 2012-07-23  6:13   ` Sebastian Schuberth
  2012-07-23  6:21     ` David Aguilar
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Schuberth @ 2012-07-23  6:13 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Tim Henigan, git

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

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

* Re: [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  6:13   ` Sebastian Schuberth
@ 2012-07-23  6:21     ` David Aguilar
  2012-07-23  6:30       ` Sebastian Schuberth
  0 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-23  6:21 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: Junio C Hamano, Tim Henigan, git

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

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

* Re: [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  6:21     ` David Aguilar
@ 2012-07-23  6:30       ` Sebastian Schuberth
  2012-07-23  6:44         ` David Aguilar
  0 siblings, 1 reply; 15+ messages in thread
From: Sebastian Schuberth @ 2012-07-23  6:30 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Tim Henigan, git

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

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

* Re: [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  6:30       ` Sebastian Schuberth
@ 2012-07-23  6:44         ` David Aguilar
  2012-07-23  6:54           ` Sebastian Schuberth
  0 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-23  6:44 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: Junio C Hamano, Tim Henigan, git

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

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

* Re: [PATCH 2/5] difftool: Eliminate global variables
  2012-07-23  6:44         ` David Aguilar
@ 2012-07-23  6:54           ` Sebastian Schuberth
  0 siblings, 0 replies; 15+ messages in thread
From: Sebastian Schuberth @ 2012-07-23  6:54 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, Tim Henigan, git

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

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

* Re: [PATCH 1/5] difftool: Simplify print_tool_help()
  2012-07-23  3:42 [PATCH 1/5] difftool: Simplify print_tool_help() David Aguilar
                   ` (2 preceding siblings ...)
  2012-07-23  3:42 ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
@ 2012-07-24 12:43 ` Tim Henigan
  2012-07-25  1:52   ` David Aguilar
  3 siblings, 1 reply; 15+ messages in thread
From: Tim Henigan @ 2012-07-24 12:43 UTC (permalink / raw)
  To: David Aguilar; +Cc: Junio C Hamano, git

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

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

* Re: [PATCH 1/5] difftool: Simplify print_tool_help()
  2012-07-24 12:43 ` [PATCH 1/5] difftool: Simplify print_tool_help() Tim Henigan
@ 2012-07-25  1:52   ` David Aguilar
  2012-07-25  2:18     ` David Aguilar
  0 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-25  1:52 UTC (permalink / raw)
  To: Tim Henigan; +Cc: Junio C Hamano, git

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

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

* Re: [PATCH 1/5] difftool: Simplify print_tool_help()
  2012-07-25  1:52   ` David Aguilar
@ 2012-07-25  2:18     ` David Aguilar
  2012-07-25  2:40       ` Junio C Hamano
  0 siblings, 1 reply; 15+ messages in thread
From: David Aguilar @ 2012-07-25  2:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Tim Henigan

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

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

* Re: [PATCH 1/5] difftool: Simplify print_tool_help()
  2012-07-25  2:18     ` David Aguilar
@ 2012-07-25  2:40       ` Junio C Hamano
  2012-07-25  3:00         ` David Aguilar
  0 siblings, 1 reply; 15+ messages in thread
From: Junio C Hamano @ 2012-07-25  2:40 UTC (permalink / raw)
  To: David Aguilar; +Cc: git, Tim Henigan

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.

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

* Re: [PATCH 1/5] difftool: Simplify print_tool_help()
  2012-07-25  2:40       ` Junio C Hamano
@ 2012-07-25  3:00         ` David Aguilar
  0 siblings, 0 replies; 15+ messages in thread
From: David Aguilar @ 2012-07-25  3:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Tim Henigan

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

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

end of thread, other threads:[~2012-07-25  3:01 UTC | newest]

Thread overview: 15+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2012-07-23  3:42 [PATCH 1/5] difftool: Simplify print_tool_help() David Aguilar
2012-07-23  3:42 ` [PATCH 2/5] difftool: Eliminate global variables David Aguilar
2012-07-23  6:13   ` Sebastian Schuberth
2012-07-23  6:21     ` David Aguilar
2012-07-23  6:30       ` Sebastian Schuberth
2012-07-23  6:44         ` David Aguilar
2012-07-23  6:54           ` Sebastian Schuberth
2012-07-23  3:42 ` [PATCH 3/5] difftool: Move option values into a hash David Aguilar
2012-07-23  3:49   ` David Aguilar
2012-07-23  3:42 ` [PATCH 5/5] difftool: Use symlinks when diffing against the worktree David Aguilar
2012-07-24 12:43 ` [PATCH 1/5] difftool: Simplify print_tool_help() Tim Henigan
2012-07-25  1:52   ` David Aguilar
2012-07-25  2:18     ` David Aguilar
2012-07-25  2:40       ` Junio C Hamano
2012-07-25  3:00         ` 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).