{"thread":{"id":"32716","subject":"[PATCH 0/4] Fix \"git difftool --tool-help\"","startedAt":"2013-01-24T19:55:12Z","lastAt":"2013-01-25T10:47:22Z","messageCount":10,"participants":["John Keeping","Junio C Hamano","David Aguilar"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"207707","messageId":"cover.1359057056.git.john@keeping.me.uk","threadId":"32716","inReplyTo":null,"subject":"[PATCH 0/4] Fix \"git difftool --tool-help\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-24T19:55:12Z","receivedAt":"2013-01-24T19:55:12Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The \"--tool-help\" option to git-difftool currently displays incorrect\noutput since it uses the names of the files in\n\"$GIT_EXEC_PATH/mergetools/\" rather than the list of command names in\ngit-mergetool--lib.\n\nThis series fixes this by changing it to simply delegate to a function\nin git-mergetool--lib.\n\nThe first three patches are just refactorings to move the show_tool_help\nfunction from git-mergetool to git-mergetool--lib and make it usable by\ngit-difftool.  The final patch switches git-difftool to use this\nfunction.\n\n\nJohn Keeping (4):\n  git-mergetool: move show_tool_help to mergetool--lib\n  git-mergetool: remove redundant assignment\n  git-mergetool: don't hardcode 'mergetool' in show_tool_help\n  git-difftool: use git-mergetool--lib for \"--tool-help\"\n\n git-difftool.perl     | 57 ++++++++-------------------------------------------\n git-mergetool--lib.sh | 38 ++++++++++++++++++++++++++++++++++\n git-mergetool.sh      | 37 ---------------------------------\n 3 files changed, 46 insertions(+), 86 deletions(-)\n\n-- \n1.8.1\n"},{"id":"207708","messageId":"7045909ebfafde008c5f1667088d59d5f1f6c354.1359057056.git.john@keeping.me.uk","threadId":"32716","inReplyTo":"cover.1359057056.git.john@keeping.me.uk","subject":"[PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-24T19:55:13Z","receivedAt":"2013-01-24T19:55:13Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"This is the first step in unifying \"git difftool --tool-help\" and\n\"git mergetool --tool-help\".\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 37 +++++++++++++++++++++++++++++++++++++\n git-mergetool.sh      | 37 -------------------------------------\n 2 files changed, 37 insertions(+), 37 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex f013a03..89a857f 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -174,6 +174,43 @@ list_merge_tool_candidates () {\n \tesac\n }\n \n+show_tool_help () {\n+\tTOOL_MODE=merge\n+\tlist_merge_tool_candidates\n+\tunavailable= available= LF='\n+'\n+\tfor i in $tools\n+\tdo\n+\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n+\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n+\t\tthen\n+\t\t\tavailable=\"$available$i$LF\"\n+\t\telse\n+\t\t\tunavailable=\"$unavailable$i$LF\"\n+\t\tfi\n+\tdone\n+\tif test -n \"$available\"\n+\tthen\n+\t\techo \"'git mergetool --tool=<tool>' may be set to one of the following:\"\n+\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n+\telse\n+\t\techo \"No suitable tool for 'git mergetool --tool=<tool>' found.\"\n+\tfi\n+\tif test -n \"$unavailable\"\n+\tthen\n+\t\techo\n+\t\techo 'The following tools are valid, but not currently available:'\n+\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n+\tfi\n+\tif test -n \"$unavailable$available\"\n+\tthen\n+\t\techo\n+\t\techo \"Some of the tools listed above only work in a windowed\"\n+\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n+\tfi\n+\texit 0\n+}\n+\n guess_merge_tool () {\n \tlist_merge_tool_candidates\n \techo >&2 \"merge tool candidates: $tools\"\ndiff --git a/git-mergetool.sh b/git-mergetool.sh\nindex c50e18a..c0ee9aa 100755\n--- a/git-mergetool.sh\n+++ b/git-mergetool.sh\n@@ -315,43 +315,6 @@ merge_file () {\n \treturn 0\n }\n \n-show_tool_help () {\n-\tTOOL_MODE=merge\n-\tlist_merge_tool_candidates\n-\tunavailable= available= LF='\n-'\n-\tfor i in $tools\n-\tdo\n-\t\tmerge_tool_path=$(translate_merge_tool_path \"$i\")\n-\t\tif type \"$merge_tool_path\" >/dev/null 2>&1\n-\t\tthen\n-\t\t\tavailable=\"$available$i$LF\"\n-\t\telse\n-\t\t\tunavailable=\"$unavailable$i$LF\"\n-\t\tfi\n-\tdone\n-\tif test -n \"$available\"\n-\tthen\n-\t\techo \"'git mergetool --tool=<tool>' may be set to one of the following:\"\n-\t\techo \"$available\" | sort | sed -e 's/^/\t/'\n-\telse\n-\t\techo \"No suitable tool for 'git mergetool --tool=<tool>' found.\"\n-\tfi\n-\tif test -n \"$unavailable\"\n-\tthen\n-\t\techo\n-\t\techo 'The following tools are valid, but not currently available:'\n-\t\techo \"$unavailable\" | sort | sed -e 's/^/\t/'\n-\tfi\n-\tif test -n \"$unavailable$available\"\n-\tthen\n-\t\techo\n-\t\techo \"Some of the tools listed above only work in a windowed\"\n-\t\techo \"environment. If run in a terminal-only session, they will fail.\"\n-\tfi\n-\texit 0\n-}\n-\n prompt=$(git config --bool mergetool.prompt || echo true)\n \n while test $# != 0\n-- \n1.8.1\n"},{"id":"207709","messageId":"25bfcc56fca4181f9ee97360de801139ffa8ce18.1359057056.git.john@keeping.me.uk","threadId":"32716","inReplyTo":"cover.1359057056.git.john@keeping.me.uk","subject":"[PATCH 2/4] git-mergetool: remove redundant assignment","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-24T19:55:14Z","receivedAt":"2013-01-24T19:55:14Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"TOOL_MODE is set at the top of git-mergetool.sh so there is no need to\nset it again in show_tool_help.  Removing this lets us re-use\nshow_tool_help in git-difftool.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 89a857f..1748315 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -175,7 +175,6 @@ list_merge_tool_candidates () {\n }\n \n show_tool_help () {\n-\tTOOL_MODE=merge\n \tlist_merge_tool_candidates\n \tunavailable= available= LF='\n '\n-- \n1.8.1\n"},{"id":"207710","messageId":"bbf604e6903a23c5c8e7b920b38175a39564f946.1359057056.git.john@keeping.me.uk","threadId":"32716","inReplyTo":"cover.1359057056.git.john@keeping.me.uk","subject":"[PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-24T19:55:15Z","receivedAt":"2013-01-24T19:55:15Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"When using show_tool_help from git-difftool we will want it to print\n\"git difftool\" not \"git mergetool\" so use \"git ${TOOL_MODE}tool\".\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-mergetool--lib.sh | 6 ++++--\n 1 file changed, 4 insertions(+), 2 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex 1748315..4c1e129 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -188,12 +188,14 @@ show_tool_help () {\n \t\t\tunavailable=\"$unavailable$i$LF\"\n \t\tfi\n \tdone\n+\n+\tcmd_name=${TOOL_MODE}tool\n \tif test -n \"$available\"\n \tthen\n-\t\techo \"'git mergetool --tool=<tool>' may be set to one of the following:\"\n+\t\techo \"'git $cmd_name --tool=<tool>' may be set to one of the following:\"\n \t\techo \"$available\" | sort | sed -e 's/^/\t/'\n \telse\n-\t\techo \"No suitable tool for 'git mergetool --tool=<tool>' found.\"\n+\t\techo \"No suitable tool for 'git $cmd_name --tool=<tool>' found.\"\n \tfi\n \tif test -n \"$unavailable\"\n \tthen\n-- \n1.8.1\n"},{"id":"207711","messageId":"b791e866c02b0c118f08bde1d7ca6c41d6239989.1359057056.git.john@keeping.me.uk","threadId":"32716","inReplyTo":"cover.1359057056.git.john@keeping.me.uk","subject":"[PATCH 4/4] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-24T19:55:16Z","receivedAt":"2013-01-24T19:55:16Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"The \"--tool-help\" option to git-difftool currently displays incorrect\noutput since it uses the names of the files in\n\"$GIT_EXEC_PATH/mergetools/\" rather than the list of command names in\ngit-mergetool--lib.\n\nFix this by simply delegating the \"--tool-help\" argument to the\nshow_tool_help function in git-mergetool--lib.\n\nSigned-off-by: John Keeping <john@keeping.me.uk>\n---\n git-difftool.perl | 57 ++++++++-----------------------------------------------\n 1 file changed, 8 insertions(+), 49 deletions(-)\n\ndiff --git a/git-difftool.perl b/git-difftool.perl\nindex edd0493..0a90de4 100755\n--- a/git-difftool.perl\n+++ b/git-difftool.perl\n@@ -59,57 +59,16 @@ sub find_worktree\n \treturn $worktree;\n }\n \n-sub filter_tool_scripts\n-{\n-\tmy ($tools) = @_;\n-\tif (-d $_) {\n-\t\tif ($_ ne \".\") {\n-\t\t\t# Ignore files in subdirectories\n-\t\t\t$File::Find::prune = 1;\n-\t\t}\n-\t} else {\n-\t\tif ((-f $_) && ($_ ne \"defaults\")) {\n-\t\t\tpush(@$tools, $_);\n-\t\t}\n-\t}\n-}\n-\n sub print_tool_help\n {\n-\tmy ($cmd, @found, @notfound, @tools);\n-\tmy $gitpath = Git::exec_path();\n-\n-\tfind(sub { filter_tool_scripts(\\@tools) }, \"$gitpath/mergetools\");\n-\n-\tforeach my $tool (@tools) {\n-\t\t$cmd  = \"TOOL_MODE=diff\";\n-\t\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n-\t\t$cmd .= \" && get_merge_tool_path $tool >/dev/null 2>&1\";\n-\t\t$cmd .= \" && can_diff >/dev/null 2>&1\";\n-\t\tif (system('sh', '-c', $cmd) == 0) {\n-\t\t\tpush(@found, $tool);\n-\t\t} else {\n-\t\t\tpush(@notfound, $tool);\n-\t\t}\n-\t}\n-\n-\tprint << 'EOF';\n-'git difftool --tool=<tool>' may be set to one of the following:\n-EOF\n-\tprint \"\\t$_\\n\" for (sort(@found));\n-\n-\tprint << 'EOF';\n-\n-The following tools are valid, but not currently available:\n-EOF\n-\tprint \"\\t$_\\n\" for (sort(@notfound));\n-\n-\tprint << 'EOF';\n-\n-NOTE: Some of the tools listed above only work in a windowed\n-environment. If run in a terminal-only session, they will fail.\n-EOF\n-\texit(0);\n+\tmy $cmd = 'TOOL_MODE=diff';\n+\t$cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n+\t$cmd .= ' && show_tool_help';\n+\n+\t# See the comment at the bottom of file_diff() for the reason behind\n+\t# using system() followed by exit() instead of exec().\n+\tmy $rc = system('sh', '-c', $cmd);\n+\texit($rc | ($rc >> 8));\n }\n \n sub exit_cleanup\n-- \n1.8.1\n"},{"id":"207719","messageId":"7vehha9tn6.fsf@alter.siamese.dyndns.org","threadId":"32716","inReplyTo":"cover.1359057056.git.john@keeping.me.uk","subject":"Re: [PATCH 0/4] Fix \"git difftool --tool-help\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-01-24T21:07:57Z","receivedAt":"2013-01-24T21:07:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nice code reduction ;-)\n\nThanks, will queue.\n"},{"id":"207762","messageId":"CAJDDKr4ZpQr029FW0v8LzwvhXZYmvAONbbZNuOq_E=Q1UzufvA@mail.gmail.com","threadId":"32716","inReplyTo":"b791e866c02b0c118f08bde1d7ca6c41d6239989.1359057056.git.john@keeping.me.uk","subject":"Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T05:29:58Z","receivedAt":"2013-01-25T05:29:58Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:\n> The \"--tool-help\" option to git-difftool currently displays incorrect\n> output since it uses the names of the files in\n> \"$GIT_EXEC_PATH/mergetools/\" rather than the list of command names in\n> git-mergetool--lib.\n>\n> Fix this by simply delegating the \"--tool-help\" argument to the\n> show_tool_help function in git-mergetool--lib.\n\nVery nice.\n\nOne thought I had was that the unified show_tool_help should\nprobably check TOOL_MODE=diff and skip over the\n!can_diff entries.\n\nThe current output of \"git difftool --tool-help\" before your\npatches has the problem that it will list tools such as\n\"tortoisemerge\" as \"valid but not available\" because it\ndoes not differentiate between missing and !can_diff.\n\n\n> diff --git a/git-difftool.perl b/git-difftool.perl\n> index edd0493..0a90de4 100755\n> --- a/git-difftool.perl\n> +++ b/git-difftool.perl\n> @@ -59,57 +59,16 @@ sub find_worktree\n>         return $worktree;\n>  }\n>\n> -sub filter_tool_scripts\n> -{\n> -       my ($tools) = @_;\n> -       if (-d $_) {\n> -               if ($_ ne \".\") {\n> -                       # Ignore files in subdirectories\n> -                       $File::Find::prune = 1;\n> -               }\n> -       } else {\n> -               if ((-f $_) && ($_ ne \"defaults\")) {\n> -                       push(@$tools, $_);\n> -               }\n> -       }\n> -}\n> -\n>  sub print_tool_help\n>  {\n> -       my ($cmd, @found, @notfound, @tools);\n> -       my $gitpath = Git::exec_path();\n> -\n> -       find(sub { filter_tool_scripts(\\@tools) }, \"$gitpath/mergetools\");\n> -\n> -       foreach my $tool (@tools) {\n> -               $cmd  = \"TOOL_MODE=diff\";\n> -               $cmd .= ' && . \"$(git --exec-path)/git-mergetool--lib\"';\n> -               $cmd .= \" && get_merge_tool_path $tool >/dev/null 2>&1\";\n> -               $cmd .= \" && can_diff >/dev/null 2>&1\";\n> -               if (system('sh', '-c', $cmd) == 0) {\n> -                       push(@found, $tool);\n> -               } else {\n> -                       push(@notfound, $tool);\n> -               }\n> -       }\n\nThis is where we conflated can_diff with \"is the tool available?\".\nThe nice thing is that the old code did actually check can_diff,\nbut since it conflated these things it ended up putting them\nin the @notfound list.  It should ignore those completely, IMO.\n\nThat could be a nice follow-up patch.  What do you think?\n-- \nDavid\n"},{"id":"207782","messageId":"20130125091918.GV7498@serenity.lan","threadId":"32716","inReplyTo":"CAJDDKr4ZpQr029FW0v8LzwvhXZYmvAONbbZNuOq_E=Q1UzufvA@mail.gmail.com","subject":"Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T09:19:18Z","receivedAt":"2013-01-25T09:19:18Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:\n> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:\n> > The \"--tool-help\" option to git-difftool currently displays incorrect\n> > output since it uses the names of the files in\n> > \"$GIT_EXEC_PATH/mergetools/\" rather than the list of command names in\n> > git-mergetool--lib.\n> >\n> > Fix this by simply delegating the \"--tool-help\" argument to the\n> > show_tool_help function in git-mergetool--lib.\n> \n> Very nice.\n> \n> One thought I had was that the unified show_tool_help should\n> probably check TOOL_MODE=diff and skip over the\n> !can_diff entries.\n> \n> The current output of \"git difftool --tool-help\" before your\n> patches has the problem that it will list tools such as\n> \"tortoisemerge\" as \"valid but not available\" because it\n> does not differentiate between missing and !can_diff.\n\nlist_merge_tool_candidates does this for us, so it should Just Work\nsince we use that to generate the list of tools that we loop over.\n\n\nJohn\n"},{"id":"207792","messageId":"CAJDDKr5Xd4-e6VO-iO=EmwOcg0RimJHyO4ey-dFk6aENt+qZBQ@mail.gmail.com","threadId":"32716","inReplyTo":"20130125091918.GV7498@serenity.lan","subject":"Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2013-01-25T09:55:03Z","receivedAt":"2013-01-25T09:55:03Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Jan 25, 2013 at 1:19 AM, John Keeping <john@keeping.me.uk> wrote:\n> On Thu, Jan 24, 2013 at 09:29:58PM -0800, David Aguilar wrote:\n>> On Thu, Jan 24, 2013 at 11:55 AM, John Keeping <john@keeping.me.uk> wrote:\n>> > The \"--tool-help\" option to git-difftool currently displays incorrect\n>> > output since it uses the names of the files in\n>> > \"$GIT_EXEC_PATH/mergetools/\" rather than the list of command names in\n>> > git-mergetool--lib.\n>> >\n>> > Fix this by simply delegating the \"--tool-help\" argument to the\n>> > show_tool_help function in git-mergetool--lib.\n>>\n>> Very nice.\n>>\n>> One thought I had was that the unified show_tool_help should\n>> probably check TOOL_MODE=diff and skip over the\n>> !can_diff entries.\n>>\n>> The current output of \"git difftool --tool-help\" before your\n>> patches has the problem that it will list tools such as\n>> \"tortoisemerge\" as \"valid but not available\" because it\n>> does not differentiate between missing and !can_diff.\n>\n> list_merge_tool_candidates does this for us, so it should Just Work\n> since we use that to generate the list of tools that we loop over.\n\nYup, kind of.  I looked more closely and found a lot of\nspecial-cases around vim which I've eliminated in the series\nI just sent (which includes your patches as its base).\n\nlist_merge_tool_candidates() has a bunch of other special cases\nfor $EDITOR, $DISPLAY, $GNOME-something and such so I think\nwe should keep using it only for the guess_merge_tool() path.\n\nI honestly want to remove list_merge_tool_candidates every\ntime I read it, but I recognize that it does serve a purpose\nfor users who have not configured anything.\n\nIn order to be useful for the documentation I think the\ncode could be refactored a tiny bit more beyond what I've\nsent so far.  The gathering of available tools can be split\noff from the reporting, and then show_tool_help() can use\nthe gatherer.  With what I sent, though, there's at least\na 1:1 correspondance between the name of the scriptlets\nand the names accepted by --tool=<tool>, which is the first\nstep towards doing that.\n\nI have to check out for now but I'll keep poking at this\nwhen the weekend rolls around.\n\ncheers,\n-- \nDavid\n"},{"id":"207800","messageId":"20130125104722.GY7498@serenity.lan","threadId":"32716","inReplyTo":"CAJDDKr5Xd4-e6VO-iO=EmwOcg0RimJHyO4ey-dFk6aENt+qZBQ@mail.gmail.com","subject":"Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for \"--tool-help\"","fromName":"John Keeping","fromEmail":"john@keeping.me.uk","sentAt":"2013-01-25T10:47:22Z","receivedAt":"2013-01-25T10:47:22Z","isPatch":true,"sender":{"key":"john@keeping.me.uk","avatar":"https://avatars.githubusercontent.com/u/1702081?v=4"},"body":"On Fri, Jan 25, 2013 at 01:55:03AM -0800, David Aguilar wrote:\n> list_merge_tool_candidates() has a bunch of other special cases\n> for $EDITOR, $DISPLAY, $GNOME-something and such so I think\n> we should keep using it only for the guess_merge_tool() path.\n> \n> I honestly want to remove list_merge_tool_candidates every\n> time I read it, but I recognize that it does serve a purpose\n> for users who have not configured anything.\n\nActually, I'm not sure it does.  I asked one of my colleagues whether\nhe used git-mergetool the other day and he said no because he couldn't\nunderstand the OS X FileMerge tool and was happier to edit things\nmanually in vim.  I don't think he'd realised that he could configure a\ndifferent mergetool.\n\nPerhaps we're trying to be too clever by guessing what the user wants\nand should instead exit with a message saying:\n\n   You have not configured a merge tool to use.  Please select one of\n   the following tools and configure it using:\n\n        git config merge.tool <tool>\n\n    These tools are availalble on your system:\n        ...\n\n    These tools are supported but unavailable:\n        ...\n\nThis may be too much of a regression for current users though.\n\n\nJohn\n"}]}