{"thread":{"id":"31071","subject":"[PATCH 1/5] difftool: Simplify print_tool_help()","startedAt":"2012-07-23T03:42:16Z","lastAt":"2012-07-25T03:00:55Z","messageCount":15,"participants":["David Aguilar","Sebastian Schuberth","Tim Henigan","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"195473","messageId":"1343014940-16439-1-git-send-email-davvid@gmail.com","threadId":"31071","inReplyTo":null,"subject":"[PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T03:42:16Z","receivedAt":"2012-07-23T03:42:16Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Eliminate a global variable and File::Find usage by building upon\nbasename() and glob() instead.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 25 +++++--------------------\n 1 file changed, 5 insertions(+), 20 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex c079854..ac0ed63 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -13,17 +13,15 @@\n use 5.008;\n use strict;\n use warnings;\n-use File::Basename qw(dirname);\n+use File::Basename qw(basename dirname);\n use File::Copy;\n use File::Compare;\n-use File::Find;\n use File::stat;\n use File::Path qw(mkpath);\n use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n-my @tools;\n my @working_tree;\n my $rc;\n my $repo = Git->repository();\n@@ -65,26 +63,13 @@ sub find_worktree\n \n my $workdir = find_worktree();\n \n-sub filter_tool_scripts\n-{\n-\tif (-d $_) {\n-\t\tif ($_ ne \".\") {\n-\t\t\t# Ignore files in subdirectories\n-\t\t\t$File::Find::prune = 1;\n-\t\t}\n-\t} else {\n-\t\tif ((-f $_) && ($_ ne \"defaults\")) {\n-\t\t\tpush(@tools, $_);\n-\t\t}\n-\t}\n-}\n-\n sub print_tool_help\n {\n \tmy ($cmd, @found, @notfound);\n \tmy $gitpath = Git::exec_path();\n \n-\tfind(\\&filter_tool_scripts, \"$gitpath/mergetools\");\n+\tmy @files = map { basename($_) } glob(\"$gitpath/mergetools/*\");\n+\tmy @tools = sort(grep { !m{^defaults$} } @files);\n \n \tforeach my $tool (@tools) {\n \t\t$cmd  = \"TOOL_MODE=diff\";\n@@ -99,10 +84,10 @@ sub print_tool_help\n \t}\n \n \tprint \"'git difftool --tool=<tool>' may be set to one of the following:\\n\";\n-\tprint \"\\t$_\\n\" for (sort(@found));\n+\tprint \"\\t$_\\n\" for (@found);\n \n \tprint \"\\nThe following tools are valid, but not currently available:\\n\";\n-\tprint \"\\t$_\\n\" for (sort(@notfound));\n+\tprint \"\\t$_\\n\" for (@notfound);\n \n \tprint \"\\nNOTE: Some of the tools listed above only work in a windowed\\n\";\n \tprint \"environment. If run in a terminal-only session, they will fail.\\n\";\n-- \n1.7.11.2.255.g5f133da\n"},{"id":"195474","messageId":"1343014940-16439-2-git-send-email-davvid@gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/5] difftool: Eliminate global variables","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T03:42:17Z","receivedAt":"2012-07-23T03:42:17Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Organize the script so that it has a single main() function which\ncalls out to dir_diff() and file_diff() functions. This eliminates\n\"dir-diff\"-specific variables that do not need to be calculated when\nperforming a regular file-diff.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 128 ++++++++++++++++++++++++++++++++----------------------\n 1 file changed, 75 insertions(+), 53 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex ac0ed63..41ba932 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -22,11 +22,6 @@ use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n-my @working_tree;\n-my $rc;\n-my $repo = Git->repository();\n-my $repo_path = $repo->repo_path();\n-\n sub usage\n {\n \tmy $exitcode = shift;\n@@ -43,6 +38,8 @@ USAGE\n \n sub find_worktree\n {\n+\tmy ($repo) = @_;\n+\n \t# Git->repository->wc_path() does not honor changes to the working\n \t# tree location made by $ENV{GIT_WORK_TREE} or the 'core.worktree'\n \t# config variable.\n@@ -61,8 +58,6 @@ sub find_worktree\n \treturn $worktree;\n }\n \n-my $workdir = find_worktree();\n-\n sub print_tool_help\n {\n \tmy ($cmd, @found, @notfound);\n@@ -97,10 +92,13 @@ sub print_tool_help\n \n sub setup_dir_diff\n {\n+\tmy ($repo, $workdir) = @_;\n+\n \t# Run the diff; exit immediately if no diff found\n \t# 'Repository' and 'WorkingCopy' must be explicitly set to insure that\n \t# if $GIT_DIR and $GIT_WORK_TREE are set in ENV, they are actually used\n \t# by Git->repository->command*.\n+\tmy $repo_path = $repo->repo_path();\n \tmy $diffrepo = Git->repository(Repository => $repo_path, WorkingCopy => $workdir);\n \tmy $diffrtn = $diffrepo->command_oneline('diff', '--raw', '--no-abbrev', '-z', @ARGV);\n \texit(0) if (length($diffrtn) == 0);\n@@ -121,6 +119,7 @@ sub setup_dir_diff\n \tmy $rindex = '';\n \tmy %submodule;\n \tmy %symlink;\n+\tmy @working_tree = ();\n \tmy @rawdiff = split('\\0', $diffrtn);\n \n \tmy $i = 0;\n@@ -188,7 +187,7 @@ sub setup_dir_diff\n \t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n \tprint($inpipe $lindex);\n \t$repo->command_close_pipe($inpipe, $ctx);\n-\t$rc = system('git', 'checkout-index', '--all', \"--prefix=$ldir/\");\n+\tmy $rc = system('git', 'checkout-index', '--all', \"--prefix=$ldir/\");\n \texit($rc | ($rc >> 8)) if ($rc != 0);\n \n \t$ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n@@ -238,7 +237,7 @@ sub setup_dir_diff\n \t\t}\n \t}\n \n-\treturn ($ldir, $rdir);\n+\treturn ($ldir, $rdir, @working_tree);\n }\n \n sub write_to_file\n@@ -261,54 +260,70 @@ sub write_to_file\n \tclose($fh);\n }\n \n-# parse command-line options. all unrecognized options and arguments\n-# are passed through to the 'git diff' command.\n-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);\n-GetOptions('g|gui!' => \\$gui,\n-\t'd|dir-diff' => \\$dirdiff,\n-\t'h' => \\$help,\n-\t'prompt!' => \\$prompt,\n-\t'y' => sub { $prompt = 0; },\n-\t't|tool:s' => \\$difftool_cmd,\n-\t'tool-help' => \\$tool_help,\n-\t'x|extcmd:s' => \\$extcmd);\n-\n-if (defined($help)) {\n-\tusage(0);\n-}\n-if (defined($tool_help)) {\n-\tprint_tool_help();\n-}\n-if (defined($difftool_cmd)) {\n-\tif (length($difftool_cmd) > 0) {\n-\t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n-\t} else {\n-\t\tprint \"No <tool> given for --tool=<tool>\\n\";\n-\t\tusage(1);\n+sub main\n+{\n+\t# parse command-line options. all unrecognized options and arguments\n+\t# are passed through to the 'git diff' command.\n+\tmy ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);\n+\tGetOptions('g|gui!' => \\$gui,\n+\t\t'd|dir-diff' => \\$dirdiff,\n+\t\t'h' => \\$help,\n+\t\t'prompt!' => \\$prompt,\n+\t\t'y' => sub { $prompt = 0; },\n+\t\t't|tool:s' => \\$difftool_cmd,\n+\t\t'tool-help' => \\$tool_help,\n+\t\t'x|extcmd:s' => \\$extcmd);\n+\n+\tif (defined($help)) {\n+\t\tusage(0);\n \t}\n-}\n-if (defined($extcmd)) {\n-\tif (length($extcmd) > 0) {\n-\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n-\t} else {\n-\t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n-\t\tusage(1);\n+\tif (defined($tool_help)) {\n+\t\tprint_tool_help();\n \t}\n-}\n-if ($gui) {\n-\tmy $guitool = '';\n-\t$guitool = Git::config('diff.guitool');\n-\tif (length($guitool) > 0) {\n-\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n+\tif (defined($difftool_cmd)) {\n+\t\tif (length($difftool_cmd) > 0) {\n+\t\t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n+\t\t} else {\n+\t\t\tprint \"No <tool> given for --tool=<tool>\\n\";\n+\t\t\tusage(1);\n+\t\t}\n+\t}\n+\tif (defined($extcmd)) {\n+\t\tif (length($extcmd) > 0) {\n+\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n+\t\t} else {\n+\t\t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n+\t\t\tusage(1);\n+\t\t}\n+\t}\n+\tif ($gui) {\n+\t\tmy $guitool = '';\n+\t\t$guitool = Git::config('diff.guitool');\n+\t\tif (length($guitool) > 0) {\n+\t\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n+\t\t}\n+\t}\n+\n+\t# In directory diff mode, 'git-difftool--helper' is called once\n+\t# to compare the a/b directories.  In file diff mode, 'git diff'\n+\t# will invoke a separate instance of 'git-difftool--helper' for\n+\t# each file that changed.\n+\tif (defined($dirdiff)) {\n+\t\tdir_diff($extcmd);\n+\t} else {\n+\t\tfile_diff($prompt);\n \t}\n }\n \n-# In directory diff mode, 'git-difftool--helper' is called once\n-# to compare the a/b directories.  In file diff mode, 'git diff'\n-# will invoke a separate instance of 'git-difftool--helper' for\n-# each file that changed.\n-if (defined($dirdiff)) {\n-\tmy ($a, $b) = setup_dir_diff();\n+sub dir_diff\n+{\n+\tmy ($extcmd) = @_;\n+\n+\tmy $rc;\n+\tmy $repo = Git->repository();\n+\n+\tmy $workdir = find_worktree($repo);\n+\tmy ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir);\n \tif (defined($extcmd)) {\n \t\t$rc = system($extcmd, $a, $b);\n \t} else {\n@@ -327,7 +342,12 @@ if (defined($dirdiff)) {\n \t\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n \t\t}\n \t}\n-} else {\n+}\n+\n+sub file_diff\n+{\n+\tmy ($prompt) = @_;\n+\n \tif (defined($prompt)) {\n \t\tif ($prompt) {\n \t\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n@@ -347,3 +367,5 @@ if (defined($dirdiff)) {\n \tmy $rc = system('git', 'diff', @ARGV);\n \texit($rc | ($rc >> 8));\n }\n+\n+main();\n-- \n1.7.11.2.255.g5f133da\n"},{"id":"195475","messageId":"1343014940-16439-3-git-send-email-davvid@gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/5] difftool: Move option values into a hash","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T03:42:18Z","receivedAt":"2012-07-23T03:42:18Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Shorten the \"my\" declaration for all of the option-specific variables\nby wrapping all of them in a hash.  This makes also gives us a place\nto specify default values, should we need them.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool.perl | 55 +++++++++++++++++++++++++++++++------------------------\n 1 file changed, 31 insertions(+), 24 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 41ba932..0ce6168 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -264,41 +264,48 @@ sub main\n {\n \t# parse command-line options. all unrecognized options and arguments\n \t# are passed through to the 'git diff' command.\n-\tmy ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);\n-\tGetOptions('g|gui!' => \\$gui,\n-\t\t'd|dir-diff' => \\$dirdiff,\n-\t\t'h' => \\$help,\n-\t\t'prompt!' => \\$prompt,\n-\t\t'y' => sub { $prompt = 0; },\n-\t\t't|tool:s' => \\$difftool_cmd,\n-\t\t'tool-help' => \\$tool_help,\n-\t\t'x|extcmd:s' => \\$extcmd);\n-\n-\tif (defined($help)) {\n+\tmy %opts = (\n+\t\tdifftool_cmd => undef,\n+\t\tdirdiff => undef,\n+\t\textcmd => undef,\n+\t\tgui => undef,\n+\t\thelp => undef,\n+\t\tprompt => undef,\n+\t\ttool_help => undef,\n+\t);\n+\tGetOptions('g|gui!' => \\$opts{gui},\n+\t\t'd|dir-diff' => \\$opts{dirdiff},\n+\t\t'h' => \\$opts{help},\n+\t\t'prompt!' => \\$opts{prompt},\n+\t\t'y' => sub { $opts{prompt} = 0; },\n+\t\t't|tool:s' => \\$opts{difftool_cmd},\n+\t\t'tool-help' => \\$opts{tool_help},\n+\t\t'x|extcmd:s' => \\$opts{extcmd});\n+\n+\tif (defined($opts{help})) {\n \t\tusage(0);\n \t}\n-\tif (defined($tool_help)) {\n+\tif (defined($opts{tool_help})) {\n \t\tprint_tool_help();\n \t}\n-\tif (defined($difftool_cmd)) {\n-\t\tif (length($difftool_cmd) > 0) {\n-\t\t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n+\tif (defined($opts{difftool_cmd})) {\n+\t\tif (length($opts{difftool_cmd}) > 0) {\n+\t\t\t$ENV{GIT_DIFF_TOOL} = $opts{difftool_cmd};\n \t\t} else {\n \t\t\tprint \"No <tool> given for --tool=<tool>\\n\";\n \t\t\tusage(1);\n \t\t}\n \t}\n-\tif (defined($extcmd)) {\n-\t\tif (length($extcmd) > 0) {\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $extcmd;\n+\tif (defined($opts{extcmd})) {\n+\t\tif (length($opts{extcmd}) > 0) {\n+\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $opts{extcmd};\n \t\t} else {\n \t\t\tprint \"No <cmd> given for --extcmd=<cmd>\\n\";\n \t\t\tusage(1);\n \t\t}\n \t}\n-\tif ($gui) {\n-\t\tmy $guitool = '';\n-\t\t$guitool = Git::config('diff.guitool');\n+\tif ($opts{gui}) {\n+\t\tmy $guitool = Git::config('diff.guitool');\n \t\tif (length($guitool) > 0) {\n \t\t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t\t}\n@@ -308,10 +315,10 @@ sub main\n \t# to compare the a/b directories.  In file diff mode, 'git diff'\n \t# will invoke a separate instance of 'git-difftool--helper' for\n \t# each file that changed.\n-\tif (defined($dirdiff)) {\n-\t\tdir_diff($extcmd);\n+\tif (defined($opts{dirdiff})) {\n+\t\tdir_diff($opts{extcmd});\n \t} else {\n-\t\tfile_diff($prompt);\n+\t\tfile_diff($opts{prompt});\n \t}\n }\n \n-- \n1.7.11.2.255.g5f133da\n"},{"id":"195476","messageId":"1343014940-16439-5-git-send-email-davvid@gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-1-git-send-email-davvid@gmail.com","subject":"[PATCH 5/5] difftool: Use symlinks when diffing against the worktree","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T03:42:20Z","receivedAt":"2012-07-23T03:42:20Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Teach difftool's --dir-diff mode to use symlinks to represent\nfiles from the working copy, and make it the default behavior\nfor the non-Windows platforms.\n\nUsing symlinks is simpler and safer since we do not need to\nworry about copying files back into the worktree.\nThe old behavior is still available as --no-symlinks.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n Documentation/git-difftool.txt |  8 ++++++++\n git-difftool.perl              | 30 +++++++++++++++++++++++-------\n 2 files changed, 31 insertions(+), 7 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex 31fc2e3..313d54e 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -66,6 +66,14 @@ of the diff post-image.  `$MERGED` is the name of the file which is\n being compared. `$BASE` is provided for compatibility\n with custom merge tool commands and has the same value as `$MERGED`.\n \n+--symlinks::\n+--no-symlinks::\n+\t'git difftool''s default behavior is create symlinks to the\n+\tworking tree when run in `--dir-diff` mode.\n++\n+\tSpecifying `--no-symlinks` instructs 'git difftool' to create\n+\tcopies instead.  `--no-symlinks` is the default on Windows.\n+\n --tool-help::\n \tPrint a list of diff tools that may be used with `--tool`.\n \ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 2ae344c..b8f8057 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -92,7 +92,7 @@ sub print_tool_help\n \n sub setup_dir_diff\n {\n-\tmy ($repo, $workdir) = @_;\n+\tmy ($repo, $workdir, $symlinks) = @_;\n \n \t# Run the diff; exit immediately if no diff found\n \t# 'Repository' and 'WorkingCopy' must be explicitly set to insure that\n@@ -209,8 +209,13 @@ sub setup_dir_diff\n \t\tunless (-d \"$rdir/$dir\") {\n \t\t\tmkpath(\"$rdir/$dir\") or die $!;\n \t\t}\n-\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or die $!;\n-\t\tchmod(stat(\"$workdir/$file\")->mode, \"$rdir/$file\") or die $!;\n+\t\tif ($symlinks) {\n+\t\t\tsymlink(\"$workdir/$file\", \"$rdir/$file\") or die $!;\n+\t\t} else {\n+\t\t\tcopy(\"$workdir/$file\", \"$rdir/$file\") or die $!;\n+\t\t\tmy $mode = stat(\"$workdir/$file\")->mode;\n+\t\t\tchmod($mode, \"$rdir/$file\") or die $!;\n+\t\t}\n \t}\n \n \t# Changes to submodules require special treatment. This loop writes a\n@@ -271,6 +276,7 @@ sub main\n \t\tgui => undef,\n \t\thelp => undef,\n \t\tprompt => undef,\n+\t\tsymlinks => $^O ne 'MSWin32' && $^O ne 'msys',\n \t\ttool_help => undef,\n \t);\n \tGetOptions('g|gui!' => \\$opts{gui},\n@@ -278,6 +284,8 @@ sub main\n \t\t'h' => \\$opts{help},\n \t\t'prompt!' => \\$opts{prompt},\n \t\t'y' => sub { $opts{prompt} = 0; },\n+\t\t'symlinks' => \\$opts{symlinks},\n+\t\t'no-symlinks' => sub { $opts{symlinks} = 0; },\n \t\t't|tool:s' => \\$opts{difftool_cmd},\n \t\t'tool-help' => \\$opts{tool_help},\n \t\t'x|extcmd:s' => \\$opts{extcmd});\n@@ -316,7 +324,7 @@ sub main\n \t# will invoke a separate instance of 'git-difftool--helper' for\n \t# each file that changed.\n \tif (defined($opts{dirdiff})) {\n-\t\tdir_diff($opts{extcmd});\n+\t\tdir_diff($opts{extcmd}, $opts{symlinks});\n \t} else {\n \t\tfile_diff($opts{prompt});\n \t}\n@@ -324,13 +332,13 @@ sub main\n \n sub dir_diff\n {\n-\tmy ($extcmd) = @_;\n+\tmy ($extcmd, $symlinks) = @_;\n \n \tmy $rc;\n \tmy $repo = Git->repository();\n \n \tmy $workdir = find_worktree($repo);\n-\tmy ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir);\n+\tmy ($a, $b, @working_tree) = setup_dir_diff($repo, $workdir, $symlinks);\n \tif (defined($extcmd)) {\n \t\t$rc = system($extcmd, $a, $b);\n \t} else {\n@@ -340,15 +348,23 @@ sub dir_diff\n \n \texit($rc | ($rc >> 8)) if ($rc != 0);\n \n+\t# Do not copy back files when symlinks are used\n+\tif ($symlinks) {\n+\t\texit(0);\n+\t}\n+\n \t# If the diff including working copy files and those\n \t# files were modified during the diff, then the changes\n \t# should be copied back to the working tree\n+\n \tfor my $file (@working_tree) {\n \t\tif (-e \"$b/$file\" && compare(\"$b/$file\", \"$workdir/$file\")) {\n \t\t\tcopy(\"$b/$file\", \"$workdir/$file\") or die $!;\n-\t\t\tchmod(stat(\"$b/$file\")->mode, \"$workdir/$file\") or die $!;\n+\t\t\tmy $mode = stat(\"$b/$file\")->mode;\n+\t\t\tchmod($mode, \"$workdir/$file\") or die $!;\n \t\t}\n \t}\n+\texit(0);\n }\n \n sub file_diff\n-- \n1.7.11.2.255.g5f133da\n"},{"id":"195477","messageId":"CAJDDKr7NA7rQ8xM-uZbboDmhwi2D4nZA2V2Qiij6h569Es9exA@mail.gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-3-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/5] difftool: Move option values into a hash","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T03:49:21Z","receivedAt":"2012-07-23T03:49:21Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jul 22, 2012 at 8:42 PM, David Aguilar <davvid@gmail.com> wrote:\n> ... This makes also gives us a place to specify default values\n\nOops, please drop the word \"makes\" from the commit message here if you\napply this, or I'll fix it in a re-roll if review finds other issues.\n\nThanks,\n-- \nDavid\n"},{"id":"195498","messageId":"500CEB9F.4030406@gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 2/5] difftool: Eliminate global variables","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T06:13:51Z","receivedAt":"2012-07-23T06:13:51Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On 23.07.2012 05:42, David Aguilar wrote:\n\n> Organize the script so that it has a single main() function which\n> calls out to dir_diff() and file_diff() functions. This eliminates\n> \"dir-diff\"-specific variables that do not need to be calculated when\n> performing a regular file-diff.\n\nFunny, I just have prepared a patch along the same lines so that one can \ncall git-difftool -h and --tool-help also outside a repository, see \nbelow. Does you patch offer the same? If so, I'll drop mine.\n\n---\n  git-difftool.perl | 12 ++++++++----\n  1 file changed, 8 insertions(+), 4 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex ae1e052..e7b71c9 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -25,8 +25,9 @@ use Git;\n  my @tools;\n  my @working_tree;\n  my $rc;\n-my $repo = Git->repository();\n-my $repo_path = $repo->repo_path();\n+my $repo;\n+my $repo_path;\n+my $workdir;\n\n  sub usage\n  {\n@@ -62,8 +63,6 @@ sub find_worktree\n  \treturn $worktree;\n  }\n\n-my $workdir = find_worktree();\n-\n  sub filter_tool_scripts\n  {\n  \tif (-d $_) {\n@@ -293,6 +292,11 @@ if (defined($help)) {\n  if (defined($tool_help)) {\n  \tprint_tool_help();\n  }\n+\n+$repo = Git->repository();\n+$repo_path = $repo->repo_path();\n+$workdir = find_worktree();\n+\n  if (defined($difftool_cmd)) {\n  \tif (length($difftool_cmd) > 0) {\n  \t\t$ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n-- \n1.7.11.msysgit.2\n"},{"id":"195499","messageId":"CAJDDKr7oDc2CxoiWqUMri93FM3WHecanCYBkezLYY1A_BZXdxw@mail.gmail.com","threadId":"31071","inReplyTo":"500CEB9F.4030406@gmail.com","subject":"Re: [PATCH 2/5] difftool: Eliminate global variables","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T06:21:39Z","receivedAt":"2012-07-23T06:21:39Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jul 22, 2012 at 11:13 PM, Sebastian Schuberth\n<sschuberth@gmail.com> wrote:\n> On 23.07.2012 05:42, David Aguilar wrote:\n>\n>> Organize the script so that it has a single main() function which\n>> calls out to dir_diff() and file_diff() functions. This eliminates\n>> \"dir-diff\"-specific variables that do not need to be calculated when\n>> performing a regular file-diff.\n>\n>\n> Funny, I just have prepared a patch along the same lines so that one can\n> call git-difftool -h and --tool-help also outside a repository, see below.\n> Does you patch offer the same? If so, I'll drop mine.\n\nIt *should*. I did not consider this case but that is indeed the\ndesired behavior.\n\nWhat my patch did not offer was a test to ensure this behavior...\n\n\n>\n> ---\n>  git-difftool.perl | 12 ++++++++----\n>  1 file changed, 8 insertions(+), 4 deletions(-)\n>\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index ae1e052..e7b71c9 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -25,8 +25,9 @@ use Git;\n>  my @tools;\n>  my @working_tree;\n>\n>  my $rc;\n> -my $repo = Git->repository();\n> -my $repo_path = $repo->repo_path();\n> +my $repo;\n> +my $repo_path;\n> +my $workdir;\n>\n>  sub usage\n>  {\n> @@ -62,8 +63,6 @@ sub find_worktree\n>\n>         return $worktree;\n>  }\n>\n> -my $workdir = find_worktree();\n> -\n>  sub filter_tool_scripts\n>  {\n>         if (-d $_) {\n> @@ -293,6 +292,11 @@ if (defined($help)) {\n>  if (defined($tool_help)) {\n>         print_tool_help();\n>  }\n> +\n> +$repo = Git->repository();\n> +$repo_path = $repo->repo_path();\n> +$workdir = find_worktree();\n> +\n>  if (defined($difftool_cmd)) {\n>         if (length($difftool_cmd) > 0) {\n>                 $ENV{GIT_DIFF_TOOL} = $difftool_cmd;\n> --\n> 1.7.11.msysgit.2\n\n\n\n-- \nDavid\n"},{"id":"195502","messageId":"CAHGBnuM9LgeLiOoFfny8NbF547okjQBLDXfzrbTj6vt+d=M4Eg@mail.gmail.com","threadId":"31071","inReplyTo":"CAJDDKr7oDc2CxoiWqUMri93FM3WHecanCYBkezLYY1A_BZXdxw@mail.gmail.com","subject":"Re: [PATCH 2/5] difftool: Eliminate global variables","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T06:30:26Z","receivedAt":"2012-07-23T06:30:26Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 8:21 AM, David Aguilar <davvid@gmail.com> wrote:\n\n>> Funny, I just have prepared a patch along the same lines so that one can\n>> call git-difftool -h and --tool-help also outside a repository, see below.\n>> Does you patch offer the same? If so, I'll drop mine.\n>\n> It *should*. I did not consider this case but that is indeed the\n> desired behavior.\n>\n> What my patch did not offer was a test to ensure this behavior...\n\nWell, I'm not asking for a test. From my side, I'd be happy if you\ncould just try it and confirm that it works, as I currently cannot\neasily apply your patch.\n\n-- \nSebastian Schuberth\n"},{"id":"195503","messageId":"CAJDDKr77d0vot30txya7Z0QpOj2xgV4-0ZhOuyXiJbG2OSUNQQ@mail.gmail.com","threadId":"31071","inReplyTo":"CAHGBnuM9LgeLiOoFfny8NbF547okjQBLDXfzrbTj6vt+d=M4Eg@mail.gmail.com","subject":"Re: [PATCH 2/5] difftool: Eliminate global variables","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-23T06:44:07Z","receivedAt":"2012-07-23T06:44:07Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Jul 22, 2012 at 11:30 PM, Sebastian Schuberth\n<sschuberth@gmail.com> wrote:\n> On Mon, Jul 23, 2012 at 8:21 AM, David Aguilar <davvid@gmail.com> wrote:\n>\n>>> Funny, I just have prepared a patch along the same lines so that one can\n>>> call git-difftool -h and --tool-help also outside a repository, see below.\n>>> Does you patch offer the same? If so, I'll drop mine.\n>>\n>> It *should*. I did not consider this case but that is indeed the\n>> desired behavior.\n>>\n>> What my patch did not offer was a test to ensure this behavior...\n>\n> Well, I'm not asking for a test. From my side, I'd be happy if you\n> could just try it and confirm that it works, as I currently cannot\n> easily apply your patch.\n\nHeheh.. I was hoping you could provide a test ;-)\n\nI just tried it now.  They work outside of a repository.\n-- \nDavid\n"},{"id":"195504","messageId":"CAHGBnuMvcW-yZQ7viUfZg+aomk=KyCm3kEdUD7TizYdLRYQUXA@mail.gmail.com","threadId":"31071","inReplyTo":"CAJDDKr77d0vot30txya7Z0QpOj2xgV4-0ZhOuyXiJbG2OSUNQQ@mail.gmail.com","subject":"Re: [PATCH 2/5] difftool: Eliminate global variables","fromName":"Sebastian Schuberth","fromEmail":"sschuberth@gmail.com","sentAt":"2012-07-23T06:54:44Z","receivedAt":"2012-07-23T06:54:44Z","isPatch":true,"sender":{"key":"sschuberth@gmail.com","avatar":"https://avatars.githubusercontent.com/u/349154?v=4"},"body":"On Mon, Jul 23, 2012 at 8:44 AM, David Aguilar <davvid@gmail.com> wrote:\n\n>> Well, I'm not asking for a test. From my side, I'd be happy if you\n>> could just try it and confirm that it works, as I currently cannot\n>> easily apply your patch.\n>\n> Heheh.. I was hoping you could provide a test ;-)\n\n;-)\n\n> I just tried it now.  They work outside of a repository.\n\nGreat, thanks! I'm dropping my patch then.\n\n-- \nSebastian Schuberth\n"},{"id":"195627","messageId":"CAFouetjhGChULHa-Ax9oR--TccyEBnqn=UATbMpJxcdm3AOE1w@mail.gmail.com","threadId":"31071","inReplyTo":"1343014940-16439-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-07-24T12:43:25Z","receivedAt":"2012-07-24T12:43:25Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:\n> Eliminate a global variable and File::Find usage by building upon\n> basename() and glob() instead.\n\nglob was used in an early revision of the patch that led to bf73fc2\n(difftool: print list of valid tools with '--tool-help') as well [1].\nHowever, if the path to git or the path under 'mergetools' includes\nspaces, glob fails.  To work around the problem, File::Find was used\ninstead [2].\n\nDoes this implementation handle that case?  I'm sorry, but I haven't\nhad time to apply and test myself.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/194158\n"},{"id":"195680","messageId":"CAJDDKr47F9ZAPPCnFBdPYhQsx8qGGggHHrFWeZw2zpf-mukq5w@mail.gmail.com","threadId":"31071","inReplyTo":"CAFouetjhGChULHa-Ax9oR--TccyEBnqn=UATbMpJxcdm3AOE1w@mail.gmail.com","subject":"Re: [PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T01:52:00Z","receivedAt":"2012-07-25T01:52:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jul 24, 2012 at 5:43 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:\n>> Eliminate a global variable and File::Find usage by building upon\n>> basename() and glob() instead.\n>\n> glob was used in an early revision of the patch that led to bf73fc2\n> (difftool: print list of valid tools with '--tool-help') as well [1].\n> However, if the path to git or the path under 'mergetools' includes\n> spaces, glob fails.  To work around the problem, File::Find was used\n> instead [2].\n>\n> Does this implementation handle that case?  I'm sorry, but I haven't\n> had time to apply and test myself.\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925\n> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158\n\nGood catch.  Eliminating the globals should be the goal, then.\nI'll have to re-roll.\n-- \nDavid\n"},{"id":"195681","messageId":"CAJDDKr6JcbKkvg-9M6Sf4i0+YrMFJeKhmVoxQ_g1QBCRnoGRxw@mail.gmail.com","threadId":"31071","inReplyTo":"CAJDDKr47F9ZAPPCnFBdPYhQsx8qGGggHHrFWeZw2zpf-mukq5w@mail.gmail.com","subject":"Re: [PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T02:18:00Z","receivedAt":"2012-07-25T02:18:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jul 24, 2012 at 6:52 PM, David Aguilar <davvid@gmail.com> wrote:\n> On Tue, Jul 24, 2012 at 5:43 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>> On Sun, Jul 22, 2012 at 11:42 PM, David Aguilar <davvid@gmail.com> wrote:\n>>> Eliminate a global variable and File::Find usage by building upon\n>>> basename() and glob() instead.\n>>\n>> glob was used in an early revision of the patch that led to bf73fc2\n>> (difftool: print list of valid tools with '--tool-help') as well [1].\n>> However, if the path to git or the path under 'mergetools' includes\n>> spaces, glob fails.  To work around the problem, File::Find was used\n>> instead [2].\n>>\n>> Does this implementation handle that case?  I'm sorry, but I haven't\n>> had time to apply and test myself.\n>>\n>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925\n>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158\n>\n> Good catch.  Eliminating the globals should be the goal, then.\n> I'll have to re-roll.\n\nThese landed in next.\n\nJunio, would you prefer follow-up patches or a rebased series?\n-- \nDavid\n"},{"id":"195682","messageId":"7v629cef6h.fsf@alter.siamese.dyndns.org","threadId":"31071","inReplyTo":"CAJDDKr6JcbKkvg-9M6Sf4i0+YrMFJeKhmVoxQ_g1QBCRnoGRxw@mail.gmail.com","subject":"Re: [PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-07-25T02:40:38Z","receivedAt":"2012-07-25T02:40:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n>>> Does this implementation handle that case?  I'm sorry, but I haven't\n>>> had time to apply and test myself.\n>>>\n>>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925\n>>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158\n>>\n>> Good catch.  Eliminating the globals should be the goal, then.\n>> I'll have to re-roll.\n>\n> Junio, would you prefer follow-up patches or a rebased series?\n\nIncremental patches, please.\n\nThanks.\n"},{"id":"195690","messageId":"CAJDDKr6=t5bstW9fGxJd7imNkCt47=159_AnNtDPE9U_6f9Jug@mail.gmail.com","threadId":"31071","inReplyTo":"7v629cef6h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 1/5] difftool: Simplify print_tool_help()","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-07-25T03:00:55Z","receivedAt":"2012-07-25T03:00:55Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Jul 24, 2012 at 7:40 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> David Aguilar <davvid@gmail.com> writes:\n>\n>>>> Does this implementation handle that case?  I'm sorry, but I haven't\n>>>> had time to apply and test myself.\n>>>>\n>>>> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193233/focus=193925\n>>>> [2]: http://thread.gmane.org/gmane.comp.version-control.git/194158\n>>>\n>>> Good catch.  Eliminating the globals should be the goal, then.\n>>> I'll have to re-roll.\n>>\n>> Junio, would you prefer follow-up patches or a rebased series?\n>\n> Incremental patches, please.\n>\n> Thanks.\n\nOkay, I saw this just before hitting send-email (and sent the whole series).\n\nI'll prep the incremental ones shortly, so please ignore the one I just sent...\n-- \nDavid\n"}]}