[PATCH] contrib/difftool: use a separate config namespace for difftool commands

[PATCH] contrib/difftool: use a separate config namespace for difftool commands

[David Aguilar]

Some users have different mergetool and difftool settings, so teach
difftool to read config vars from the difftool.* namespace.  This allows
having distinct configurations for the diff and merge scenarios.

We don't want to force existing users to set new values for no reason
so difftool falls back to existing mergetool config variables when the
difftool equivalents are not defined.

Signed-off-by: David Aguilar <davvid@gmail.com>
---

This patch was motivated by an upset user telling me that
"difftool is wasting screen real-estate by launching a 3-way diff".
After looking into it I realized that they needed to be able to
support a diff.tool that is separate from git-mergetool's merge.tool.

This patch brought up a few questions:

	Custom difftool commands use $LOCAL and $REMOTE just like
mergetool but in a diff there really is no $LOCAL or $REMOTE.
At best there's $PRE and $POST, but even that breaks down when
viewing reverse diffs. So really there's just $A and $B.
I felt that keeping difftool and mergetool consistent was the
best choice since users are already familiar with $LOCAL and $REMOTE.

	A lot of the difftool code traces its lineage to the
mergetool command and thus the code uses variables such as
$merge_tool when talking about diff tools.  Renaming all of the
variables seemed pointless and would also have made reviewing
this patch much more difficult so I chose to leave the variables
named as-is.  If someone is really bothered by the variable naming
then I can send a second patch that renames things, but doing so
basically results in a complete rewrite of difftool-helper and
the documentation with no real added benefit.

	Why does difftool fallback to mergetool when choosing
defaults?  Answer: backwards compatibility, respect existing users,
and be friendly to lazy users that don't feel like configuring a
separate config variable just for difftool when they've already
configured a merge.tool that works in both situations as-is
(e.g. any of the built-in tools such as xxdiff, vimdiff, etc.)

 contrib/difftool/git-difftool        |    6 +++---
 contrib/difftool/git-difftool-helper |   19 ++++++++++++++-----
 contrib/difftool/git-difftool.txt    |   30 +++++++++++++++---------------
 3 files changed, 32 insertions(+), 23 deletions(-)

diff --git a/contrib/difftool/git-difftool b/contrib/difftool/git-difftool
index 0cda3d2..0deda3a 100755
--- a/contrib/difftool/git-difftool
+++ b/contrib/difftool/git-difftool
@@ -4,7 +4,7 @@
 # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible
 # git-difftool-helper script.  This script exports
 # GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and
-# GIT_DIFFTOOL_NO_PROMPT and GIT_MERGE_TOOL for use by git-difftool-helper.
+# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool-helper.
 # Any arguments that are unknown to this script are forwarded to 'git diff'.
 
 use strict;
@@ -49,12 +49,12 @@ sub generate_command
 		}
 		if ($arg eq '-t' or $arg eq '--tool') {
 			usage() if $#ARGV <= $idx;
-			$ENV{GIT_MERGE_TOOL} = $ARGV[$idx + 1];
+			$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];
 			$skip_next = 1;
 			next;
 		}
 		if ($arg =~ /^--tool=/) {
-			$ENV{GIT_MERGE_TOOL} = substr($arg, 7);
+			$ENV{GIT_DIFF_TOOL} = substr($arg, 7);
 			next;
 		}
 		if ($arg eq '--no-prompt') {
diff --git a/contrib/difftool/git-difftool-helper b/contrib/difftool/git-difftool-helper
index db3af6a..9c0a134 100755
--- a/contrib/difftool/git-difftool-helper
+++ b/contrib/difftool/git-difftool-helper
@@ -128,8 +128,10 @@ launch_merge_tool () {
 	cleanup_temp_files
 }
 
-# Verifies that mergetool.<tool>.cmd exists
+# Verifies that (difftool|mergetool).<tool>.cmd exists
 valid_custom_tool() {
+	merge_tool_cmd="$(git config difftool.$1.cmd)"
+	test -z "$merge_tool_cmd" &&
 	merge_tool_cmd="$(git config mergetool.$1.cmd)"
 	test -n "$merge_tool_cmd"
 }
@@ -150,8 +152,11 @@ valid_tool() {
 }
 
 # Sets up the merge_tool_path variable.
-# This handles the mergetool.<tool>.path configuration.
+# This handles the difftool.<tool>.path configuration.
+# This also falls back to mergetool defaults.
 init_merge_tool_path() {
+	merge_tool_path=$(git config difftool."$1".path)
+	test -z "$merge_tool_path" &&
 	merge_tool_path=$(git config mergetool."$1".path)
 	if test -z "$merge_tool_path"; then
 		case "$1" in
@@ -165,15 +170,19 @@ init_merge_tool_path() {
 	fi
 }
 
-# Allow the GIT_MERGE_TOOL variable to provide a default value
+# Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values
 test -n "$GIT_MERGE_TOOL" && merge_tool="$GIT_MERGE_TOOL"
+test -n "$GIT_DIFF_TOOL" && merge_tool="$GIT_DIFF_TOOL"
 
-# If not merge tool was specified then use the merge.tool
+# If merge tool was not specified then use the diff.tool
 # configuration variable.  If that's invalid then reset merge_tool.
+# Fallback to merge.tool.
 if test -z "$merge_tool"; then
+	merge_tool=$(git config diff.tool)
+	test -z "$merge_tool" &&
 	merge_tool=$(git config merge.tool)
 	if test -n "$merge_tool" && ! valid_tool "$merge_tool"; then
-		echo >&2 "git config option merge.tool set to unknown tool: $merge_tool"
+		echo >&2 "git config option diff.tool set to unknown tool: $merge_tool"
 		echo >&2 "Resetting to default..."
 		unset merge_tool
 	fi
diff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt
index 6e2610c..2b7bc03 100644
--- a/contrib/difftool/git-difftool.txt
+++ b/contrib/difftool/git-difftool.txt
@@ -32,23 +32,23 @@ OPTIONS
 	vimdiff, gvimdiff, ecmerge, and opendiff
 +
 If a merge resolution program is not specified, 'git-difftool'
-will use the configuration variable `merge.tool`.  If the
-configuration variable `merge.tool` is not set, 'git difftool'
+will use the configuration variable `diff.tool`.  If the
+configuration variable `diff.tool` is not set, 'git-difftool'
 will pick a suitable default.
 +
 You can explicitly provide a full path to the tool by setting the
-configuration variable `mergetool.<tool>.path`. For example, you
+configuration variable `difftool.<tool>.path`. For example, you
 can configure the absolute path to kdiff3 by setting
-`mergetool.kdiff3.path`. Otherwise, 'git-difftool' assumes the
+`difftool.kdiff3.path`. Otherwise, 'git-difftool' assumes the
 tool is available in PATH.
 +
 Instead of running one of the known merge tool programs,
 'git-difftool' can be customized to run an alternative program
 by specifying the command line to invoke in a configuration
-variable `mergetool.<tool>.cmd`.
+variable `difftool.<tool>.cmd`.
 +
 When 'git-difftool' is invoked with this tool (either through the
-`-t` or `--tool` option or the `merge.tool` configuration variable)
+`-t` or `--tool` option or the `diff.tool` configuration variable)
 the configured command line will be invoked with the following
 variables available: `$LOCAL` is set to the name of the temporary
 file containing the contents of the diff pre-image and `$REMOTE`
@@ -61,24 +61,24 @@ with custom merge tool commands and has the same value as `$LOCAL`.
 
 CONFIG VARIABLES
 ----------------
-merge.tool::
-	The default merge tool to use.
-+
-See the `--tool=<tool>` option above for more details.
+'git-difftool' falls back to 'git-mergetool' config variables when the
+difftool equivalents have not been defined.
 
-merge.keepBackup::
-	The original, unedited file content can be saved to a file with
-	a `.orig` extension.  Defaults to `true` (i.e. keep the backup files).
+diff.tool::
+	The default merge tool to use.
 
-mergetool.<tool>.path::
+difftool.<tool>.path::
 	Override the path for the given tool.  This is useful in case
 	your tool is not in the PATH.
 
-mergetool.<tool>.cmd::
+difftool.<tool>.cmd::
 	Specify the command to invoke the specified merge tool.
 +
 See the `--tool=<tool>` option above for more details.
 
+merge.keepBackup::
+	The original, unedited file content can be saved to a file with
+	a `.orig` extension.  Defaults to `true` (i.e. keep the backup files).
 
 SEE ALSO
 --------
-- 
1.6.2.77.g8cc3f

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[Jay Soffian]

On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar <davvid@gmail.com> wrote:
>  contrib/difftool/git-difftool        |    6 +++---

Aside, (for Junio I guess...), what's the reason this command is in
contrib, and by what criteria might it graduate to being installed
with the rest of the git commands?

j.

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[David Aguilar]

On  0, Jay Soffian <jaysoffian@gmail.com> wrote:
> On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar <davvid@gmail.com> wrote:
> >  contrib/difftool/git-difftool        |    6 +++---
> 
> Aside, (for Junio I guess...), what's the reason this command is in
> contrib, and by what criteria might it graduate to being installed
> with the rest of the git commands?
> 
> j.

My thoughts (also for Junio, I guess..):

If y'all feel that it can live with the rest of the git
commands then that would be great =)

I'm definitely willing to help maintain difftool.
It's a low-maintance script and making it easily available
means more testing which is good.  I should probably look
at the regression tests in place for mergetool and
see if I can start actually testing difftool using a
similar strategy.

-- 

	David

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[Markus Heidelberg]

David Aguilar, 10.03.2009:
> On  0, Jay Soffian <jaysoffian@gmail.com> wrote:
> > On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar <davvid@gmail.com> wrote:
> > >  contrib/difftool/git-difftool        |    6 +++---
> > 
> > Aside, (for Junio I guess...), what's the reason this command is in
> > contrib, and by what criteria might it graduate to being installed
> > with the rest of the git commands?
> > 
> > j.
> 
> My thoughts (also for Junio, I guess..):
> 
> If y'all feel that it can live with the rest of the git
> commands then that would be great =)

I'd like to see it as a general git tool, too.
Maybe it can even share some common functionality with git-mergetool.

Markus

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[Junio C Hamano]

Markus Heidelberg <markus.heidelberg@web.de> writes:

> David Aguilar, 10.03.2009:
>> On  0, Jay Soffian <jaysoffian@gmail.com> wrote:
>> > On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar <davvid@gmail.com> wrote:
>> > >  contrib/difftool/git-difftool        |    6 +++---
>> > 
>> > Aside, (for Junio I guess...), what's the reason this command is in
>> > contrib, and by what criteria might it graduate to being installed
>> > with the rest of the git commands?
>> > 
>> > j.
>> 
>> My thoughts (also for Junio, I guess..):
>> 
>> If y'all feel that it can live with the rest of the git
>> commands then that would be great =)
>
> I'd like to see it as a general git tool, too.
> Maybe it can even share some common functionality with git-mergetool.

The code was copied and pasted very heavily, and I think (IIRC) the author
was a bit too ashamed to have it outside contrib/ before it is properly
refactored or something like that.  Which I happen to agree with, by the
way.

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[David Aguilar]

Junio C Hamano <gitster@pobox.com> wrote:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
> 
> > Jay Soffian <jaysoffian@gmail.com> wrote:
> > > 
> > > Aside, (for Junio I guess...), what's the reason this command is in
> > > contrib, and by what criteria might it graduate to being installed
> > > with the rest of the git commands?
> > > 
> > > j.
> >
> > I'd like to see it as a general git tool, too.
> > Maybe it can even share some common functionality with git-mergetool.
> 
> The code was copied and pasted very heavily, and I think (IIRC) the author
> was a bit too ashamed to have it outside contrib/ before it is properly
> refactored or something like that.  Which I happen to agree with, by the
> way.

I'll work on some patches to get the ball rolling on this.


Here's what I see as the steps I would take:

1. move difftool into the root, update Makefile, etc.

2. factor out the similarities between merge/difftool and
put them in maybe git-tool-lib.sh?

Suggestions/places to look for examples are highly appreciated.
I know we have git-sh-setup but this isn't nearly as generic.

3. adjust merge/difftool to use the common functions

I remember someone mentioning that mergetool should use
hard tabs instead of mixing tabs+spaces so a patch
to fix that up would make sense in there somewhere as well
assuming that's in line with the list's sensibilities.


-- 

	David

Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands

[Wincent Colaiuta]

El 18/3/2009, a las 5:35, David Aguilar escribió:

> Junio C Hamano <gitster@pobox.com> wrote:
>> Markus Heidelberg <markus.heidelberg@web.de> writes:
>>
>>> Jay Soffian <jaysoffian@gmail.com> wrote:
>>>>
>>>> Aside, (for Junio I guess...), what's the reason this command is in
>>>> contrib, and by what criteria might it graduate to being installed
>>>> with the rest of the git commands?
>>>>
>>>> j.
>>>
>>> I'd like to see it as a general git tool, too.
>>> Maybe it can even share some common functionality with git- 
>>> mergetool.
>>
>> The code was copied and pasted very heavily, and I think (IIRC) the  
>> author
>> was a bit too ashamed to have it outside contrib/ before it is  
>> properly
>> refactored or something like that.  Which I happen to agree with,  
>> by the
>> way.
>
> I'll work on some patches to get the ball rolling on this.
>
>
> Here's what I see as the steps I would take:
>
> 1. move difftool into the root, update Makefile, etc.
>
> 2. factor out the similarities between merge/difftool and
> put them in maybe git-tool-lib.sh?

Given that git-difftool shares basically all the same options as "git  
diff", I think a good long-term plan would be looking at adding the "-- 
tool" option to "git diff" itself so that users wouldn't have to learn  
a new subcommand, just a new option.

(Whether this is done by rolling the functionality of "git-difftool"  
into "git diff" itself, or delegating to a separate "git-difftool"  
command doesn't matter so much, now that most commands are neatly  
tucked away in $(git --exec-path) now.)

Cheers,
Wincent