[PATCH v3 0/9] difftool: teach command to perform directory diffs

[PATCH v3 0/9] difftool: teach command to perform directory diffs

[Tim Henigan]

Changes in v3:
  - Removed unused shell variable (SUBDIRECTORY_OK) from patch 7.
  - Changed patches 7 and 8 to support calling 'difftool --dir-diff' from
    a subdirectory of the repo.
  - Changed patch 8 to support 
  - Added patch 10, which makes mixing --prompt and --no-prompt and error.
  - Added tests for new difftool options in patches 11 and 12.

  - Patches 1-6 and 9 did not change and have not been resent.
  

Series Overview:

'git difftool' is a very useful command that allows git diffs to be opened
in an external tool. Currently, difftool opens a separate instance of the
external tool for each file that changed.  This can be tedious when many
files have changed.

This series teaches difftool to perform directory diffs, so that all file
changes can be opened/reviewed in a single instance of the external tool.

This is the second phase of development for this feature.  The first phase
was added as a separate command (git diffall) in 1252bbe (contrib: add
git-diffall script).  During review of that script on the Git developers
list, an informal development roadmap was suggested [1]. The next phase
of that plan is to integrate the 'git-diffall' feature into 'difftool'.
This series gets that done.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/191297/focus=191383


Tim Henigan (12):
  difftool: parse options using Getopt::Long
  difftool: exit(0) when usage is printed
  difftool: remove explicit change of PATH
  difftool: stop appending '.exe' to git
  difftool: eliminate setup_environment function
  difftool: replace system call with Git::command_noisy
  difftool: teach difftool to handle directory diffs
  difftool: teach dir-diff to copy modified files back to working tree
  difftool: print list of valid tools with '--tool-help'
  difftool: do not allow mix of '--prompt' with '--no-prompt'
  t7800: add test for difftool --tool-help
  t7800: add tests for difftool --dir-diff

 Documentation/git-difftool.txt |   17 ++-
 git-difftool--helper.sh        |   19 ++-
 git-difftool.perl              |  264 +++++++++++++++++++++++++++-------------
 t/t7800-difftool.sh            |   59 +++++++--
 4 files changed, 254 insertions(+), 105 deletions(-)

-- 
1.7.10.rc1.39.g6e141f

[PATCH v3 7/9] difftool: teach difftool to handle directory diffs

[Tim Henigan]

Prior to this commit, the difftool utility could only open files one
at a time.  The new '--dir-diff' option copies all the modified files
to a temporary location and runs a directory diff on them.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This replaces 7/9 in v2 of the series.

Changed in v3:
  - Removed SUBDIRECTORY_OK from git-difftool--helper.sh. Since git-sh-setup
    is not sourced, this variable is not used.
  - $ENV{GIT_DIR} must be explicitly set if 'difftool --dir-diff' is called
    from a subdirectory in the repo.
  - Changed from 'Git::command' to 'system' when calling 'git checkout-index'.
    Without this change, 'difftool --dir-diff' fails when called from a
    subdirectory in the repo.


 Documentation/git-difftool.txt |    6 ++
 git-difftool--helper.sh        |   19 ++++--
 git-difftool.perl              |  145 ++++++++++++++++++++++++++++++++++++----
 3 files changed, 152 insertions(+), 18 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index fe38f66..aba5e76 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -19,6 +19,12 @@ linkgit:git-diff[1].
 
 OPTIONS
 -------
+-d::
+--dir-diff::
+	Copy the modified files to a temporary location and perform
+	a directory diff on them. This mode never prompts before
+	launching the diff tool.
+
 -y::
 --no-prompt::
 	Do not prompt before launching a diff tool.
diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index e6558d1..3d0fe0c 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -73,9 +73,16 @@ then
 	fi
 fi
 
-# Launch the merge tool on each path provided by 'git diff'
-while test $# -gt 6
-do
-	launch_merge_tool "$1" "$2" "$5"
-	shift 7
-done
+if test -n "$GIT_DIFFTOOL_DIRDIFF"
+then
+	LOCAL="$1"
+	REMOTE="$2"
+	run_merge_tool "$merge_tool" false
+else
+	# Launch the merge tool on each path provided by 'git diff'
+	while test $# -gt 6
+	do
+		launch_merge_tool "$1" "$2" "$5"
+		shift 7
+	done
+fi
diff --git a/git-difftool.perl b/git-difftool.perl
index 8498089..082e887 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -1,18 +1,24 @@
 #!/usr/bin/env perl
 # Copyright (c) 2009, 2010 David Aguilar
+# Copyright (c) 2012 Tim Henigan
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
 # git-difftool--helper script.
 #
 # This script exports GIT_EXTERNAL_DIFF and GIT_PAGER for use by git.
-# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, and GIT_DIFF_TOOL
-# are exported for use by git-difftool--helper.
+# GIT_DIFFTOOL_NO_PROMPT, GIT_DIFFTOOL_PROMPT, GIT_DIFFTOOL_DIRDIFF,
+# and GIT_DIFF_TOOL are exported for use by git-difftool--helper.
 #
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use 5.008;
 use strict;
 use warnings;
+use File::Basename qw(dirname);
+use File::Copy;
+use File::stat;
+use File::Path qw(mkpath);
+use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
@@ -22,15 +28,110 @@ sub usage
 	print << 'USAGE';
 usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
                     [-y|--no-prompt]   [-g|--gui]
+                    [-d|--dir-diff]
                     ['git diff' options]
 USAGE
 	exit($exitcode);
 }
 
+sub setup_dir_diff
+{
+	# Run the diff; exit immediately if no diff found
+	my $repo = Git->repository();
+	my $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);
+	exit(0) if (length($diffrtn) == 0);
+
+	# Setup temp directories
+	my $tmpdir = tempdir('git-diffall.XXXXX', CLEANUP => 1, TMPDIR => 1);
+	my $ldir = "$tmpdir/left";
+	my $rdir = "$tmpdir/right";
+	mkpath($ldir) or die $!;
+	mkpath($rdir) or die $!;
+
+	# Build index info for left and right sides of the diff
+	my $submodule_mode = "160000";
+	my $null_mode = 0 x 6;
+	my $null_sha1 = 0 x 40;
+	my $lindex = "";
+	my $rindex = "";
+	my @working_tree;
+	my %submodule;
+	my @rawdiff = split('\0', $diffrtn);
+
+	for (my $i=0; $i<@rawdiff; $i+=2) {
+		my ($lmode, $rmode, $lsha1, $rsha1, $status) = split(' ', substr($rawdiff[$i], 1));
+		my $path = $rawdiff[$i + 1];
+
+		if (($lmode eq $submodule_mode) or ($rmode eq $submodule_mode)) {
+			$submodule{$path}{left} = $lsha1;
+			$submodule{$path}{right} = $rsha1;
+			next;
+		}
+
+		if ($lmode ne $null_mode) {
+			$lindex .= "$lmode $lsha1\t$path\0";
+		}
+
+		if ($rmode ne $null_mode) {
+			if ($rsha1 ne $null_sha1) {
+				$rindex .= "$rmode $rsha1\t$path\0";
+			} else {
+				push(@working_tree, $path);
+			}
+		}
+	}
+
+	# Populate the left and right directories based on each index file
+	my ($inpipe, $ctx);
+	$ENV{GIT_DIR} = $repo->repo_path();
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/lindex";
+	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	print($inpipe $lindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+	system(('git', 'checkout-index', '--all', "--prefix=$ldir/"));
+
+	$ENV{GIT_INDEX_FILE} = "$tmpdir/rindex";
+	($inpipe, $ctx) = $repo->command_input_pipe(qw/update-index -z --index-info/);
+	print($inpipe $rindex);
+	$repo->command_close_pipe($inpipe, $ctx);
+	system(('git', 'checkout-index', '--all', "--prefix=$rdir/"));
+
+	# Changes in the working tree need special treatment since they are
+	# not part of the index
+	my $workdir = $repo->wc_path();
+	for (@working_tree) {
+		my $dir = dirname($_);
+		unless (-d "$rdir/$dir") {
+			mkpath("$rdir/$dir") or die $!;
+		}
+		copy("$workdir/$_", "$rdir/$_") or die $!;
+		chmod(stat("$workdir/$_")->mode, "$rdir/$_") or die $!;
+	}
+
+	# Changes to submodules require special treatment. This loop writes a
+	# temporary file to both the left and right directories to show the
+	# change in the recorded SHA1 for the submodule.
+	foreach my $path (keys %submodule) {
+		if (defined $submodule{$path}{left}) {
+			open(my $fh, ">", "$ldir/$path") or die $!;
+			print($fh "Subproject commit $submodule{$path}{left}");
+			close($fh);
+		}
+		if (defined $submodule{$path}{right}) {
+			open(my $fh, ">", "$rdir/$path") or die $!;
+			print($fh "Subproject commit $submodule{$path}{right}");
+			close($fh);
+		}
+	}
+
+	return ($ldir, $rdir);
+}
+
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
-my ($difftool_cmd, $extcmd, $gui, $help, $no_prompt, $prompt);
+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt);
 GetOptions('g|gui' => \$gui,
+	'd|dir-diff' => \$dirdiff,
 	'h' => \$help,
 	'prompt' => \$prompt,
 	't|tool:s' => \$difftool_cmd,
@@ -63,13 +164,33 @@ if (defined($gui)) {
 		$ENV{GIT_DIFF_TOOL} = $guitool;
 	}
 }
-if (defined($prompt)) {
-	$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-}
-elsif (defined($no_prompt)) {
-	$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
-}
 
-$ENV{GIT_PAGER} = '';
-$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+# In directory diff mode, 'git-difftool--helper' is called once
+# to compare the a/b directories.  In file diff mode, 'git diff'
+# will invoke a separate instance of 'git-difftool--helper' for
+# each file that changed.
+if (defined($dirdiff)) {
+	my ($a, $b) = setup_dir_diff();
+	if (defined($extcmd)) {
+		system(($extcmd, $a, $b));
+	} else {
+		$ENV{GIT_DIFFTOOL_DIRDIFF} = 'true';
+		git_cmd_try {
+			Git::command_noisy(('difftool--helper', $a, $b))
+		} 'exit code %d';
+	}
+	# TODO: if the diff including working copy files and those
+	# files were modified during the diff, then the changes
+	# should be copied back to the working tree
+} else {
+	if (defined($prompt)) {
+		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+	}
+	elsif (defined($no_prompt)) {
+		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+	}
+
+	$ENV{GIT_PAGER} = '';
+	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
+	git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+}
-- 
1.7.10.rc1.39.g6e141f

[PATCH v3 8/9] difftool: teach dir-diff to copy modified files back to working tree

[Tim Henigan]

If the user runs a diff when there are modified files in the working copy,
they expect to be able to be edit the files in the diff viewer and save
the changes. When using difftool in file mode (i.e. diffing one file at
a time), this works as expected.  However, when using difftool in directory
diff mode, the changes are not saved.

This commit teaches the directory diff mode to copy changes from the diff
viewer back to the working copy.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This replaces patch 8/9 in v2 of the series.

Changed in v3
  - Replaced calls to '$repo->wc_path()' with '$repo->repo_path() . "/.."'
    so that paths are properly handled when 'difftool --dir-diff' is
    called from a subdirectory of the repo.


 git-difftool.perl |   14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 082e887..9201b89 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,8 @@ use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
+my @working_tree;
+
 sub usage
 {
 	my $exitcode = shift;
@@ -54,7 +56,6 @@ sub setup_dir_diff
 	my $null_sha1 = 0 x 40;
 	my $lindex = "";
 	my $rindex = "";
-	my @working_tree;
 	my %submodule;
 	my @rawdiff = split('\0', $diffrtn);
 
@@ -98,7 +99,7 @@ sub setup_dir_diff
 
 	# Changes in the working tree need special treatment since they are
 	# not part of the index
-	my $workdir = $repo->wc_path();
+	my $workdir = $repo->repo_path() . "/..";
 	for (@working_tree) {
 		my $dir = dirname($_);
 		unless (-d "$rdir/$dir") {
@@ -179,9 +180,16 @@ if (defined($dirdiff)) {
 			Git::command_noisy(('difftool--helper', $a, $b))
 		} 'exit code %d';
 	}
-	# TODO: if the diff including working copy files and those
+
+	# If the diff including working copy files and those
 	# files were modified during the diff, then the changes
 	# should be copied back to the working tree
+	my $repo = Git->repository();
+	my $workdir = $repo->repo_path() . "/..";
+	for (@working_tree) {
+		copy("$b/$_", "$workdir/$_") or die $!;
+		chmod(stat("$b/$_")->mode, "$workdir/$_") or die $!;
+	}
 } else {
 	if (defined($prompt)) {
 		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-- 
1.7.10.rc1.39.g6e141f

[PATCH v3 10/9] difftool: do not allow mix of '--prompt' with '--no-prompt'

[Tim Henigan]

When difftool was changed to use Getopt::Long, it changed the way that
the '--prompt' and '--no-prompt' options are handled. Prior to the change,
if both options were given, then the last option was used.  After the
change, if both were given, then '--prompt' was always used.

This commit teaches difftool to exit with an error if both options are
given at the command line. t7800 was updated to match.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 git-difftool.perl   |    6 +++++-
 t/t7800-difftool.sh |   16 +++++-----------
 2 files changed, 10 insertions(+), 12 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 9f0f9a9..9629811 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -202,7 +202,11 @@ if (defined($dirdiff)) {
 		chmod(stat("$b/$_")->mode, "$workdir/$_") or die $!;
 	}
 } else {
-	if (defined($prompt)) {
+	if (defined($prompt) and defined($no_prompt)) {
+		print("Cannot command '--prompt' and '--no-prompt' at the same time.\n");
+		usage(1);
+	}
+	elsif (defined($prompt)) {
 		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
 	}
 	elsif (defined($no_prompt)) {
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 4fb4c93..dc181cf 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -202,17 +202,11 @@ test_expect_success PERL 'difftool.prompt can overridden with --prompt' '
 	restore_test_defaults
 '
 
-# Test that the last flag passed on the command-line wins
-test_expect_success PERL 'difftool last flag wins' '
-	diff=$(git difftool --prompt --no-prompt branch) &&
-	test "$diff" = "branch" &&
-
-	restore_test_defaults &&
-
-	prompt=$(echo | git difftool --no-prompt --prompt branch | tail -1) &&
-	prompt_given "$prompt" &&
-
-	restore_test_defaults
+# Test that an error is given when both --prompt and --no-prompt are
+# commanded
+test_expect_success PERL '--prompt / --no-prompt conflict' '
+	test_must_fail git difftool --prompt --no-prompt branch &&
+	test_must_fail git difftool --no-prompt --prompt branch
 '
 
 # git-difftool falls back to git-mergetool config variables
-- 
1.7.10.rc1.39.g6e141f

[PATCH v3 11/9] t7800: add test for difftool --tool-help

[Tim Henigan]

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 t/t7800-difftool.sh |    5 +++++
 1 file changed, 5 insertions(+)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index dc181cf..663247c 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -300,4 +300,9 @@ test_expect_success PERL 'say no to the second file' '
 	echo "$diff" | stdin_doesnot_contain br2
 '
 
+test_expect_success PERL 'difftool --tool-help' '
+	tool_help=$(git difftool --tool-help) &&
+	echo "$tool_help" | stdin_contains tool
+'
+
 test_done
-- 
1.7.10.rc1.39.g6e141f

[PATCH v3 12/9] t7800: add tests for difftool --dir-diff

[Tim Henigan]

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---
 t/t7800-difftool.sh |   38 ++++++++++++++++++++++++++++++++++++++
 1 file changed, 38 insertions(+)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 663247c..fca49d1 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -305,4 +305,42 @@ test_expect_success PERL 'difftool --tool-help' '
 	echo "$tool_help" | stdin_contains tool
 '
 
+test_expect_success PERL 'setup change in subdirectory' '
+	git checkout master &&
+	mkdir sub &&
+	echo master >sub/sub &&
+	git add sub/sub &&
+	git commit -m "added sub/sub" &&
+	echo test >>file &&
+	echo test >>sub/sub &&
+	git add . &&
+	git commit -m "modified both"
+'
+
+test_expect_success PERL 'difftool -d' '
+	diff=$(git difftool -d --extcmd ls branch) &&
+	echo "$diff" | stdin_contains sub &&
+	echo "$diff" | stdin_contains file
+'
+
+test_expect_success PERL 'difftool --dir-diff' '
+	diff=$(git difftool --dir-diff --extcmd ls branch) &&
+	echo "$diff" | stdin_contains sub &&
+	echo "$diff" | stdin_contains file
+'
+
+test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
+	diff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&
+	echo "$diff" | stdin_contains sub &&
+	echo "$diff" | stdin_contains file
+'
+
+test_expect_success PERL 'difftool --dir-diff from subdirectory' '
+	cd sub &&
+	diff=$(git difftool --dir-diff --extcmd ls branch) &&
+	echo "$diff" | stdin_contains sub &&
+	echo "$diff" | stdin_contains file &&
+	cd ..
+'
+
 test_done
-- 
1.7.10.rc1.39.g6e141f

Re: [PATCH v3 10/9] difftool: do not allow mix of '--prompt' with '--no-prompt'

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> When difftool was changed to use Getopt::Long, it changed the way that
> the '--prompt' and '--no-prompt' options are handled. Prior to the change,
> if both options were given, then the last option was used.  After the
> change, if both were given, then '--prompt' was always used.
>
> This commit teaches difftool to exit with an error if both options are
> given at the command line. t7800 was updated to match.

Hrm, people with "[alias] mdt = difftool --prompt" have every right to
expect their

	$ git mdt --no-prompt

to keep working, no?

[PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Tim Henigan]

When difftool was changed to use Getopt::Long, it changed the way
that the '--prompt' and '--no-prompt' options were handled. The
expected behavior is that the two options may be given any number
of times. The last option given "wins".

For example, if a user sets "[alias] mdt = difftool --prompt", the
following must still run without error:

$ git mdt --no-prompt

The changes made during the switch to Getopt::Long broke this
behavior. This commit teaches difftool to handle them properly
again.

Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
---

This replaces 10/9 in v3 of the series.

Changes in v4:
  - '--prompt' and '--no-prompt' may be used together again.
  - Removed changes made to t7800.  The old test now passes.


 git-difftool.perl |   32 ++++++++++++++++++++++----------
 1 file changed, 22 insertions(+), 10 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 9f0f9a9..41c6557 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -22,6 +22,7 @@ use File::Temp qw(tempdir);
 use Getopt::Long qw(:config pass_through);
 use Git;
 
+my @diffargs;
 my @working_tree;
 
 sub usage
@@ -41,7 +42,7 @@ sub setup_dir_diff
 {
 	# Run the diff; exit immediately if no diff found
 	my $repo = Git->repository();
-	my $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @ARGV]);
+	my $diffrtn = $repo->command_oneline(['diff', '--raw', '--no-abbrev', '-z', @diffargs]);
 	exit(0) if (length($diffrtn) == 0);
 
 	# Setup temp directories
@@ -131,15 +132,25 @@ sub setup_dir_diff
 
 # parse command-line options. all unrecognized options and arguments
 # are passed through to the 'git diff' command.
-my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $no_prompt, $prompt, $tool_help);
+my ($difftool_cmd, $dirdiff, $extcmd, $gui, $help, $prompt, $tool_help);
 GetOptions('g|gui' => \$gui,
 	'd|dir-diff' => \$dirdiff,
 	'h' => \$help,
-	'prompt' => \$prompt,
 	't|tool:s' => \$difftool_cmd,
 	'tool-help' => \$tool_help,
-	'x|extcmd:s' => \$extcmd,
-	'y|no-prompt' => \$no_prompt);
+	'x|extcmd:s' => \$extcmd);
+
+# the '--prompt' and '--no-prompt' options require special treatment
+# because they may be specified more than once...the last one "wins".
+for (@ARGV) {
+	if (($_ eq "-y") or ($_ eq "--no-prompt")) {
+		$prompt = 0;
+	} elsif ($_ eq "--prompt") {
+		$prompt = 1;
+	} else {
+		push(@diffargs, $_);
+	}
+}
 
 if (defined($help)) {
 	usage(0);
@@ -203,13 +214,14 @@ if (defined($dirdiff)) {
 	}
 } else {
 	if (defined($prompt)) {
-		$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
-	}
-	elsif (defined($no_prompt)) {
-		$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+		if ($prompt) {
+			$ENV{GIT_DIFFTOOL_PROMPT} = 'true';
+		} else {
+			$ENV{GIT_DIFFTOOL_NO_PROMPT} = 'true';
+		}
 	}
 
 	$ENV{GIT_PAGER} = '';
 	$ENV{GIT_EXTERNAL_DIFF} = 'git-difftool--helper';
-	git_cmd_try { Git::command_noisy(('diff', @ARGV)) } 'exit code %d';
+	git_cmd_try { Git::command_noisy(('diff', @diffargs)) } 'exit code %d';
 }
-- 
1.7.10.rc1.39.gdeb0e

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> +# the '--prompt' and '--no-prompt' options require special treatment
> +# because they may be specified more than once...the last one "wins".
> +for (@ARGV) {
> +	if (($_ eq "-y") or ($_ eq "--no-prompt")) {
> +		$prompt = 0;
> +	} elsif ($_ eq "--prompt") {
> +		$prompt = 1;
> +	} else {
> +		push(@diffargs, $_);
> +	}
> +}

I really do not like the direction in which this series is going.  We do
not have a similar --no-gui option to defeat --gui option that may appear
earlier on the command line, but when we fix that bug (isn't it a bug?),
we would have to teach this loop about that option, wouldn't we?

In the end, won't you end up resurrecting the argument parsing loop that
you got rid of with the first patch in your series?  Isn't this working
around the problem introduced only because you are using Getopt::Long and
hitting its limitations?

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Thomas Rast]

Junio C Hamano <gitster@pobox.com> writes:

> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> +# the '--prompt' and '--no-prompt' options require special treatment
>> +# because they may be specified more than once...the last one "wins".
>> +for (@ARGV) {
>> +	if (($_ eq "-y") or ($_ eq "--no-prompt")) {
>> +		$prompt = 0;
>> +	} elsif ($_ eq "--prompt") {
>> +		$prompt = 1;
>> +	} else {
>> +		push(@diffargs, $_);
>> +	}
>> +}
>
> I really do not like the direction in which this series is going.  We do
> not have a similar --no-gui option to defeat --gui option that may appear
> earlier on the command line, but when we fix that bug (isn't it a bug?),
> we would have to teach this loop about that option, wouldn't we?
>
> In the end, won't you end up resurrecting the argument parsing loop that
> you got rid of with the first patch in your series?  Isn't this working
> around the problem introduced only because you are using Getopt::Long and
> hitting its limitations?

Limitations?  You can basically steal code from git-send-email.  As an
example:

---- 8< ----
#!/usr/bin/perl

use strict;
use warnings;
use Getopt::Long;

my $foo = 0;
my $rc = GetOptions("foo!" => \$foo,
		    "n" => sub { $foo = 0; },
		    "y" => \$foo);

print "$foo\n";
---- 8< ----

$ ./getopt-test.perl -n
0
$ ./getopt-test.perl -y
1
$ ./getopt-test.perl --foo
1
$ ./getopt-test.perl --no-foo
0
$ ./getopt-test.perl --foo --no-foo
0
$ ./getopt-test.perl -y -n
0
$ ./getopt-test.perl --foo -n
0

--
Thomas Rast
trast@{inf,student}.ethz.ch

Re: [PATCH v3 12/9] t7800: add tests for difftool --dir-diff

[David Aguilar]

On Wed, Mar 21, 2012 at 12:36 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> Signed-off-by: Tim Henigan <tim.henigan@gmail.com>
> ---
>  t/t7800-difftool.sh |   38 ++++++++++++++++++++++++++++++++++++++
>  1 file changed, 38 insertions(+)
>
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 663247c..fca49d1 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -305,4 +305,42 @@ test_expect_success PERL 'difftool --tool-help' '
>        echo "$tool_help" | stdin_contains tool
>  '
>
> +test_expect_success PERL 'setup change in subdirectory' '
> +       git checkout master &&
> +       mkdir sub &&
> +       echo master >sub/sub &&
> +       git add sub/sub &&
> +       git commit -m "added sub/sub" &&
> +       echo test >>file &&
> +       echo test >>sub/sub &&
> +       git add . &&
> +       git commit -m "modified both"
> +'
> +
> +test_expect_success PERL 'difftool -d' '
> +       diff=$(git difftool -d --extcmd ls branch) &&
> +       echo "$diff" | stdin_contains sub &&
> +       echo "$diff" | stdin_contains file
> +'
> +
> +test_expect_success PERL 'difftool --dir-diff' '
> +       diff=$(git difftool --dir-diff --extcmd ls branch) &&
> +       echo "$diff" | stdin_contains sub &&
> +       echo "$diff" | stdin_contains file
> +'
> +
> +test_expect_success PERL 'difftool --dir-diff ignores --prompt' '
> +       diff=$(git difftool --dir-diff --prompt --extcmd ls branch) &&
> +       echo "$diff" | stdin_contains sub &&
> +       echo "$diff" | stdin_contains file
> +'
> +
> +test_expect_success PERL 'difftool --dir-diff from subdirectory' '
> +       cd sub &&
> +       diff=$(git difftool --dir-diff --extcmd ls branch) &&
> +       echo "$diff" | stdin_contains sub &&
> +       echo "$diff" | stdin_contains file &&
> +       cd ..

If we wrap the subdir operations in parentheses then the sub-shell
saves us from having to do "cd ..".  It also helps prevent leakage
from earlier failures, which is helpful when writing new tests.

(Please excuse any gmail whitespace mangling)

e.g.:

test_expect_success PERL 'difftool --dir-diff from subdirectory' '
       (
              cd sub &&
              diff=$(git difftool --dir-diff --extcmd ls branch) &&
              echo "$diff" | stdin_contains sub &&
              echo "$diff" | stdin_contains file
       )
'

It'd also be pretty neat if we could gather file content somehow.
This would allow us to make assertions about the content of the
left/right parameters passed to the tool.  I don't know if that's test
overkill ;-) but it seems like it could be helpful.
-- 
David

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Tim Henigan]

On Thu, Mar 22, 2012 at 4:19 AM, Thomas Rast <trast@student.ethz.ch> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Tim Henigan <tim.henigan@gmail.com> writes:
>>
>> I really do not like the direction in which this series is going.  We do
>> not have a similar --no-gui option to defeat --gui option that may appear
>> earlier on the command line, but when we fix that bug (isn't it a bug?),
>> we would have to teach this loop about that option, wouldn't we?

Of course, Thomas is correct and the limitation was with my use of
Getopt::Long rather than the module itself.

v5 of the patch will use Getopt to parse the '--[no-]prompt' options
again.  I will also add a new patch to the series to implement
'--no-gui'.


> Limitations?  You can basically steal code from git-send-email.  As an
> example:

Thank you for the example...I should have found this when I read the
documentation earlier.

Re: [PATCH v3 12/9] t7800: add tests for difftool --dir-diff

[Tim Henigan]

On Thu, Mar 22, 2012 at 5:53 AM, David Aguilar <davvid@gmail.com> wrote:
> On Wed, Mar 21, 2012 at 12:36 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
>
> If we wrap the subdir operations in parentheses then the sub-shell
> saves us from having to do "cd ..".  It also helps prevent leakage
> from earlier failures, which is helpful when writing new tests.

I will update the patch to include this change.  Thank you for the example.


> It'd also be pretty neat if we could gather file content somehow.
> This would allow us to make assertions about the content of the
> left/right parameters passed to the tool.  I don't know if that's test
> overkill ;-) but it seems like it could be helpful.

This sounds like a good change for some future patch ;)

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Junio C Hamano]

Thomas Rast <trast@student.ethz.ch> writes:

> Limitations?  You can basically steal code from git-send-email.

Thanks ;-)

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> v5 of the patch will use Getopt to parse the '--[no-]prompt' options
> again.  I will also add a new patch to the series to implement
> '--no-gui'.

Sensible.  At that point, it should be rolled into an updated [1/9] so
that Getopt::Long is used correctly from the get-go, I think.

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Tim Henigan]

On Thu, Mar 22, 2012 at 12:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> v5 of the patch will use Getopt to parse the '--[no-]prompt' options
>> again.  I will also add a new patch to the series to implement
>> '--no-gui'.
>
> Sensible.  At that point, it should be rolled into an updated [1/9] so
> that Getopt::Long is used correctly from the get-go, I think.

That sounds reasonable.  I will re-roll the series with the following changes:
  - Squash "fix regression in '--prompt' options" into the first patch
  - Squash "teach dir-diff to copy modified files back" into the
implementation of --dir-diff
  - Squash "add test for difftool --tool-help" into implementation of
--tool-help
  - Squash "add tests for difftool --dir-diff" into the implementation
of --dir-diff
  - Reorder so that "add '--no-gui' as an option" is patch #2
(immediately after change to Getopts::Long)

This should make the series look cleaner and also insure that new
features and their tests are added in the same commit.

I will resend the entire patch series (will be v6) when this is done.

Does this sound like a good plan?

Re: [PATCH 10/9 v4] difftool: fix regression in '--prompt' options

[Junio C Hamano]

Tim Henigan <tim.henigan@gmail.com> writes:

> I will resend the entire patch series (will be v6) when this is done.
>
> Does this sound like a good plan?

Absolutely.  Thanks for everything.