threads / patch / 22230

patch, 3 partsdifftool: Add '-x' and as an alias for '--extcmd'

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

## tl;dr

9 messages between Jan 15, 2010 and Jan 16, 2010. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

David Aguilar· Jan 15, 2010, 07:16 UTC · lore

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

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(-)
Show changes to t/t7800-difftool.sh +5 −8
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· Jan 15, 2010, 07:16 UTC · re: David Aguilar · lore

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(-)
Show changes to 3 files +24 −8

Documentation/git-difftool.txt, git-difftool.perl, t/t7800-difftool.sh

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
Junio C Hamano· Jan 15, 2010, 19:46 UTC · re: David Aguilar · lore

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

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· Jan 15, 2010, 19:54 UTC · re: Junio C Hamano · lore

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

On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:
Show 12 quoted lines
>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· Jan 16, 2010, 01:05 UTC · re: Bill Lear · lore

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

Bill Lear <rael@zopyra.com> writes:
Show 15 quoted lines
> 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· Jan 16, 2010, 13:44 UTC · re: Junio C Hamano · lore

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

On Friday, January 15, 2010 at 17:05:17 (-0800) Junio C Hamano writes:
Show 32 quoted lines
>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
David Aguilar· Jan 15, 2010, 07:16 UTC · re: David Aguilar · lore

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

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(-)
Show changes to 2 files +14 −2

git-difftool--helper.sh, t/t7800-difftool.sh

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· Jan 15, 2010, 08:40 UTC · re: David Aguilar · lore

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

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· Jan 15, 2010, 17:59 UTC · re: Johannes Sixt · lore

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

On Fri, Jan 15, 2010 at 09:40:44AM +0100, Johannes Sixt wrote:
Show 10 quoted lines
> 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

← back to recent threads