* [PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test
@ 2010-01-15 7:16 David Aguilar
2010-01-15 7:16 ` [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd' David Aguilar
2010-01-15 7:16 ` [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions David Aguilar
0 siblings, 2 replies; 9+ messages in thread
From: David Aguilar @ 2010-01-15 7:16 UTC (permalink / raw)
To: gitster; +Cc: git
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
2010-01-15 7:16 [PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test David Aguilar
@ 2010-01-15 7:16 ` David Aguilar
2010-01-15 19:46 ` Junio C Hamano
2010-01-15 7:16 ` [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions David Aguilar
1 sibling, 1 reply; 9+ messages in thread
From: David Aguilar @ 2010-01-15 7:16 UTC (permalink / raw)
To: gitster; +Cc: git
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
2010-01-15 7:16 ` [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd' David Aguilar
@ 2010-01-15 19:46 ` Junio C Hamano
2010-01-15 19:54 ` Bill Lear
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2010-01-15 19:46 UTC (permalink / raw)
To: David Aguilar; +Cc: gitster, git
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
2010-01-15 19:46 ` Junio C Hamano
@ 2010-01-15 19:54 ` Bill Lear
2010-01-16 1:05 ` Junio C Hamano
0 siblings, 1 reply; 9+ messages in thread
From: Bill Lear @ 2010-01-15 19:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
2010-01-15 19:54 ` Bill Lear
@ 2010-01-16 1:05 ` Junio C Hamano
2010-01-16 13:44 ` Bill Lear
0 siblings, 1 reply; 9+ messages in thread
From: Junio C Hamano @ 2010-01-16 1:05 UTC (permalink / raw)
To: Bill Lear; +Cc: David Aguilar, git
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.
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'
2010-01-16 1:05 ` Junio C Hamano
@ 2010-01-16 13:44 ` Bill Lear
0 siblings, 0 replies; 9+ messages in thread
From: Bill Lear @ 2010-01-16 13:44 UTC (permalink / raw)
To: Junio C Hamano; +Cc: David Aguilar, git
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
2010-01-15 7:16 [PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test David Aguilar
2010-01-15 7:16 ` [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd' David Aguilar
@ 2010-01-15 7:16 ` David Aguilar
2010-01-15 8:40 ` Johannes Sixt
1 sibling, 1 reply; 9+ messages in thread
From: David Aguilar @ 2010-01-15 7:16 UTC (permalink / raw)
To: gitster; +Cc: git
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
^ permalink raw reply related [flat|nested] 9+ messages in thread* Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
2010-01-15 7:16 ` [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions David Aguilar
@ 2010-01-15 8:40 ` Johannes Sixt
2010-01-15 17:59 ` David Aguilar
0 siblings, 1 reply; 9+ messages in thread
From: Johannes Sixt @ 2010-01-15 8:40 UTC (permalink / raw)
To: David Aguilar; +Cc: gitster, git
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
^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions
2010-01-15 8:40 ` Johannes Sixt
@ 2010-01-15 17:59 ` David Aguilar
0 siblings, 0 replies; 9+ messages in thread
From: David Aguilar @ 2010-01-15 17:59 UTC (permalink / raw)
To: Johannes Sixt; +Cc: gitster, git
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
^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2010-01-16 13:44 UTC | newest]
Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2010-01-15 7:16 [PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test David Aguilar
2010-01-15 7:16 ` [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd' David Aguilar
2010-01-15 19:46 ` Junio C Hamano
2010-01-15 19:54 ` Bill Lear
2010-01-16 1:05 ` Junio C Hamano
2010-01-16 13:44 ` Bill Lear
2010-01-15 7:16 ` [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions David Aguilar
2010-01-15 8:40 ` Johannes Sixt
2010-01-15 17:59 ` David Aguilar
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.