{"thread":{"id":"30147","subject":"[PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","startedAt":"2012-04-04T19:21:05Z","lastAt":"2012-04-12T03:21:00Z","messageCount":9,"participants":["Tim Henigan","David Aguilar","Junio C Hamano"],"isPatch":true,"patchVersion":11,"patchTotal":9},"messages":[{"id":"188510","messageId":"1333567265-23986-1-git-send-email-tim.henigan@gmail.com","threadId":"30147","inReplyTo":null,"subject":"[PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-04T19:21:05Z","receivedAt":"2012-04-04T19:21:05Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When 'difftool' is called to compare a range of commits that modify\nmore than one file, it opens a separate instance of the diff tool for\neach file that changed.\n\nThe new '--dir-diff' option copies all the modified files to a temporary\nlocation and runs a directory diff on them in a single instance of the\ndiff tool.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis replaces commit 4d6efe0 in the 'th/difftool-diffall' branch based\non pu. This version corrects some problems with submodule support in\ndifftool.\n\nChanges in v11:\n  - Fix handling of submodules in subdirectories.\n    \n    If the diff includes a submodule that lives in a subdirectory of\n    the project, the script fails with \"No such file or directory\".\n    For example, if a submodule is located at '<repo root>/lib/sub',\n    the script fails when it tries to write the submodule SHA1 to\n    '/tmp/.../lib/sub' because the 'lib' directory does not exist.\n    This commit teaches difftool to create the subdirectory path in\n    the tmp location prior to writing the submodule file.\n\n  - Append \"dirty\" to submodule SHA1 when needed\n    \n    If a submodule is dirty (i.e. has uncommitted changes) then\n      'git diff' reports \"<SHA1 of parent>-dirty\"\n    while\n      'git diff --raw' reports \"<SHA1 of parent>\"\n    difftool uses the '--raw' diff format, so the \"dirty\" flag is lost.\n    To make the output more closely match that of 'git diff', teach\n    difftool to append \"-dirty\" to the submodule SHA1 when needed.\n\n\n Documentation/git-difftool.txt |    6 ++\n git-difftool--helper.sh        |   19 +++--\n git-difftool.perl              |  170 +++++++++++++++++++++++++++++++++++-----\n t/t7800-difftool.sh            |   39 +++++++++\n 4 files changed, 209 insertions(+), 25 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex fe38f66..aba5e76 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -19,6 +19,12 @@ linkgit:git-diff[1].\n \n OPTIONS\n -------\n+-d::\n+--dir-diff::\n+\tCopy the modified files to a temporary location and perform\n+\ta directory diff on them. This mode never prompts before\n+\tlaunching the diff tool.\n+\n -y::\n --no-prompt::\n \tDo not prompt before launching a diff tool.\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex e6558d1..3d0fe0c 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -73,9 +73,16 @@ then\n \tfi\n fi\n \n-# Launch the merge tool on each path provided by 'git diff'\n-while test $# -gt 6\n-do\n-\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n-\tshift 7\n-done\n+if test -n \"$GIT_DIFFTOOL_DIRDIFF\"\n+then\n+\tLOCAL=\"$1\"\n+\tREMOTE=\"$2\"\n+\trun_merge_tool \"$merge_tool\" false\n+else\n+\t# Launch the merge tool on each path provided by 'git diff'\n+\twhile test $# -gt 6\n+\tdo\n+\t\tlaunch_merge_tool \"$1\" \"$2\" \"$5\"\n+\t\tshift 7\n+\tdone\n+fi\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex d4fe998..5bb01e1 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -1,21 +1,29 @@\n #!/usr/bin/env perl\n # Copyright (c) 2009, 2010 David Aguilar\n+# Copyright (c) 2012 Tim Henigan\n #\n # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible\n # git-difftool--helper script.\n #\n # This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.\n-# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL\n-# are exported for use by git-difftool--helper.\n+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, GIT_DIFFTOOL_DIRDIFF,\n+# and GIT_DIFF_TOOL are exported for use by git-difftool--helper.\n #\n # Any arguments that are unknown to this script are forwarded to 'git diff'.\n \n use 5.008;\n use strict;\n use warnings;\n+use File::Basename qw(dirname);\n+use File::Copy;\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 @working_tree;\n+\n sub usage\n {\n \tmy $exitcode = shift;\n@@ -24,15 +32,121 @@ usage: git difftool [-t|--tool=<tool>]\n                     [-x|--extcmd=<cmd>]\n                     [-g|--gui] [--no-gui]\n                     [--prompt] [-y|--no-prompt]\n+                    [-d|--dir-diff]\n                     ['git diff' options]\n USAGE\n \texit($exitcode);\n }\n \n+sub setup_dir_diff\n+{\n+\t# Run the diff; exit immediately if no diff found\n+\tmy $repo = Git->repository();\n+\tmy $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);\n+\texit(0) if (length($diffrtn) == 0);\n+\n+\t# Setup temp directories\n+\tmy $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);\n+\tmy $ldir = \"$tmpdir/left\";\n+\tmy $rdir = \"$tmpdir/right\";\n+\tmkpath($ldir) or die $!;\n+\tmkpath($rdir) or die $!;\n+\n+\t# Build index info for left and right sides of the diff\n+\tmy $submodule_mode = \"160000\";\n+\tmy $null_mode = 0 x 6;\n+\tmy $null_sha1 = 0 x 40;\n+\tmy $lindex = \"\";\n+\tmy $rindex = \"\";\n+\tmy %submodule;\n+\tmy @rawdiff = split('\\0', $diffrtn);\n+\n+\tfor (my $i=0; $i<@rawdiff; $i+=2) {\n+\t\tmy ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));\n+\t\tmy $path = $rawdiff[$i + 1];\n+\n+\t\tif (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {\n+\t\t\t$submodule{$path}{left} = $lsha1;\n+\t\t\tif ($lsha1 ne $rsha1) {\n+\t\t\t\t$submodule{$path}{right} = $rsha1;\n+\t\t\t} else {\n+\t\t\t\t$submodule{$path}{right} = \"$rsha1-dirty\";\n+\t\t\t}\n+\t\t\tnext;\n+\t\t}\n+\n+\t\tif ($lmode ne $null_mode) {\n+\t\t\t$lindex .= \"$lmode $lsha1\\t$path\\0\";\n+\t\t}\n+\n+\t\tif ($rmode ne $null_mode) {\n+\t\t\tif ($rsha1 ne $null_sha1) {\n+\t\t\t\t$rindex .= \"$rmode $rsha1\\t$path\\0\";\n+\t\t\t} else {\n+\t\t\t\tpush(@working_tree, $path);\n+\t\t\t}\n+\t\t}\n+\t}\n+\n+\t# Populate the left and right directories based on each index file\n+\tmy ($inpipe, $ctx);\n+\t$ENV{GIT_DIR} = $repo->repo_path();\n+\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\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+\tsystem(('git', 'checkout-index', '--all', \"--prefix=$ldir/\"));\n+\n+\t$ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n+\t($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n+\tprint($inpipe $rindex);\n+\t$repo->command_close_pipe($inpipe, $ctx);\n+\tsystem(('git', 'checkout-index', '--all', \"--prefix=$rdir/\"));\n+\n+\t# Changes in the working tree need special treatment since they are\n+\t# not part of the index\n+\tmy $workdir = $repo->repo_path() . \"/..\";\n+\tfor (@working_tree) {\n+\t\tmy $dir = dirname($_);\n+\t\tunless (-d \"$rdir/$dir\") {\n+\t\t\tmkpath(\"$rdir/$dir\") or die $!;\n+\t\t}\n+\t\tcopy(\"$workdir/$_\", \"$rdir/$_\") or die $!;\n+\t\tchmod(stat(\"$workdir/$_\")->mode, \"$rdir/$_\") or die $!;\n+\t}\n+\n+\t# Changes to submodules require special treatment. This loop writes a\n+\t# temporary file to both the left and right directories to show the\n+\t# change in the recorded SHA1 for the submodule.\n+\tforeach my $path (keys %submodule) {\n+\t\tif (defined $submodule{$path}{left}) {\n+\t\t\tmy $dir = dirname($path);\n+\t\t\tunless (-d \"$ldir/$dir\") {\n+\t\t\t\tmkpath(\"$ldir/$dir\") or die $!;\n+\t\t\t}\n+\t\t\topen(my $fh, \">\", \"$ldir/$path\") or die $!;\n+\t\t\tprint($fh \"Subproject commit $submodule{$path}{left}\");\n+\t\t\tclose($fh);\n+\t\t}\n+\t\tif (defined $submodule{$path}{right}) {\n+\t\t\tmy $dir = dirname($path);\n+\t\t\tunless (-d \"$rdir/$dir\") {\n+\t\t\t\tmkpath(\"$rdir/$dir\") or die $!;\n+\t\t\t}\n+\t\t\topen(my $fh, \">\", \"$rdir/$path\") or die $!;\n+\t\t\tprint($fh \"Subproject commit $submodule{$path}{right}\");\n+\t\t\tclose($fh);\n+\t\t}\n+\t}\n+\n+\treturn ($ldir, $rdir);\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, $extcmd, $gui, $help, $prompt);\n+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt);\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@@ -65,22 +179,40 @@ if ($gui) {\n \t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t}\n }\n-if (defined($prompt)) {\n-\tif ($prompt) {\n-\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\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+\tif (defined($extcmd)) {\n+\t\tsystem(($extcmd, $a, $b));\n \t} else {\n-\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+\t\t$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n+\t\tsystem(('git', 'difftool--helper', $a, $b));\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+\tmy $repo = Git->repository();\n+\tmy $workdir = $repo->repo_path() . \"/..\";\n+\tfor (@working_tree) {\n+\t\tcopy(\"$b/$_\", \"$workdir/$_\") or die $!;\n+\t\tchmod(stat(\"$b/$_\")->mode, \"$workdir/$_\") or die $!;\n+\t}\n+} else {\n+\tif (defined($prompt)) {\n+\t\tif ($prompt) {\n+\t\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n+\t\t} else {\n+\t\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+\t\t}\n \t}\n-}\n \n-$ENV{GIT_PAGER} = '';\n-$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-my @command = ('git', 'diff', @ARGV);\n-\n-# ActiveState Perl for Win32 does not implement POSIX semantics of\n-# exec* system call. It just spawns the given executable and finishes\n-# the starting program, exiting with code 0.\n-# system will at least catch the errors returned by git diff,\n-# allowing the caller of git difftool better handling of failures.\n-my $rc = system(@command);\n-exit($rc | ($rc >> 8));\n+\t$ENV{GIT_PAGER} = '';\n+\t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n+\tmy $rc = system(('git', 'diff', @ARGV));\n+\texit($rc | ($rc >> 8));\n+}\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex e716d06..478c1be 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -319,4 +319,43 @@ test_expect_success PERL 'say no to the second file' '\n \techo \"$diff\" | stdin_doesnot_contain br2\n '\n \n+test_expect_success PERL 'setup change in subdirectory' '\n+\tgit checkout master &&\n+\tmkdir sub &&\n+\techo master >sub/sub &&\n+\tgit add sub/sub &&\n+\tgit commit -m \"added sub/sub\" &&\n+\techo test >>file &&\n+\techo test >>sub/sub &&\n+\tgit add . &&\n+\tgit commit -m \"modified both\"\n+'\n+\n+test_expect_success PERL 'difftool -d' '\n+\tdiff=$(git difftool -d --extcmd ls branch) &&\n+\techo \"$diff\" | stdin_contains sub &&\n+\techo \"$diff\" | stdin_contains file\n+'\n+\n+test_expect_success PERL 'difftool --dir-diff' '\n+\tdiff=$(git difftool --dir-diff --extcmd ls branch) &&\n+\techo \"$diff\" | stdin_contains sub &&\n+\techo \"$diff\" | stdin_contains file\n+'\n+\n+test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n+\tdiff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&\n+\techo \"$diff\" | stdin_contains sub &&\n+\techo \"$diff\" | stdin_contains file\n+'\n+\n+test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n+\t(\n+\t\tcd sub &&\n+\t\tdiff=$(git difftool --dir-diff --extcmd ls branch) &&\n+\t\techo \"$diff\" | stdin_contains sub &&\n+\t\techo \"$diff\" | stdin_contains file\n+\t)\n+'\n+\n test_done\n-- \n1.7.10.rc2.22.ga52d4\n"},{"id":"188512","messageId":"CAFouetjn9hW6YruCvsBQR+a4dcsCfV8nQ8Wma=3reSu__84_UQ@mail.gmail.com","threadId":"30147","inReplyTo":"1333567265-23986-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-04T19:31:25Z","receivedAt":"2012-04-04T19:31:25Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"> This replaces commit 4d6efe0 in the 'th/difftool-diffall' branch based\n> on pu. This version corrects some problems with submodule support in\n> difftool.\n\nPlease s/4d6efe0/4b6efe0/.  Sorry for the confusion.\n"},{"id":"188793","messageId":"CAJDDKr76eMiA4rOHQhar3aToVThDfbc8Ki5tr2PGU_UpDMAVeA@mail.gmail.com","threadId":"30147","inReplyTo":"1333567265-23986-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-04-09T12:14:49Z","receivedAt":"2012-04-09T12:14:49Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index d4fe998..5bb01e1 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -1,21 +1,29 @@\n>  #!/usr/bin/env perl\n>  # Copyright (c) 2009, 2010 David Aguilar\n> +# Copyright (c) 2012 Tim Henigan\n>  #\n>  # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible\n>  # git-difftool--helper script.\n>  #\n>  # This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.\n> -# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL\n> -# are exported for use by git-difftool--helper.\n> +# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, GIT_DIFFTOOL_DIRDIFF,\n> +# and GIT_DIFF_TOOL are exported for use by git-difftool--helper.\n\nWhat if we punt on enumerating each variable and reword this to:\n\n# This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.\n# The GIT_DIFF* variables are exported for use by git-difftool--helper.\n\nI also think we should change the shebang line to #!/usr/bin/perl.\n\n\n> @@ -24,15 +32,121 @@ usage: git difftool [-t|--tool=<tool>]\n>                     [-x|--extcmd=<cmd>]\n>                     [-g|--gui] [--no-gui]\n>                     [--prompt] [-y|--no-prompt]\n> +                    [-d|--dir-diff]\n>                     ['git diff' options]\n>  USAGE\n>        exit($exitcode);\n>  }\n>\n> +sub setup_dir_diff\n> +{\n> +       # Run the diff; exit immediately if no diff found\n> +       my $repo = Git->repository();\n> +       my $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);\n> +       exit(0) if (length($diffrtn) == 0);\n> +\n> +       # Setup temp directories\n> +       my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);\n> +       my $ldir = \"$tmpdir/left\";\n> +       my $rdir = \"$tmpdir/right\";\n> +       mkpath($ldir) or die $!;\n> +       mkpath($rdir) or die $!;\n> +\n> +       # Build index info for left and right sides of the diff\n> +       my $submodule_mode = \"160000\";\n> +       my $null_mode = 0 x 6;\n> +       my $null_sha1 = 0 x 40;\n\nI know Perl allows it, but my eyes get confused.\n\nHow about writing \"0\" x 6 and \"0\" x 40?\nThat way we can visually see that the result is a string.\n\n\n> +       my $lindex = \"\";\n> +       my $rindex = \"\";\n> +       my %submodule;\n> +       my @rawdiff = split('\\0', $diffrtn);\n> +\n> +       for (my $i=0; $i<@rawdiff; $i+=2) {\n\nWe use $i + 1 to grab list elements, so how about $#rawdiff instead of @rawdiff?\nIt doesn't matter in practice, though...\n\n\n> +               my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));\n> +               my $path = $rawdiff[$i + 1];\n> +\n> +               if (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {\n> +                       $submodule{$path}{left} = $lsha1;\n> +                       if ($lsha1 ne $rsha1) {\n> +                               $submodule{$path}{right} = $rsha1;\n> +                       } else {\n> +                               $submodule{$path}{right} = \"$rsha1-dirty\";\n> +                       }\n> +                       next;\n> +               }\n> +\n> +               if ($lmode ne $null_mode) {\n> +                       $lindex .= \"$lmode $lsha1\\t$path\\0\";\n> +               }\n> +\n> +               if ($rmode ne $null_mode) {\n> +                       if ($rsha1 ne $null_sha1) {\n> +                               $rindex .= \"$rmode $rsha1\\t$path\\0\";\n> +                       } else {\n> +                               push(@working_tree, $path);\n> +                       }\n> +               }\n> +       }\n> +\n> +       # Populate the left and right directories based on each index file\n> +       my ($inpipe, $ctx);\n> +       $ENV{GIT_DIR} = $repo->repo_path();\n> +       $ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\n> +       ($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n> +       print($inpipe $lindex);\n> +       $repo->command_close_pipe($inpipe, $ctx);\n> +       system(('git', 'checkout-index', '--all', \"--prefix=$ldir/\"));\n\nPlease drop the extra parens.  Perl ignores them.\nWe should also check the return value from system() here.\n\n\n> +\n> +       $ENV{GIT_INDEX_FILE} = \"$tmpdir/rindex\";\n> +       ($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n> +       print($inpipe $rindex);\n> +       $repo->command_close_pipe($inpipe, $ctx);\n> +       system(('git', 'checkout-index', '--all', \"--prefix=$rdir/\"));\n> +\n> +       # Changes in the working tree need special treatment since they are\n> +       # not part of the index\n> +       my $workdir = $repo->repo_path() . \"/..\";\n> +       for (@working_tree) {\n\nPlease use the \"for my $foo (@list)\" form so we can say $foo instead of $_.\n\n\n> +               my $dir = dirname($_);\n> +               unless (-d \"$rdir/$dir\") {\n> +                       mkpath(\"$rdir/$dir\") or die $!;\n> +               }\n> +               copy(\"$workdir/$_\", \"$rdir/$_\") or die $!;\n> +               chmod(stat(\"$workdir/$_\")->mode, \"$rdir/$_\") or die $!;\n> +       }\n> +\n> +       # Changes to submodules require special treatment. This loop writes a\n> +       # temporary file to both the left and right directories to show the\n> +       # change in the recorded SHA1 for the submodule.\n> +       foreach my $path (keys %submodule) {\n\nI think it's better to use \"for\" instead of \"foreach\" since we do not\nmodify $path.\n\n\n> +               if (defined $submodule{$path}{left}) {\n\nIn some places we write \"defined(...)\" but here it's \"defined ...\".\nWe should be consistent...\n\n\n> @@ -65,22 +179,40 @@ if ($gui) {\n>                $ENV{GIT_DIFF_TOOL} = $guitool;\n>        }\n>  }\n> -if (defined($prompt)) {\n> -       if ($prompt) {\n> -               $ENV{GIT_DIFFTOOL_PROMPT} = 'true';\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> +       my ($a, $b) = setup_dir_diff();\n> +       if (defined($extcmd)) {\n> +               system(($extcmd, $a, $b));\n>        } else {\n> -               $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n> +               $ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';\n> +               system(('git', 'difftool--helper', $a, $b));\n> +       }\n> +\n> +       # If the diff including working copy files and those\n> +       # files were modified during the diff, then the changes\n> +       # should be copied back to the working tree\n> +       my $repo = Git->repository();\n> +       my $workdir = $repo->repo_path() . \"/..\";\n\nDoes this work when $GIT_WORK_TREE / core.worktree are defined?\n\n\n> +       for (@working_tree) {\n> +               copy(\"$b/$_\", \"$workdir/$_\") or die $!;\n> +               chmod(stat(\"$b/$_\")->mode, \"$workdir/$_\") or die $!;\n> +       }\n\n\"for my ...\"\n\n\n> +} else {\n> +       if (defined($prompt)) {\n> +               if ($prompt) {\n> +                       $ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n> +               } else {\n> +                       $ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n> +               }\n>        }\n> -}\n>\n> -$ENV{GIT_PAGER} = '';\n> -$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n> -my @command = ('git', 'diff', @ARGV);\n> -\n> -# ActiveState Perl for Win32 does not implement POSIX semantics of\n> -# exec* system call. It just spawns the given executable and finishes\n> -# the starting program, exiting with code 0.\n> -# system will at least catch the errors returned by git diff,\n> -# allowing the caller of git difftool better handling of failures.\n> -my $rc = system(@command);\n> -exit($rc | ($rc >> 8));\n> +       $ENV{GIT_PAGER} = '';\n> +       $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n> +       my $rc = system(('git', 'diff', @ARGV));\n> +       exit($rc | ($rc >> 8));\n> +}\n\n\nWe went back and forth a few times on this section,\neventually landing back on using system().\n\nShould we retain this comment to help future readers from\nhaving to re-learn it the hard way again?\n\nWe could also link to the ML threads, if you think that's helpful.\n-- \nDavid\n"},{"id":"188798","messageId":"7v1unwyea7.fsf@alter.siamese.dyndns.org","threadId":"30147","inReplyTo":"CAJDDKr76eMiA4rOHQhar3aToVThDfbc8Ki5tr2PGU_UpDMAVeA@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-09T18:02:24Z","receivedAt":"2012-04-09T18:02:24Z","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> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index d4fe998..5bb01e1 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -1,21 +1,29 @@\n>>  #!/usr/bin/env perl\n> ...\n> I also think we should change the shebang line to #!/usr/bin/perl.\n\nGood point; it would not make a difference in the end result, but\nconsistency is good.\n\n>> -# ActiveState Perl for Win32 does not implement POSIX semantics of\n>> -# exec* system call. It just spawns the given executable and finishes\n>> -# the starting program, exiting with code 0.\n>> -# system will at least catch the errors returned by git diff,\n>> -# allowing the caller of git difftool better handling of failures.\n>> -my $rc = system(@command);\n>> -exit($rc | ($rc >> 8));\n>> +       $ENV{GIT_PAGER} = '';\n>> +       $ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n>> +       my $rc = system(('git', 'diff', @ARGV));\n>> +       exit($rc | ($rc >> 8));\n>> +}\n>\n> \n> We went back and forth a few times on this section,\n> eventually landing back on using system().\n>\n> Should we retain this comment to help future readers from\n> having to re-learn it the hard way again?\n\nWell, I kept typing \"good point\" but ended up agreeing with everything you\nsaid in your message, and removed all \"Me too\"s ;-).\n\nThanks for a thorough review.\n"},{"id":"188871","messageId":"CAFouetiBMK+WWU9yOLaO5SLyUO1K_S-YW0LTt5tE5UTcMM8K=w@mail.gmail.com","threadId":"30147","inReplyTo":"CAJDDKr76eMiA4rOHQhar3aToVThDfbc8Ki5tr2PGU_UpDMAVeA@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-10T15:01:22Z","receivedAt":"2012-04-10T15:01:22Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Hi David,\n\nThank you for the review.  I am working on the changes you suggested,\nbut want to discuss one of them:\n\nOn Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>> +\n>> +       # If the diff including working copy files and those\n>> +       # files were modified during the diff, then the changes\n>> +       # should be copied back to the working tree\n>> +       my $repo = Git->repository();\n>> +       my $workdir = $repo->repo_path() . \"/..\";\n>\n> Does this work when $GIT_WORK_TREE / core.worktree are defined?\n\nI had not considered this case and the existing code will certainly\nignore alternate work tree locations.  To fix the problem, I plan to\nadd something like this:\n\n+sub find_worktree\n+{\n+       my $worktree;\n+       my $env_worktree = $ENV{GIT_WORK_TREE};\n+       my $core_worktree = Git::config('core.worktree');\n+\n+       if (length($env_worktree) > 0) {\n+               $worktree = $env_worktree;\n+       } elsif (length($core_worktree) > 0) {\n+               $worktree = $core_worktree;\n+       } else {\n+               my $repo = Git->repository();\n+               $worktree = $repo->repo_path() . \"/..\";\n+       }\n+\n+       return $worktree;\n+}\n\nDoes this look like a reasonable solution?  I don't typically use\n'$GIT_WORK_TREE' or 'core.worktree'.  The above function works for me,\nbut I would appreciate some feedback before I send the next revision\nof the patch (v12!).\n\nThanks,\nTim\n"},{"id":"188887","messageId":"CAFouetijF5vt9r1GfbntJ0BEqoo0FpztkeqrmeN=-6DK1Q_XRA@mail.gmail.com","threadId":"30147","inReplyTo":"CAJDDKr76eMiA4rOHQhar3aToVThDfbc8Ki5tr2PGU_UpDMAVeA@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-10T17:24:52Z","receivedAt":"2012-04-10T17:24:52Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>> diff --git a/git-difftool.perl b/git-difftool.perl\n>> index d4fe998..5bb01e1 100755\n>> --- a/git-difftool.perl\n>> +++ b/git-difftool.perl\n>> @@ -1,21 +1,29 @@\n\n...snip...\n\n>> +       $ENV{GIT_DIR} = $repo->repo_path();\n>> +       $ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\n>> +       ($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n>> +       print($inpipe $lindex);\n>> +       $repo->command_close_pipe($inpipe, $ctx);\n>> +       system(('git', 'checkout-index', '--all', \"--prefix=$ldir/\"));\n\n...snip...\n\n>> +       # If the diff including working copy files and those\n>> +       # files were modified during the diff, then the changes\n>> +       # should be copied back to the working tree\n>> +       my $repo = Git->repository();\n>> +       my $workdir = $repo->repo_path() . \"/..\";\n>\n> Does this work when $GIT_WORK_TREE / core.worktree are defined?\n\nShould I also be concerned that the existing code always overrides $GIT_DIR?\n\nFor example, should I squash in the following before calling 'git update-index'?\n\n-       $ENV{GIT_DIR} = $repo->repo_path();\n+      if (not defined($ENV{GIT_DIR})) {\n+              $ENV{GIT_DIR} = $repo->repo_path();\n+      }\n\nThis seems like the right thing to do, but again I have not used\n$GIT_DIR except for the implementation of this script.\n"},{"id":"188935","messageId":"CAJDDKr7XXqWBRVXhEenWVcE+7526sYVV821Ytty0xbZBvshd-w@mail.gmail.com","threadId":"30147","inReplyTo":"CAFouetijF5vt9r1GfbntJ0BEqoo0FpztkeqrmeN=-6DK1Q_XRA@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-04-11T09:02:26Z","receivedAt":"2012-04-11T09:02:26Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Tue, Apr 10, 2012 at 10:24 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> On Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:\n>> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>>> diff --git a/git-difftool.perl b/git-difftool.perl\n>>> index d4fe998..5bb01e1 100755\n>>> --- a/git-difftool.perl\n>>> +++ b/git-difftool.perl\n>>> @@ -1,21 +1,29 @@\n>\n> ...snip...\n>\n>>> +       $ENV{GIT_DIR} = $repo->repo_path();\n>>> +       $ENV{GIT_INDEX_FILE} = \"$tmpdir/lindex\";\n>>> +       ($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);\n>>> +       print($inpipe $lindex);\n>>> +       $repo->command_close_pipe($inpipe, $ctx);\n>>> +       system(('git', 'checkout-index', '--all', \"--prefix=$ldir/\"));\n>\n> ...snip...\n>\n>>> +       # If the diff including working copy files and those\n>>> +       # files were modified during the diff, then the changes\n>>> +       # should be copied back to the working tree\n>>> +       my $repo = Git->repository();\n>>> +       my $workdir = $repo->repo_path() . \"/..\";\n>>\n>> Does this work when $GIT_WORK_TREE / core.worktree are defined?\n>\n> Should I also be concerned that the existing code always overrides $GIT_DIR?\n> For example, should I squash in the following before calling 'git update-index'?\n>\n> -       $ENV{GIT_DIR} = $repo->repo_path();\n> +      if (not defined($ENV{GIT_DIR})) {\n> +              $ENV{GIT_DIR} = $repo->repo_path();\n> +      }\n\nI think the right thing to do would be to not override GIT_DIR at all.\n I haven't read it deeply enough to know whether it was being\noverridden for a specific reason, but I think it should be safe to\nleave it as-is.\n\nGit.pm ends up overriding these variables itself anyways when calling commands.\n\nThe GIT_WORK_TREE check should use $repo->wc_path().  Git.pm's already\ndone all the hard work ;-)\n-- \nDavid\n"},{"id":"188990","messageId":"CAFouetgfPuiYO9xKPGQ1GGjr6JvMXM7et_7oYrWRrg+RMj9mCQ@mail.gmail.com","threadId":"30147","inReplyTo":"CAJDDKr7XXqWBRVXhEenWVcE+7526sYVV821Ytty0xbZBvshd-w@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-04-11T16:46:41Z","receivedAt":"2012-04-11T16:46:41Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Wed, Apr 11, 2012 at 5:02 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Tue, Apr 10, 2012 at 10:24 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>> On Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:\n>>> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>\n> I think the right thing to do would be to not override GIT_DIR at all.\n>  I haven't read it deeply enough to know whether it was being\n> overridden for a specific reason, but I think it should be safe to\n> leave it as-is.\n>\n> Git.pm ends up overriding these variables itself anyways when calling commands.\n\nI tried to avoid setting $GIT_DIR in earlier versions of the patch.\nHowever as discussed here [1], either 'git update-index' or 'git\ncheckout-index' did not work as expected without explicitly setting\n$GIT_DIR.\n\nIf $GIT_DIR is not set, 'update-index' and 'checkout-index' will only\nwork if 'difftool' is called from the repo root.  If 'difftool' is\ncalled from a subdirectory, then one of the commands fails.\n\nI suspect that when $GIT_INDEX_FILE is set but $GIT_DIR is not, then\n$GIT_DIR is assumed to be 'pwd'.  However, I was not able to prove\nthat.\n\n\n> The GIT_WORK_TREE check should use $repo->wc_path().  Git.pm's already\n> done all the hard work ;-)\n\nI also tried this in an earlier version of the patch.  As noted here\n[2], I found that when 'difftool' was run from a subdirectory of the\nrepo root, '$repo->wc_path()' returned the subdirectory rather than\nthe repo root.\n\nThinking about this again now, I realize it was a side-effect of\n$GIT_DIR being set in the script.  The man page for git config states\nthat:\n\n    If --git-dir or GIT_DIR is specified but none of --work-tree, GIT_WORK_TREE\n    and core.worktree is specified, the current working directory is regarded as\n    the top level of your working tree.\n\nSo, if I explicitly set $GIT_DIR just for the 'update-index' and\n'checkout-index' commands, I need to unset it afterwards.  This should\nallow '$repo->wc_path()' to behave as expected.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/193296/focus=193302\n[2]: http://thread.gmane.org/gmane.comp.version-control.git/193601/focus=193603\n"},{"id":"189068","messageId":"CAJDDKr5q=a+5D0SKUiCTuL=qCxNBoLWJc=_aBFWu2ZfD_0VUzw@mail.gmail.com","threadId":"30147","inReplyTo":"CAFouetgfPuiYO9xKPGQ1GGjr6JvMXM7et_7oYrWRrg+RMj9mCQ@mail.gmail.com","subject":"Re: [PATCH 8/9 v11] difftool: teach difftool to handle directory diffs","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-04-12T03:21:00Z","receivedAt":"2012-04-12T03:21:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Apr 11, 2012 at 9:46 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> On Wed, Apr 11, 2012 at 5:02 AM, David Aguilar <davvid@gmail.com> wrote:\n>> On Tue, Apr 10, 2012 at 10:24 AM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>>> On Mon, Apr 9, 2012 at 8:14 AM, David Aguilar <davvid@gmail.com> wrote:\n>>>> On Wed, Apr 4, 2012 at 12:21 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>>\n>> I think the right thing to do would be to not override GIT_DIR at all.\n>>  I haven't read it deeply enough to know whether it was being\n>> overridden for a specific reason, but I think it should be safe to\n>> leave it as-is.\n>>\n>> Git.pm ends up overriding these variables itself anyways when calling commands.\n>\n> I tried to avoid setting $GIT_DIR in earlier versions of the patch.\n> However as discussed here [1], either 'git update-index' or 'git\n> checkout-index' did not work as expected without explicitly setting\n> $GIT_DIR.\n>\n> If $GIT_DIR is not set, 'update-index' and 'checkout-index' will only\n> work if 'difftool' is called from the repo root.  If 'difftool' is\n> called from a subdirectory, then one of the commands fails.\n>\n> I suspect that when $GIT_INDEX_FILE is set but $GIT_DIR is not, then\n> $GIT_DIR is assumed to be 'pwd'.  However, I was not able to prove\n> that.\n>\n>\n>> The GIT_WORK_TREE check should use $repo->wc_path().  Git.pm's already\n>> done all the hard work ;-)\n>\n> I also tried this in an earlier version of the patch.  As noted here\n> [2], I found that when 'difftool' was run from a subdirectory of the\n> repo root, '$repo->wc_path()' returned the subdirectory rather than\n> the repo root.\n>\n> Thinking about this again now, I realize it was a side-effect of\n> $GIT_DIR being set in the script.  The man page for git config states\n> that:\n>\n>    If --git-dir or GIT_DIR is specified but none of --work-tree, GIT_WORK_TREE\n>    and core.worktree is specified, the current working directory is regarded as\n>    the top level of your working tree.\n>\n> So, if I explicitly set $GIT_DIR just for the 'update-index' and\n> 'checkout-index' commands, I need to unset it afterwards.  This should\n> allow '$repo->wc_path()' to behave as expected.\n>\n> [1]: http://thread.gmane.org/gmane.comp.version-control.git/193296/focus=193302\n> [2]: http://thread.gmane.org/gmane.comp.version-control.git/193601/focus=193603\n\nThat's right.  If we set it for one command we have to remember to\nunset it afterwards else it won't work as expected.\n\nIf we're setting GIT_DIR then we should probably set GIT_WORK_TREE\ntoo.  It seems like one way would be to call repo_path() and wc_path()\nearly, set the variables with the returned values, and then worry\nabout managing GIT_WORK_TREE before+after calling checkout-index.\nThat might work.\n\nAnd in case the code needs it, there's a GIT_PREFIX variable that is\nset when the current directory is a subdir beneath the repo root.\n-- \nDavid\n"}]}