{"thread":{"id":"18225","subject":"[PATCH] contrib/difftool: use a separate config namespace for difftool commands","startedAt":"2009-03-09T09:12:36Z","lastAt":"2009-03-18T10:53:31Z","messageCount":7,"participants":["David Aguilar","Jay Soffian","Markus Heidelberg","Junio C Hamano","Wincent Colaiuta"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"107440","messageId":"1236589956-13486-1-git-send-email-davvid@gmail.com","threadId":"18225","inReplyTo":null,"subject":"[PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-03-09T09:12:36Z","receivedAt":"2009-03-09T09:12:36Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Some users have different mergetool and difftool settings, so teach\ndifftool to read config vars from the difftool.* namespace. This allows\nhaving distinct configurations for the diff and merge scenarios.\n\nWe don't want to force existing users to set new values for no reason\nso difftool falls back to existing mergetool config variables when the\ndifftool equivalents are not defined.\n\nSigned-off-by: David Aguilar \n---\n\nThis patch was motivated by an upset user telling me that\n\"difftool is wasting screen real-estate by launching a 3-way diff\".\nAfter looking into it I realized that they needed to be able to\nsupport a diff.tool that is separate from git-mergetool's merge.tool.\n\nThis patch brought up a few questions:\n\n\tCustom difftool commands use $LOCAL and $REMOTE just like\nmergetool but in a diff there really is no $LOCAL or $REMOTE.\nAt best there's $PRE and $POST, but even that breaks down when\nviewing reverse diffs. So really there's just $A and $B.\nI felt that keeping difftool and mergetool consistent was the\nbest choice since users are already familiar with $LOCAL and $REMOTE.\n\n\tA lot of the difftool code traces its lineage to the\nmergetool command and thus the code uses variables such as\n$merge_tool when talking about diff tools. Renaming all of the\nvariables seemed pointless and would also have made reviewing\nthis patch much more difficult so I chose to leave the variables\nnamed as-is. If someone is really bothered by the variable naming\nthen I can send a second patch that renames things, but doing so\nbasically results in a complete rewrite of difftool-helper and\nthe documentation with no real added benefit.\n\n\tWhy does difftool fallback to mergetool when choosing\ndefaults? Answer: backwards compatibility, respect existing users,\nand be friendly to lazy users that don't feel like configuring a\nseparate config variable just for difftool when they've already\nconfigured a merge.tool that works in both situations as-is\n(e.g. any of the built-in tools such as xxdiff, vimdiff, etc.)\n\n contrib/difftool/git-difftool | 6 +++---\n contrib/difftool/git-difftool-helper | 19 ++++++++++++++-----\n contrib/difftool/git-difftool.txt | 30 +++++++++++++++---------------\n 3 files changed, 32 insertions(+), 23 deletions(-)\n\ndiff --git a/contrib/difftool/git-difftool b/contrib/difftool/git-difftool\nindex 0cda3d2..0deda3a 100755\n--- a/contrib/difftool/git-difftool\n+++ b/contrib/difftool/git-difftool\n@@ -4,7 +4,7 @@\n # This is a wrapper around the GIT_EXTERNAL_DIFF-compatible\n # git-difftool-helper script. This script exports\n # GIT_EXTERNAL_DIFF and GIT_PAGER for use by git, and\n-# GIT_DIFFTOOL_NO_PROMPT and GIT_MERGE_TOOL for use by git-difftool-helper.\n+# GIT_DIFFTOOL_NO_PROMPT and GIT_DIFF_TOOL for use by git-difftool-helper.\n # Any arguments that are unknown to this script are forwarded to 'git diff'.\n \n use strict;\n@@ -49,12 +49,12 @@ sub generate_command\n \t\t}\n \t\tif ($arg eq '-t' or $arg eq '--tool') {\n \t\t\tusage() if $#ARGV <= $idx;\n-\t\t\t$ENV{GIT_MERGE_TOOL} = $ARGV[$idx + 1];\n+\t\t\t$ENV{GIT_DIFF_TOOL} = $ARGV[$idx + 1];\n \t\t\t$skip_next = 1;\n \t\t\tnext;\n \t\t}\n \t\tif ($arg =~ /^--tool=/) {\n-\t\t\t$ENV{GIT_MERGE_TOOL} = substr($arg, 7);\n+\t\t\t$ENV{GIT_DIFF_TOOL} = substr($arg, 7);\n \t\t\tnext;\n \t\t}\n \t\tif ($arg eq '--no-prompt') {\ndiff --git a/contrib/difftool/git-difftool-helper b/contrib/difftool/git-difftool-helper\nindex db3af6a..9c0a134 100755\n--- a/contrib/difftool/git-difftool-helper\n+++ b/contrib/difftool/git-difftool-helper\n@@ -128,8 +128,10 @@ launch_merge_tool () {\n \tcleanup_temp_files\n }\n \n-# Verifies that mergetool..cmd exists\n+# Verifies that (difftool|mergetool)..cmd exists\n valid_custom_tool() {\n+\tmerge_tool_cmd=\"$(git config difftool.$1.cmd)\"\n+\ttest -z \"$merge_tool_cmd\" &&\n \tmerge_tool_cmd=\"$(git config mergetool.$1.cmd)\"\n \ttest -n \"$merge_tool_cmd\"\n }\n@@ -150,8 +152,11 @@ valid_tool() {\n }\n \n # Sets up the merge_tool_path variable.\n-# This handles the mergetool..path configuration.\n+# This handles the difftool..path configuration.\n+# This also falls back to mergetool defaults.\n init_merge_tool_path() {\n+\tmerge_tool_path=$(git config difftool.\"$1\".path)\n+\ttest -z \"$merge_tool_path\" &&\n \tmerge_tool_path=$(git config mergetool.\"$1\".path)\n \tif test -z \"$merge_tool_path\"; then\n \t\tcase \"$1\" in\n@@ -165,15 +170,19 @@ init_merge_tool_path() {\n \tfi\n }\n \n-# Allow the GIT_MERGE_TOOL variable to provide a default value\n+# Allow GIT_DIFF_TOOL and GIT_MERGE_TOOL to provide default values\n test -n \"$GIT_MERGE_TOOL\" && merge_tool=\"$GIT_MERGE_TOOL\"\n+test -n \"$GIT_DIFF_TOOL\" && merge_tool=\"$GIT_DIFF_TOOL\"\n \n-# If not merge tool was specified then use the merge.tool\n+# If merge tool was not specified then use the diff.tool\n # configuration variable. If that's invalid then reset merge_tool.\n+# Fallback to merge.tool.\n if test -z \"$merge_tool\"; then\n+\tmerge_tool=$(git config diff.tool)\n+\ttest -z \"$merge_tool\" &&\n \tmerge_tool=$(git config merge.tool)\n \tif test -n \"$merge_tool\" && ! valid_tool \"$merge_tool\"; then\n-\t\techo >&2 \"git config option merge.tool set to unknown tool: $merge_tool\"\n+\t\techo >&2 \"git config option diff.tool set to unknown tool: $merge_tool\"\n \t\techo >&2 \"Resetting to default...\"\n \t\tunset merge_tool\n \tfi\ndiff --git a/contrib/difftool/git-difftool.txt b/contrib/difftool/git-difftool.txt\nindex 6e2610c..2b7bc03 100644\n--- a/contrib/difftool/git-difftool.txt\n+++ b/contrib/difftool/git-difftool.txt\n@@ -32,23 +32,23 @@ OPTIONS\n \tvimdiff, gvimdiff, ecmerge, and opendiff\n +\n If a merge resolution program is not specified, 'git-difftool'\n-will use the configuration variable `merge.tool`. If the\n-configuration variable `merge.tool` is not set, 'git difftool'\n+will use the configuration variable `diff.tool`. If the\n+configuration variable `diff.tool` is not set, 'git-difftool'\n will pick a suitable default.\n +\n You can explicitly provide a full path to the tool by setting the\n-configuration variable `mergetool..path`. For example, you\n+configuration variable `difftool..path`. For example, you\n can configure the absolute path to kdiff3 by setting\n-`mergetool.kdiff3.path`. Otherwise, 'git-difftool' assumes the\n+`difftool.kdiff3.path`. Otherwise, 'git-difftool' assumes the\n tool is available in PATH.\n +\n Instead of running one of the known merge tool programs,\n 'git-difftool' can be customized to run an alternative program\n by specifying the command line to invoke in a configuration\n-variable `mergetool..cmd`.\n+variable `difftool..cmd`.\n +\n When 'git-difftool' is invoked with this tool (either through the\n-`-t` or `--tool` option or the `merge.tool` configuration variable)\n+`-t` or `--tool` option or the `diff.tool` configuration variable)\n the configured command line will be invoked with the following\n variables available: `$LOCAL` is set to the name of the temporary\n file containing the contents of the diff pre-image and `$REMOTE`\n@@ -61,24 +61,24 @@ with custom merge tool commands and has the same value as `$LOCAL`.\n \n CONFIG VARIABLES\n ----------------\n-merge.tool::\n-\tThe default merge tool to use.\n-+\n-See the `--tool=` option above for more details.\n+'git-difftool' falls back to 'git-mergetool' config variables when the\n+difftool equivalents have not been defined.\n \n-merge.keepBackup::\n-\tThe original, unedited file content can be saved to a file with\n-\ta `.orig` extension. Defaults to `true` (i.e. keep the backup files).\n+diff.tool::\n+\tThe default merge tool to use.\n \n-mergetool..path::\n+difftool..path::\n \tOverride the path for the given tool. This is useful in case\n \tyour tool is not in the PATH.\n \n-mergetool..cmd::\n+difftool..cmd::\n \tSpecify the command to invoke the specified merge tool.\n +\n See the `--tool=` option above for more details.\n \n+merge.keepBackup::\n+\tThe original, unedited file content can be saved to a file with\n+\ta `.orig` extension. Defaults to `true` (i.e. keep the backup files).\n \n SEE ALSO\n --------\n-- \n1.6.2.77.g8cc3f\n"},{"id":"107469","messageId":"76718490903090852se7fc756m818f5d8ba49278b5@mail.gmail.com","threadId":"18225","inReplyTo":"1236589956-13486-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"Jay Soffian","fromEmail":"jaysoffian@gmail.com","sentAt":"2009-03-09T15:52:50Z","receivedAt":"2009-03-09T15:52:50Z","isPatch":true,"sender":{"key":"jaysoffian@gmail.com","avatar":"https://avatars.githubusercontent.com/u/155970?v=4"},"body":"On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar wrote:\n>  contrib/difftool/git-difftool        |    6 +++---\n\nAside, (for Junio I guess...), what's the reason this command is in\ncontrib, and by what criteria might it graduate to being installed\nwith the rest of the git commands?\n\nj.\n"},{"id":"107545","messageId":"20090310070122.GB4523@gmail.com","threadId":"18225","inReplyTo":"76718490903090852se7fc756m818f5d8ba49278b5@mail.gmail.com","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-03-10T07:01:24Z","receivedAt":"2009-03-10T07:01:24Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On 0, Jay Soffian wrote:\n> On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar wrote:\n> >  contrib/difftool/git-difftool        |    6 +++---\n> \n> Aside, (for Junio I guess...), what's the reason this command is in\n> contrib, and by what criteria might it graduate to being installed\n> with the rest of the git commands?\n> \n> j.\n\nMy thoughts (also for Junio, I guess..):\n\nIf y'all feel that it can live with the rest of the git\ncommands then that would be great =)\n\nI'm definitely willing to help maintain difftool.\nIt's a low-maintance script and making it easily available\nmeans more testing which is good. I should probably look\nat the regression tests in place for mergetool and\nsee if I can start actually testing difftool using a\nsimilar strategy.\n\n-- \n\n\tDavid\n"},{"id":"108277","messageId":"200903172054.46063.markus.heidelberg@web.de","threadId":"18225","inReplyTo":"20090310070122.GB4523@gmail.com","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"Markus Heidelberg","fromEmail":"markus.heidelberg@web.de","sentAt":"2009-03-17T19:54:45Z","receivedAt":"2009-03-17T19:54:45Z","isPatch":true,"sender":{"key":"markus.heidelberg@web.de","avatar":"https://avatars.githubusercontent.com/u/6334512?v=4"},"body":"David Aguilar, 10.03.2009:\n> On 0, Jay Soffian wrote:\n> > On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar wrote:\n> > >  contrib/difftool/git-difftool        |    6 +++---\n> > \n> > Aside, (for Junio I guess...), what's the reason this command is in\n> > contrib, and by what criteria might it graduate to being installed\n> > with the rest of the git commands?\n> > \n> > j.\n> \n> My thoughts (also for Junio, I guess..):\n> \n> If y'all feel that it can live with the rest of the git\n> commands then that would be great =)\n\nI'd like to see it as a general git tool, too.\nMaybe it can even share some common functionality with git-mergetool.\n\nMarkus\n"},{"id":"108294","messageId":"7veiwvpvhp.fsf@gitster.siamese.dyndns.org","threadId":"18225","inReplyTo":"200903172054.46063.markus.heidelberg@web.de","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-03-17T23:42:10Z","receivedAt":"2009-03-17T23:42:10Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Markus Heidelberg writes:\n\n> David Aguilar, 10.03.2009:\n>> On 0, Jay Soffian wrote:\n>> > On Mon, Mar 9, 2009 at 5:12 AM, David Aguilar wrote:\n>> > >  contrib/difftool/git-difftool        |    6 +++---\n>> > \n>> > Aside, (for Junio I guess...), what's the reason this command is in\n>> > contrib, and by what criteria might it graduate to being installed\n>> > with the rest of the git commands?\n>> > \n>> > j.\n>> \n>> My thoughts (also for Junio, I guess..):\n>> \n>> If y'all feel that it can live with the rest of the git\n>> commands then that would be great =)\n>\n> I'd like to see it as a general git tool, too.\n> Maybe it can even share some common functionality with git-mergetool.\n\nThe code was copied and pasted very heavily, and I think (IIRC) the author\nwas a bit too ashamed to have it outside contrib/ before it is properly\nrefactored or something like that. Which I happen to agree with, by the\nway.\n"},{"id":"108328","messageId":"20090318043543.GB13653@gmail.com","threadId":"18225","inReplyTo":"7veiwvpvhp.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2009-03-18T04:35:44Z","receivedAt":"2009-03-18T04:35:44Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"Junio C Hamano wrote:\n> Markus Heidelberg writes:\n> \n> > Jay Soffian wrote:\n> > > \n> > > Aside, (for Junio I guess...), what's the reason this command is in\n> > > contrib, and by what criteria might it graduate to being installed\n> > > with the rest of the git commands?\n> > > \n> > > j.\n> >\n> > I'd like to see it as a general git tool, too.\n> > Maybe it can even share some common functionality with git-mergetool.\n> \n> The code was copied and pasted very heavily, and I think (IIRC) the author\n> was a bit too ashamed to have it outside contrib/ before it is properly\n> refactored or something like that. Which I happen to agree with, by the\n> way.\n\nI'll work on some patches to get the ball rolling on this.\n\n\nHere's what I see as the steps I would take:\n\n1. move difftool into the root, update Makefile, etc.\n\n2. factor out the similarities between merge/difftool and\nput them in maybe git-tool-lib.sh?\n\nSuggestions/places to look for examples are highly appreciated.\nI know we have git-sh-setup but this isn't nearly as generic.\n\n3. adjust merge/difftool to use the common functions\n\nI remember someone mentioning that mergetool should use\nhard tabs instead of mixing tabs+spaces so a patch\nto fix that up would make sense in there somewhere as well\nassuming that's in line with the list's sensibilities.\n\n\n-- \n\n\tDavid\n"},{"id":"108365","messageId":"EBA0E851-C259-4CA7-BF59-302300C0C3DC@wincent.com","threadId":"18225","inReplyTo":"20090318043543.GB13653@gmail.com","subject":"Re: [PATCH] contrib/difftool: use a separate config namespace for difftool commands","fromName":"Wincent Colaiuta","fromEmail":"win@wincent.com","sentAt":"2009-03-18T10:53:31Z","receivedAt":"2009-03-18T10:53:31Z","isPatch":true,"sender":{"key":"greg@hurrell.net","avatar":"https://avatars.githubusercontent.com/u/7074?v=4"},"body":"El 18/3/2009, a las 5:35, David Aguilar escribió:\n\n> Junio C Hamano wrote:\n>> Markus Heidelberg writes:\n>>\n>>> Jay Soffian wrote:\n>>>>\n>>>> Aside, (for Junio I guess...), what's the reason this command is in\n>>>> contrib, and by what criteria might it graduate to being installed\n>>>> with the rest of the git commands?\n>>>>\n>>>> j.\n>>>\n>>> I'd like to see it as a general git tool, too.\n>>> Maybe it can even share some common functionality with git- \n>>> mergetool.\n>>\n>> The code was copied and pasted very heavily, and I think (IIRC) the \n>> author\n>> was a bit too ashamed to have it outside contrib/ before it is \n>> properly\n>> refactored or something like that. Which I happen to agree with, \n>> by the\n>> way.\n>\n> I'll work on some patches to get the ball rolling on this.\n>\n>\n> Here's what I see as the steps I would take:\n>\n> 1. move difftool into the root, update Makefile, etc.\n>\n> 2. factor out the similarities between merge/difftool and\n> put them in maybe git-tool-lib.sh?\n\nGiven that git-difftool shares basically all the same options as \"git \ndiff\", I think a good long-term plan would be looking at adding the \"-- \ntool\" option to \"git diff\" itself so that users wouldn't have to learn \na new subcommand, just a new option.\n\n(Whether this is done by rolling the functionality of \"git-difftool\" \ninto \"git diff\" itself, or delegating to a separate \"git-difftool\" \ncommand doesn't matter so much, now that most commands are neatly \ntucked away in $(git --exec-path) now.)\n\nCheers,\nWincent\n"}]}