* [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).