[PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test

[PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test

[David Aguilar]

Instead of running 'grep', 'echo', and 'wc' we simply compare
git-difftool's output against a known good value.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 t/t7800-difftool.sh |   13 +++++--------
 1 files changed, 5 insertions(+), 8 deletions(-)

diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 8ee186a..1d9e07b 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -15,6 +15,9 @@ if ! test_have_prereq PERL; then
 	test_done
 fi
 
+LF='
+'
+
 remove_config_vars()
 {
 	# Unset all config variables used by git-difftool
@@ -219,19 +222,13 @@ test_expect_success 'difftool.<tool>.path' '
 	restore_test_defaults
 '
 
-test_expect_success 'difftool --extcmd=...' '
+test_expect_success 'difftool --extcmd=cat' '
 	diff=$(git difftool --no-prompt --extcmd=cat branch) &&
+	test "$diff" = branch"$LF"master
 
-	lines=$(echo "$diff" | wc -l) &&
-	test "$lines" -eq 2 &&
 
-	lines=$(echo "$diff" | grep master | wc -l) &&
-	test "$lines" -eq 1 &&
 
-	lines=$(echo "$diff" | grep branch | wc -l) &&
-	test "$lines" -eq 1 &&
 
-	restore_test_defaults
 '
 
 test_done
-- 
1.6.6.6.g627fb.dirty

[PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'

[David Aguilar]

This adds '-x' as a shorthand for the '--extcmd' option.
Arguments to '--extcmd' can be specified separately, which
was not originally possible.

This also fixes the brief help text so that it mentions
both '-x' and '--extcmd'.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 Documentation/git-difftool.txt |    3 ++-
 git-difftool.perl              |   21 ++++++++++++++-------
 t/t7800-difftool.sh            |    8 ++++++++
 3 files changed, 24 insertions(+), 8 deletions(-)

diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index f67d2db..5c68cff 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -7,7 +7,7 @@ git-difftool - Show changes using common diff tools
 
 SYNOPSIS
 --------
-'git difftool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<'git diff' options>]
+'git difftool' [<options>] <commit>{0,2} [--] [<path>...]
 
 DESCRIPTION
 -----------
@@ -58,6 +58,7 @@ is set to the name of the temporary file containing the contents
 of the diff post-image.  `$BASE` is provided for compatibility
 with custom merge tool commands and has the same value as `$LOCAL`.
 
+-x <command>::
 --extcmd=<command>::
 	Specify a custom command for viewing diffs.
 	'git-difftool' ignores the configured defaults and runs
diff --git a/git-difftool.perl b/git-difftool.perl
index f8ff245..d639de3 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -1,5 +1,5 @@
 #!/usr/bin/env perl
-# Copyright (c) 2009 David Aguilar
+# Copyright (c) 2009-2010 David Aguilar
 #
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
 # git-difftool--helper script.
@@ -23,8 +23,9 @@ my $DIR = abs_path(dirname($0));
 sub usage
 {
 	print << 'USAGE';
-usage: git difftool [-g|--gui] [-t|--tool=<tool>] [-y|--no-prompt]
-                    ["git diff" options]
+usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]
+                    [-y|--no-prompt]   [-g|--gui]
+                    ['git diff' options]
 USAGE
 	exit 1;
 }
@@ -62,14 +63,20 @@ sub generate_command
 			$skip_next = 1;
 			next;
 		}
-		if ($arg =~ /^--extcmd=/) {
-			$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
-			next;
-		}
 		if ($arg =~ /^--tool=/) {
 			$ENV{GIT_DIFF_TOOL} = substr($arg, 7);
 			next;
 		}
+		if ($arg eq '-x' || $arg eq '--extcmd') {
+			usage() if $#ARGV <= $idx;
+			$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];
+			$skip_next = 1;
+			next;
+		}
+		if ($arg =~ /^--extcmd=/) {
+			$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);
+			next;
+		}
 		if ($arg eq '-g' || $arg eq '--gui') {
 			my $tool = Git::command_oneline('config',
 			                                'diff.guitool');
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 1d9e07b..69e1c34 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -225,8 +225,16 @@ test_expect_success 'difftool.<tool>.path' '
 test_expect_success 'difftool --extcmd=cat' '
 	diff=$(git difftool --no-prompt --extcmd=cat branch) &&
 	test "$diff" = branch"$LF"master
+'
 
+test_expect_success 'difftool --extcmd cat' '
+	diff=$(git difftool --no-prompt --extcmd cat branch) &&
+	test "$diff" = branch"$LF"master
+'
 
+test_expect_success 'difftool -x cat' '
+	diff=$(git difftool --no-prompt -x cat branch) &&
+	test "$diff" = branch"$LF"master
 
 
 '
-- 
1.6.6.6.g627fb.dirty

[PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions

[David Aguilar]

It was not possible to pass quoted commands to '--extcmd'.
By using 'eval' we ensure that expressions with spaces and
quotes are supported.

Signed-off-by: David Aguilar <davvid@gmail.com>
---
 git-difftool--helper.sh |    3 +--
 t/t7800-difftool.sh     |   13 +++++++++++++
 2 files changed, 14 insertions(+), 2 deletions(-)

diff --git a/git-difftool--helper.sh b/git-difftool--helper.sh
index d806eae..a1c5c09 100755
--- a/git-difftool--helper.sh
+++ b/git-difftool--helper.sh
@@ -48,11 +48,10 @@ launch_merge_tool () {
 	fi
 
 	if use_ext_cmd; then
-		$GIT_DIFFTOOL_EXTCMD "$LOCAL" "$REMOTE"
+		(eval $GIT_DIFFTOOL_EXTCMD "\"$LOCAL\"" "\"$REMOTE\"")
 	else
 		run_merge_tool "$merge_tool"
 	fi
-
 }
 
 if ! use_ext_cmd; then
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 69e1c34..a183f1d 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -235,8 +235,21 @@ test_expect_success 'difftool --extcmd cat' '
 test_expect_success 'difftool -x cat' '
 	diff=$(git difftool --no-prompt -x cat branch) &&
 	test "$diff" = branch"$LF"master
+'
+
+test_expect_success 'difftool --extcmd echo arg1' '
+	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"echo\ \$1\" branch)
+	test "$diff" = file
+'
 
+test_expect_success 'difftool --extcmd cat arg1' '
+	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$1\" branch)
+	test "$diff" = master
+'
 
+test_expect_success 'difftool --extcmd cat arg2' '
+	diff=$(git difftool --no-prompt --extcmd sh\ -c\ \"cat\ \$2\" branch)
+	test "$diff" = branch
 '
 
 test_done
-- 
1.6.6.6.g627fb.dirty

Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions

[Johannes Sixt]

David Aguilar schrieb:
> -		$GIT_DIFFTOOL_EXTCMD "$LOCAL" "$REMOTE"
> +		(eval $GIT_DIFFTOOL_EXTCMD "\"$LOCAL\"" "\"$REMOTE\"")

The new code is broken if $LOCAL or $REMOTE can contain double-quotes. How
about this alternative:

		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'

which I find more readable as well.

What's the reason for the sub-shell? Do you want to protect from shell
code in $GIT_DIFFTOOL_EXTCMD that modifies difftool's variables?

-- Hannes

Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions

[David Aguilar]

On Fri, Jan 15, 2010 at 09:40:44AM +0100, Johannes Sixt wrote:
> David Aguilar schrieb:
> > -		$GIT_DIFFTOOL_EXTCMD "$LOCAL" "$REMOTE"
> > +		(eval $GIT_DIFFTOOL_EXTCMD "\"$LOCAL\"" "\"$REMOTE\"")
> 
> The new code is broken if $LOCAL or $REMOTE can contain double-quotes. How
> about this alternative:
> 
> 		eval $GIT_DIFFTOOL_EXTCMD '"$LOCAL"' '"$REMOTE"'
> 
> which I find more readable as well.

I'll resend a patch later today (can't quite right now, but will
have time later).

> What's the reason for the sub-shell? Do you want to protect from shell
> code in $GIT_DIFFTOOL_EXTCMD that modifies difftool's variables?
> 
> -- Hannes

None, really, so we can do without that as well.

Thanks for your notes,

-- 
		David

Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'

[Junio C Hamano]

David Aguilar <davvid@gmail.com> writes:

> -# Copyright (c) 2009 David Aguilar
> +# Copyright (c) 2009-2010 David Aguilar

Just a very minor issue.  I'd prefer to see:

	Copyright (c) 2008, 2009, 2010

over

	Copyright (c) 2008-2010

Copyright lawyers may say parenthesized-c does not have any legal meaning
and must be circle-c, but I am not a lawyer.

Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'

[Bill Lear]

On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:
>David Aguilar <davvid@gmail.com> writes:
>
>> -# Copyright (c) 2009 David Aguilar
>> +# Copyright (c) 2009-2010 David Aguilar
>
>Just a very minor issue.  I'd prefer to see:
>
>	Copyright (c) 2008, 2009, 2010
>
>over
>
>	Copyright (c) 2008-2010

Why?


Bill

Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'

[Junio C Hamano]

Bill Lear <rael@zopyra.com> writes:

> On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:
>>David Aguilar <davvid@gmail.com> writes:
>>
>>> -# Copyright (c) 2009 David Aguilar
>>> +# Copyright (c) 2009-2010 David Aguilar
>>
>>Just a very minor issue.  I'd prefer to see:
>>
>>	Copyright (c) 2008, 2009, 2010
>>
>>over
>>
>>	Copyright (c) 2008-2010
>
> Why?

I learned this from <http://www.gnu.org/licenses/gpl-howto.html>.  The
advice doesn't say _why_, but my understanding of the rationale behind it
is that the international convention that governs this copyright
notice specifically mentions "the year of publication", not "range of
years" (UCC Geneva text, Sept. 06, 1952, Article III 1.).

Berne convention does not require such a copyright notice, and many
countries are signatories of both treaties, so the whole copyright notice
may be a moot point in many countries, but it matters in some.  As long as
the file (and the GNU advice cited above) is being cautious by having the
notice, it would be better to be equally cautious and spell the years of
publication out.

But I am not a lawyer.

Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'

[Bill Lear]

On Friday, January 15, 2010 at 17:05:17 (-0800) Junio C Hamano writes:
>Bill Lear <rael@zopyra.com> writes:
>
>> On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:
>>>David Aguilar <davvid@gmail.com> writes:
>>>
>>>> -# Copyright (c) 2009 David Aguilar
>>>> +# Copyright (c) 2009-2010 David Aguilar
>>>
>>>Just a very minor issue.  I'd prefer to see:
>>>
>>>	Copyright (c) 2008, 2009, 2010
>>>
>>>over
>>>
>>>	Copyright (c) 2008-2010
>>
>> Why?
>
>I learned this from <http://www.gnu.org/licenses/gpl-howto.html>.  The
>advice doesn't say _why_, but my understanding of the rationale behind it
>is that the international convention that governs this copyright
>notice specifically mentions "the year of publication", not "range of
>years" (UCC Geneva text, Sept. 06, 1952, Article III 1.).
>
>Berne convention does not require such a copyright notice, and many
>countries are signatories of both treaties, so the whole copyright notice
>may be a moot point in many countries, but it matters in some.  As long as
>the file (and the GNU advice cited above) is being cautious by having the
>notice, it would be better to be equally cautious and spell the years of
>publication out.
>
>But I am not a lawyer.

Hmm, interesting.  My wife is a lawyer, maybe I'll sic her on this.
My guess is that if this came to court, a judge would laugh at anyone
who tried to assert a meaningful difference.

Good to know though, thank you for taking the time to explain this.


Bill