{"thread":{"id":"22230","subject":"[PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","startedAt":"2010-01-15T07:16:00Z","lastAt":"2010-01-16T13:44:18Z","messageCount":9,"participants":["David Aguilar","Johannes Sixt","Junio C Hamano","Bill Lear"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"131705","messageId":"1263539762-8269-1-git-send-email-davvid@gmail.com","threadId":"22230","inReplyTo":null,"subject":"[PATCH 1/3] t7800-difftool.sh: Simplify the --extcmd test","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-01-15T07:16:00Z","receivedAt":"2010-01-15T07:16:00Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Instead of running 'grep', 'echo', and 'wc' we simply compare\ngit-difftool's output against a known good value.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n t/t7800-difftool.sh |   13 +++++--------\n 1 files changed, 5 insertions(+), 8 deletions(-)\n\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 8ee186a..1d9e07b 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -15,6 +15,9 @@ if ! test_have_prereq PERL; then\n \ttest_done\n fi\n \n+LF='\n+'\n+\n remove_config_vars()\n {\n \t# Unset all config variables used by git-difftool\n@@ -219,19 +222,13 @@ test_expect_success 'difftool.<tool>.path' '\n \trestore_test_defaults\n '\n \n-test_expect_success 'difftool --extcmd=...' '\n+test_expect_success 'difftool --extcmd=cat' '\n \tdiff=$(git difftool --no-prompt --extcmd=cat branch) &&\n+\ttest \"$diff\" = branch\"$LF\"master\n \n-\tlines=$(echo \"$diff\" | wc -l) &&\n-\ttest \"$lines\" -eq 2 &&\n \n-\tlines=$(echo \"$diff\" | grep master | wc -l) &&\n-\ttest \"$lines\" -eq 1 &&\n \n-\tlines=$(echo \"$diff\" | grep branch | wc -l) &&\n-\ttest \"$lines\" -eq 1 &&\n \n-\trestore_test_defaults\n '\n \n test_done\n-- \n1.6.6.6.g627fb.dirty\n"},{"id":"131704","messageId":"1263539762-8269-2-git-send-email-davvid@gmail.com","threadId":"22230","inReplyTo":"1263539762-8269-1-git-send-email-davvid@gmail.com","subject":"[PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-01-15T07:16:01Z","receivedAt":"2010-01-15T07:16:01Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"This adds '-x' as a shorthand for the '--extcmd' option.\nArguments to '--extcmd' can be specified separately, which\nwas not originally possible.\n\nThis also fixes the brief help text so that it mentions\nboth '-x' and '--extcmd'.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n Documentation/git-difftool.txt |    3 ++-\n git-difftool.perl              |   21 ++++++++++++++-------\n t/t7800-difftool.sh            |    8 ++++++++\n 3 files changed, 24 insertions(+), 8 deletions(-)\n\ndiff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt\nindex f67d2db..5c68cff 100644\n--- a/Documentation/git-difftool.txt\n+++ b/Documentation/git-difftool.txt\n@@ -7,7 +7,7 @@ git-difftool - Show changes using common diff tools\n \n SYNOPSIS\n --------\n-'git difftool' [--tool=<tool>] [-y|--no-prompt|--prompt] [<'git diff' options>]\n+'git difftool' [<options>] <commit>{0,2} [--] [<path>...]\n \n DESCRIPTION\n -----------\n@@ -58,6 +58,7 @@ is set to the name of the temporary file containing the contents\n of the diff post-image.  `$BASE` is provided for compatibility\n with custom merge tool commands and has the same value as `$LOCAL`.\n \n+-x <command>::\n --extcmd=<command>::\n \tSpecify a custom command for viewing diffs.\n \t'git-difftool' ignores the configured defaults and runs\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex f8ff245..d639de3 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -1,5 +1,5 @@\n #!/usr/bin/env perl\n-# Copyright (c) 2009 David Aguilar\n+# Copyright (c) 2009-2010 David Aguilar\n #\n # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible\n # git-difftool--helper script.\n@@ -23,8 +23,9 @@ my $DIR = abs_path(dirname($0));\n sub usage\n {\n \tprint << 'USAGE';\n-usage: git difftool [-g|--gui] [-t|--tool=<tool>] [-y|--no-prompt]\n-                    [\"git diff\" options]\n+usage: git difftool [-t|--tool=<tool>] [-x|--extcmd=<cmd>]\n+                    [-y|--no-prompt]   [-g|--gui]\n+                    ['git diff' options]\n USAGE\n \texit 1;\n }\n@@ -62,14 +63,20 @@ sub generate_command\n \t\t\t$skip_next = 1;\n \t\t\tnext;\n \t\t}\n-\t\tif ($arg =~ /^--extcmd=/) {\n-\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);\n-\t\t\tnext;\n-\t\t}\n \t\tif ($arg =~ /^--tool=/) {\n \t\t\t$ENV{GIT_DIFF_TOOL} = substr($arg, 7);\n \t\t\tnext;\n \t\t}\n+\t\tif ($arg eq '-x' || $arg eq '--extcmd') {\n+\t\t\tusage() if $#ARGV <= $idx;\n+\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = $ARGV[$idx + 1];\n+\t\t\t$skip_next = 1;\n+\t\t\tnext;\n+\t\t}\n+\t\tif ($arg =~ /^--extcmd=/) {\n+\t\t\t$ENV{GIT_DIFFTOOL_EXTCMD} = substr($arg, 9);\n+\t\t\tnext;\n+\t\t}\n \t\tif ($arg eq '-g' || $arg eq '--gui') {\n \t\t\tmy $tool = Git::command_oneline('config',\n \t\t\t                                'diff.guitool');\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 1d9e07b..69e1c34 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -225,8 +225,16 @@ test_expect_success 'difftool.<tool>.path' '\n test_expect_success 'difftool --extcmd=cat' '\n \tdiff=$(git difftool --no-prompt --extcmd=cat branch) &&\n \ttest \"$diff\" = branch\"$LF\"master\n+'\n \n+test_expect_success 'difftool --extcmd cat' '\n+\tdiff=$(git difftool --no-prompt --extcmd cat branch) &&\n+\ttest \"$diff\" = branch\"$LF\"master\n+'\n \n+test_expect_success 'difftool -x cat' '\n+\tdiff=$(git difftool --no-prompt -x cat branch) &&\n+\ttest \"$diff\" = branch\"$LF\"master\n \n \n '\n-- \n1.6.6.6.g627fb.dirty\n"},{"id":"131706","messageId":"1263539762-8269-3-git-send-email-davvid@gmail.com","threadId":"22230","inReplyTo":"1263539762-8269-1-git-send-email-davvid@gmail.com","subject":"[PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-01-15T07:16:02Z","receivedAt":"2010-01-15T07:16:02Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"It was not possible to pass quoted commands to '--extcmd'.\nBy using 'eval' we ensure that expressions with spaces and\nquotes are supported.\n\nSigned-off-by: David Aguilar <davvid@gmail.com>\n---\n git-difftool--helper.sh |    3 +--\n t/t7800-difftool.sh     |   13 +++++++++++++\n 2 files changed, 14 insertions(+), 2 deletions(-)\n\ndiff --git a/git-difftool--helper.sh b/git-difftool--helper.sh\nindex d806eae..a1c5c09 100755\n--- a/git-difftool--helper.sh\n+++ b/git-difftool--helper.sh\n@@ -48,11 +48,10 @@ launch_merge_tool () {\n \tfi\n \n \tif use_ext_cmd; then\n-\t\t$GIT_DIFFTOOL_EXTCMD \"$LOCAL\" \"$REMOTE\"\n+\t\t(eval $GIT_DIFFTOOL_EXTCMD \"\\\"$LOCAL\\\"\" \"\\\"$REMOTE\\\"\")\n \telse\n \t\trun_merge_tool \"$merge_tool\"\n \tfi\n-\n }\n \n if ! use_ext_cmd; then\ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 69e1c34..a183f1d 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -235,8 +235,21 @@ test_expect_success 'difftool --extcmd cat' '\n test_expect_success 'difftool -x cat' '\n \tdiff=$(git difftool --no-prompt -x cat branch) &&\n \ttest \"$diff\" = branch\"$LF\"master\n+'\n+\n+test_expect_success 'difftool --extcmd echo arg1' '\n+\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"echo\\ \\$1\\\" branch)\n+\ttest \"$diff\" = file\n+'\n \n+test_expect_success 'difftool --extcmd cat arg1' '\n+\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"cat\\ \\$1\\\" branch)\n+\ttest \"$diff\" = master\n+'\n \n+test_expect_success 'difftool --extcmd cat arg2' '\n+\tdiff=$(git difftool --no-prompt --extcmd sh\\ -c\\ \\\"cat\\ \\$2\\\" branch)\n+\ttest \"$diff\" = branch\n '\n \n test_done\n-- \n1.6.6.6.g627fb.dirty\n"},{"id":"131711","messageId":"4B502A0C.50108@viscovery.net","threadId":"22230","inReplyTo":"1263539762-8269-3-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2010-01-15T08:40:44Z","receivedAt":"2010-01-15T08:40:44Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"David Aguilar schrieb:\n> -\t\t$GIT_DIFFTOOL_EXTCMD \"$LOCAL\" \"$REMOTE\"\n> +\t\t(eval $GIT_DIFFTOOL_EXTCMD \"\\\"$LOCAL\\\"\" \"\\\"$REMOTE\\\"\")\n\nThe new code is broken if $LOCAL or $REMOTE can contain double-quotes. How\nabout this alternative:\n\n\t\teval $GIT_DIFFTOOL_EXTCMD '\"$LOCAL\"' '\"$REMOTE\"'\n\nwhich I find more readable as well.\n\nWhat's the reason for the sub-shell? Do you want to protect from shell\ncode in $GIT_DIFFTOOL_EXTCMD that modifies difftool's variables?\n\n-- Hannes\n"},{"id":"131739","messageId":"20100115175913.GA21106@gmail.com","threadId":"22230","inReplyTo":"4B502A0C.50108@viscovery.net","subject":"Re: [PATCH 3/3] difftool: Use eval to expand '--extcmd' expressions","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2010-01-15T17:59:15Z","receivedAt":"2010-01-15T17:59:15Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Jan 15, 2010 at 09:40:44AM +0100, Johannes Sixt wrote:\n> David Aguilar schrieb:\n> > -\t\t$GIT_DIFFTOOL_EXTCMD \"$LOCAL\" \"$REMOTE\"\n> > +\t\t(eval $GIT_DIFFTOOL_EXTCMD \"\\\"$LOCAL\\\"\" \"\\\"$REMOTE\\\"\")\n> \n> The new code is broken if $LOCAL or $REMOTE can contain double-quotes. How\n> about this alternative:\n> \n> \t\teval $GIT_DIFFTOOL_EXTCMD '\"$LOCAL\"' '\"$REMOTE\"'\n> \n> which I find more readable as well.\n\nI'll resend a patch later today (can't quite right now, but will\nhave time later).\n\n> What's the reason for the sub-shell? Do you want to protect from shell\n> code in $GIT_DIFFTOOL_EXTCMD that modifies difftool's variables?\n> \n> -- Hannes\n\nNone, really, so we can do without that as well.\n\nThanks for your notes,\n\n-- \n\t\tDavid\n"},{"id":"131755","messageId":"7vvdf3xiav.fsf@alter.siamese.dyndns.org","threadId":"22230","inReplyTo":"1263539762-8269-2-git-send-email-davvid@gmail.com","subject":"Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-15T19:46:32Z","receivedAt":"2010-01-15T19:46:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar <davvid@gmail.com> writes:\n\n> -# Copyright (c) 2009 David Aguilar\n> +# Copyright (c) 2009-2010 David Aguilar\n\nJust a very minor issue.  I'd prefer to see:\n\n\tCopyright (c) 2008, 2009, 2010\n\nover\n\n\tCopyright (c) 2008-2010\n\nCopyright lawyers may say parenthesized-c does not have any legal meaning\nand must be circle-c, but I am not a lawyer.\n"},{"id":"131757","messageId":"19280.51182.981853.561841@blake.zopyra.com","threadId":"22230","inReplyTo":"7vvdf3xiav.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-01-15T19:54:22Z","receivedAt":"2010-01-15T19:54:22Z","isPatch":true,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:\n>David Aguilar <davvid@gmail.com> writes:\n>\n>> -# Copyright (c) 2009 David Aguilar\n>> +# Copyright (c) 2009-2010 David Aguilar\n>\n>Just a very minor issue.  I'd prefer to see:\n>\n>\tCopyright (c) 2008, 2009, 2010\n>\n>over\n>\n>\tCopyright (c) 2008-2010\n\nWhy?\n\n\nBill\n"},{"id":"131833","messageId":"7vbpguoo4y.fsf@alter.siamese.dyndns.org","threadId":"22230","inReplyTo":"19280.51182.981853.561841@blake.zopyra.com","subject":"Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-01-16T01:05:17Z","receivedAt":"2010-01-16T01:05:17Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Bill Lear <rael@zopyra.com> writes:\n\n> On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:\n>>David Aguilar <davvid@gmail.com> writes:\n>>\n>>> -# Copyright (c) 2009 David Aguilar\n>>> +# Copyright (c) 2009-2010 David Aguilar\n>>\n>>Just a very minor issue.  I'd prefer to see:\n>>\n>>\tCopyright (c) 2008, 2009, 2010\n>>\n>>over\n>>\n>>\tCopyright (c) 2008-2010\n>\n> Why?\n\nI learned this from <http://www.gnu.org/licenses/gpl-howto.html>.  The\nadvice doesn't say _why_, but my understanding of the rationale behind it\nis that the international convention that governs this copyright\nnotice specifically mentions \"the year of publication\", not \"range of\nyears\" (UCC Geneva text, Sept. 06, 1952, Article III 1.).\n\nBerne convention does not require such a copyright notice, and many\ncountries are signatories of both treaties, so the whole copyright notice\nmay be a moot point in many countries, but it matters in some.  As long as\nthe file (and the GNU advice cited above) is being cautious by having the\nnotice, it would be better to be equally cautious and spell the years of\npublication out.\n\nBut I am not a lawyer.\n"},{"id":"131879","messageId":"19281.49842.4522.770104@blake.zopyra.com","threadId":"22230","inReplyTo":"7vbpguoo4y.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/3] difftool: Add '-x' and as an alias for '--extcmd'","fromName":"Bill Lear","fromEmail":"rael@zopyra.com","sentAt":"2010-01-16T13:44:18Z","receivedAt":"2010-01-16T13:44:18Z","isPatch":true,"sender":{"key":"rael@zopyra.com","avatar":"https://gravatar.com/avatar/c4f2d2790ca3828d3b4e7dfebabf61d2fe94fd82fa49cdac2a5295dd2d46a874?d=mp&s=160"},"body":"On Friday, January 15, 2010 at 17:05:17 (-0800) Junio C Hamano writes:\n>Bill Lear <rael@zopyra.com> writes:\n>\n>> On Friday, January 15, 2010 at 11:46:32 (-0800) Junio C Hamano writes:\n>>>David Aguilar <davvid@gmail.com> writes:\n>>>\n>>>> -# Copyright (c) 2009 David Aguilar\n>>>> +# Copyright (c) 2009-2010 David Aguilar\n>>>\n>>>Just a very minor issue.  I'd prefer to see:\n>>>\n>>>\tCopyright (c) 2008, 2009, 2010\n>>>\n>>>over\n>>>\n>>>\tCopyright (c) 2008-2010\n>>\n>> Why?\n>\n>I learned this from <http://www.gnu.org/licenses/gpl-howto.html>.  The\n>advice doesn't say _why_, but my understanding of the rationale behind it\n>is that the international convention that governs this copyright\n>notice specifically mentions \"the year of publication\", not \"range of\n>years\" (UCC Geneva text, Sept. 06, 1952, Article III 1.).\n>\n>Berne convention does not require such a copyright notice, and many\n>countries are signatories of both treaties, so the whole copyright notice\n>may be a moot point in many countries, but it matters in some.  As long as\n>the file (and the GNU advice cited above) is being cautious by having the\n>notice, it would be better to be equally cautious and spell the years of\n>publication out.\n>\n>But I am not a lawyer.\n\nHmm, interesting.  My wife is a lawyer, maybe I'll sic her on this.\nMy guess is that if this came to court, a judge would laugh at anyone\nwho tried to assert a meaningful difference.\n\nGood to know though, thank you for taking the time to explain this.\n\n\nBill\n"}]}