{"thread":{"id":"30018","subject":"[PATCH v3 0/9] difftool: teach command to perform directory diffs","startedAt":"2012-03-21T19:35:55Z","lastAt":"2012-03-22T19:13:29Z","messageCount":17,"participants":["Tim Henigan","Junio C Hamano","Thomas Rast","David Aguilar"],"isPatch":true,"patchVersion":3,"patchTotal":9},"messages":[{"id":"187418","messageId":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":null,"subject":"[PATCH v3 0/9] difftool: teach command to perform directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:35:55Z","receivedAt":"2012-03-21T19:35:55Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Changes in v3:\n  - Removed unused shell variable (SUBDIRECTORY_OK) from patch 7.\n  - Changed patches 7 and 8 to support calling 'difftool --dir-diff' from\n    a subdirectory of the repo.\n  - Changed patch 8 to support \n  - Added patch 10, which makes mixing --prompt and --no-prompt and error.\n  - Added tests for new difftool options in patches 11 and 12.\n\n  - Patches 1-6 and 9 did not change and have not been resent.\n  \n\nSeries Overview:\n\n'git difftool' is a very useful command that allows git diffs to be opened\nin an external tool. Currently, difftool opens a separate instance of the\nexternal tool for each file that changed.  This can be tedious when many\nfiles have changed.\n\nThis series teaches difftool to perform directory diffs, so that all file\nchanges can be opened/reviewed in a single instance of the external tool.\n\nThis is the second phase of development for this feature.  The first phase\nwas added as a separate command (git diffall) in 1252bbe (contrib: add\ngit-diffall script).  During review of that script on the Git developers\nlist, an informal development roadmap was suggested [1]. The next phase\nof that plan is to integrate the 'git-diffall' feature into 'difftool'.\nThis series gets that done.\n\n[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383\n\n\nTim Henigan (12):\n  difftool: parse options using Getopt::Long\n  difftool: exit(0) when usage is printed\n  difftool: remove explicit change of PATH\n  difftool: stop appending '.exe' to git\n  difftool: eliminate setup_environment function\n  difftool: replace system call with Git::command_noisy\n  difftool: teach difftool to handle directory diffs\n  difftool: teach dir-diff to copy modified files back to working tree\n  difftool: print list of valid tools with '--tool-help'\n  difftool: do not allow mix of '--prompt' with '--no-prompt'\n  t7800: add test for difftool --tool-help\n  t7800: add tests for difftool --dir-diff\n\n Documentation/git-difftool.txt |   17 ++-\n git-difftool--helper.sh        |   19 ++-\n git-difftool.perl              |  264 +++++++++++++++++++++++++++-------------\n t/t7800-difftool.sh            |   59 +++++++--\n 4 files changed, 254 insertions(+), 105 deletions(-)\n\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187419","messageId":"1332358560-13774-2-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH v3 7/9] difftool: teach difftool to handle directory diffs","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:35:56Z","receivedAt":"2012-03-21T19:35:56Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Prior to this commit, the difftool utility could only open files one\nat a time.  The new '--dir-diff' option copies all the modified files\nto a temporary location and runs a directory diff on them.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis replaces 7/9 in v2 of the series.\n\nChanged in v3:\n  - Removed SUBDIRECTORY_OK from git-difftool--helper.sh. Since git-sh-setup\n    is not sourced, this variable is not used.\n  - $ENV{GIT_DIR} must be explicitly set if 'difftool --dir-diff' is called\n    from a subdirectory in the repo.\n  - Changed from 'Git::command' to 'system' when calling 'git checkout-index'.\n    Without this change, 'difftool --dir-diff' fails when called from a\n    subdirectory in the repo.\n\n\n Documentation/git-difftool.txt |    6 ++\n git-difftool--helper.sh        |   19 ++++--\n git-difftool.perl              |  145 ++++++++++++++++++++++++++++++++++++----\n 3 files changed, 152 insertions(+), 18 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 8498089..082e887 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -1,18 +1,24 @@\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@@ -22,15 +28,110 @@ sub usage\n \tprint << 'USAGE';\n usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n                     [-y|--no-prompt]   [-g|--gui]\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 @working_tree;\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\t$submodule{$path}{right} = $rsha1;\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->wc_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\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\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, $no_prompt, $prompt);\n+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt);\n GetOptions('g|gui' => \\$gui,\n+\t'd|dir-diff' => \\$dirdiff,\n \t'h' => \\$help,\n \t'prompt' => \\$prompt,\n \t't|tool:s' => \\$difftool_cmd,\n@@ -63,13 +164,33 @@ if (defined($gui)) {\n \t\t$ENV{GIT_DIFF_TOOL} = $guitool;\n \t}\n }\n-if (defined($prompt)) {\n-\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-}\n-elsif (defined($no_prompt)) {\n-\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n-}\n \n-$ENV{GIT_PAGER} = '';\n-$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\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_DIRDIFF} = 'true';\n+\t\tgit_cmd_try {\n+\t\t\tGit::command_noisy(('difftool--helper', $a, $b))\n+\t\t} 'exit code %d';\n+\t}\n+\t# TODO: 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+} else {\n+\tif (defined($prompt)) {\n+\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n+\t}\n+\telsif (defined($no_prompt)) {\n+\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\n+\t}\n+\n+\t$ENV{GIT_PAGER} = '';\n+\t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n+\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n+}\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187420","messageId":"1332358560-13774-3-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH v3 8/9] difftool: teach dir-diff to copy modified files back to working tree","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:35:57Z","receivedAt":"2012-03-21T19:35:57Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"If the user runs a diff when there are modified files in the working copy,\nthey expect to be able to be edit the files in the diff viewer and save\nthe changes. When using difftool in file mode (i.e. diffing one file at\na time), this works as expected.  However, when using difftool in directory\ndiff mode, the changes are not saved.\n\nThis commit teaches the directory diff mode to copy changes from the diff\nviewer back to the working copy.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis replaces patch 8/9 in v2 of the series.\n\nChanged in v3\n  - Replaced calls to '$repo->wc_path()' with '$repo->repo_path() . \"/..\"'\n    so that paths are properly handled when 'difftool --dir-diff' is\n    called from a subdirectory of the repo.\n\n\n git-difftool.perl |   14 +++++++++++---\n 1 file changed, 11 insertions(+), 3 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 082e887..9201b89 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -22,6 +22,8 @@ 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@@ -54,7 +56,6 @@ sub setup_dir_diff\n \tmy $null_sha1 = 0 x 40;\n \tmy $lindex = \"\";\n \tmy $rindex = \"\";\n-\tmy @working_tree;\n \tmy %submodule;\n \tmy @rawdiff = split('\\0', $diffrtn);\n \n@@ -98,7 +99,7 @@ sub setup_dir_diff\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->wc_path();\n+\tmy $workdir = $repo->repo_path() . \"/..\";\n \tfor (@working_tree) {\n \t\tmy $dir = dirname($_);\n \t\tunless (-d \"$rdir/$dir\") {\n@@ -179,9 +180,16 @@ if (defined($dirdiff)) {\n \t\t\tGit::command_noisy(('difftool--helper', $a, $b))\n \t\t} 'exit code %d';\n \t}\n-\t# TODO: if the diff including working copy files and those\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\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187421","messageId":"1332358560-13774-4-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH v3 10/9] difftool: do not allow mix of '--prompt' with '--no-prompt'","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:35:58Z","receivedAt":"2012-03-21T19:35:58Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When difftool was changed to use Getopt::Long, it changed the way that\nthe '--prompt' and '--no-prompt' options are handled. Prior to the change,\nif both options were given, then the last option was used.  After the\nchange, if both were given, then '--prompt' was always used.\n\nThis commit teaches difftool to exit with an error if both options are\ngiven at the command line. t7800 was updated to match.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n git-difftool.perl   |    6 +++++-\n t/t7800-difftool.sh |   16 +++++-----------\n 2 files changed, 10 insertions(+), 12 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 9f0f9a9..9629811 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -202,7 +202,11 @@ if (defined($dirdiff)) {\n \t\tchmod(stat(\"$b/$_\")->mode, \"$workdir/$_\") or die $!;\n \t}\n } else {\n-\tif (defined($prompt)) {\n+\tif (defined($prompt) and defined($no_prompt)) {\n+\t\tprint(\"Cannot command '--prompt' and '--no-prompt' at the same time.\\n\");\n+\t\tusage(1);\n+\t}\n+\telsif (defined($prompt)) {\n \t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n \t}\n \telsif (defined($no_prompt)) {\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 4fb4c93..dc181cf 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -202,17 +202,11 @@ test_expect_success PERL 'difftool.prompt can overridden with --prompt' '\n \trestore_test_defaults\n '\n \n-# Test that the last flag passed on the command-line wins\n-test_expect_success PERL 'difftool last flag wins' '\n-\tdiff=$(git difftool --prompt --no-prompt branch) &&\n-\ttest \"$diff\" = \"branch\" &&\n-\n-\trestore_test_defaults &&\n-\n-\tprompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&\n-\tprompt_given \"$prompt\" &&\n-\n-\trestore_test_defaults\n+# Test that an error is given when both --prompt and --no-prompt are\n+# commanded\n+test_expect_success PERL '--prompt / --no-prompt conflict' '\n+\ttest_must_fail git difftool --prompt --no-prompt branch &&\n+\ttest_must_fail git difftool --no-prompt --prompt branch\n '\n \n # git-difftool falls back to git-mergetool config variables\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187422","messageId":"1332358560-13774-5-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH v3 11/9] t7800: add test for difftool --tool-help","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:35:59Z","receivedAt":"2012-03-21T19:35:59Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Signed-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n t/t7800-difftool.sh |    5 +++++\n 1 file changed, 5 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex dc181cf..663247c 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -300,4 +300,9 @@ test_expect_success PERL 'say no to the second file' '\n \techo \"$diff\" | stdin_doesnot_contain br2\n '\n \n+test_expect_success PERL 'difftool --tool-help' '\n+\ttool_help=$(git difftool --tool-help) &&\n+\techo \"$tool_help\" | stdin_contains tool\n+'\n+\n test_done\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187423","messageId":"1332358560-13774-6-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-1-git-send-email-tim.henigan@gmail.com","subject":"[PATCH v3 12/9] t7800: add tests for difftool --dir-diff","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-21T19:36:00Z","receivedAt":"2012-03-21T19:36:00Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"Signed-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n t/t7800-difftool.sh |   38 ++++++++++++++++++++++++++++++++++++++\n 1 file changed, 38 insertions(+)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 663247c..fca49d1 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -305,4 +305,42 @@ test_expect_success PERL 'difftool --tool-help' '\n \techo \"$tool_help\" | stdin_contains tool\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+\tcd sub &&\n+\tdiff=$(git difftool --dir-diff --extcmd ls branch) &&\n+\techo \"$diff\" | stdin_contains sub &&\n+\techo \"$diff\" | stdin_contains file &&\n+\tcd ..\n+'\n+\n test_done\n-- \n1.7.10.rc1.39.g6e141f\n"},{"id":"187428","messageId":"7vehsl1zl0.fsf@alter.siamese.dyndns.org","threadId":"30018","inReplyTo":"1332358560-13774-4-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v3 10/9] difftool: do not allow mix of '--prompt' with '--no-prompt'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-21T20:14:19Z","receivedAt":"2012-03-21T20:14:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> When difftool was changed to use Getopt::Long, it changed the way that\n> the '--prompt' and '--no-prompt' options are handled. Prior to the change,\n> if both options were given, then the last option was used.  After the\n> change, if both were given, then '--prompt' was always used.\n>\n> This commit teaches difftool to exit with an error if both options are\n> given at the command line. t7800 was updated to match.\n\nHrm, people with \"[alias] mdt = difftool --prompt\" have every right to\nexpect their\n\n\t$ git mdt --no-prompt\n\nto keep working, no?\n"},{"id":"187444","messageId":"1332381236-16004-1-git-send-email-tim.henigan@gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-4-git-send-email-tim.henigan@gmail.com","subject":"[PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-22T01:53:56Z","receivedAt":"2012-03-22T01:53:56Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"When difftool was changed to use Getopt::Long, it changed the way\nthat the '--prompt' and '--no-prompt' options were handled. The\nexpected behavior is that the two options may be given any number\nof times. The last option given \"wins\".\n\nFor example, if a user sets \"[alias] mdt = difftool --prompt\", the\nfollowing must still run without error:\n\n$ git mdt --no-prompt\n\nThe changes made during the switch to Getopt::Long broke this\nbehavior. This commit teaches difftool to handle them properly\nagain.\n\nSigned-off-by: Tim Henigan <tim.henigan@gmail.com>\n---\n\nThis replaces 10/9 in v3 of the series.\n\nChanges in v4:\n  - '--prompt' and '--no-prompt' may be used together again.\n  - Removed changes made to t7800.  The old test now passes.\n\n\n git-difftool.perl |   32 ++++++++++++++++++++++----------\n 1 file changed, 22 insertions(+), 10 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex 9f0f9a9..41c6557 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -22,6 +22,7 @@ use File::Temp qw(tempdir);\n use Getopt::Long qw(:config pass_through);\n use Git;\n \n+my @diffargs;\n my @working_tree;\n \n sub usage\n@@ -41,7 +42,7 @@ 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+\tmy $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @diffargs]);\n \texit(0) if (length($diffrtn) == 0);\n \n \t# Setup temp directories\n@@ -131,15 +132,25 @@ sub setup_dir_diff\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, $no_prompt, $prompt, $tool_help);\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't|tool:s' => \\$difftool_cmd,\n \t'tool-help' => \\$tool_help,\n-\t'x|extcmd:s' => \\$extcmd,\n-\t'y|no-prompt' => \\$no_prompt);\n+\t'x|extcmd:s' => \\$extcmd);\n+\n+# the '--prompt' and '--no-prompt' options require special treatment\n+# because they may be specified more than once...the last one \"wins\".\n+for (@ARGV) {\n+\tif (($_ eq \"-y\") or ($_ eq \"--no-prompt\")) {\n+\t\t$prompt = 0;\n+\t} elsif ($_ eq \"--prompt\") {\n+\t\t$prompt = 1;\n+\t} else {\n+\t\tpush(@diffargs, $_);\n+\t}\n+}\n \n if (defined($help)) {\n \tusage(0);\n@@ -203,13 +214,14 @@ if (defined($dirdiff)) {\n \t}\n } else {\n \tif (defined($prompt)) {\n-\t\t$ENV{GIT_DIFFTOOL_PROMPT} = 'true';\n-\t}\n-\telsif (defined($no_prompt)) {\n-\t\t$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';\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 \t$ENV{GIT_PAGER} = '';\n \t$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';\n-\tgit_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';\n+\tgit_cmd_try { Git::command_noisy(('diff', @diffargs)) } 'exit code %d';\n }\n-- \n1.7.10.rc1.39.gdeb0e\n"},{"id":"187446","messageId":"7viphxz37j.fsf@alter.siamese.dyndns.org","threadId":"30018","inReplyTo":"1332381236-16004-1-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T04:09:36Z","receivedAt":"2012-03-22T04:09:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> +# the '--prompt' and '--no-prompt' options require special treatment\n> +# because they may be specified more than once...the last one \"wins\".\n> +for (@ARGV) {\n> +\tif (($_ eq \"-y\") or ($_ eq \"--no-prompt\")) {\n> +\t\t$prompt = 0;\n> +\t} elsif ($_ eq \"--prompt\") {\n> +\t\t$prompt = 1;\n> +\t} else {\n> +\t\tpush(@diffargs, $_);\n> +\t}\n> +}\n\nI really do not like the direction in which this series is going.  We do\nnot have a similar --no-gui option to defeat --gui option that may appear\nearlier on the command line, but when we fix that bug (isn't it a bug?),\nwe would have to teach this loop about that option, wouldn't we?\n\nIn the end, won't you end up resurrecting the argument parsing loop that\nyou got rid of with the first patch in your series?  Isn't this working\naround the problem introduced only because you are using Getopt::Long and\nhitting its limitations?\n"},{"id":"187449","messageId":"871uoljbe9.fsf@thomas.inf.ethz.ch","threadId":"30018","inReplyTo":"7viphxz37j.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Thomas Rast","fromEmail":"trast@student.ethz.ch","sentAt":"2012-03-22T08:19:26Z","receivedAt":"2012-03-22T08:19:26Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n>> +# the '--prompt' and '--no-prompt' options require special treatment\n>> +# because they may be specified more than once...the last one \"wins\".\n>> +for (@ARGV) {\n>> +\tif (($_ eq \"-y\") or ($_ eq \"--no-prompt\")) {\n>> +\t\t$prompt = 0;\n>> +\t} elsif ($_ eq \"--prompt\") {\n>> +\t\t$prompt = 1;\n>> +\t} else {\n>> +\t\tpush(@diffargs, $_);\n>> +\t}\n>> +}\n>\n> I really do not like the direction in which this series is going.  We do\n> not have a similar --no-gui option to defeat --gui option that may appear\n> earlier on the command line, but when we fix that bug (isn't it a bug?),\n> we would have to teach this loop about that option, wouldn't we?\n>\n> In the end, won't you end up resurrecting the argument parsing loop that\n> you got rid of with the first patch in your series?  Isn't this working\n> around the problem introduced only because you are using Getopt::Long and\n> hitting its limitations?\n\nLimitations?  You can basically steal code from git-send-email.  As an\nexample:\n\n---- 8< ----\n#!/usr/bin/perl\n\nuse strict;\nuse warnings;\nuse Getopt::Long;\n\nmy $foo = 0;\nmy $rc = GetOptions(\"foo!\" => \\$foo,\n\t\t    \"n\" => sub { $foo = 0; },\n\t\t    \"y\" => \\$foo);\n\nprint \"$foo\\n\";\n---- 8< ----\n\n$ ./getopt-test.perl -n\n0\n$ ./getopt-test.perl -y\n1\n$ ./getopt-test.perl --foo\n1\n$ ./getopt-test.perl --no-foo\n0\n$ ./getopt-test.perl --foo --no-foo\n0\n$ ./getopt-test.perl -y -n\n0\n$ ./getopt-test.perl --foo -n\n0\n\n--\nThomas Rast\ntrast@{inf,student}.ethz.ch\n"},{"id":"187453","messageId":"CAJDDKr7m3Ur-RNHffkW9rvbT6kpXWjKMG+Kiw5ausQw5058LHw@mail.gmail.com","threadId":"30018","inReplyTo":"1332358560-13774-6-git-send-email-tim.henigan@gmail.com","subject":"Re: [PATCH v3 12/9] t7800: add tests for difftool --dir-diff","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2012-03-22T09:53:26Z","receivedAt":"2012-03-22T09:53:26Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Wed, Mar 21, 2012 at 12:36 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>\n> ---\n>  t/t7800-difftool.sh |   38 ++++++++++++++++++++++++++++++++++++++\n>  1 file changed, 38 insertions(+)\n>\n> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\n> index 663247c..fca49d1 100755\n> --- a/t/t7800-difftool.sh\n> +++ b/t/t7800-difftool.sh\n> @@ -305,4 +305,42 @@ test_expect_success PERL 'difftool --tool-help' '\n>        echo \"$tool_help\" | stdin_contains tool\n>  '\n>\n> +test_expect_success PERL 'setup change in subdirectory' '\n> +       git checkout master &&\n> +       mkdir sub &&\n> +       echo master >sub/sub &&\n> +       git add sub/sub &&\n> +       git commit -m \"added sub/sub\" &&\n> +       echo test >>file &&\n> +       echo test >>sub/sub &&\n> +       git add . &&\n> +       git commit -m \"modified both\"\n> +'\n> +\n> +test_expect_success PERL 'difftool -d' '\n> +       diff=$(git difftool -d --extcmd ls branch) &&\n> +       echo \"$diff\" | stdin_contains sub &&\n> +       echo \"$diff\" | stdin_contains file\n> +'\n> +\n> +test_expect_success PERL 'difftool --dir-diff' '\n> +       diff=$(git difftool --dir-diff --extcmd ls branch) &&\n> +       echo \"$diff\" | stdin_contains sub &&\n> +       echo \"$diff\" | stdin_contains file\n> +'\n> +\n> +test_expect_success PERL 'difftool --dir-diff ignores --prompt' '\n> +       diff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&\n> +       echo \"$diff\" | stdin_contains sub &&\n> +       echo \"$diff\" | stdin_contains file\n> +'\n> +\n> +test_expect_success PERL 'difftool --dir-diff from subdirectory' '\n> +       cd sub &&\n> +       diff=$(git difftool --dir-diff --extcmd ls branch) &&\n> +       echo \"$diff\" | stdin_contains sub &&\n> +       echo \"$diff\" | stdin_contains file &&\n> +       cd ..\n\nIf we wrap the subdir operations in parentheses then the sub-shell\nsaves us from having to do \"cd ..\".  It also helps prevent leakage\nfrom earlier failures, which is helpful when writing new tests.\n\n(Please excuse any gmail whitespace mangling)\n\ne.g.:\n\ntest_expect_success PERL 'difftool --dir-diff from subdirectory' '\n       (\n              cd sub &&\n              diff=$(git difftool --dir-diff --extcmd ls branch) &&\n              echo \"$diff\" | stdin_contains sub &&\n              echo \"$diff\" | stdin_contains file\n       )\n'\n\nIt'd also be pretty neat if we could gather file content somehow.\nThis would allow us to make assertions about the content of the\nleft/right parameters passed to the tool.  I don't know if that's test\noverkill ;-) but it seems like it could be helpful.\n-- \nDavid\n"},{"id":"187467","messageId":"CAFouetjqzAwCe3mHkpJB4Xod4rJEF9OD77ch906TGUd9+-KZWA@mail.gmail.com","threadId":"30018","inReplyTo":"871uoljbe9.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-22T13:51:14Z","receivedAt":"2012-03-22T13:51:14Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Thu, Mar 22, 2012 at 4:19 AM, Thomas Rast <trast@student.ethz.ch> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Tim Henigan <tim.henigan@gmail.com> writes:\n>>\n>> I really do not like the direction in which this series is going.  We do\n>> not have a similar --no-gui option to defeat --gui option that may appear\n>> earlier on the command line, but when we fix that bug (isn't it a bug?),\n>> we would have to teach this loop about that option, wouldn't we?\n\nOf course, Thomas is correct and the limitation was with my use of\nGetopt::Long rather than the module itself.\n\nv5 of the patch will use Getopt to parse the '--[no-]prompt' options\nagain.  I will also add a new patch to the series to implement\n'--no-gui'.\n\n\n> Limitations?  You can basically steal code from git-send-email.  As an\n> example:\n\nThank you for the example...I should have found this when I read the\ndocumentation earlier.\n"},{"id":"187468","messageId":"CAFouetiDZYAx0MyU310Dxke4bS36D171tzazmKJ-sb4YPR12-Q@mail.gmail.com","threadId":"30018","inReplyTo":"CAJDDKr7m3Ur-RNHffkW9rvbT6kpXWjKMG+Kiw5ausQw5058LHw@mail.gmail.com","subject":"Re: [PATCH v3 12/9] t7800: add tests for difftool --dir-diff","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-22T13:55:54Z","receivedAt":"2012-03-22T13:55:54Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Thu, Mar 22, 2012 at 5:53 AM, David Aguilar <davvid@gmail.com> wrote:\n> On Wed, Mar 21, 2012 at 12:36 PM, Tim Henigan <tim.henigan@gmail.com> wrote:\n>\n> If we wrap the subdir operations in parentheses then the sub-shell\n> saves us from having to do \"cd ..\".  It also helps prevent leakage\n> from earlier failures, which is helpful when writing new tests.\n\nI will update the patch to include this change.  Thank you for the example.\n\n\n> It'd also be pretty neat if we could gather file content somehow.\n> This would allow us to make assertions about the content of the\n> left/right parameters passed to the tool.  I don't know if that's test\n> overkill ;-) but it seems like it could be helpful.\n\nThis sounds like a good change for some future patch ;)\n"},{"id":"187476","messageId":"7v7gyczi7n.fsf@alter.siamese.dyndns.org","threadId":"30018","inReplyTo":"871uoljbe9.fsf@thomas.inf.ethz.ch","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T16:57:48Z","receivedAt":"2012-03-22T16:57:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thomas Rast <trast@student.ethz.ch> writes:\n\n> Limitations?  You can basically steal code from git-send-email.\n\nThanks ;-)\n"},{"id":"187477","messageId":"7v3990zi5h.fsf@alter.siamese.dyndns.org","threadId":"30018","inReplyTo":"CAFouetjqzAwCe3mHkpJB4Xod4rJEF9OD77ch906TGUd9+-KZWA@mail.gmail.com","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T16:59:06Z","receivedAt":"2012-03-22T16:59:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> v5 of the patch will use Getopt to parse the '--[no-]prompt' options\n> again.  I will also add a new patch to the series to implement\n> '--no-gui'.\n\nSensible.  At that point, it should be rolled into an updated [1/9] so\nthat Getopt::Long is used correctly from the get-go, I think.\n"},{"id":"187499","messageId":"CAFouetj+myKuUqw3-SwnbZ-=Sey29q9xYdaW4noHp_ebkmrk-A@mail.gmail.com","threadId":"30018","inReplyTo":"7v3990zi5h.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Tim Henigan","fromEmail":"tim.henigan@gmail.com","sentAt":"2012-03-22T19:01:12Z","receivedAt":"2012-03-22T19:01:12Z","isPatch":true,"sender":{"key":"tim.henigan@gmail.com","avatar":"https://avatars.githubusercontent.com/u/42022?v=4"},"body":"On Thu, Mar 22, 2012 at 12:59 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Tim Henigan <tim.henigan@gmail.com> writes:\n>\n>> v5 of the patch will use Getopt to parse the '--[no-]prompt' options\n>> again.  I will also add a new patch to the series to implement\n>> '--no-gui'.\n>\n> Sensible.  At that point, it should be rolled into an updated [1/9] so\n> that Getopt::Long is used correctly from the get-go, I think.\n\nThat sounds reasonable.  I will re-roll the series with the following changes:\n  - Squash \"fix regression in '--prompt' options\" into the first patch\n  - Squash \"teach dir-diff to copy modified files back\" into the\nimplementation of --dir-diff\n  - Squash \"add test for difftool --tool-help\" into implementation of\n--tool-help\n  - Squash \"add tests for difftool --dir-diff\" into the implementation\nof --dir-diff\n  - Reorder so that \"add '--no-gui' as an option\" is patch #2\n(immediately after change to Getopts::Long)\n\nThis should make the series look cleaner and also insure that new\nfeatures and their tests are added in the same commit.\n\nI will resend the entire patch series (will be v6) when this is done.\n\nDoes this sound like a good plan?\n"},{"id":"187503","messageId":"7vsjh0wism.fsf@alter.siamese.dyndns.org","threadId":"30018","inReplyTo":"CAFouetj+myKuUqw3-SwnbZ-=Sey29q9xYdaW4noHp_ebkmrk-A@mail.gmail.com","subject":"Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-22T19:13:29Z","receivedAt":"2012-03-22T19:13:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Tim Henigan <tim.henigan@gmail.com> writes:\n\n> I will resend the entire patch series (will be v6) when this is done.\n>\n> Does this sound like a good plan?\n\nAbsolutely.  Thanks for everything.\n"}]}