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

9 messages from 2010-01-15 to 2010-01-16. Participants: David Aguilar, Johannes Sixt, Junio C Hamano, Bill Lear.
Thread: https://gitlist.dev/t/22230

## David Aguilar, 2010-01-15 07:16

Subject: [PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test
Message-ID: <1263539762-8269-1-git-send-email-davvid@gmail.com>
URL: https://gitlist.dev/e/1263539762-8269-1-git-send-email-davvid%40gmail.com

```
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

```

## David Aguilar, 2010-01-15 07:16

Subject: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
Message-ID: <1263539762-8269-2-git-send-email-davvid@gmail.com>
URL: https://gitlist.dev/e/1263539762-8269-2-git-send-email-davvid%40gmail.com
In-Reply-To: <1263539762-8269-1-git-send-email-davvid@gmail.com>

```
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

```

## David Aguilar, 2010-01-15 07:16

Subject: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
Message-ID: <1263539762-8269-3-git-send-email-davvid@gmail.com>
URL: https://gitlist.dev/e/1263539762-8269-3-git-send-email-davvid%40gmail.com
In-Reply-To: <1263539762-8269-1-git-send-email-davvid@gmail.com>

```
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

```

## Johannes Sixt, 2010-01-15 08:40

Subject: Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
Message-ID: <4B502A0C.50108@viscovery.net>
URL: https://gitlist.dev/e/4B502A0C.50108%40viscovery.net
In-Reply-To: <1263539762-8269-3-git-send-email-davvid@gmail.com>

```
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

```

## David Aguilar, 2010-01-15 17:59

Subject: Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
Message-ID: <20100115175913.GA21106@gmail.com>
URL: https://gitlist.dev/e/20100115175913.GA21106%40gmail.com
In-Reply-To: <4B502A0C.50108@viscovery.net>

```
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

```

## Junio C Hamano, 2010-01-15 19:46

Subject: Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
Message-ID: <7vvdf3xiav.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vvdf3xiav.fsf%40alter.siamese.dyndns.org
In-Reply-To: <1263539762-8269-2-git-send-email-davvid@gmail.com>

```
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.

```

## Bill Lear, 2010-01-15 19:54

Subject: Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
Message-ID: <19280.51182.981853.561841@blake.zopyra.com>
URL: https://gitlist.dev/e/19280.51182.981853.561841%40blake.zopyra.com
In-Reply-To: <7vvdf3xiav.fsf@alter.siamese.dyndns.org>

```
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

```

## Junio C Hamano, 2010-01-16 01:05

Subject: Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
Message-ID: <7vbpguoo4y.fsf@alter.siamese.dyndns.org>
URL: https://gitlist.dev/e/7vbpguoo4y.fsf%40alter.siamese.dyndns.org
In-Reply-To: <19280.51182.981853.561841@blake.zopyra.com>

```
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.

```

## Bill Lear, 2010-01-16 13:44

Subject: Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
Message-ID: <19281.49842.4522.770104@blake.zopyra.com>
URL: https://gitlist.dev/e/19281.49842.4522.770104%40blake.zopyra.com
In-Reply-To: <7vbpguoo4y.fsf@alter.siamese.dyndns.org>

```
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

```